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

policy for seccomp-profile selection #4806

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Jan 7, 2020

Implement a policy for selecting a seccomp profile. In addition to the
default behaviour (default profile unless --security-opt seccomp is set)
add a second policy doing a lookup in the image annotation.

If the image has the "io.containers.seccomp.profile" set its value will be
interpreted as a seccomp profile. The policy can be selected via the
new --seccomp-policy CLI flag.

Once the containers.conf support is merged into libpod, we can add an
option there as well.

Note that this feature is marked as experimental and may change in the
future.

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@openshift-ci-robot openshift-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 Jan 7, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Jan 7, 2020
@vrothberg
Copy link
Member Author

Note that tests are still missing. I will push an image to Quay that we can use for testing.

Rename `data` to `imageData` to make it more obvious which kind of data
the variable refers to.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg vrothberg force-pushed the seccomp branch 2 times, most recently from ae7fc1b to 3e3ccb7 Compare January 9, 2020 11:34
@vrothberg vrothberg changed the title WIP - policy for seccomp-profile selection policy for seccomp-profile selection Jan 9, 2020
@openshift-ci-robot openshift-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 Jan 9, 2020
@vrothberg
Copy link
Member Author

@rhatdan @mheon PTAL for an initial discussion.

@vrothberg
Copy link
Member Author

Still need to add tests. @baude gave me access to the libpod project on Quay where I'll push an image with a seccomp profile.

@vrothberg vrothberg added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020
@openshift-ci-robot openshift-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 Jan 9, 2020
@vrothberg
Copy link
Member Author

Now with tests

@vrothberg
Copy link
Member Author

@vbatts WDYT?

pkg/spec/createconfig.go Outdated Show resolved Hide resolved
if policy, err := cc.LookupSeccompPolicy(c.String("seccomp-policy")); err != nil {
return nil, err
} else {
secConfig.SeccompPolicy = policy
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is actually consumed - where are we trying to decode/use the image seccomp annotation

Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing that in pkg/spec.getSeccompConfig

@vrothberg vrothberg force-pushed the seccomp branch 2 times, most recently from 5c4f795 to 155a0f3 Compare January 9, 2020 15:46
@vrothberg
Copy link
Member Author

Dang. We can't use Quay as docker v1 doesn't support annotations -.-

Implement a policy for selecting a seccomp profile.  In addition to the
default behaviour (default profile unless --security-opt seccomp is set)
add a second policy doing a lookup in the image annotation.

If the image has the "io.containers.seccomp.profile" set its value will be
interpreted as a seccomp profile.  The policy can be selected via the
new --seccomp-policy CLI flag.

Once the containers.conf support is merged into libpod, we can add an
option there as well.

Note that this feature is marked as experimental and may change in the
future.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg
Copy link
Member Author

The images are now at docker.io/libpod/alpine-with.... Note that Docker Hub still has bugs with OCI images. I first had to push the images in docker s2v2 to properly create the repository/namepsace and then pushed the OCI images.

@vbatts
Copy link
Collaborator

vbatts commented Jan 9, 2020

Dang. We can't use Quay as docker v1 doesn't support annotations -.-

you can request your repo on quay to be switched to v2.s2

}
if policy, err := cc.LookupSeccompPolicy(c.String("seccomp-policy")); err != nil {
return nil, err
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As there is a return inside the if clause, you could happy path this.

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2020

I believe we could do this by default, iff we could determine the seccomp file specified in the image has less access then the default policy. Then we don't need the user to be involved in the decision. In my opinion, if the user has to be involved, then no one will use it. I could see the developer of the image going and specifying a locked down seccomp filter.

The problem is whether or not the image should be trusted to specify its own security constraints. If the image can specify limited seccomp rules, that are provable more secure, then we could use by default. For that matter we could also add annotations to the image to specify the list of capabiltiies required by the container. Which would be even easier for the developer to access, then the container engine compares the list of capabilities to its default list and runs the container with less or fails to run if the image requires more caps, then the caller specified....

@vrothberg
Copy link
Member Author

I believe we could do this by default, iff we could determine the seccomp file specified in the image has less access then the default policy. Then we don't need the user to be involved in the decision. In my opinion, if the user has to be involved, then no one will use it. I could see the developer of the image going and specifying a locked down seccomp filter.

The problem is whether or not the image should be trusted to specify its own security constraints. If the image can specify limited seccomp rules, that are provable more secure, then we could use by default. For that matter we could also add annotations to the image to specify the list of capabiltiies required by the container. Which would be even easier for the developer to access, then the container engine compares the list of capabilities to its default list and runs the container with less or fails to run if the image requires more caps, then the caller specified....

Doing that for capabilities is much easier as it boils down to diff slices of strings. Doing that for seccomp files is much harder where we'll soon run into difficult choices:

  • What if the filters are conditional (e.g., based on arguments or even on certain values of certain arguments)?

Such choices might be answered differently depending on the person or use-case etc. Hence, I feel rather objective to implement comparing the seccomp profiles.

I believe we could do this by default, iff we could determine the seccomp file specified in the image has less access then the default policy.

I don't think we can do this reliably (see comments above). I believe that a policy such as suggested in this PR is the way forward. It somehow boils down to the questions whether we can trust an image. If we are concerned that an image could open the fence via a less secure seccomp profile, then we should check and inspect the entire image in any case. For instance, an image might specify volumes and send them to a remote server etc.

To give a different perspective/use case: Currently, seccomp profiles in OpenShift/Kubernetes must reside on the host. With this PR (assuming we'll lift it to the K8s world) we can attach profiles to the image and make seccomp portable. Certainly, we should only do this when we trust the image. If we don't then there's a whole load of other things we must disable.

One use case might be to further tighten the security, another might be to explicitly allow certain syscalls which might be needed for a certain image. So we have opt-in AND opt-out use cases. In the opt-out case, we could have a much tighter default profile and use the image to opt-out. I think this is another example for not comparing seccomp profiles: How do we measure more or less secure?

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2020

Doing that for capabilities is much easier as it boils down to diff slices of strings. Doing that for seccomp files is much harder where we'll soon run into difficult choices:

Not sure I agree. If we run the hosts seccomp files and the images seccomp files through the same algorithm to fire off the filters and get the final list of syscalls available to the container. If image lists contains a syscall not listed in the host list, then container is blocked.

@vrothberg
Copy link
Member Author

vrothberg commented Jan 13, 2020

Not sure I agree. If we run the hosts seccomp files and the images seccomp files through the same algorithm to fire off the filters and get the final list of syscalls available to the container. If image lists contains a syscall not listed in the host list, then container is blocked.

We don't have a list of syscalls. We have a list of rules and those are conditional. An image might specify to only allow/block a syscall when an argument has a certain value. And that's where it's getting hairy as some might argue that a conditional blocking of an argument is safer or more dangerous or just needed for the sake of portability.

In the end, this PR allows to add more policies in the future. I would still vote the "default" policy to mean "default chosen by the podman". We can still add a "fallback" policy in the future.

@mheon
Copy link
Member

mheon commented Jan 13, 2020 via email

@vrothberg
Copy link
Member Author

What about calls that block individual arguments? Whitelist vs blacklist? I remember looking into something similar (but composing the two profiles together instead) and abandoning it because it did not seem possible.

That's exactly my concern as well. We could drive some heuristics when comparing but I don't think that's user friendly. I feel objective to semantics that exceed an if-then-else in the docs.

@rhatdan
Copy link
Member

rhatdan commented Jan 13, 2020

I think there is a list of syscalls that end up being loaded into the kernel. This list is done by prepossessing all of the capabilities to get the final list. I think we could get pretty close to the correct list. Then we could look at syscalls that have specific fields, and force the user to override then, since it gets difficult. In the general case, I believe we could tell the difference.

@vrothberg
Copy link
Member Author

Alright. @rhatdan and I agreed to merge this PR as is but to also look into the idea to create a policy that only uses the image's seccomp profile if it doesn't allow syscalls that are blocked by the default profile.

@rhatdan
Copy link
Member

rhatdan commented Jan 15, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0aa9dba into containers:master Jan 15, 2020
@vrothberg vrothberg deleted the seccomp branch March 13, 2020 12:58
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants