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: Security Context #6287

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@pweil-
Member

pweil- commented Apr 1, 2015

first run at types and kubelet integration

/cc @smarterclayton @thockin @pmorie @liggitt

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Apr 1, 2015

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

googlebot commented Apr 1, 2015

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Apr 1, 2015

Show outdated Hide outdated pkg/api/types.go
AllowPrivileged bool `json:"allowPrivileged,omitempty"`
// AllowedVolumeTypes lists the types of volumes that a container can bind
AllowedVolumeTypes []string `json:"allowVolumeTypes,omitempty"`

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

What is this for?

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

What is this for?

Show outdated Hide outdated pkg/api/types.go
// SecurityContext specifies the security constraints associated with a service account
type SecurityContext struct {
TypeMeta `json:",inline"`
ObjectMeta `json:"metadata,omitempty"`

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

This is not an ObjectMeta. There are two security context structs in play - one that would be part of a Container definition (or maybe a pod) and one that would be part of a Service Account. There is no independent security context object.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

This is not an ObjectMeta. There are two security context structs in play - one that would be part of a Container definition (or maybe a pod) and one that would be part of a Service Account. There is no independent security context object.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Please split this into two objects - one that is what the pod uses (the security context the pod should run in) and one that is a declarative "this is the settings that are allowed". I'll hold comments until you make that separation.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Please split this into two objects - one that is what the pod uses (the security context the pod should run in) and one that is a declarative "this is the settings that are allowed". I'll hold comments until you make that separation.

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 1, 2015

Member

@smarterclayton - removed the types and added a placeholder for a spec attached to the pod so I can focus on the kubelet side. Removed the AllowedVolumeTypes

Member

pweil- commented Apr 1, 2015

@smarterclayton - removed the types and added a placeholder for a spec attached to the pod so I can focus on the kubelet side. Removed the AllowedVolumeTypes

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 1, 2015

Member

Update: added some basic types back after discussing with Cesar, wired a simple flag --default_sc_provider=true to enable via command line, added some logging statements to make sure the provider was reached for my own quick testing.

Member

pweil- commented Apr 1, 2015

Update: added some basic types back after discussing with Cesar, wired a simple flag --default_sc_provider=true to enable via command line, added some logging statements to make sure the provider was reached for my own quick testing.

Show outdated Hide outdated pkg/api/types.go
// SecurityContextSpec holds security configuration that will be applied to the pod's containers
type SecurityContextSpec struct {
TypeMeta `json:",inline"`
ObjectMeta `json:"metadata,omitempty"`

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Still shouldn't have object meta.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Still shouldn't have object meta.

This comment has been minimized.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

Nor typemeta.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

Nor typemeta.

This comment has been minimized.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

Also, pkg/api/types.go objects should never be serialized, so the versioned APIs would also need to be updated.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

Also, pkg/api/types.go objects should never be serialized, so the versioned APIs would also need to be updated.

This comment has been minimized.

@pweil-

pweil- Apr 2, 2015

Member

Understood. I wanted to get some confirmation on the types, which was good since they were all wrong :) Once those are done I'll make sure all the types, fuzzer, and conversion updates are correct.

@pweil-

pweil- Apr 2, 2015

Member

Understood. I wanted to get some confirmation on the types, which was good since they were all wrong :) Once those are done I'll make sure all the types, fuzzer, and conversion updates are correct.

Show outdated Hide outdated pkg/securitycontext/types.go
}
// SecurityContext specifies the security constraints associated with a service account
type SecurityContext struct {

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

None of these should be here - they should still be in the API. The point was that there is a separate API object which defines what you can do from the object that defines what you should have.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

None of these should be here - they should still be in the API. The point was that there is a separate API object which defines what you can do from the object that defines what you should have.

This comment has been minimized.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

How are you planning to deliver this to Kubelet? It seems like it should work like pods: Kubelet watches apiserver but could also read the config from local files.

@bgrant0607

bgrant0607 Apr 2, 2015

Member

How are you planning to deliver this to Kubelet? It seems like it should work like pods: Kubelet watches apiserver but could also read the config from local files.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 2, 2015

Contributor

I think so - for step one we wanted to demonstrate the pod side and enforcement. Step two would be apiserver enforcement. Step three would be integration with service account.

The mental model I had was like resources - just like there might be an auto sizing resource initializer, there would be an auto enforcing security context initializer (or in the short term, admission controller). Kubelet might choose to not start a pod until security context was set on each container.

The kubelet policy and the apiserver imposed policy would have strong parallels, perhaps be representable the same way.

On Apr 1, 2015, at 10:37 PM, Brian Grant notifications@github.com wrote:

In pkg/securitycontext/types.go:

  • // The security context provider can make changes to the Config with which
  • // the container is created.
  • // An error is returned if it's not possible to secure the container as
  • // requested with a security context.
  • ModifyContainerConfig(pod *api.Pod, container *api.Container, config *docker.Config) error
  • // ModifyHostConfig is called before the Docker runContainer call.
  • // The security context provider can make changes to the HostConfig, affecting
  • // security options, whether the container is privileged, volume binds, etc.
  • // An error is returned if it's not possible to secure the container as requested
  • // with a security context.
  • ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *docker.HostConfig)
    +}

+// SecurityContext specifies the security constraints associated with a service account
+type SecurityContext struct {
How are you planning to deliver this to Kubelet? It seems like it should work like pods: Kubelet watches apiserver but could also read the config from local files.


Reply to this email directly or view it on GitHub.

@smarterclayton

smarterclayton Apr 2, 2015

Contributor

I think so - for step one we wanted to demonstrate the pod side and enforcement. Step two would be apiserver enforcement. Step three would be integration with service account.

The mental model I had was like resources - just like there might be an auto sizing resource initializer, there would be an auto enforcing security context initializer (or in the short term, admission controller). Kubelet might choose to not start a pod until security context was set on each container.

The kubelet policy and the apiserver imposed policy would have strong parallels, perhaps be representable the same way.

On Apr 1, 2015, at 10:37 PM, Brian Grant notifications@github.com wrote:

In pkg/securitycontext/types.go:

  • // The security context provider can make changes to the Config with which
  • // the container is created.
  • // An error is returned if it's not possible to secure the container as
  • // requested with a security context.
  • ModifyContainerConfig(pod *api.Pod, container *api.Container, config *docker.Config) error
  • // ModifyHostConfig is called before the Docker runContainer call.
  • // The security context provider can make changes to the HostConfig, affecting
  • // security options, whether the container is privileged, volume binds, etc.
  • // An error is returned if it's not possible to secure the container as requested
  • // with a security context.
  • ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *docker.HostConfig)
    +}

+// SecurityContext specifies the security constraints associated with a service account
+type SecurityContext struct {
How are you planning to deliver this to Kubelet? It seems like it should work like pods: Kubelet watches apiserver but could also read the config from local files.


Reply to this email directly or view it on GitHub.

Show outdated Hide outdated pkg/api/types.go
// UserIDMapping is a request to have user ids from X through Y to be mapped to
// the container. For example, map a host user to UID 1000 in the container or
// map n host users to 1000-n in the container
UserIDMappings []UserIDMappingRange `json:"userIDMappings,omitempty"`

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

We shouldn't add userIDMappings yet because user namespaces aren't even implemented in Docker.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

We shouldn't add userIDMappings yet because user namespaces aren't even implemented in Docker.

Show outdated Hide outdated pkg/api/types.go
// Capabilities represents the caps to add or remove from all containers in the pod
Capabilities Capabilities `json:"capabilities,omitempty"`

This comment has been minimized.

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Containers (not pods) have a security context. The context is declarative:

  • I want to run as user ID 6
  • I want to run as group ID 10
  • I want to run privileged
  • I want to add these capabilities
  • I want to drop these capabilities
  • I want to run with user namespaces in range mode using the UIDs from 2,810,000 to 2,820,000.
  • I want to run with these MCS labels for selinux

The security provider in the kubelet, and the security context that is associated with the service account, will say what the containers can do:

  • Containers under this security context can:
    • ask to be run as privileged
    • run as only this user and god
    • run as any user except root
    • must run as a random UID on the host
    • may ask for these capabilities
    • must run with these selinux labels
    • must run with a unique set of MCS labels on the host

One of these objects is part of the pod spec and is "do this"
One of these objects is used to constrain the pod spec - "reject pod specs that don't fit these bounds" or "default to these settings".

@smarterclayton

smarterclayton Apr 1, 2015

Contributor

Containers (not pods) have a security context. The context is declarative:

  • I want to run as user ID 6
  • I want to run as group ID 10
  • I want to run privileged
  • I want to add these capabilities
  • I want to drop these capabilities
  • I want to run with user namespaces in range mode using the UIDs from 2,810,000 to 2,820,000.
  • I want to run with these MCS labels for selinux

The security provider in the kubelet, and the security context that is associated with the service account, will say what the containers can do:

  • Containers under this security context can:
    • ask to be run as privileged
    • run as only this user and god
    • run as any user except root
    • must run as a random UID on the host
    • may ask for these capabilities
    • must run with these selinux labels
    • must run with a unique set of MCS labels on the host

One of these objects is part of the pod spec and is "do this"
One of these objects is used to constrain the pod spec - "reject pod specs that don't fit these bounds" or "default to these settings".

Show outdated Hide outdated pkg/securitycontext/types.go
}
// ContainerIsolationSpec indicates intent for container isolation
type ContainerIsolationSpec struct {

This comment has been minimized.

@pmorie

pmorie Apr 1, 2015

Member

@smarterclayton @pweil- is ContainerIsolationSpec or IDMapping the right place to have a label attribute? We need more than just UID mapping to model MCS labels.

@pmorie

pmorie Apr 1, 2015

Member

@smarterclayton @pweil- is ContainerIsolationSpec or IDMapping the right place to have a label attribute? We need more than just UID mapping to model MCS labels.

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie Apr 1, 2015

Member

@smarterclayton @pweil- @thockin @erictune One thing that isn't spelled out by the current state of the security context proposal is how we will expose information to volume plugins that need to set the selinux context of files they create on disk. Volume plugins will need to know the right MCS label to set on mountpoints they make, since the mountpoints may not receive a usable SELinux context by default. As an example of this, tmpfs mounts by default do not receive a readable context relative to the container's selinux context; I have been working on a fix to #6238 and in that light came to the conclusion that we need to expand the proposal to account for this. The reason it is necessary to set the context during the mount operation is that there's a race condition if you do a chcon after the mount.

Any thoughts?

Member

pmorie commented Apr 1, 2015

@smarterclayton @pweil- @thockin @erictune One thing that isn't spelled out by the current state of the security context proposal is how we will expose information to volume plugins that need to set the selinux context of files they create on disk. Volume plugins will need to know the right MCS label to set on mountpoints they make, since the mountpoints may not receive a usable SELinux context by default. As an example of this, tmpfs mounts by default do not receive a readable context relative to the container's selinux context; I have been working on a fix to #6238 and in that light came to the conclusion that we need to expand the proposal to account for this. The reason it is necessary to set the context during the mount operation is that there's a race condition if you do a chcon after the mount.

Any thoughts?

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 2, 2015

Member

@pmorie I think the ideal solution is that the plugin(s) creating mounts are passed or are aware of the context prior to initiating the mount. I have not dug into the volume pieces too much but if they are aware of the pod then perhaps this is how they should discover the correct labels since the context on the pod will be what is actually applied.

Member

pweil- commented Apr 2, 2015

@pmorie I think the ideal solution is that the plugin(s) creating mounts are passed or are aware of the context prior to initiating the mount. I have not dug into the volume pieces too much but if they are aware of the pod then perhaps this is how they should discover the correct labels since the context on the pod will be what is actually applied.

@pmorie

This comment has been minimized.

Show comment
Hide comment
@pmorie

pmorie Apr 2, 2015

Member

@pweil- I think so too. The main challenge with that is determining where
to put the label information in the model, since the model is supposed to
be orthogonal to the context mechanism. Maybe it's sufficient to lump it
together with the rest of the selinux context info, and not have a distinct
API for label.
On Wed, Apr 1, 2015 at 8:57 PM Paul notifications@github.com wrote:

@pmorie https://github.com/pmorie I think the ideal solution is that
the plugin(s) creating mounts is passed or is aware of the context prior to
initiating the mount. I have not dug into the volume pieces too much but if
they are aware of the pod then perhaps this is how they should discover the
correct labels since the context on the pod will be what is actually
applied.


Reply to this email directly or view it on GitHub
#6287 (comment)
.

Member

pmorie commented Apr 2, 2015

@pweil- I think so too. The main challenge with that is determining where
to put the label information in the model, since the model is supposed to
be orthogonal to the context mechanism. Maybe it's sufficient to lump it
together with the rest of the selinux context info, and not have a distinct
API for label.
On Wed, Apr 1, 2015 at 8:57 PM Paul notifications@github.com wrote:

@pmorie https://github.com/pmorie I think the ideal solution is that
the plugin(s) creating mounts is passed or is aware of the context prior to
initiating the mount. I have not dug into the volume pieces too much but if
they are aware of the pod then perhaps this is how they should discover the
correct labels since the context on the pod will be what is actually
applied.


Reply to this email directly or view it on GitHub
#6287 (comment)
.

@brendandburns brendandburns added cla: yes and removed cla: no labels Apr 2, 2015

@googlebot googlebot added cla: no and removed cla: yes labels Apr 6, 2015

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 6, 2015

Member

@smarterclayton - ok, I've revised the types. There is now a SecurityContext reference on the Container which reflects what should actually be applied at runtime. There is also a SecurityConstraints object which reflects the administrative settings for a SecurityContextProvider. The SecurityConstraints object also has a reference to a SecurityContext object which would be the default context applied if a container spec doesn't specify anything.

The provider is fed to the kubelet and subsequently to the new DockerManager which asks the SecurityContextProvider to apply settings before the container is created and before running the container.

I'm not super thrilled with the name, suggestions welcome but I didn't want to get hung up on that if it still wasn't the right direction

@pmorie as discusses on #6379 I can use the label options to create the correct context to be applied to the tmpfs directory. If we want to separate those options out from what is applied to the container it should be easy enough. For now I didn't add a new VolumeOptions like I initially thought I would.

Member

pweil- commented Apr 6, 2015

@smarterclayton - ok, I've revised the types. There is now a SecurityContext reference on the Container which reflects what should actually be applied at runtime. There is also a SecurityConstraints object which reflects the administrative settings for a SecurityContextProvider. The SecurityConstraints object also has a reference to a SecurityContext object which would be the default context applied if a container spec doesn't specify anything.

The provider is fed to the kubelet and subsequently to the new DockerManager which asks the SecurityContextProvider to apply settings before the container is created and before running the container.

I'm not super thrilled with the name, suggestions welcome but I didn't want to get hung up on that if it still wasn't the right direction

@pmorie as discusses on #6379 I can use the label options to create the correct context to be applied to the tmpfs directory. If we want to separate those options out from what is applied to the container it should be easy enough. For now I didn't add a new VolumeOptions like I initially thought I would.

Show outdated Hide outdated pkg/api/types.go
// SELinuxOptions are the labels to be applied to the container
// and volumes
SELinuxOptions

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

If we someday wanted to support grsecurity, apparmour, and/or tomoyo, how do you envision the API growing?

@erictune

erictune Apr 6, 2015

Member

If we someday wanted to support grsecurity, apparmour, and/or tomoyo, how do you envision the API growing?

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

This is, admittedly, pretty specific so I would expect the API would have options that grew as other security facilities are supported much like how persistent disks must be implementation specific (https://github.com/markturansky/kubernetes/blob/pv_api/pkg/api/types.go#L177). After a quick discussion with @pmorie I decided to just go with it as is but if there is a good way to make some of this more generic I'll change it.

In particular, there is already an app armor profile flag supported by docker so we could already have a second AppArmorOptions.

@pweil-

pweil- Apr 6, 2015

Member

This is, admittedly, pretty specific so I would expect the API would have options that grew as other security facilities are supported much like how persistent disks must be implementation specific (https://github.com/markturansky/kubernetes/blob/pv_api/pkg/api/types.go#L177). After a quick discussion with @pmorie I decided to just go with it as is but if there is a good way to make some of this more generic I'll change it.

In particular, there is already an app armor profile flag supported by docker so we could already have a second AppArmorOptions.

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

Okay, that seems fine.

On Mon, Apr 6, 2015 at 1:07 PM, Paul notifications@github.com wrote:

In pkg/api/types.go
#6287 (comment)
:

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
  • // SELinuxOptions are the labels to be applied to the container
  • // and volumes
  • SELinuxOptions

This is, admittedly, pretty specific so I would expect the API would have
options that grew as other security facilities are supported much like how
persistent disks must be implementation specific (
https://github.com/markturansky/kubernetes/blob/pv_api/pkg/api/types.go#L177).
After a quick discussion with @pmorie https://github.com/pmorie I
decided to just go with it as is but if there is a good way to make some of
this more generic I'll change it.

In particular, there is already an app armor profile flag supported by
docker so we could already have a second AppArmorOptions.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27825405
.

@erictune

erictune Apr 6, 2015

Member

Okay, that seems fine.

On Mon, Apr 6, 2015 at 1:07 PM, Paul notifications@github.com wrote:

In pkg/api/types.go
#6287 (comment)
:

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
  • // SELinuxOptions are the labels to be applied to the container
  • // and volumes
  • SELinuxOptions

This is, admittedly, pretty specific so I would expect the API would have
options that grew as other security facilities are supported much like how
persistent disks must be implementation specific (
https://github.com/markturansky/kubernetes/blob/pv_api/pkg/api/types.go#L177).
After a quick discussion with @pmorie https://github.com/pmorie I
decided to just go with it as is but if there is a good way to make some of
this more generic I'll change it.

In particular, there is already an app armor profile flag supported by
docker so we could already have a second AppArmorOptions.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27825405
.

Show outdated Hide outdated pkg/api/types.go
type SecurityContext struct {
// Capabilities are the capabilities to add/drop when running the container
// TODO: will need to refactor this from the container spec to here
Capabilities Capabilities

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

How does this relate to the Container.Capabilities?

@erictune

erictune Apr 6, 2015

Member

How does this relate to the Container.Capabilities?

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

Okay, I see the TODO comment.

@erictune

erictune Apr 6, 2015

Member

Okay, I see the TODO comment.

Show outdated Hide outdated pkg/api/types.go
// SecurityConstraintPolicyPermissive means that any containers that do not meet policy constraints will be
// modified to meet the constraints (settings will be changed) before they are run
SecurityConstraintPolicyPermissive = "Permissive"

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

I can't imagine how this would be useful. Can you give a scenario?

@erictune

erictune Apr 6, 2015

Member

I can't imagine how this would be useful. Can you give a scenario?

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

I pictured this working on the enforcement side (kubelet/api server) by not outright rejecting a pod spec with disallowed configuration and just ignoring the requests for the blacklisted capabilities, label changes, etc. Reject/accept is probably better though and will prevent the inevitable "why doesn't my container work" support.

@pweil-

pweil- Apr 6, 2015

Member

I pictured this working on the enforcement side (kubelet/api server) by not outright rejecting a pod spec with disallowed configuration and just ignoring the requests for the blacklisted capabilities, label changes, etc. Reject/accept is probably better though and will prevent the inevitable "why doesn't my container work" support.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

On Apr 6, 2015, at 4:24 PM, Paul notifications@github.com wrote:

In pkg/api/types.go:

  • // AllowDisable --security-opt="label:disable"
  • AllowDisable bool
    +}

+// SecurityConstraintPolicy dictates how the security context provider should behave with regards to contexts that
+// do not meet the requirements of the policy.
+type SecurityConstraintPolicy string
+
+const (

  • // SecurityConstraintPolicyDisabled means that any containers that do not meet policy constraints are still
  • // allowed to run
  • SecurityConstraintPolicyDisabled = "Disabled"
  • // SecurityConstraintPolicyPermissive means that any containers that do not meet policy constraints will be
  • // modified to meet the constraints (settings will be changed) before they are run
  • SecurityConstraintPolicyPermissive = "Permissive"
    I pictured this working on the enforcement side (kubelet/api server) by not outright rejecting a pod spec with disallowed configuration and just ignoring the requests for the blacklisted capabilities, label changes, etc. Reject/accept is probably better though and will prevent the inevitable "why doesn't my container work" support.

It would help to list each of the fields and describe / find scenarios when the kubelet / apiserver would reject, silently ignore, or drop them based on policy settings. Example:

Uid, when set, should always be applied, because it's likely the user's image only runs with that uid.
Uid, when set, when the cluster policy is to run all pods as node unique uids, and user namespaces are not enabled, should be rejected by the apiserver because there is no way the cluster can satisfy it
Uid, when set, when the kubelet cluster policy is to run pods in user namespaces always, should be accepted by the cluster.

I suspect most of the options will have clear outcomes. Let's assume for the moment that security policy is imposed either per namespace (via service account), globally at the api server, or only at the kubelet.


Reply to this email directly or view it on GitHub.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

On Apr 6, 2015, at 4:24 PM, Paul notifications@github.com wrote:

In pkg/api/types.go:

  • // AllowDisable --security-opt="label:disable"
  • AllowDisable bool
    +}

+// SecurityConstraintPolicy dictates how the security context provider should behave with regards to contexts that
+// do not meet the requirements of the policy.
+type SecurityConstraintPolicy string
+
+const (

  • // SecurityConstraintPolicyDisabled means that any containers that do not meet policy constraints are still
  • // allowed to run
  • SecurityConstraintPolicyDisabled = "Disabled"
  • // SecurityConstraintPolicyPermissive means that any containers that do not meet policy constraints will be
  • // modified to meet the constraints (settings will be changed) before they are run
  • SecurityConstraintPolicyPermissive = "Permissive"
    I pictured this working on the enforcement side (kubelet/api server) by not outright rejecting a pod spec with disallowed configuration and just ignoring the requests for the blacklisted capabilities, label changes, etc. Reject/accept is probably better though and will prevent the inevitable "why doesn't my container work" support.

It would help to list each of the fields and describe / find scenarios when the kubelet / apiserver would reject, silently ignore, or drop them based on policy settings. Example:

Uid, when set, should always be applied, because it's likely the user's image only runs with that uid.
Uid, when set, when the cluster policy is to run all pods as node unique uids, and user namespaces are not enabled, should be rejected by the apiserver because there is no way the cluster can satisfy it
Uid, when set, when the kubelet cluster policy is to run pods in user namespaces always, should be accepted by the cluster.

I suspect most of the options will have clear outcomes. Let's assume for the moment that security policy is imposed either per namespace (via service account), globally at the api server, or only at the kubelet.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@pweil-

pweil- Apr 7, 2015

Member

I think we're in a situation where they all read like this if a policy is broken and we operate in a "give it a try anyway" permissive mode:

  1. api server: logs warning about broken policy, still accepts update (possibly manipulates applied context here based on the default context provided in SecurityConstraints)
  2. kubelet: removes offending policy or sets to default (if api server doesn't). Container may function correctly but probably not.

To keep it boxed to what we can currently do today with docker we're talking about something like this:

  1. Add/remove caps
  • cap request would be removed
  • any missing required caps (based on policy) would be added - note this is not shown in the current proposed types. I would need to add a required cap list
  1. Set SELinux labels (user, role, type, level, disabled)
  • label request would be removed
  • default label policy would be applied if set
  1. Set AppArmor profile
  • profile request would be removed
  • profile policy would be applied if set
  1. Run as privileged container
  • priv set to false

Requests to drop capabilities seem pretty benign, no issues there. Specific requests for privs beyond the default docker capabilities, labels, or app armor profile are likely to cause issues if rejected or replaced.

@pweil-

pweil- Apr 7, 2015

Member

I think we're in a situation where they all read like this if a policy is broken and we operate in a "give it a try anyway" permissive mode:

  1. api server: logs warning about broken policy, still accepts update (possibly manipulates applied context here based on the default context provided in SecurityConstraints)
  2. kubelet: removes offending policy or sets to default (if api server doesn't). Container may function correctly but probably not.

To keep it boxed to what we can currently do today with docker we're talking about something like this:

  1. Add/remove caps
  • cap request would be removed
  • any missing required caps (based on policy) would be added - note this is not shown in the current proposed types. I would need to add a required cap list
  1. Set SELinux labels (user, role, type, level, disabled)
  • label request would be removed
  • default label policy would be applied if set
  1. Set AppArmor profile
  • profile request would be removed
  • profile policy would be applied if set
  1. Run as privileged container
  • priv set to false

Requests to drop capabilities seem pretty benign, no issues there. Specific requests for privs beyond the default docker capabilities, labels, or app armor profile are likely to cause issues if rejected or replaced.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I don't think of "Permissive" as normally "change the incoming request". I think of it as "allow users to do anything". So at a minimum I think the name should be changed - SecurityConstraintPolicyRestrict - Restricts running pods to run within this policy.

Disabled seems like a temporary thing, not that common. An admin might disable the policy on a namespace while a user tests something.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I don't think of "Permissive" as normally "change the incoming request". I think of it as "allow users to do anything". So at a minimum I think the name should be changed - SecurityConstraintPolicyRestrict - Restricts running pods to run within this policy.

Disabled seems like a temporary thing, not that common. An admin might disable the policy on a namespace while a user tests something.

This comment has been minimized.

@pweil-

pweil- Apr 7, 2015

Member

Good idea. Restrict rather than permissive sounds a lot better if we want to go the route of accepting the request but manipulating it to fit policy.

@pweil-

pweil- Apr 7, 2015

Member

Good idea. Restrict rather than permissive sounds a lot better if we want to go the route of accepting the request but manipulating it to fit policy.

This comment has been minimized.

@pmorie

pmorie Apr 7, 2015

Member

@pweil-, @smarterclayton So there will not be anything like SELinux permissive (check, don't enforce but log), right? 'Restrict' is the logical opposite of 'permissive'.

@pmorie

pmorie Apr 7, 2015

Member

@pweil-, @smarterclayton So there will not be anything like SELinux permissive (check, don't enforce but log), right? 'Restrict' is the logical opposite of 'permissive'.

This comment has been minimized.

@pweil-

pweil- Apr 7, 2015

Member

At this point the definition of permissive/restrict in this code is: check against defined policy, manipulate to fit within constraints. So, no, there is no explicit permissive although we could make disabled work like SELinux permissive (check, warn, still run).

To me the question is still: is there a valid use case for the restrict option or is it just going to cause issues down the road? I think it is likely that if a container is requesting escalated privileges then it needs them to run correctly.

@pweil-

pweil- Apr 7, 2015

Member

At this point the definition of permissive/restrict in this code is: check against defined policy, manipulate to fit within constraints. So, no, there is no explicit permissive although we could make disabled work like SELinux permissive (check, warn, still run).

To me the question is still: is there a valid use case for the restrict option or is it just going to cause issues down the road? I think it is likely that if a container is requesting escalated privileges then it needs them to run correctly.

This comment has been minimized.

@pmorie

pmorie Apr 7, 2015

Member

@pweil- I agree, unless we have a definite use-case for this, I think we should drop it from this PR.

@pmorie

pmorie Apr 7, 2015

Member

@pweil- I agree, unless we have a definite use-case for this, I think we should drop it from this PR.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I agree.

----- Original Message -----

  • // AllowDisable --security-opt="label:disable"
  • AllowDisable bool
    +}

+// SecurityConstraintPolicy dictates how the security context provider
should behave with regards to contexts that
+// do not meet the requirements of the policy.
+type SecurityConstraintPolicy string
+
+const (

  • // SecurityConstraintPolicyDisabled means that any containers that do not
    meet policy constraints are still
  • // allowed to run
  • SecurityConstraintPolicyDisabled = "Disabled"
  • // SecurityConstraintPolicyPermissive means that any containers that do
    not meet policy constraints will be
  • // modified to meet the constraints (settings will be changed) before
    they are run
  • SecurityConstraintPolicyPermissive = "Permissive"

@pweil- I agree, unless we have a definite use-case for this, I think we
should drop it from this PR.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27902479

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I agree.

----- Original Message -----

  • // AllowDisable --security-opt="label:disable"
  • AllowDisable bool
    +}

+// SecurityConstraintPolicy dictates how the security context provider
should behave with regards to contexts that
+// do not meet the requirements of the policy.
+type SecurityConstraintPolicy string
+
+const (

  • // SecurityConstraintPolicyDisabled means that any containers that do not
    meet policy constraints are still
  • // allowed to run
  • SecurityConstraintPolicyDisabled = "Disabled"
  • // SecurityConstraintPolicyPermissive means that any containers that do
    not meet policy constraints will be
  • // modified to meet the constraints (settings will be changed) before
    they are run
  • SecurityConstraintPolicyPermissive = "Permissive"

@pweil- I agree, unless we have a definite use-case for this, I think we
should drop it from this PR.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27902479

Show outdated Hide outdated pkg/api/types.go
const (
// SecurityConstraintPolicyDisabled means that any containers that do not meet policy constraints are still
// allowed to run
SecurityConstraintPolicyDisabled = "Disabled"

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

How does this differ from not having a SecurityProvider?

@erictune

erictune Apr 6, 2015

Member

How does this differ from not having a SecurityProvider?

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

It doesn't. I pictured it being a way to turn off enforcement at run time, perhaps with some logging about items that break policy.

@pweil-

pweil- Apr 6, 2015

Member

It doesn't. I pictured it being a way to turn off enforcement at run time, perhaps with some logging about items that break policy.

@@ -1664,6 +1667,104 @@ type SecretList struct {
Items []Secret `json:"items"`
}
// SecurityContext holds security configuration that will be applied to a container. If a security context
// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

Which component is going to read this object? Kubelet?

@erictune

erictune Apr 6, 2015

Member

Which component is going to read this object? Kubelet?

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

The question was about which component is going to read SecurityConstraints.

@erictune

erictune Apr 6, 2015

Member

The question was about which component is going to read SecurityConstraints.

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

Maybe SecurityConstraints and its dependencies are part of the kubelet API but not necessarily part of the core kubenetes API? Take a look at how scheduler has its own API:
plugin/pkg/scheduler/api/...

SecurityContext seems like it is part of the core API since Pods use it.

@erictune

erictune Apr 6, 2015

Member

Maybe SecurityConstraints and its dependencies are part of the kubelet API but not necessarily part of the core kubenetes API? Take a look at how scheduler has its own API:
plugin/pkg/scheduler/api/...

SecurityContext seems like it is part of the core API since Pods use it.

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

What we've discussed so far was that enforcement of constraints by the SecurityPolicyProvider can belong on two levels: api server and kubelet. On both sides the policy can allow or disallow containers that meet the constraints to run either by outright preventing the docker run (kubelet) or rejecting the pod create/mod (api server). It is likely that the policy would be represented the same on both sides, I haven't thought of a scenario where they would differ.

#6287 (comment)

@pweil-

pweil- Apr 6, 2015

Member

What we've discussed so far was that enforcement of constraints by the SecurityPolicyProvider can belong on two levels: api server and kubelet. On both sides the policy can allow or disallow containers that meet the constraints to run either by outright preventing the docker run (kubelet) or rejecting the pod create/mod (api server). It is likely that the policy would be represented the same on both sides, I haven't thought of a scenario where they would differ.

#6287 (comment)

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

Apologies, poor pronoun use. we := myself and @smarterclayton discussed in the link provided above.

@pweil-

pweil- Apr 6, 2015

Member

Apologies, poor pronoun use. we := myself and @smarterclayton discussed in the link provided above.

// ensure that applied SecurityContext requests follow. When a setting is provided in the SecurityConstraints
// that conflicts with an actual request it will be implementation specific whether that request is
// ignored and the container is still run (without the requested constraint) or if the container will be failed
type SecurityConstraints struct {

This comment has been minimized.

@erictune

erictune Apr 6, 2015

Member

Are you expecting that there is one SecurityConstraints for the whole cluster, or per namespace, or per user or what?

@erictune

erictune Apr 6, 2015

Member

Are you expecting that there is one SecurityConstraints for the whole cluster, or per namespace, or per user or what?

This comment has been minimized.

@pweil-

pweil- Apr 6, 2015

Member

These would eventually be scoped to a ServiceAccount.

@pweil-

pweil- Apr 6, 2015

Member

These would eventually be scoped to a ServiceAccount.

@erictune

This comment has been minimized.

Show comment
Hide comment
@erictune

erictune Apr 6, 2015

Member

All the individual bits mostly make sense in isolation. Just trying to see how this fits together and how it will evolve. Some questions above.

Member

erictune commented Apr 6, 2015

All the individual bits mostly make sense in isolation. Just trying to see how this fits together and how it will evolve. Some questions above.

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 7, 2015

Member

Updated to remove the unnecessary policy. Broke this up into two commits for easier digestion. The second commit is an example of the kubelet side enforcement of the policy by:

  1. have SecurityContextProvider apply the default SecurityContext if the pod does not have one set
  2. validate the pod's SecurityContext complies with the policy restrictions in kubelet.canRunPod
  3. integration with the DockerManager to have the SecurityPolicy apply the security context to the docker config and host config (demostrated with the privileged setting only for now)

The approach is basically to apply and validate before getting to the docker manager so once it gets to there we can just apply whatever is in the container.SecurityPolicy to the docker configs.

Member

pweil- commented Apr 7, 2015

Updated to remove the unnecessary policy. Broke this up into two commits for easier digestion. The second commit is an example of the kubelet side enforcement of the policy by:

  1. have SecurityContextProvider apply the default SecurityContext if the pod does not have one set
  2. validate the pod's SecurityContext complies with the policy restrictions in kubelet.canRunPod
  3. integration with the DockerManager to have the SecurityPolicy apply the security context to the docker config and host config (demostrated with the privileged setting only for now)

The approach is basically to apply and validate before getting to the docker manager so once it gets to there we can just apply whatever is in the container.SecurityPolicy to the docker configs.

Show outdated Hide outdated pkg/api/types.go
// Run the container in privileged mode
// TODO: will need to refactor this from the container spec to here
Privileged bool

This comment has been minimized.

@thockin

thockin Apr 7, 2015

Member

Moving this out of the container object starts to feel weird when you apply it to host networking and even weirder when you apply it to host ports. Is this the right pattern?

@thockin

thockin Apr 7, 2015

Member

Moving this out of the container object starts to feel weird when you apply it to host networking and even weirder when you apply it to host ports. Is this the right pattern?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

Agree Host Ports is weird.

Privileged though overlaps with capabilities, which overlaps with namespaces. It's possible that we could not have a grouping for many of these - I liked the idea of the serious security problems all being in one spot. But I'm not married to it.

----- Original Message -----

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a
container. If a security context
+// is set on the container spec then it must comply with any constraints
defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is
running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the
    container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Moving this out of the container object starts to feel weird when you apply
it to host networking and even weirder when you apply it to host ports. Is
this the right pattern?


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27928586

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

Agree Host Ports is weird.

Privileged though overlaps with capabilities, which overlaps with namespaces. It's possible that we could not have a grouping for many of these - I liked the idea of the serious security problems all being in one spot. But I'm not married to it.

----- Original Message -----

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a
container. If a security context
+// is set on the container spec then it must comply with any constraints
defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is
running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the
    container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Moving this out of the container object starts to feel weird when you apply
it to host networking and even weirder when you apply it to host ports. Is
this the right pattern?


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27928586

This comment has been minimized.

@thockin

thockin Apr 7, 2015

Member

I am NOT a security person, so I am just speaking from aesthetics. I like
the model that security settings enable other flags but do not replace
other flags. Especially when they go 1:N like HostPorts.

It's not clear what I would write, as a user, to enable HostPorts in this
externalized mode.

On Tue, Apr 7, 2015 at 4:02 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Agree Host Ports is weird. Privileged though overlaps with capabilities,
which overlaps with namespaces. It's possible that we could not have a
grouping for many of these - I liked the idea of the serious security
problems all being in one spot. But I'm not married to it.
… <#14c961ebe3c6ec68_>
----- Original Message -----

@@ -1712,6 +1714,103 @@ type SecretList struct { > Items []Secret
json:"items" > } > > +// SecurityContext holds security configuration
that will be applied to a > container. If a security context > +// is set
on the container spec then it must comply with any constraints > defined in
the SecurityConstraints context > +// it is running in. If a security
context is not supplied and the pod is > running under a
SecurityConstraints context > +// then a default SecurityContext may be
applied. > +type SecurityContext struct { > + // Capabilities are the
capabilities to add/drop when running the > container > + // TODO: will
need to refactor this from the container spec to here > + Capabilities
Capabilities > + > + // Run the container in privileged mode > + // TODO:
will need to refactor this from the container spec to here > + Privileged
bool Moving this out of the container object starts to feel weird when you
apply it to host networking and even weirder when you apply it to host
ports. Is this the right pattern? --- Reply to this email directly or view
it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27928586


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27929692
.

@thockin

thockin Apr 7, 2015

Member

I am NOT a security person, so I am just speaking from aesthetics. I like
the model that security settings enable other flags but do not replace
other flags. Especially when they go 1:N like HostPorts.

It's not clear what I would write, as a user, to enable HostPorts in this
externalized mode.

On Tue, Apr 7, 2015 at 4:02 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Agree Host Ports is weird. Privileged though overlaps with capabilities,
which overlaps with namespaces. It's possible that we could not have a
grouping for many of these - I liked the idea of the serious security
problems all being in one spot. But I'm not married to it.
… <#14c961ebe3c6ec68_>
----- Original Message -----

@@ -1712,6 +1714,103 @@ type SecretList struct { > Items []Secret
json:"items" > } > > +// SecurityContext holds security configuration
that will be applied to a > container. If a security context > +// is set
on the container spec then it must comply with any constraints > defined in
the SecurityConstraints context > +// it is running in. If a security
context is not supplied and the pod is > running under a
SecurityConstraints context > +// then a default SecurityContext may be
applied. > +type SecurityContext struct { > + // Capabilities are the
capabilities to add/drop when running the > container > + // TODO: will
need to refactor this from the container spec to here > + Capabilities
Capabilities > + > + // Run the container in privileged mode > + // TODO:
will need to refactor this from the container spec to here > + Privileged
bool Moving this out of the container object starts to feel weird when you
apply it to host networking and even weirder when you apply it to host
ports. Is this the right pattern? --- Reply to this email directly or view
it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27928586


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27929692
.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I think HostPorts would be enabled by the Kubelet. I would not expect many of these flags to conflict with each other, and where they did it's reasonable to remove the conflict. I think HostPorts could just be HostPorts.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

I think HostPorts would be enabled by the Kubelet. I would not expect many of these flags to conflict with each other, and where they did it's reasonable to remove the conflict. I think HostPorts could just be HostPorts.

This comment has been minimized.

@thockin

thockin Apr 7, 2015

Member

The question is whether ANYONE can create host ports or just with special
permissions? I really want us to steer people AWAY from bad patterns.

On Tue, Apr 7, 2015 at 4:39 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

I think HostPorts would be enabled by the Kubelet. I would not expect many
of these flags to conflict with each other, and where they did it's
reasonable to remove the conflict. I think HostPorts could just be
HostPorts.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27931610
.

@thockin

thockin Apr 7, 2015

Member

The question is whether ANYONE can create host ports or just with special
permissions? I really want us to steer people AWAY from bad patterns.

On Tue, Apr 7, 2015 at 4:39 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

I think HostPorts would be enabled by the Kubelet. I would not expect many
of these flags to conflict with each other, and where they did it's
reasonable to remove the conflict. I think HostPorts could just be
HostPorts.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27931610
.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 7, 2015

Contributor

Technically that would be the policy side - policy rejecting fields not in the security context seems reasonable. An example is whether you are allowed to pull an image from public registries, or use hostdir

On Apr 7, 2015, at 7:56 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/types.go:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
    The question is whether ANYONE can create host ports or just with special permissions? I really want us to steer people AWAY from bad patterns.


    Reply to this email directly or view it on GitHub.
@smarterclayton

smarterclayton Apr 7, 2015

Contributor

Technically that would be the policy side - policy rejecting fields not in the security context seems reasonable. An example is whether you are allowed to pull an image from public registries, or use hostdir

On Apr 7, 2015, at 7:56 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/types.go:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
    The question is whether ANYONE can create host ports or just with special permissions? I really want us to steer people AWAY from bad patterns.


    Reply to this email directly or view it on GitHub.

This comment has been minimized.

@thockin

thockin Apr 8, 2015

Member

Hmm, if policy rejections happen up-front (good), then I guess I am fine
with this, though it still feels a little inconsistent as to what goes here
vs elsewhere.

On Tue, Apr 7, 2015 at 4:58 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Technically that would be the policy side - policy rejecting fields not in
the security context seems reasonable. An example is whether you are
allowed to pull an image from public registries, or use hostdir
… <#14c9652a3e456526_>
On Apr 7, 2015, at 7:56 PM, Tim Hockin notifications@github.com wrote:
In pkg/api/types.go: > @@ -1712,6 +1714,103 @@ type SecretList struct { >
Items []Secret json:"items" > } > > +// SecurityContext holds security
configuration that will be applied to a container. If a security context >
+// is set on the container spec then it must comply with any constraints
defined in the SecurityConstraints context > +// it is running in. If a
security context is not supplied and the pod is running under a
SecurityConstraints context > +// then a default SecurityContext may be
applied. > +type SecurityContext struct { > + // Capabilities are the
capabilities to add/drop when running the container > + // TODO: will need
to refactor this from the container spec to here > + Capabilities
Capabilities > + > + // Run the container in privileged mode > + // TODO:
will need to refactor this from the container spec to here > + Privileged
bool The question is whether ANYONE can create host ports or just with
special permissions? I really want us to steer people AWAY from bad
patterns. … — Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27932568
.

@thockin

thockin Apr 8, 2015

Member

Hmm, if policy rejections happen up-front (good), then I guess I am fine
with this, though it still feels a little inconsistent as to what goes here
vs elsewhere.

On Tue, Apr 7, 2015 at 4:58 PM, Clayton Coleman notifications@github.com
wrote:

In pkg/api/types.go
#6287 (comment)
:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool

Technically that would be the policy side - policy rejecting fields not in
the security context seems reasonable. An example is whether you are
allowed to pull an image from public registries, or use hostdir
… <#14c9652a3e456526_>
On Apr 7, 2015, at 7:56 PM, Tim Hockin notifications@github.com wrote:
In pkg/api/types.go: > @@ -1712,6 +1714,103 @@ type SecretList struct { >
Items []Secret json:"items" > } > > +// SecurityContext holds security
configuration that will be applied to a container. If a security context >
+// is set on the container spec then it must comply with any constraints
defined in the SecurityConstraints context > +// it is running in. If a
security context is not supplied and the pod is running under a
SecurityConstraints context > +// then a default SecurityContext may be
applied. > +type SecurityContext struct { > + // Capabilities are the
capabilities to add/drop when running the container > + // TODO: will need
to refactor this from the container spec to here > + Capabilities
Capabilities > + > + // Run the container in privileged mode > + // TODO:
will need to refactor this from the container spec to here > + Privileged
bool The question is whether ANYONE can create host ports or just with
special permissions? I really want us to steer people AWAY from bad
patterns. … — Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r27932568
.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 8, 2015

Contributor

Yeah. It could be "executioncontext" or "user context" or "process context" or "process options".

On Apr 7, 2015, at 8:19 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/types.go:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
    Hmm, if policy rejections happen up-front (good), then I guess I am fine with this, though it still feels a little inconsistent as to what goes here vs elsewhere.


    Reply to this email directly or view it on GitHub.
@smarterclayton

smarterclayton Apr 8, 2015

Contributor

Yeah. It could be "executioncontext" or "user context" or "process context" or "process options".

On Apr 7, 2015, at 8:19 PM, Tim Hockin notifications@github.com wrote:

In pkg/api/types.go:

@@ -1712,6 +1714,103 @@ type SecretList struct {
Items []Secret json:"items"
}

+// SecurityContext holds security configuration that will be applied to a container. If a security context
+// is set on the container spec then it must comply with any constraints defined in the SecurityConstraints context
+// it is running in. If a security context is not supplied and the pod is running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities Capabilities
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool
    Hmm, if policy rejections happen up-front (good), then I guess I am fine with this, though it still feels a little inconsistent as to what goes here vs elsewhere.


    Reply to this email directly or view it on GitHub.
@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 8, 2015

Member

Updated based on feedback for types (changes to pointers and TODOs for refactoring) and rebased

Member

pweil- commented Apr 8, 2015

Updated based on feedback for types (changes to pointers and TODOs for refactoring) and rebased

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Apr 8, 2015

We have a patch to docker to support "Z" and "z" which relabel content that is volume mounted into the container to be either a SharedMCSLabel ("z") or a PrivateMCSLabel ("Z")

moby/moby#5910

Also Realize that Docker is probably going to expand the amount of "SecurityOptions" available. Right now they support SELinux and perhaps AppArmor, but we have a pull request to add seccomp labeling also.

rhatdan commented Apr 8, 2015

We have a patch to docker to support "Z" and "z" which relabel content that is volume mounted into the container to be either a SharedMCSLabel ("z") or a PrivateMCSLabel ("Z")

moby/moby#5910

Also Realize that Docker is probably going to expand the amount of "SecurityOptions" available. Right now they support SELinux and perhaps AppArmor, but we have a pull request to add seccomp labeling also.

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 8, 2015

Member

Thanks Dan! I've been trying to subscribe to the various PRs that may affect this work. 5910 will definitely be relevant when it comes to volume options, particularly the work in #6379 which is expected to be supported by this PR. I'll keep my eyes open for anything else that may pop up

Member

pweil- commented Apr 8, 2015

Thanks Dan! I've been trying to subscribe to the various PRs that may affect this work. 5910 will definitely be relevant when it comes to volume options, particularly the work in #6379 which is expected to be supported by this PR. I'll keep my eyes open for anything else that may pop up

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 10, 2015

Member

I've updated this with more implementation of applying the constraints/context to the container. If this prototype is looking ok in terms of applying a security context the next steps would be:

  1. update the docker cli godep to pull in the new security options fields
  2. provide the ability to define security contexts as a resource, ideally attached to service account if that work is ready (or perhaps by sketching in a bare bones type for this to reside in)
  3. move the application and enforcement to the api server and allow the kubelet to just validate and use or reject the pod

Thoughts?

Member

pweil- commented Apr 10, 2015

I've updated this with more implementation of applying the constraints/context to the container. If this prototype is looking ok in terms of applying a security context the next steps would be:

  1. update the docker cli godep to pull in the new security options fields
  2. provide the ability to define security contexts as a resource, ideally attached to service account if that work is ready (or perhaps by sketching in a bare bones type for this to reside in)
  3. move the application and enforcement to the api server and allow the kubelet to just validate and use or reject the pod

Thoughts?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 10, 2015

Contributor

----- Original Message -----

I've updated this with more implementation of applying the
constraints/context to the container. If this prototype is looking ok in
terms of applying a security context the next steps would be:

  1. update the docker cli godep to pull in the new security options fields
  2. provide the ability to define security contexts as a resource, ideally
    attached to service account if that work is ready (or perhaps by sketching
    in a bare bones type for this to reside in)

Hold off on this - @liggitt can swoop in after.

  1. move the application and enforcement to the api server and allow the
    kubelet to just validate and use or reject the pod

Look at an admission controller that rejects pods as well. Talk to derek.

Thoughts?


Reply to this email directly or view it on GitHub:
#6287 (comment)

Contributor

smarterclayton commented Apr 10, 2015

----- Original Message -----

I've updated this with more implementation of applying the
constraints/context to the container. If this prototype is looking ok in
terms of applying a security context the next steps would be:

  1. update the docker cli godep to pull in the new security options fields
  2. provide the ability to define security contexts as a resource, ideally
    attached to service account if that work is ready (or perhaps by sketching
    in a bare bones type for this to reside in)

Hold off on this - @liggitt can swoop in after.

  1. move the application and enforcement to the api server and allow the
    kubelet to just validate and use or reject the pod

Look at an admission controller that rejects pods as well. Talk to derek.

Thoughts?


Reply to this email directly or view it on GitHub:
#6287 (comment)

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 15, 2015

Member

This is now in a working prototype state. Here are the details:

Policies

  1. For context, policy should be retrieved from a service account. That logic is not intended to be part of this PR (#6287 (comment))
  2. Right now there are 2 providers that can be used: restrict and permit.
    • Permit does nothing and allows anything to pass through.
    • Restrict will a) validate against policy and b) apply defaults if set

Api Server

  1. The api server is hard coded to use the restrict policy in the admission controller
  2. The admission controller has only been enabled in local-up-cluster.sh right now
  3. The admission controller takes 3 actions:
    • Perform an initial validation with the policy and warn if it is broken
    • Apply the defaults (the provider does this, the admission controller just initiates it)
    • Revalidate and either deny or accept

Kubelet

  1. The kubelet policy is controlled by the security_context flag. In this PR it is set in the local-up-cluster.sh only and is set to restrict

Example of restrict policy enforcing defaults (api server step 2) in the admission controller:

W0415 15:10:11.088457   14489 admission.go:67] Initial validation of pod default/hello-openshift failed, applying defaults. Broken policy: [hello-openshift.SecurityContext.Privileged: invalid value 'true': SecurityContext does not allow privileged containers]

Example of the kubelet running in restrict mode and api server running in permit mode that causes a kubelet rejection of the pod

I0415 15:08:03.173954   13457 event.go:200] Event(api.ObjectReference{Kind:"Pod", Namespace:"default", Name:"hello-openshift", UID:"bd81666e-e3a2-11e4-a443-54ee752009cb", APIVersion:"v1beta3", ResourceVersion:"26", FieldPath:""}): reason: 'failedSync' Error syncing pod, skipping: pod with UID "bd81666e-e3a2-11e4-a443-54ee752009cb" does not comply with the security context
Member

pweil- commented Apr 15, 2015

This is now in a working prototype state. Here are the details:

Policies

  1. For context, policy should be retrieved from a service account. That logic is not intended to be part of this PR (#6287 (comment))
  2. Right now there are 2 providers that can be used: restrict and permit.
    • Permit does nothing and allows anything to pass through.
    • Restrict will a) validate against policy and b) apply defaults if set

Api Server

  1. The api server is hard coded to use the restrict policy in the admission controller
  2. The admission controller has only been enabled in local-up-cluster.sh right now
  3. The admission controller takes 3 actions:
    • Perform an initial validation with the policy and warn if it is broken
    • Apply the defaults (the provider does this, the admission controller just initiates it)
    • Revalidate and either deny or accept

Kubelet

  1. The kubelet policy is controlled by the security_context flag. In this PR it is set in the local-up-cluster.sh only and is set to restrict

Example of restrict policy enforcing defaults (api server step 2) in the admission controller:

W0415 15:10:11.088457   14489 admission.go:67] Initial validation of pod default/hello-openshift failed, applying defaults. Broken policy: [hello-openshift.SecurityContext.Privileged: invalid value 'true': SecurityContext does not allow privileged containers]

Example of the kubelet running in restrict mode and api server running in permit mode that causes a kubelet rejection of the pod

I0415 15:08:03.173954   13457 event.go:200] Event(api.ObjectReference{Kind:"Pod", Namespace:"default", Name:"hello-openshift", UID:"bd81666e-e3a2-11e4-a443-54ee752009cb", APIVersion:"v1beta3", ResourceVersion:"26", FieldPath:""}): reason: 'failedSync' Error syncing pod, skipping: pod with UID "bd81666e-e3a2-11e4-a443-54ee752009cb" does not comply with the security context
@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 15, 2015

Member

Known TODOs:

  1. round out unit tests
  2. there is code in the restrict provider that should be reused by permit. That said, I don't expect these to stick around in a separated form. We should inject the constraints and default from the service account settings and use a single provider
  3. refactoring to remove old caps/priv requests
Member

pweil- commented Apr 15, 2015

Known TODOs:

  1. round out unit tests
  2. there is code in the restrict provider that should be reused by permit. That said, I don't expect these to stick around in a separated form. We should inject the constraints and default from the service account settings and use a single provider
  3. refactoring to remove old caps/priv requests
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 15, 2015

Contributor

I got goosebumps

Contributor

smarterclayton commented Apr 15, 2015

I got goosebumps

@derekwaynecarr

This comment has been minimized.

Show comment
Hide comment
@derekwaynecarr

derekwaynecarr Apr 15, 2015

Member

Admission control plugin looks good.

I think we need to settle on a set of plugins (or an agreed upon set of defaults) that always get invoked without requiring manual configuration.

This may be a good candidate to always include in the default list.

Sent from my iPhone

On Apr 15, 2015, at 3:51 PM, Clayton Coleman notifications@github.com wrote:

I got goosebumps


Reply to this email directly or view it on GitHub.

Member

derekwaynecarr commented Apr 15, 2015

Admission control plugin looks good.

I think we need to settle on a set of plugins (or an agreed upon set of defaults) that always get invoked without requiring manual configuration.

This may be a good candidate to always include in the default list.

Sent from my iPhone

On Apr 15, 2015, at 3:51 PM, Clayton Coleman notifications@github.com wrote:

I got goosebumps


Reply to this email directly or view it on GitHub.

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 17, 2015

Member

any other feedback? I will start addressing the TODOs for refactoring if everything else is looking good

Member

pweil- commented Apr 17, 2015

any other feedback? I will start addressing the TODOs for refactoring if everything else is looking good

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 21, 2015

Member

@erictune - any thoughts on this prototype?

Member

pweil- commented Apr 21, 2015

@erictune - any thoughts on this prototype?

@pweil-

This comment has been minimized.

Show comment
Hide comment
@pweil-
Member

pweil- commented Apr 23, 2015

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

glog.V(4).Infof() on subsequent calls

glog.V(4).Infof() on subsequent calls

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

Oh wait, you can't. Uh... Hrm. I don't like this silently failing.

smarterclayton replied Apr 23, 2015

Oh wait, you can't. Uh... Hrm. I don't like this silently failing.

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

Also global variables are an antipatern trap and we should not be using capabilities like this. How much code actually references this?

smarterclayton replied Apr 23, 2015

Also global variables are an antipatern trap and we should not be using capabilities like this. How much code actually references this?

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

The validation could be controlled by the admission controller.

smarterclayton replied Apr 23, 2015

The validation could be controlled by the admission controller.

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

Docker manager should be injected by security context manager on the kubelet

smarterclayton replied Apr 23, 2015

Docker manager should be injected by security context manager on the kubelet

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

API server command should be passed in to the admission controller.

smarterclayton replied Apr 23, 2015

API server command should be passed in to the admission controller.

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

Kube app should be security context initialization.

So it sounds like all of them.

smarterclayton replied Apr 23, 2015

Kube app should be security context initialization.

So it sounds like all of them.

This comment has been minimized.

Show comment
Hide comment
@pweil-

pweil- Apr 23, 2015

Owner

👍 on it. You can disregard the big old "this is a warning comment" in the kubelet until these changes shake out

Owner

pweil- replied Apr 23, 2015

👍 on it. You can disregard the big old "this is a warning comment" in the kubelet until these changes shake out

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Apr 23, 2015

So call out explicitly there is a follow up to remove Capabilities and replace it with policy/context injection.

smarterclayton replied Apr 23, 2015

So call out explicitly there is a follow up to remove Capabilities and replace it with policy/context injection.

@@ -449,6 +458,43 @@ func RunKubelet(kcfg *KubeletConfig, builder KubeletBuilder) {
glog.Infof("Started kubelet")
}
// InitializeSecurityContextProvider initializes a global security context and sets the security context provider.

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

Nit: Suggest you wrap this text at no more than 100 chars.

@pmorie

pmorie Apr 23, 2015

Member

Nit: Suggest you wrap this text at no more than 100 chars.

@@ -579,6 +579,9 @@ func init() {
if err := s.Convert(&in.Capabilities, &out.Capabilities, 0); err != nil {

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

This conversion doesn't need to be called this way anymore, since this is a field on SecurityContext now.

@pmorie

pmorie Apr 23, 2015

Member

This conversion doesn't need to be called this way anymore, since this is a field on SecurityContext now.

@@ -366,6 +366,9 @@ func init() {
if err := s.Convert(&in.Capabilities, &out.Capabilities, 0); err != nil {

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

Same comment here re: capabilities conversion, no longer necessary.

@pmorie

pmorie Apr 23, 2015

Member

Same comment here re: capabilities conversion, no longer necessary.

Capabilities *Capabilities `json:"capabilities,omitempty"`
// Run the container in privileged mode
// TODO: will need to refactor this from the container spec to here

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

I think you can take this TODO out now

@pmorie

pmorie Apr 23, 2015

Member

I think you can take this TODO out now

// Optional: Capabilities for container.
Capabilities Capabilities `json:"capabilities,omitempty" description:"capabilities for container; cannot be updated"`
// SecurityContext defines the security context the pod should run with
SecurityContext `json:"securityContext,omitempty"`

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

Nit, shouldn't this be json:"inline" ?

@pmorie

pmorie Apr 23, 2015

Member

Nit, shouldn't this be json:"inline" ?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Should be json:",inline" but yes

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Should be json:",inline" but yes

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Not a nit, a bug!

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Not a nit, a bug!

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

🐛 🐛 🐛

@pmorie

pmorie Apr 23, 2015

Member

🐛 🐛 🐛

// and volumes
SELinuxOptions *SELinuxOptions `json:"seLinuxOptions,omitempty"`
// RunAsUser is the UID to run the entrypoint of the container process. Corresponding option is --user or -u

This comment has been minimized.

@erictune

erictune Apr 23, 2015

Member

Corresponding option in what tool? Docker?

@erictune

erictune Apr 23, 2015

Member

Corresponding option in what tool? Docker?

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Yeah. Do we want to wait to add it until there is an implementation that can provide it?

----- Original Message -----

+// it is running in. If a security context is not supplied and the pod is
running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the
    container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities *Capabilities json:"capabilities,omitempty"
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool json:"privileged,omitempty"
  • // SELinuxOptions are the labels to be applied to the container
  • // and volumes
  • SELinuxOptions *SELinuxOptions json:"seLinuxOptions,omitempty"
  • // RunAsUser is the UID to run the entrypoint of the container process.
    Corresponding option is --user or -u

Corresponding option in what tool? Docker?


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r28996781

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

Yeah. Do we want to wait to add it until there is an implementation that can provide it?

----- Original Message -----

+// it is running in. If a security context is not supplied and the pod is
running under a SecurityConstraints context
+// then a default SecurityContext may be applied.
+type SecurityContext struct {

  • // Capabilities are the capabilities to add/drop when running the
    container
  • // TODO: will need to refactor this from the container spec to here
  • Capabilities *Capabilities json:"capabilities,omitempty"
  • // Run the container in privileged mode
  • // TODO: will need to refactor this from the container spec to here
  • Privileged bool json:"privileged,omitempty"
  • // SELinuxOptions are the labels to be applied to the container
  • // and volumes
  • SELinuxOptions *SELinuxOptions json:"seLinuxOptions,omitempty"
  • // RunAsUser is the UID to run the entrypoint of the container process.
    Corresponding option is --user or -u

Corresponding option in what tool? Docker?


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r28996781

This comment has been minimized.

@pweil-

pweil- Apr 23, 2015

Member

Right, that was meant to indicate that it was equivalent to the -u option of docker https://docs.docker.com/reference/run/#user

@pweil-

pweil- Apr 23, 2015

Member

Right, that was meant to indicate that it was equivalent to the -u option of docker https://docs.docker.com/reference/run/#user

Disabled bool `json:"disabled,omitempty"`
}
// SecurityConstraints provides the constraints that at security context provider will

This comment has been minimized.

@erictune

erictune Apr 23, 2015

Member

s/at/a/

@erictune

erictune Apr 23, 2015

Member

s/at/a/

// that conflicts with an actual request it will be implementation specific whether that request is
// ignored and the container is still run (without the requested constraint) or if the container will be failed
type SecurityConstraints struct {
// EnforcementPolicy will drive behavior for how the constraints are enforce

This comment has been minimized.

@erictune

erictune Apr 23, 2015

Member

s/enforce/enforced/

@erictune

erictune Apr 23, 2015

Member

s/enforce/enforced/

// SecurityConstraints provides the constraints that at security context provider will
// ensure that applied SecurityContext requests follow. When a setting is provided in the SecurityConstraints
// that conflicts with an actual request it will be implementation specific whether that request is

This comment has been minimized.

@erictune

erictune Apr 23, 2015

Member

Is it "implementation defined" or is it explicitly controlled by "enforcementPolicy"?

@erictune

erictune Apr 23, 2015

Member

Is it "implementation defined" or is it explicitly controlled by "enforcementPolicy"?

// 3. if there are selinux options on the security constraints AND options on the container then check each
// setting individually. If the individual setting is not allowed then remove it or set it to the default if one exists
func (p *provider) applySELinux(container *api.Container) {
// no security context settings for SELinux

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

Just a note we talked about breaking this one up.

@pmorie

pmorie Apr 23, 2015

Member

Just a note we talked about breaking this one up.

hasDefaultSELinux := hasDefault && constraints.DefaultSecurityContext.SELinuxOptions != nil
// if the container has not defined SELinux options then apply the default if it exists
if container.SecurityContext.SELinuxOptions == nil {

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

I expect @smarterclayton not to like this but i think doing container.SecurityContext -> containerContext and constraints.SELinux -> selConstraints would make this much more readable and you could get away without breaking this method up, just a thought.

@pmorie

pmorie Apr 23, 2015

Member

I expect @smarterclayton not to like this but i think doing container.SecurityContext -> containerContext and constraints.SELinux -> selConstraints would make this much more readable and you could get away without breaking this method up, just a thought.

This comment has been minimized.

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

I don't like it because it's an API change - the time for that has come and gone - we will look at it for the next release.

----- Original Message -----

+// 2. if there are selinux options on the security constraints AND there
are no options defined on the container
+// AND there is a default security context then use all the default
settings
+// 3. if there are selinux options on the security constraints AND
options on the container then check each
+// setting individually. If the individual setting is not allowed then
remove it or set it to the default if one exists
+func (p *provider) applySELinux(container *api.Container) {

  • // no security context settings for SELinux
  • if p.SecurityConstraints.SELinux == nil {
  •   return
    
  • }
  • constraints := p.SecurityConstraints
  • hasDefault := constraints.DefaultSecurityContext != nil
  • hasDefaultSELinux := hasDefault &&
    constraints.DefaultSecurityContext.SELinuxOptions != nil
  • // if the container has not defined SELinux options then apply the
    default if it exists
  • if container.SecurityContext.SELinuxOptions == nil {

I expect @smarterclayton not to like this but i think doing
container.SecurityContext -> containerContext and constraints.SELinux
-> selConstraints would make this much more readable and you could get
away without breaking this method up, just a thought.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r28998305

@smarterclayton

smarterclayton Apr 23, 2015

Contributor

I don't like it because it's an API change - the time for that has come and gone - we will look at it for the next release.

----- Original Message -----

+// 2. if there are selinux options on the security constraints AND there
are no options defined on the container
+// AND there is a default security context then use all the default
settings
+// 3. if there are selinux options on the security constraints AND
options on the container then check each
+// setting individually. If the individual setting is not allowed then
remove it or set it to the default if one exists
+func (p *provider) applySELinux(container *api.Container) {

  • // no security context settings for SELinux
  • if p.SecurityConstraints.SELinux == nil {
  •   return
    
  • }
  • constraints := p.SecurityConstraints
  • hasDefault := constraints.DefaultSecurityContext != nil
  • hasDefaultSELinux := hasDefault &&
    constraints.DefaultSecurityContext.SELinuxOptions != nil
  • // if the container has not defined SELinux options then apply the
    default if it exists
  • if container.SecurityContext.SELinuxOptions == nil {

I expect @smarterclayton not to like this but i think doing
container.SecurityContext -> containerContext and constraints.SELinux
-> selConstraints would make this much more readable and you could get
away without breaking this method up, just a thought.


Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/6287/files#r28998305

// check individual settings of the container's request
if !constraints.SELinux.AllowDisable && container.SecurityContext.SELinuxOptions.Disabled {
glog.V(4).Infof("Resetting SELinuxOptions.Disabled for %s", container.Name)

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

Is it 'resetting' or 'setting' ?

@pmorie

pmorie Apr 23, 2015

Member

Is it 'resetting' or 'setting' ?

if !constraints.SELinux.AllowLevelLabel {
glog.V(4).Infof("Resetting SELinuxOptions.Level for %s", container.Name)
level := ""
if hasDefault && hasDefaultSELinux {

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

hasDefault && hasDefaultSELinux is repeated a lot, I would offer this as something else that can get a local defaultSElinux var so you don't have to repeat the test.

@pmorie

pmorie Apr 23, 2015

Member

hasDefault && hasDefaultSELinux is repeated a lot, I would offer this as something else that can get a local defaultSElinux var so you don't have to repeat the test.

// requested add/drop list. This is an override, not a additive operation.
func (p *provider) applyCapRequests(container *api.Container) {
context := p.SecurityConstraints
if !context.AllowCapabilities {

This comment has been minimized.

@pmorie

pmorie Apr 23, 2015

Member

This might read a little better if the blocks and test were reversed.

@pmorie

pmorie Apr 23, 2015

Member

This might read a little better if the blocks and test were reversed.

@bgrant0607 bgrant0607 referenced this pull request Apr 23, 2015

Closed

Create the v1 API #7018

@@ -70,6 +70,8 @@ func init() {
&PodProxyOptions{},
&ComponentStatus{},
&ComponentStatusList{},
&SecurityContext{},

This comment has been minimized.

@erictune

erictune Apr 23, 2015

Member

I don't think this is a top level object because it doesn't have a kind.

@erictune

erictune Apr 23, 2015

Member

I don't think this is a top level object because it doesn't have a kind.

This comment has been minimized.

@pweil-

pweil- Apr 23, 2015

Member

You're right, it needs to be removed.

@pweil-

pweil- Apr 23, 2015

Member

You're right, it needs to be removed.

@@ -114,3 +116,5 @@ func (*PodExecOptions) IsAnAPIObject() {}
func (*PodProxyOptions) IsAnAPIObject() {}
func (*ComponentStatus) IsAnAPIObject() {}
func (*ComponentStatusList) IsAnAPIObject() {}
func (*SecurityContext) IsAnAPIObject() {}

<