-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
turn ProtocolInspectPolicy into AclDstHostRuleSet #344
turn ProtocolInspectPolicy into AclDstHostRuleSet #344
Conversation
I will do a full review after back to work on 10.10. |
Meaning in a month or what timeline can I expect? This is not to demand anything, but more so I can manage my own expectations and reminders. Either answer is fine, it is just to know :) |
the day after tomorrow, maybe I should write 10/10 in English? (⊙o⊙) |
All good. Thank you for clarifying. Now I understand. Thanks! |
default_action: Action, | ||
} | ||
|
||
impl<Action: ActionContract> Clone for AclNetworkRule<Action> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *Bulder
struct is used in config/*
module to allow Clone/PartialEq
. You can avoid Clone
here by using the corresponding Builder
in g3proxy/src/config/audit/auditor.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand your feedback here. Could you try in more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use AclDstHostRuleSetBuilder
/ProtocolInspectPolicyBuilder
in AuditorConfig
, and build AclDstHostRuleSet
/ProtocolInspectPolicy
as new fields in Auditor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am correct to assume that you take this one over as well after merge. Or did I misunderstand that?
|
||
let exact_rule = self.exact.as_ref().map(|rule| { | ||
missed_action = missed_action.restrict(rule.missed_action()); | ||
missed_action = rule.missed_action().max(missed_action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern here is that, wr can not say intercept
is more restrict than detour
. A different build approach may be needed for AclAction
and ProtocolInspectAction
. You can squash the commits into smaller ones, and I will update the build logic here after merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll get to the squashing now so you can take over.
Note that besides your feedback regarding the restrict
thing and also the builder clone that you can probably easily fix yourself, for which thank you, there is also the more severe issue, which is the bug I was talking to you about in the issue. Regarding also blocking non-ws traffic once the ws policy is defined. So that's something to look out for as well.
6b2f67a
to
78b6975
Compare
78b6975
to
1a52f14
Compare
Double checked the code in this PR, looks still fine even after all the rebasing. I also do not see immediately where there could be a bug. Gonna double check running this code in a demo env and see if I still have the same issue, because as far as I can see the code maps 1 to 1 with the old logic, with the only difference being that we support acl-like sets now. Assuming the |
Great news @zh-jq I pulled this branch into a demo env and all seems to work. E.g.
Blocks that host, but all others are fine. Seems that I had a mistake in my demo env due to a wrong merge at some point. So seems no buggy code after all. So I think you can merge this code with just those two small remarks that are probably easier for you to get right as I have a feeling you have a specific solution in mind that is probably faster to just do yourself than communicate to me. But if you think it is useful for me to do it do tell me with more explanation and detail and I will do my best to contribute it as well. All good for me. |
Closes #341
tested it with web sockets on a local g3 proxy, and it seems to work as expected:
Should work just as well for
h2_inspect_policy
,smtp_inspect_policy
andimap_inspect_policy
. But these I haven't tested. Code is same however.