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

Change chread namespace #1222

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Change chread namespace #1222

merged 2 commits into from
Jun 24, 2020

Conversation

marier-nico
Copy link
Contributor

@marier-nico marier-nico commented May 19, 2020 •
edited by fntlnz
Loading

What type of PR is this?

/kind feature
/kind rule-update

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

Editing the exe_running_docker_save macro:
While using Falco, I noticed we were getting many events that were virtually identical to those that were previously filtered out by the exe_running_docker_save macro, but where the cmdline was something like exe /var/run/docker/netns/cc5c7b9bb110 all false. I believe this is caused by the use of docker-in-docker. This is removed by the new or proc.cmdline contains "/var/run/docker" part of the macro.

Change thread namespace rule edit:
These application binaries raise events in the Change thread namespace rule as part of their normal operation.

Here are more details regarding each binary :

  • protokube : See this
  • dockerd : The dockerd process name is whitelisted already in this
    rule, but not if it is the parent, which will happen if you are doing
    docker-in-docker.
  • tini : See this
  • aws : This one I noticed because Falco itself uses the AWS CLI to
    send events to SNS, which was triggering this rule.

Which issue(s) this PR fixes:

None

Does this PR introduce a user-facing change?:

rule(Change thread namespace): Allow `protokube`, `dockerd`, `tini` and `aws` binaries to change thread namespace.
rule(macro exe_running_docker_save): to filter out cmdlines containing `/var/run/docker`.

@poiana poiana requested review from Kaizhe and mstemm May 19, 2020 12:26
@leodido
Copy link
Member

leodido commented May 19, 2020

/milestone 0.24.0

@poiana poiana added this to the 0.24.0 milestone May 19, 2020
@Kaizhe
Copy link
Contributor

Kaizhe commented May 19, 2020

@marier-nico thanks for submitting this PR. I like the changes to the rule Change thread namesapce. However, you loosen too much in the macro exe_running_docker_save. I am not convinced by the changes.

@marier-nico
Copy link
Contributor Author

@marier-nico thanks for submitting this PR. I like the changes to the rule Change thread namesapce. However, you loosen too much in the macro exe_running_docker_save. I am not convinced by the changes.

@Kaizhe yeah, I was hoping to discuss that a bit! I had the feeling it would be too loose, but I didn't get much feedback in the Slack channel, so I figured it could be discussed in this PR. The thing is, I'm not really sure what to do with that macro, because there are thousands of events that I'm getting where proc.pname is just <NA>, is it possible that there is a bug somewhere in Falco? To the best of my knowledge, there wasn't any massive change to our infrastructure that would have spawned those events.

@Kaizhe
Copy link
Contributor

Kaizhe commented May 20, 2020

@marier-nico thanks for submitting this PR. I like the changes to the rule Change thread namesapce. However, you loosen too much in the macro exe_running_docker_save. I am not convinced by the changes.

@Kaizhe yeah, I was hoping to discuss that a bit! I had the feeling it would be too loose, but I didn't get much feedback in the Slack channel, so I figured it could be discussed in this PR. The thing is, I'm not really sure what to do with that macro, because there are thousands of events that I'm getting where proc.pname is just <NA>, is it possible that there is a bug somewhere in Falco? To the best of my knowledge, there wasn't any massive change to our infrastructure that would have spawned those events.

can you describe more about your scenario?

@marier-nico
Copy link
Contributor Author

can you describe more about your scenario?

@Kaizhe Sure! So it's a bit odd, but I just went in our log stack to find some sample events to show you what types of events I wanted to ignore, and I couldn't find any that had proc.pname=<NA>. I'm not sure what happened to those events, because I definitely know I saw a bunch like that, although it might have been an isolated incident.

Given that I can't find those events anymore, I think I'll just remove the commit that makes the macro too permissive! 😄

Nicolas Marier added 2 commits May 22, 2020 08:26
rule(macro exe_running_docker_save): add new cmdline
e748c28 
While using Falco, I noticed we were getting many events that were
virtually identical to those that were previously filtered out by the
`exexe_running_docker_save` macro, but where the `cmdline` was something
like `exe /var/run/docker/netns/cc5c7b9bb110 all false`. I believe this
is caused by the use of docker-in-docker.

Signed-off-by: Nicolas Marier <nmarier@coveo.com>
rule(Change thread namespace): whitelist protokube, dockerd, `tin…
4f71b95 
…i` and `aws`

These application binaries raise events in the `Change thread namespace`
rule as part of their normal operation.

Here are more details regarding each binary :

- `protokube` : See [this](https://github.com/kubernetes/kops/tree/master/protokube)
- `dockerd` : The `dockerd` process name is whitelisted already in this
  rule, but not if it is the parent, which will happen if you are doing
  docker-in-docker.
- `tini` : See [this](https://github.com/krallin/tini)
- `aws` : This one I noticed because Falco itself uses the AWS CLI to
  send events to SNS, which was triggering this rule.

Signed-off-by: Nicolas Marier <nmarier@coveo.com>
Kaizhe
Kaizhe approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@Kaizhe Kaizhe left a comment

Choose a reason for hiding this comment

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

/lgtm

@poiana poiana added the lgtm label May 27, 2020
@poiana
Copy link
Contributor

poiana commented May 27, 2020

LGTM label has been added.

Git tree hash: 6ff5548ff9f7ae1cc072cc87705df293ba44f33a

@Kaizhe
Copy link
Contributor

Kaizhe commented Jun 3, 2020

@leodido test hangs?

@leodido leodido closed this Jun 22, 2020
@leodido leodido reopened this Jun 22, 2020
@fntlnz
Copy link
Contributor

fntlnz commented Jun 24, 2020

Thanks again @marier-nico !

@poiana poiana merged commit 298ba29 into falcosecurity:master Jun 24, 2020
@poiana
Copy link
Contributor

poiana commented Jun 24, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@antoinedeschenes antoinedeschenes mentioned this pull request Jun 29, 2020
Closed
@fntlnz
Copy link
Contributor

fntlnz commented Jul 14, 2020

If you see an edit in the description, I just edited the release notes to follow our convention

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

@Kaizhe Kaizhe Kaizhe approved these changes

@mstemm mstemm Awaiting requested review from mstemm

Assignees

@Kaizhe Kaizhe

@fntlnz fntlnz

Labels
approved area/rules dco-signoff: yes kind/feature kind/rule-update lgtm release-note size/XS
Projects
None yet
Milestone
0.24.0
Development

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