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

CRI: pass full seccomp profile to runtime #36997

Closed
feiskyer opened this issue Nov 17, 2016 · 24 comments
Closed

CRI: pass full seccomp profile to runtime #36997

feiskyer opened this issue Nov 17, 2016 · 24 comments
Assignees
Labels
Milestone

Comments

@feiskyer
Copy link
Member

@feiskyer feiskyer commented Nov 17, 2016

Seccomp profile is passed to runtime by annotations in CRI:

    // 2. Seccomp
    //
    //      key: security.alpha.kubernetes.io/seccomp/pod
    //      description: the seccomp profile for the containers of an entire pod.
    //      value: see below.
    //
    //      key: security.alpha.kubernetes.io/seccomp/container/<container name>
    //      description: the seccomp profile for the container (overides pod).
    //      values: see below
    //
    //      The value of seccomp is runtime agnostic:
    //      * runtime/default: the default profile for the container runtime
    //      * unconfined: unconfined profile, ie, no seccomp sandboxing
    //      * localhost/<profile-name>: the profile installed to the node's
    //        local seccomp profile root

localhost/<profile-name> is relative to node's local seccomp profile root, it is defined in kubelet. But runtime doesn't know it. We should pass full profile path in CRI.

cc/ @yujuhong @kubernetes/sig-node

Will file a PR to fix this later.

@grodrigues3
Copy link
Contributor

@grodrigues3 grodrigues3 commented Nov 17, 2016

@yujuhong Add support for Seccomp sounds like a new feature. The milestone should be reset to next-candidate, correct?

@yujuhong
Copy link
Member

@yujuhong yujuhong commented Nov 17, 2016

@grodrigues3 not really. The current implementation supports seccomp already. If it's missing critical information to function properly, this would be a bug.

@dims
Copy link
Member

@dims dims commented Nov 18, 2016

@yujuhong are we blocking releases for CRI in 1.5? i was not sure, so marking this as non-release-blocker and leaving 1.5 milestone. please correct if i am mistaken.

@euank
Copy link
Contributor

@euank euank commented Nov 18, 2016

I don't get why this is a 1.5 targetted thing myself.

Yes we want CRI to be in a good state in 1.5, but afaik we have zero intention of anyone actually using (at the very least because of the lack of exec auth regressing kubelet security).

@yujuhong why should this be part of 1.5?

@yujuhong
Copy link
Member

@yujuhong yujuhong commented Nov 18, 2016

Yes we want CRI to be in a good state in 1.5, but afaik we have zero intention of anyone actually using (at the very least because of the lack of exec auth regressing kubelet security).

First, this is a small enough change that allows non-docker integrations to properly support seccomp. Second, if "we have zero intention of anyone actually using" is a valid reason, we should never fix any bugs in CRI. We do want people to try using it (perhaps not in production yet). However, given that seccomp is only in alpha, I am ok with dropping this and just continue the docker support for now.

@euank
Copy link
Contributor

@euank euank commented Nov 18, 2016

this is a small enough change

Perhaps I misunderstood the meaning of code freeze, but I thought it was only bug-fixes regardless of size of change (or exceptions that have gone through some process)

Second, if "we have zero intention of anyone actually using" is a valid reason, we should never fix any bugs in CRI

What I meant is we don't intend this to be used generally as part of the 1.5 release. Because of that, I don't think we should consider bugs in CRI bugs for the purpose of the 1.5 release freeze. My statement is just that we shouldn't violate code-freeze for this. We should totally fix this right after the 1.5 freeze window ends, and saying that I'm arguing we should never fix anything is disingenuous and taking it to an absurd extreme.

@yujuhong
Copy link
Member

@yujuhong yujuhong commented Nov 18, 2016

@euank the only thing we don't seem to agree on is whether seccomp support was considered to be supported for CRI in 1.5. My interpretation is that we assumed seccomp would be supported until we found out that it wouldn't for other runtimes due to the lack of profile path. That's the only reason why I considered this a bug. Of course we could say that this is not really an important feature in CRI, so we can drop it, but we can be more clear on that.

@euank
Copy link
Contributor

@euank euank commented Nov 18, 2016

If there's a bug in a feature no one uses, does fixing it make a sound?

I think what we're not agreeing on is the role of CRI in 1.5.

I understood it as something that is not generally usable. We can't recommend a customer use it, even as an experimental feature, due to security aspects (if nothing else). Correct me if I'm wrong.

It's a developer integration point, and developers integrating with it are doing so with the kubernetes master branch, not with some release.

Those reasons together result in my opinion that CRI is still not really a customer/release feature, and thus bugs or features related to it should not be exceptions to the freeze window, but rather should targeted at master as soon as the freeze lifts.

If this were already post-freeze, would we be cherry-picking this over to 1.5?

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Nov 18, 2016

If there's a bug in a feature no one uses, does fixing it make a sound?

@euank This is not true. cri-o is depending on this fix for apparmor support.

@euank
Copy link
Contributor

@euank euank commented Nov 18, 2016

@feiskyer is it depending on the 1.5 release branch directly, or on master?

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Nov 18, 2016

@euank I think it's better to make this in 1.5, so cri-o could work with first release with experimental CRI. It helps to encourage people to use CRI and find features misses or bugs.

@yujuhong
Copy link
Member

@yujuhong yujuhong commented Nov 18, 2016

Modifying the user-specified annotation is a little bit unsettling. I think adding a new annotation explicitly for the path would be better. Or, we should just add an explicit field for this.

I spoke to @euank and @timstclair separately. I still think this is a bug fix, but we may want to be more careful about the fix. Given the time constraint, let's just list this as a known issue and work on it for 1.6.

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Nov 19, 2016

@yujuhong OK, let's move this to next release and think carefully about seccomp feature besides this:

  • kubelet may just pass the profile as string via CRI; may assume the profile locate in a file in a known format and pass path to the runtime; or maybe some other solutions. We need to clearly define it.
  • define profile format clearly
k8s-github-robot pushed a commit that referenced this issue Nov 19, 2016
Automatic merge from submit-queue

CRI: address knows issues of seccomp

For #36997.

To move forward, we decided to list seccomp profile path problem as a know issue in CRI (see [here](#36997 (comment))).

cc/ @yujuhong @euank @kubernetes/sig-node
@saad-ali
Copy link
Member

@saad-ali saad-ali commented Nov 19, 2016

Please document known issues for 1.5 in #37134

@feiskyer feiskyer changed the title CRI: pass full seccomp profile path to runtime CRI: pass full seccomp profile to runtime Jan 5, 2017
@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Jan 5, 2017

(copied from #39128)

I did a little more investigation, and as far as I can tell a seccomp root and the associated file structure is a docker specific (invented here) concept.

If we wanted to make this container runtime agnostic, the Kubelet could convert from that json to protobuf structs which clearly specify the exact details.

I think this is attractive for a number of reasons. Most importantly, the current specification is relying on incidental communication of state via the filesystem (passing references to files over an API is basically always an anti-pattern).

There are also things, such as no-new-privileges, which interact in some way with certain seccomp options, and so it makes sense for the kubelet to resolve a seccomp specification fully itself prior to reaching out to the runtime so that the kubelet may make any additional decisions that must be made.

That would also reduce the number of annotations that have magic powers, which is always a good thing in my book.

And

@euank - I agree with you, but that's also not how seccomp works today. I'd like to be able to source seccomp (and AppArmor) profiles from ConfigMaps, and maybe at that point we could deprecate local profiles.

@euank @timstclair let's move here to discuss how to pass profiles to runtime.

Related with #39128.

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Jan 5, 2017

This is issue is related with two level APIs:

  1. In CRI, since we may also want to support profiles from ConfigMaps, I think we should pass full profile contents instead of path to runtime.
  2. This issues is also related with seccomp api in kube-apiserver, it is based on annotations today. We should define seccomp profile format clearly (see #39128) and decide whether to add struct types to pod spec or just continue the annotations way. I think it's more clear to add specific struct types to pod spec.
@jonboulle
Copy link
Contributor

@jonboulle jonboulle commented Jan 5, 2017

Agreed - the path stuff is really a crutch (and implementation-specific) - I think #39128 is the answer here.

@timstclair
Copy link

@timstclair timstclair commented Jan 6, 2017

In CRI, since we may also want to support profiles from ConfigMaps, I think we should pass full profile contents instead of path to runtime.

I don't disagree, but it's not strictly a requirement for supporting ConfigMaps (kubelet could create a file with the contents). An open question is whether to pass it as a raw string, or wait for #39128 and pass the structured data.

decide whether to add struct types to pod spec or just continue the annotations way

The annotations way is deprecated (#30819). I think the bigger question is where/how to put the data. E.g. structured data in the API, unstructured (string type) data in the API, reference to another object (e.g. ConfigMap). If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

@feiskyer
Copy link
Member Author

@feiskyer feiskyer commented Jan 6, 2017

The annotations way is deprecated (#30819).

Yes, already opened #39130 to replace annotations with typed structs.

I think the bigger question is where/how to put the data. E.g. structured data in the API, unstructured (string type) data in the API, reference to another object (e.g. ConfigMap). If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

I personally prefer a structured data type in both kubernetes API and CRI. @yujuhong @euank WDYT?

@timstclair
Copy link

@timstclair timstclair commented Jan 6, 2017

I personally prefer a structured data type in both kubernetes API and CRI. @yujuhong @euank WDYT?

A counter argument is that as the spec grows or new features are added, we need to keep updating the Kubernetes API to maintain parity, or update the code in each CRI implementation to translate the Kubernetes spec. OTOH, if we use unstructured data we can use 3rd party libraries to parse out the data we need (assuming the spec evolves in a backwards compatible way).

@jonboulle
Copy link
Contributor

@jonboulle jonboulle commented Jan 9, 2017

A counter argument is that as the spec grows or new features are added,

What spec?

@timstclair
Copy link

@timstclair timstclair commented Jan 9, 2017

Whichever seccomp profile spec is being implemented by runtimes. E.g. if we decide to base the Kubernetes seccomp profile API off the OCI seccomp profile spec, and OCI adds some new features, at some point we'd probably want to add those features to Kubernetes. It also means that if different runtimes support different feature sets, we'll be stuck either tracking only the common features, or specifying features that may be ignored by the runtime. This may be a non-issue if the seccomp feature set rarely changes.

@jonboulle
Copy link
Contributor

@jonboulle jonboulle commented Jan 9, 2017

Well, yes, but that applies to everything in the CRI, and is one of the tradeoffs we ma[d|k]e by introducing this abstraction in the first place.

@pmorie
Copy link
Member

@pmorie pmorie commented May 27, 2017

My $0.02:

Moving the existing logic/information for seccomp into a first-class field for CRI (as in #46332) is productive because of the known problems with the annotation approach.

My initial proposal for seccomp support included a resource similar to what @jessfraz has proposed in kubernetes/community#660 and we were unable to come to an agreement on the issue @timstclair refers to here: #36997 (comment)

I do not necessarily expect that we will be able to come to an agreement about the seccomp resource in time for 1.7, so I think we should proceed with #46332 and do backward compatible CRI changes later if we need to inject the content of the seccomp profile instead of the path.

If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

I think we would deeply regret embedding API surface to describe seccomp profiles in the Pod API :)

@dchen1107 dchen1107 modified the milestones: v1.8, v1.7 Jun 12, 2017
k8s-github-robot pushed a commit that referenced this issue Jul 17, 2017
Automatic merge from submit-queue

Kubelet CRI: move seccomp from annotations to security context

**What this PR does / why we need it**:

This is the final step for #39130, which moves seccomp from annotations to linux container security context. And it also fixes #36997 by set the full seccomp profile path for node-installed profiles.

Note it doesn't include spec the seccomp profile format, which should be addressed at #39128. And a following PR is required for implementing in kuberuntime and dockershim.

**Which issue this PR fixes** 

Fixes #39130
Fixes #36997

**Special notes for your reviewer**:

**Release note**:
```release-note
Kubelet CRI: move seccomp from annotations to security context.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants