Skip to content
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

tests: ips action test cases #1952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

victorjulien
Copy link
Member

I think the most interesting thing here is the difference between 6 and 7/master for the "b" case.

Also, we can debate if "a-01" should apply the pass to the flow. I'll put an inline comment too.

@@ -0,0 +1,5 @@
pass udp any any -> any 6081 (sid:1;)
pass tcp 2a03:b0c0:0002:00d0:0000:0000:0bd3:4001 any -> 2606:2800:0220:0001:0248:1893:25c8:1946 443 (msg:"PASS_CUSTOM_RULE TCP port:443 to support traffic"; flow:established; sid:201000044;)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not apply the pass to the flow, but should it? The flow starts as not_established, but after transitioning to established it stays that way, even if TCP state moves to "closed".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be coherent with the rule language as we read it.

match:
event_type: tls
- filter:
min-version: 7
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a behavior change between 6 and 7+master. I think it makes sense, as the app_proto can change.

args:
- --set stream.midstream=false
- -k none

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it important to run these as IPS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are, because ips is in the test name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, if the test says ips-state SV will run them as IPS? 🤯

@catenacyber
Copy link
Collaborator

Is this PR a draft to discuss current vs expected behavior ?

Comment on lines +2 to +4
pass tcp 2a03:b0c0:0002:00d0:0000:0000:0bd3:4001 any <> 2606:2800:0220:0001:0248:1893:25c8:1946 443 (msg:"PASS_CUSTOM_RULE TCP port:443 to support traffic"; flow:established; sid:201000044;)

pass tcp 2a03:b0c0:0002:00d0::/64 any <> any [[80,443]] (msg:"PASS_HTTP_NOT_ESTABLISHED TCP allow http/https traffic to the established state to allow further inspection"; flow:not_established; sid:201000012;)
Copy link
Contributor

@jufajardini jufajardini Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to support better seeing what's going on, couldn't we add alert to these pass rules?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, if we do, enable verdict for the alert events, too?

count: 0
match:
event_type: flow
flow.action: "drop"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, just to see if we agree on this: shouldn't we see flow.action in the logs, here? I don't see it, at all.

@catenacyber
Copy link
Collaborator

Should this PR be a draft ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants