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.

Sign up for GitHub

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

Jump to bottom

Add a macro to whitelist system binaries using the network #1070

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Add a macro to whitelist system binaries using the network #1070

merged 2 commits into from
Apr 14, 2020

Conversation

marier-nico
Copy link
Contributor

@marier-nico marier-nico commented Mar 2, 2020 •
edited by leogr
Loading

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

This PR introduces two things : a list of allowed system binaries and a macro to allow certain system binaries to use the network under certain conditions.

The list is really just for convenience and is not meant to be overridden by users. It's something I noticed many other rules had and I figured it wouldn't hurt to add it here while I was adding the macro.

The macro is useful because someone might want to allow certain system procs to use the network under certain conditions. Our use-case is that we use a program output which publishes a message to an SNS topic in AWS to be picked up by our other tools. To do this, we use AWS CLI, which obviously needs to talk to the network. The issue is that Falco runs the command sh -c aws cli ... and that raises an event because sh is a shell binary. The macro resolves this because it allows us to whitelist the command we use (instead of whitelisting the entire sh binary).

Which issue(s) this PR fixes:

No issues open for this.

Does this PR introduce a user-facing change?:

rule(macro user_expected_system_procs_network_activity_conditions): allow whitelisting system binaries using the network under specific conditions

@poiana poiana requested review from Kaizhe and mstemm March 2, 2020 21:31
@poiana poiana added the size/S label Mar 2, 2020
@Kaizhe
Copy link
Contributor

Kaizhe commented Mar 3, 2020

/lgtm

@poiana poiana added the lgtm label Mar 3, 2020
@poiana
Copy link
Contributor

poiana commented Mar 3, 2020

LGTM label has been added.

Git tree hash: a8d20d8a789a42cff29701aee203ebfd6bc6b8f9

@poiana poiana added the approved label Mar 3, 2020
Kaizhe
Kaizhe previously approved these changes Mar 3, 2020
View reviewed changes
@fntlnz
Copy link
Contributor

fntlnz commented Mar 4, 2020

This is targeting the wrong branch, changing the base. We are now using master.

@fntlnz fntlnz dismissed Kaizhe’s stale review March 4, 2020 08:26

The base branch was changed.

@fntlnz fntlnz changed the base branch from dev to master March 4, 2020 08:26
@fntlnz
Copy link
Contributor

fntlnz commented Mar 4, 2020

@Kaizhe PTAL again And thanks @marier-nico for sending over your first PR to Falco 🎉

@leodido
Copy link
Member

leodido commented Mar 4, 2020

This is the second PR of @marier-nico ! So two times thanks 🤗🤗

@leodido leodido closed this Mar 4, 2020
@leodido leodido reopened this Mar 4, 2020
@marier-nico
Copy link
Contributor Author

@leodido @fntlnz No problem! 😄

I'm not sure about this failing check though, I haven't made any code changes, could the config be invalid because of what I edited?

@poiana poiana removed the lgtm label Mar 4, 2020
@marier-nico
Copy link
Contributor Author

Caught my mistake in falco_rules.yml I was missing parens around the list of known processes!

@leodido
Copy link
Member

leodido commented Apr 8, 2020

/cc @leodido

Want to take a look at this to understand why the CI is not being triggered

/hold

@poiana poiana requested a review from leodido April 8, 2020 17:09
@fntlnz fntlnz closed this Apr 14, 2020
@fntlnz fntlnz reopened this Apr 14, 2020
@fntlnz
Copy link
Contributor

fntlnz commented Apr 14, 2020

Closing and reopening to let the CI handle this

fntlnz
fntlnz previously approved these changes Apr 14, 2020
View reviewed changes
leodido
leodido previously approved these changes Apr 14, 2020
View reviewed changes
@poiana poiana added the lgtm label Apr 14, 2020
@poiana
Copy link
Contributor

poiana commented Apr 14, 2020

LGTM label has been added.

Git tree hash: 4c2971f157e1dfe8b3c116338e1bda5ff78715fa

@leodido leodido removed the request for review from mstemm April 14, 2020 10:49
@fntlnz
Copy link
Contributor

fntlnz commented Apr 14, 2020

Approved, thanks @marier-nico

Nicolas Marier added 2 commits April 14, 2020 10:55
@leodido
rule(list known_system_procs_network_activity_binaries): add a list o…
665a1b8 
…f known procs for convenience

This makes it more convenient to add more allowed procs and many other
rules have a similar mechanism to whitelist certain processes.

Signed-off-by: Nicolas Marier <[email protected]>
@leodido
rule(macro user_expected_system_procs_network_activity_conditions): c…
64ab5c7 
…reate the macro

It's useful to ignore some system binaries that use the network under
certain conditions, so this should be overridable by the user.

Signed-off-by: Nicolas Marier <[email protected]>
@leodido leodido dismissed stale reviews from fntlnz and themself via 64ab5c7 April 14, 2020 10:56
@poiana poiana removed the lgtm label Apr 14, 2020
@poiana poiana added the lgtm label Apr 14, 2020
@poiana
Copy link
Contributor

poiana commented Apr 14, 2020

LGTM label has been added.

Git tree hash: 00bce8c3272cad81012d1da13c501fb7a2f7dd42

@poiana
Copy link
Contributor

poiana commented Apr 14, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fntlnz, Kaizhe, leodido

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

@fntlnz
Copy link
Contributor

fntlnz commented Apr 14, 2020

/hold cancel

@leodido
Copy link
Member

leodido commented Apr 14, 2020

/milestone 0.22.0

@poiana poiana added this to the 0.22.0 milestone Apr 14, 2020
@poiana poiana merged commit 91a0b51 into falcosecurity:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fntlnz fntlnz fntlnz approved these changes

@leodido leodido leodido approved these changes

@Kaizhe Kaizhe Kaizhe left review comments

Assignees

@leodido leodido

@Kaizhe Kaizhe

@fntlnz fntlnz

Labels
approved area/rules dco-signoff: yes kind/feature lgtm release-note size/S
Projects
None yet
Milestone
0.22.0
Development

Successfully merging this pull request may close these issues.
5 participants
@marier-nico @Kaizhe @poiana @fntlnz @leodido