Adds support for AppArmor #159
Conversation
mikebrow
changed the title
|
I'll try to get the dependency merged as soon as possible. |
|
Added apparmor dependency notes to the readme, install in travis, and buildtags for our runc build in the e2e test. |
|
Will take a look as soon as kubernetes/kubernetes#51167 is done. |
|
Noticed an issue with my rebase.. new code was added to filter security options when securityContext.GetPrivileged() was true. The simple rebase/merge missed that and so I was adding apparmor profile when privileged or not. Fixed. |
hack/install-deps.sh
Outdated
| git fetch --all | ||
| git checkout ${RUNC_VERSION} | ||
| make | ||
| make BUILDTAGS='seccomp apparmor' |
There was a problem hiding this comment.
We should make apparmor optional via an environment variable. Let's not build it by default, and only enable it in our travis.
I don't think people using selinux need apparmor built in. :)
There was a problem hiding this comment.
yeah was going to add the selinux option as well.
We should probably discuss this.
There was a problem hiding this comment.
Per discussion I'll add support for users overriding our default BUILDTAGS
pkg/server/container_create.go
Outdated
| // TODO(random-liu): [P2] Add seccomp. | ||
|
|
||
| // Set apparmor profile name, Note: trims default profile name prefix | ||
| appArmorProf := securityContext.GetApparmorProfile() |
There was a problem hiding this comment.
There are 3 possibilities:
- runtime/default
- localhost/<profile_name>
- empty
We should also handle 1). I'm fine with making the default as empty for now, but we should add a TODO to make the default apparmor profile configurable via a flag or config file. And if the profile is runtime/default, you may not want to write it into the spec.
There was a problem hiding this comment.
Yeah was interested to see what docker/default and/or runtime/default means in this context. Are we expected to define it or kubelet/cri? Wasn't sure just how much filtering we should do and how much to let pass through. But yes at a minimum a TODO is needed here. I was exploring how best to handle such default requests to a higher degree on the seccomp stuff.
There was a problem hiding this comment.
For runtime/default, we should have a default profile. For now, it could be empty.
But a TODO is needed here to make the default profile configurable from flag.
|
Maybe... we should discuss. For example, if they didn't want apparmor why was it used? Do we propose all CRI implementors act as filters of kublet requests? Or are we pass through? Do we suggest customers run two instances of cri-containerd one with apparmor enabled on a first sock and another with it off on another socket? Dunno I'd at least like to consider the options before we go the path of CRIO which seems more suited to a standalone client product than a container enabling interface. |
|
per discussion we'll see if we can determine if apparmor is supported by runc and if we can't we'll add a TODO for either a cri-containerd switch or go figure out how to add support in containerd/runc to figure out if it's supported. Easy enough to tell if it's supported by the platform.. not so easy to see if it's supported in runc without some PR work. Going with a TODO for now |
|
Waiting for an update to apimachinery https://github.com/kubernetes/apimachinery/issues/22 |
|
Hey @mikebrow I just merged some changes in containerd to help support apparmor and generate profiles as well based on the default that we have been using. |
|
Thx @crosbymichael I'll work that in. |
|
@mikebrow hope it helps |
|
Will take another look today. |
|
rebased .. testing |
|
@mikebrow nice and simple code LGTM |
pkg/server/container_create.go
Outdated
| // TODO (mikebrow): delete created apparmor default profile | ||
| specOpts = append(specOpts, apparmor.WithDefaultProfile(AppArmorDefaultProfileName)) | ||
| } | ||
| if appArmorProf != "" { |
There was a problem hiding this comment.
This means that both WithDefaultProfile and WithProfile will be called if the profile is RuntimeDefault.
Probably to use a switch to make it clear.
There was a problem hiding this comment.
yeah switch or else.. my bad
pkg/server/container_create.go
Outdated
| "github.com/kubernetes-incubator/cri-containerd/pkg/util" | ||
| ) | ||
|
|
||
| // ProfileNamePrefix is the prefix for loading profiles on a localhost. Eg. AppArmor localhost/profileName. |
There was a problem hiding this comment.
Please add comment for these constants, and move this comment to be in front of ProfileNamePrefix.
There was a problem hiding this comment.
And please make them private, don't think we want to expose them. :)
pkg/server/container_create.go
Outdated
| ProfileNamePrefix = "localhost/" | ||
| RuntimeDefault = "runtime/default" | ||
| AppArmorDefaultProfileName = "cri-containerd.apparmor.d" | ||
| AppArmorEnabled = true // TODO (mikebrow): make these apparmor defaults configurable |
There was a problem hiding this comment.
Let's always apply profile, if kubelet gives us an non-empty one, which is what we discussed, right?
There was a problem hiding this comment.
and I think we also wanted a switch to override.. but yea the default is on and we don't have a way to turn it off yet so if provided and it's non empty and until we implement a switch to override ...
| // Trim default profile name prefix | ||
| specOpts = append(specOpts, apparmor.WithProfile(strings.TrimPrefix(appArmorProf, ProfileNamePrefix))) | ||
| } | ||
| } |
There was a problem hiding this comment.
The behavior when profile is not specified is not clearly defined. I filed a Kubernetes issue for this: kubernetes/kubernetes#51746
I'm fine with not setting default profile for now, until it's clearly defined. Add TODO for it.
| ) | ||
|
|
||
| // ProfileNamePrefix is the prefix for loading profiles on a localhost. Eg. AppArmor localhost/profileName. | ||
| const ( |
There was a problem hiding this comment.
We should get these constants from kubernetes/kubernetes#51747.
Add TODO for it.
There was a problem hiding this comment.
the todo is 4 lines down...
There was a problem hiding this comment.
I mean we should get the first 2 constants from kubernetes package, which we haven't done yet.
| default: | ||
| // Trim default profile name prefix | ||
| specOpts = append(specOpts, apparmor.WithProfile(strings.TrimPrefix(appArmorProf, ProfileNamePrefix))) | ||
| specOpts = append(specOpts, apparmor.WithProfile(strings.TrimPrefix(appArmorProf, profileNamePrefix))) |
There was a problem hiding this comment.
if !strings.HasPrefix(appArmorProf, profileNamePrefix) {
return nil, fmt.Errorf("invalid apparmor profile %q", appArmorProf)
}
specOpts = append(specOpts, ...)There was a problem hiding this comment.
Seems overly restrictive but ok..
pkg/server/container_create.go
Outdated
| if appArmorProf != "" { | ||
| specOpts = append(specOpts, apparmor.WithDefaultProfile(appArmorDefaultProfileName)) | ||
| case "": | ||
| // TODO (mikebrow): clarify what we should do in the case where no apparmor profile was specified see kubernetes/kubernetes#51746 |
There was a problem hiding this comment.
This line is way too long. :P
There was a problem hiding this comment.
what's the proper length 80 chars? :-)
pkg/server/container_create.go
Outdated
| ProfileNamePrefix = "localhost/" | ||
| RuntimeDefault = "runtime/default" | ||
| AppArmorDefaultProfileName = "cri-containerd.apparmor.d" | ||
| AppArmorEnabled = true // TODO (mikebrow): make these apparmor defaults configurable |
There was a problem hiding this comment.
Please add TODO:
TODO(mikebrow): Get the following 2 constant strings from CRI kubernetes/kubernetes#51747.
|
LGTM with final nits. After comments addressed, and run node e2e apparmor test, I think we are good to go. |
mikebrow
changed the title
|
comments addressed and e2e node passes after running |
|
Kube staging has been pushed, so I went ahead and moved from STTS branches back to the original sources. |
|
@mikebrow LGTM. Could you cleanup your commits a bit? And then good to go. |
|
ok commits are squashed.. |
|
Will merge after test passes. |
|
Just about to merge, and found a conflict...Please rebase. |
Signed-off-by: Mike Brown <[email protected]>
Signed-off-by: Mike Brown <[email protected]>
|
This will be the next one to merge after test passes. |
Adds support for AppArmor
4 participants
Adds/Enables AppArmor
Requires, the following commit or similar fix before AppArmor profile enabled containers will be "allowed" to be created through Kublet:
kubernetes/kubernetes@747933e
Signed-off-by: Mike Brown [email protected]