Introduce rule parser to support upcoming stateful rules#184
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dfede19e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .into_iter() | ||
| .map(|rule| match rule { | ||
| Rule::Stateless(target) => target, | ||
| Rule::Stateful { .. } => unreachable!("stateful rules were rejected before conversion"), |
There was a problem hiding this comment.
Reject unsupported stateful rules without panicking
When a library caller passes a publicly constructible Rule::Stateful to the public Proxy::new API, this branch panics before the constructor can return its Result. The CLI and control-socket validators do not protect direct library callers, so a validly parsed rule such as "from @host" can crash the process; reject unsupported rules with an error at the constructor boundary instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We do ensure that there are no Rule::Stateful rules in main.rs.
fkorotkov-oai
left a comment
There was a problem hiding this comment.
I'm not a fan of this syntax in regards to existing "allow"/""block". WhenI'm looking at
--allow="from @host proto tcp port 8080"
I read it as a sentence and after "allow" I want to know what do I allow. For example, in my example of
--allow="IN tcp @host"
I can read it as "allow INbound tcp trafic from host". With the example above my mind can't construct a phrase.
|
|
||
| stateful = { direction ~ " "+ ~ target } | ||
| direction = { "from" | "to" } | ||
| target = @{ ANY+ } |
There was a problem hiding this comment.
so port and protocols will be part of "target"?
There was a problem hiding this comment.
No, they will be separate to allow composition.
This keeps the grammar unambiguous: targets describe endpoints, while protocol and port are independent match criteria that can be combined with from and to.
This rule would allow incoming traffic from the host's TCP port 8080 to the guest, with a nit that Here's an example to allow incoming traffic from the host to the guest's TCP port 8080:
How do you envision this syntax enabling us to construct more complex rules in the future? For example, what would the rule look like that allows traffic from For a PF-like language, this is already well thought out: |
|
My concern is that this proposes adopting a full PF-like grammar before Softnet needs one—and we might never need one. For example, I don’t expect Softnet to support rules such as: Adopting similar syntax may imply that this level of expressiveness is supported or planned. If we eventually need rules that complex, a multiline configuration file would probably be a better interface than For the current single-VM use case, I would keep the syntax simple: If we later need explicit source and destination endpoints, the syntax can evolve naturally: That gives us |
Nowhere does it propose that, sorry — perhaps I wasn't clear. It just takes a small subset of what we need right now, while giving us a clear path forward in case we need more extensibility in the future. From what you're already envisioning above: A PF subset would be less ambiguous here, because it associates the port with the specific endpoint: This ambiguity can already be seen in simpler rules like: Does |
|
@fkorotkov-oai I think we can combine the
Examples:
Implemented |
|
What about four tuple format that won't require a grammar: |
The idea is to: