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

Add send/recv and RFC 1918 handling to in/out macro. #828

Closed
wants to merge 5 commits into from
Closed

Add send/recv and RFC 1918 handling to in/out macro. #828

wants to merge 5 commits into from

Conversation

shane-lawrence
Copy link
Contributor

@shane-lawrence shane-lawrence commented Sep 11, 2019

What type of PR is this?

/kind cleanup

/kind rule-update

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:
Updates the combined inbound/outbound rule to reflect changes that were made to the separate inbound or outbound rules from 6ce17d6 and #470

Which issue(s) this PR fixes:
Fixes #820

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
Yes.

combined inbound_outbound now behaves like separate in/out macros

Signed-off-by: Shane Lawrence <shane@lawrence.dev>
Signed-off-by: Shane Lawrence <shane@lawrence.dev>
@poiana
Copy link
Contributor

poiana commented Sep 11, 2019

Welcome @shane-lawrence! It looks like this is your first PR to falcosecurity/falco 🎉

@shane-lawrence
Copy link
Contributor Author

/assign @Kaizhe

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Thanks for your first contribution! It's amazing :)

BTW there is a compilation error for the macro you kindly contributed.

3:14:56 DEBUG| [stderr] Wed Sep 11 03:14:56 2019: Falco initialized with configuration file /source/falco/test/../falco.yaml
03:14:56 DEBUG| [stderr] Wed Sep 11 03:14:56 2019: Loading rules from file /source/falco/test/../rules/falco_rules.yaml:
03:14:56 DEBUG| [stderr] Wed Sep 11 03:14:56 2019: Runtime error: Compilation error when compiling "(((evt.type in (accept,listen,connect) and evt.dir=<) or
03:14:56 DEBUG| [stderr]   (evt.type in (recvfrom,recvmsg,sendto,sendmsg) and evt.dir=< and
03:14:56 DEBUG| [stderr]    fd.l4proto != tcp and fd.connected=false and fd.name_changed=true)) and
03:14:56 DEBUG| [stderr]  (fd.typechar = 4 or fd.typechar = 6) and
03:14:56 DEBUG| [stderr]  (fd.ip != "0.0.0.0" and fd.net != "127.0.0.0/8" and (not fd.snet in ("10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16")) and
03:14:56 DEBUG| [stderr]  (evt.rawres >= 0 or evt.res = EINPROGRESS))
03:14:56 DEBUG| [stderr] ": 411: syntax error, unexpected 'EOF', expecting ')', 'or', 'and'
03:14:56 DEBUG| [stderr] ---
03:14:56 DEBUG| [stderr] - macro: inbound_outbound
03:14:56 DEBUG| [stderr]   condition: >
03:14:56 DEBUG| [stderr]     (((evt.type in (accept,listen,connect) and evt.dir=<) or
03:14:56 DEBUG| [stderr]       (evt.type in (recvfrom,recvmsg,sendto,sendmsg) and evt.dir=< and
03:14:56 DEBUG| [stderr]        fd.l4proto != tcp and fd.connected=false and fd.name_changed=true)) and
03:14:56 DEBUG| [stderr]      (fd.typechar = 4 or fd.typechar = 6) and
03:14:56 DEBUG| [stderr]      (fd.ip != "0.0.0.0" and fd.net != "127.0.0.0/8" and (not fd.snet in (rfc_1918_addresses)) and
03:14:56 DEBUG| [stderr]      (evt.rawres >= 0 or evt.res = EINPROGRESS))
03:14:56 DEBUG| [stderr] ---. Exiting.

@poiana
Copy link
Contributor

poiana commented Sep 11, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kaizhe

If they are not already assigned, you can assign the PR to them by writing /assign @kaizhe in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Shane Lawrence <shane@lawrence.dev>
@shane-lawrence
Copy link
Contributor Author

Thanks, I corrected my syntax error but some tests are failing now. The change has suppressed some alerts from network rules. This might be expected if test events used private IP ranges to trigger rules that rely on inbound_outbound.

@@ -331,9 +331,11 @@
# for efficiency.
- macro: inbound_outbound
condition: >
(((evt.type in (accept,listen,connect) and evt.dir=<)) or
(((evt.type in (accept,listen,connect) and evt.dir=<) or
(evt.type in (recvfrom,recvmsg,sendto,sendmsg) and evt.dir=< and
Copy link
Contributor

@Kaizhe Kaizhe Sep 12, 2019

Choose a reason for hiding this comment

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

Shouldn't the part that deals with outbound network traffic include recvfrom or recvmsg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the outbound traffic should have sendto and sendmsg, and the inbound traffic should have recvfrom and recvmsg.

@poiana poiana requested a review from fntlnz September 13, 2019 01:32
@leodido
Copy link
Member

leodido commented Sep 13, 2019

/cc fntlnz

@shane-lawrence
Copy link
Contributor Author

I overlooked that traffic with fd.snet in (rfc_1918_addresses) should still trigger the inbound portion. I'll fix it!

Signed-off-by: Shane Lawrence <shane@lawrence.dev>
@shane-lawrence
Copy link
Contributor Author

I was hoping it would work if the remote IP was filtered instead of server IP, but it didn't. I can't run the regression tests locally (looks like the same symptoms as #798) so I pushed a commit with the changes to let CI run. I'll edit this to have the same logic as inbound or outbound and make sure the tests pass, and then look at making it more efficient.

Signed-off-by: Shane Lawrence <shane@lawrence.dev>
@shane-lawrence
Copy link
Contributor Author

@Kaizhe the tests are still failing. I think inbound_outbound should be equivalent to inbound or outbound, but modified to improve efficiency. Could you please confirm that I haven't misunderstood that before I troubleshoot?

@Kaizhe
Copy link
Contributor

Kaizhe commented Sep 17, 2019

I think this might related to our test, we may just test outbound with rfc 1918 addresses. Will confirm that.

@Kaizhe
Copy link
Contributor

Kaizhe commented Oct 10, 2019

need @mstemm 's input here, the inbound_outbound test we ran, we test as an internal env right? Not real outbound I mean

@fntlnz
Copy link
Contributor

fntlnz commented Nov 27, 2019

What's the state of this @shane-lawrence @Kaizhe ?
Needs a review or are we waiting on some other work to unblock this?

@Kaizhe
Copy link
Contributor

Kaizhe commented Nov 27, 2019

I think this might related to the tests.

@shane-lawrence
Copy link
Contributor Author

I wasn't able to run the tests in my environment due to an unrelated issue, so instead of sending blind changes to Travis CI to see what happens, I was waiting for @Kaizhe or @mstemm to confirm the expected behaviour of the tests. I'll give it another try this week.

@fntlnz
Copy link
Contributor

fntlnz commented Nov 27, 2019

Ok thanks @shane-lawrence - I’m not very familiar with that part either but if this goes far I’ll happily dig with you into this.

@stale
Copy link

stale bot commented Jan 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 26, 2020
@stale stale bot closed this Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC1918 excluded from outbound macro but not inbound_outbound
5 participants