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

[WIP] Add KEP to create seccomp built-in profiles and add complain mode #1257

Closed
wants to merge 16 commits into from
Closed

[WIP] Add KEP to create seccomp built-in profiles and add complain mode #1257

wants to merge 16 commits into from

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Sep 25, 2019

This proposal focuses on creating new seccomp built-in profiles in order to support a complain mode..

Feature: #135
Security Audit Issue: kubernetes/kubernetes#81115

For context:
kubernetes/community#660
kubernetes/kubernetes#39845
kubernetes/kubernetes#20870
kubernetes/kubernetes#39128
kubernetes/kubernetes#52827

/sig-node
/sig-auth

/priority important-longterm

/assign @tallclair
/cc @jessfraz

@k8s-ci-robot
Copy link
Contributor

Welcome @pjbgf!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Sep 25, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 25, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @pjbgf. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2019
@pjbgf pjbgf changed the title [WIP] Enable seccomp by default [WIP] Add KEP for enabling seccomp by default Sep 25, 2019
Copy link
Member

@tallclair tallclair 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 working on this! You've got a good start here.

/assign @wangzhen127

### Goals

- Add audit-mode support for logging violations instead of blocking them.
- Set clusters to use the new audit profile (i.e. `runtime/default-audit`) by default.
Copy link
Member

Choose a reason for hiding this comment

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

We should reconcile the API with #1148, which I'm hoping to get into 1.17

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to take the same profile and run it in audit or enforce mode? If so, we should probably make it a *bool on the SeccompOptions rather than a profile-level switch.

Copy link
Member Author

@pjbgf pjbgf Sep 27, 2019

Choose a reason for hiding this comment

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

@tallclair yes, that is feasible. I can think of two ways to approach this, both involves amending the profile before sending it over to the CRI:

  1. Audit only unspecified actions by replacing the profile's defaultAction to SCMP_ACT_LOG. Very useful to keep allowed and blocked rules intact, but providing users a way to find extra syscalls that may need to be allowed.
  2. Audit all syscalls that are not whitelisted by replacing all defined blocking actions and the defaultAction with SCMP_ACT_LOG. This answers questions such as "why my application is being blocked even though I am running it in audit mode?"

The first approach sounds reasonable and would work fine with the default profile which currently does not blacklist anything. However, the second might be more user-friendly for custom profiles that may have blocking/tracing rules.

If we were to do this, I would go for the first approach and use a meaningful name for the flag inside the SeccompOptions. Maybe something in the lines of AuditOnlyUnspecifiedSyscalls?

UPDATE 1: A caveat here is that this would not work with the current runtime/default profiles. This would only work for the new kubernetes and/or custom profiles, as we would be able to manipulate them.

UPDATE 2: Based on another point you raised I think we may not want to make changes to profiles at runtime and rather keep them immutable. This way it will be easier to identify when profiles were tampered with. I will detail this a bit better on the custom profiles KEP, however, it may be sensible to have that applied to both custom and built-in profiles.

keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
@pjbgf pjbgf changed the title [WIP] Add KEP for enabling seccomp by default Add KEP for enabling seccomp by default Oct 2, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2019
@pjbgf
Copy link
Member Author

pjbgf commented Oct 2, 2019

PTAL

keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved
keps/sig-node/20190925-enable-seccomp-by-default.md Outdated Show resolved Hide resolved

##### Beta -> GA Graduation

- Enable audit-mode as default.
Copy link
Member

Choose a reason for hiding this comment

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

Once the tooling is sufficiently mature, and with enough soak time, do you see us eventually flipping the default to the enforcing default profile?

Copy link
Member Author

@pjbgf pjbgf Oct 15, 2019

Choose a reason for hiding this comment

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

I removed this in the new scope. The idea is to get built-in pods in either the default profile or on a custom profile - if needed - in parallel to this effort. Ultimately, the complain mode should be used only for user workloads, as an opt-in feature.

@mrbobbytables
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 14, 2019
pjbgf and others added 8 commits October 15, 2019 10:06
Signed-off-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Signed-off-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
Co-Authored-By: Tim Allclair (St. Clair) <tallclair@google.com>
Signed-off-by: Paulo Gomes <paulo.gomes.uk@gmail.com>
@pjbgf

This comment has been minimized.

@k8s-ci-robot k8s-ci-robot removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/pm sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Oct 15, 2019
@pjbgf
Copy link
Member Author

pjbgf commented Oct 15, 2019

After going through @tallclair's feedback, I decreased the scope of this KEP to only create new built-in profiles. Users will be able to use the existing mechanisms (subject to changes from #1148) to explicitly set the new profiles for containers, pods or the entire cluster.

This removes a lot of the complexity and potential backwards compatibility issues, whilst delivering the main value which is the ability for users to assess impact of kubernetes/default in complain mode.

PTAL

@dchen1107 dchen1107 added this to the v1.18 milestone Oct 15, 2019
@pjbgf pjbgf changed the title Add KEP to create seccomp built-in profiles and add complain mode [WIP] Add KEP to create seccomp built-in profiles and add complain mode Oct 31, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2019
@pjbgf
Copy link
Member Author

pjbgf commented Oct 31, 2019

I am currently reviewing this KEP, so in the interest of time, I would suggest to put on hold any reviews until the next batch of changes are pushed through.

/hold

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 29, 2020
@pjbgf
Copy link
Member Author

pjbgf commented Jan 29, 2020

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 29, 2020
@palnabarun
Copy link
Member

/milestone clear

@pjbgf
Copy link
Member Author

pjbgf commented May 6, 2020

This KEP is being withdrawn in favour of the seccomp-operator, which will eventually bring similar features out of tree.

@pjbgf pjbgf closed this May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants