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

Psp rules support #826

Merged
merged 6 commits into from Oct 15, 2019
Merged

Psp rules support #826

merged 6 commits into from Oct 15, 2019

Conversation

@mstemm
Copy link
Contributor

mstemm commented Sep 10, 2019

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind flaky-test

If contributing rules or changes to rules, please make sure to also uncomment one of the following line:

/kind rule-update

/kind rule-create

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area engine

/area rules

/area deployment

/area integrations

/area examples

What this PR does / why we need it:

Add support for converting K8s Pod Security Policies (PSPs) into set of falco rules that can be used to evaluate the conditions specified in the PSP.

See #825 for the feature proposal.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add support for converting K8s Pod Security Policies (PSPs) into set of falco rules that can be used to evaluate the conditions specified in the PSP.
@mstemm

This comment has been minimized.

Copy link
Contributor Author

mstemm commented Sep 10, 2019

The tests will fail until draios/sysdig#1501 is merged.

@mstemm mstemm mentioned this pull request Sep 10, 2019
@mstemm mstemm force-pushed the psp-rules-support branch 2 times, most recently from 144d4f2 to 4c820a6 Sep 10, 2019
@Kaizhe

This comment has been minimized.

Copy link
Contributor

Kaizhe commented Sep 23, 2019

@mstemm , would you please add a test case for requiredDropCapabilities?

@fntlnz

This comment has been minimized.

Copy link
Member

fntlnz commented Sep 24, 2019

I commented very extensively about this in the proposal here #825 (review)

Summary is that I don't think that doing this is responsibility of Falco directly (but please refer to the review in the proposal for more details)

For this reason I think that this must be discussed in the repo planning meeting, I'm sure others will find useful to understand this better too.

@mstemm

This comment has been minimized.

Copy link
Contributor Author

mstemm commented Sep 24, 2019

The feedback about who does the conversion is good and we can continue the discussion there.

A lot of this PR is changes separate from the mechanics of converting a PSP to falco rules, though. Separate from that code, there's a whole new set of filter fields and operators to look at different parts of k8s audit logs and compare them to sets of desired/forbidden ports, images, etc. I'd encourage you to look at that code as well, as it would be in falco regardless of where the conversion occurs.

@fntlnz

This comment has been minimized.

Copy link
Member

fntlnz commented Sep 25, 2019

@mstemm in general I really like what you did here and having this new feature in Falco.

However since this is a big improvement we need to take a look at it a bit closer than usual, with the rest of the community too.

The conversion part is the only I have concerns about (because I have different opinions on where it can be done).

@@ -20,7 +20,7 @@ compiler: gcc
env:
- BUILD_TYPE=debug
- BUILD_TYPE=release
sudo: required
dist: xenial

This comment has been minimized.

Copy link
@fntlnz

fntlnz Sep 25, 2019

Member

Why did you do this? Was missing before or it's a new requirement?

This comment has been minimized.

Copy link
@mstemm

mstemm Sep 25, 2019

Author Contributor

It was required to get travis tests to pass. I don't really know why I had to force travis to use xenial, as https://blog.travis-ci.com/2019-04-15-xenial-default-build-environment says xenial is the default if unspecified. The commit msg for 4c820a6 has a bit more detail.

rules/k8s_audit_rules.yaml Show resolved Hide resolved
InOp = kw("in") / "in";
PmatchOp = kw("pmatch") / "pmatch";
SetOp = kw("in") / "in" +
kw("intersects") / "intersects" +

This comment has been minimized.

Copy link
@fntlnz
userspace/engine/k8s_psp.h Outdated Show resolved Hide resolved
@leodido leodido added this to the 0.18.0 milestone Sep 25, 2019
Copy link
Member

leodido left a comment

Thanks Mark for your huge effort !

Overall this only needs some adjustments imho. As soon you can address them we'll merge this huge work!

rules/k8s_audit_rules.yaml Show resolved Hide resolved
rules/k8s_audit_rules.yaml Show resolved Hide resolved
@@ -86,6 +86,28 @@ uint32_t falco_engine::engine_version()

#define CONSOLE_LINE_LEN 79

static void wrap_text(const std::string &str, uint32_t initial_pos, uint32_t indent, uint32_t line_len)

This comment has been minimized.

Copy link
@leodido

leodido Sep 26, 2019

Member

This function should only have the responsibility to construct the string which then can be printed.

Furthermore I'll move this in an outside file (eg., utils, with falco::engine::utils namespace maybe). WDYT

This comment has been minimized.

Copy link
@mstemm

mstemm Oct 7, 2019

Author Contributor

Done (just falco::utils though).

userspace/falco/falco.cpp Outdated Show resolved Hide resolved
userspace/falco/falco.cpp Outdated Show resolved Hide resolved
rules/templates/CMakeLists.txt Outdated Show resolved Hide resolved
test/trace_files/psp/CMakeLists.txt Show resolved Hide resolved
userspace/engine/json_evt.h Show resolved Hide resolved
userspace/engine/json_evt.h Show resolved Hide resolved
userspace/engine/json_evt.cpp Show resolved Hide resolved
@leodido leodido modified the milestones: 0.18.0, 0.19.0 Oct 3, 2019
@mstemm mstemm force-pushed the psp-rules-support branch 4 times, most recently from f7fc448 to 5836be0 Oct 4, 2019
@mstemm mstemm force-pushed the psp-rules-support branch from b6ff1aa to dcfe3f6 Oct 14, 2019
@mstemm

This comment has been minimized.

Copy link
Contributor Author

mstemm commented Oct 14, 2019

Looks like all the tests are passing now. Can I get a final lgtm so we can merge?

@leodido

This comment has been minimized.

Copy link
Member

leodido commented Oct 15, 2019

@mstemm PR looks very good! Thanks

Just one last thing please: could you remove the commits c78f2b0 and bae8d1b ?

Done that this PR is definitely in for me! 🎈

Instead of using a psp_conv binary built in the falco build, download
falcoctl 0.0.2 and use its "falcoctl convert psp" subcommand to perform
the conversion.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
@mstemm mstemm force-pushed the psp-rules-support branch from dcfe3f6 to e91562a Oct 15, 2019
@mstemm

This comment has been minimized.

Copy link
Contributor Author

mstemm commented Oct 15, 2019

Ok, done. This will also undo the formatting for the changes I made, though. Let's make sure the tests just in case there were substantive changes in the formatting.

@mstemm

This comment has been minimized.

Copy link
Contributor Author

mstemm commented Oct 15, 2019

...And the tests passed!

Copy link
Member

leodido left a comment

🎳 💣

@poiana poiana added the lgtm label Oct 15, 2019
@poiana

This comment has been minimized.

Copy link

poiana commented Oct 15, 2019

LGTM label has been added.

Git tree hash: 2f92a254fd49d5573fe183da811816edb346f329

@poiana

This comment has been minimized.

Copy link

poiana commented Oct 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@poiana poiana added the approved label Oct 15, 2019
@leodido leodido merged commit b4fdaa3 into dev Oct 15, 2019
3 of 4 checks passed
3 of 4 checks passed
tide Not mergeable. Job Travis CI - Pull Request has not succeeded.
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
dco All commits have Signed-off-by
Details
@poiana poiana deleted the psp-rules-support branch Oct 15, 2019
mstemm added a commit to falcosecurity/falco-website that referenced this pull request Oct 15, 2019
Add docs describing the changes in falcosecurity/falco#826.
mstemm added a commit to falcosecurity/falco-website that referenced this pull request Oct 15, 2019
Add docs describing the changes in falcosecurity/falco#826.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit that referenced this pull request Oct 17, 2019
As a part of the changes in
#826, we added several
breaking changes to rules files like renaming/removing some filter
fields. This isn't ideal for customers who are using their own rules
files.

We shouldn't break older rules files in this way, so add some minimal
backwards compatibility which adds back the fields that were
removed *and* actually used in k8s_audit_rules.yaml. They have the same
functionality as before. One exception is
ka.req.binding.subject.has_name, which was only used in output fields
for debugging and shouldn't have been in the rules file in the first
place. This always returns the string "N/A".
mstemm added a commit that referenced this pull request Oct 18, 2019
As a part of the changes in
#826, we added several
breaking changes to rules files like renaming/removing some filter
fields. This isn't ideal for customers who are using their own rules
files.

We shouldn't break older rules files in this way, so add some minimal
backwards compatibility which adds back the fields that were
removed *and* actually used in k8s_audit_rules.yaml. They have the same
functionality as before. One exception is
ka.req.binding.subject.has_name, which was only used in a single output
field for debugging and shouldn't have been in the rules file in the
first place. This always returns the string "N/A".
mstemm added a commit that referenced this pull request Oct 18, 2019
As a part of the changes in
#826, we added several
breaking changes to rules files like renaming/removing some filter
fields. This isn't ideal for customers who are using their own rules
files.

We shouldn't break older rules files in this way, so add some minimal
backwards compatibility which adds back the fields that were
removed *and* actually used in k8s_audit_rules.yaml. They have the same
functionality as before. One exception is
ka.req.binding.subject.has_name, which was only used in a single output
field for debugging and shouldn't have been in the rules file in the
first place. This always returns the string "N/A".
poiana added a commit to falcosecurity/falco-website that referenced this pull request Oct 18, 2019
Add docs describing the changes in falcosecurity/falco#826.

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit that referenced this pull request Oct 18, 2019
As a part of the changes in
#826, we added several
breaking changes to rules files like renaming/removing some filter
fields. This isn't ideal for customers who are using their own rules
files.

We shouldn't break older rules files in this way, so add some minimal
backwards compatibility which adds back the fields that were
removed *and* actually used in k8s_audit_rules.yaml. They have the same
functionality as before. One exception is
ka.req.binding.subject.has_name, which was only used in a single output
field for debugging and shouldn't have been in the rules file in the
first place. This always returns the string "N/A".

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
mstemm added a commit that referenced this pull request Oct 21, 2019
As a part of the changes in
#826, we added several
breaking changes to rules files like renaming/removing some filter
fields. This isn't ideal for customers who are using their own rules
files.

We shouldn't break older rules files in this way, so add some minimal
backwards compatibility which adds back the fields that were
removed *and* actually used in k8s_audit_rules.yaml. They have the same
functionality as before. One exception is
ka.req.binding.subject.has_name, which was only used in a single output
field for debugging and shouldn't have been in the rules file in the
first place. This always returns the string "N/A".

Signed-off-by: Mark Stemm <mark.stemm@gmail.com>
@fntlnz fntlnz modified the milestones: 0.19.0, 0.18.0 Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.