Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

admission: split PSP plugin into mutating and validating admission #54689

Merged
merged 1 commit into from
Nov 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
143 changes: 95 additions & 48 deletions plugin/pkg/admission/security/podsecuritypolicy/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ func Register(plugins *admission.Plugins) {
// PSPMatchFn allows plugging in how PSPs are matched against user information.
type PSPMatchFn func(lister extensionslisters.PodSecurityPolicyLister, user user.Info, sa user.Info, authz authorizer.Authorizer, namespace string) ([]*extensions.PodSecurityPolicy, error)

// podSecurityPolicyPlugin holds state for and implements the admission plugin.
type podSecurityPolicyPlugin struct {
// PodSecurityPolicyPlugin holds state for and implements the admission plugin.
type PodSecurityPolicyPlugin struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we cannot return one admission interface in the constructor anymore. This was largely discussed within #54485. Golang doesn't give us better tools than this.

Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be exported now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compare #54689 (comment). In short: we implement multiple interface, but can only return one by interface in the constructor. Returning a private type is weird, super interface are broken in Golang. Broadly discussed in #54485.

*admission.Handler
strategyFactory psp.StrategyFactory
pspMatcher PSPMatchFn
Expand All @@ -68,12 +68,12 @@ type podSecurityPolicyPlugin struct {
}

// SetAuthorizer sets the authorizer.
func (plugin *podSecurityPolicyPlugin) SetAuthorizer(authz authorizer.Authorizer) {
func (plugin *PodSecurityPolicyPlugin) SetAuthorizer(authz authorizer.Authorizer) {
plugin.authz = authz
}

// ValidateInitialization ensures an authorizer is set.
func (plugin *podSecurityPolicyPlugin) ValidateInitialization() error {
func (plugin *PodSecurityPolicyPlugin) ValidateInitialization() error {
if plugin.authz == nil {
return fmt.Errorf("%s requires an authorizer", PluginName)
}
Expand All @@ -83,21 +83,22 @@ func (plugin *podSecurityPolicyPlugin) ValidateInitialization() error {
return nil
}

var _ admission.Interface = &podSecurityPolicyPlugin{}
var _ genericadmissioninit.WantsAuthorizer = &podSecurityPolicyPlugin{}
var _ kubeapiserveradmission.WantsInternalKubeInformerFactory = &podSecurityPolicyPlugin{}
var _ admission.MutationInterface = &PodSecurityPolicyPlugin{}
var _ admission.ValidationInterface = &PodSecurityPolicyPlugin{}
var _ genericadmissioninit.WantsAuthorizer = &PodSecurityPolicyPlugin{}
var _ kubeapiserveradmission.WantsInternalKubeInformerFactory = &PodSecurityPolicyPlugin{}

// NewPlugin creates a new PSP admission plugin.
func NewPlugin(strategyFactory psp.StrategyFactory, pspMatcher PSPMatchFn, failOnNoPolicies bool) *podSecurityPolicyPlugin {
return &podSecurityPolicyPlugin{
func NewPlugin(strategyFactory psp.StrategyFactory, pspMatcher PSPMatchFn, failOnNoPolicies bool) *PodSecurityPolicyPlugin {
return &PodSecurityPolicyPlugin{
Handler: admission.NewHandler(admission.Create, admission.Update),
strategyFactory: strategyFactory,
pspMatcher: pspMatcher,
failOnNoPolicies: failOnNoPolicies,
}
}

func (a *podSecurityPolicyPlugin) SetInternalKubeInformerFactory(f informers.SharedInformerFactory) {
func (a *PodSecurityPolicyPlugin) SetInternalKubeInformerFactory(f informers.SharedInformerFactory) {
podSecurityPolicyInformer := f.Extensions().InternalVersion().PodSecurityPolicies()
a.lister = podSecurityPolicyInformer.Lister()
a.SetReadyFunc(podSecurityPolicyInformer.Informer().HasSynced)
Expand All @@ -111,30 +112,93 @@ func (a *podSecurityPolicyPlugin) SetInternalKubeInformerFactory(f informers.Sha
// 3. Try to generate and validate a PSP with providers. If we find one then admit the pod
// with the validated PSP. If we don't find any reject the pod and give all errors from the
// failed attempts.
func (c *podSecurityPolicyPlugin) Admit(a admission.Attributes) error {
if a.GetResource().GroupResource() != api.Resource("pods") {
func (c *PodSecurityPolicyPlugin) Admit(a admission.Attributes) error {
if ignore, err := shouldIgnore(a); err != nil {
return err
} else if ignore {
return nil
}

if len(a.GetSubresource()) != 0 {
pod := a.GetObject().(*api.Pod)

// compute the context. If the current security context is valid, this call won't change it.
allowMutation := a.GetOperation() == admission.Create // TODO(liggitt): allow spec mutation during initializing updates?
allowedPod, pspName, validationErrs, err := c.computeSecurityContext(a, pod, allowMutation)
if err != nil {
return admission.NewForbidden(a, err)
}
if allowedPod != nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: && len(validationErrs) == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. validationErrs will be the errors of not-matching policies. If we get an allowedPod, we can ignore those. Otherwise, we print them.

*pod = *allowedPod
// annotate and accept the pod
glog.V(4).Infof("pod %s (generate: %s) in namespace %s validated against provider %s", pod.Name, pod.GenerateName, a.GetNamespace(), pspName)
if pod.ObjectMeta.Annotations == nil {
pod.ObjectMeta.Annotations = map[string]string{}
}
// set annotation to mark this as passed. Note, that the actual value is not important, the
// validating PSP might even change later-on. Also not that pspName can be the empty string
// if failOnNoPolicies is false.
// TODO: if failOnNoPolicies is toggled from false to true, we will never update the annotation anymore. Is this desired?
pod.ObjectMeta.Annotations[psputil.ValidatedPSPAnnotation] = pspName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k We could skip the mutation phase for non-create operations. But this annotation forces us to keep it for the update case. It's essential for the tests right now to expose the selected policy. If we change the tests, we might get rid of this and simplify the mutation phase to be create-only. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k We could skip the mutation phase for non-create operations. But this annotation forces us to keep it for the update case. It's essential for the tests right now to expose the selected policy. If we change the tests, we might get rid of this and simplify the mutation phase to be create-only. wdyt?

That sounds good as a future improvement. I'd like to keep this initial pull tight.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I would say that this annotation is broken by this split. For it to be accurate, you would need to require that the subsequent validate step validates using the PSP named here.

It would actually simplify the model (and speed it up) if we could require that updates to the pod be valid under the original PSP, but I guess that's a significant breaking API change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotation is for testing and for information only, or in other words: it's broken by design. You cannot trust its value in any subsequent non-mutating request. Actually, given the semantics of PSPs to be highly implicit (via an existantial quantifier), I would prefer to have this annotation go away. Then we can restrict the mutation phase to CREATE requests only and do just validation after that for any other request.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM. Follow up?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. Follow up?

Yeah. I see some value in the annotation to allow an ordering during the validation that will probably let us finish faster. Not mutating at all during update seems like a great change though.

return nil
}

// we didn't validate against any provider, reject the pod and give the errors for each attempt
glog.V(4).Infof("unable to validate pod %s (generate: %s) in namespace %s against any pod security policy: %v", pod.Name, pod.GenerateName, a.GetNamespace(), validationErrs)
return admission.NewForbidden(a, fmt.Errorf("unable to validate against any pod security policy: %v", validationErrs))
}

func (c *PodSecurityPolicyPlugin) Validate(a admission.Attributes) error {
if ignore, err := shouldIgnore(a); err != nil {
return err
} else if ignore {
return nil
}

pod := a.GetObject().(*api.Pod)

// compute the context. If the current security context is valid, this call won't change it.
allowedPod, _, validationErrs, err := c.computeSecurityContext(a, pod, false)
if err != nil {
return admission.NewForbidden(a, err)
}
if apiequality.Semantic.DeepEqual(pod, allowedPod) {
return nil
}

pod, ok := a.GetObject().(*api.Pod)
// we didn't validate against any provider, reject the pod and give the errors for each attempt
glog.V(4).Infof("unable to validate pod %s (generate: %s) in namespace %s against any pod security policy: %v", pod.Name, pod.GenerateName, a.GetNamespace(), validationErrs)
return admission.NewForbidden(a, fmt.Errorf("unable to validate against any pod security policy: %v", validationErrs))
}

func shouldIgnore(a admission.Attributes) (bool, error) {
if a.GetResource().GroupResource() != api.Resource("pods") {
return true, nil
}
if len(a.GetSubresource()) != 0 {
return true, nil
}

// if we can't convert then fail closed since we've already checked that this is supposed to be a pod object.
// this shouldn't normally happen during admission but could happen if an integrator passes a versioned
// pod object rather than an internal object.
if !ok {
return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject()))
if _, ok := a.GetObject().(*api.Pod); !ok {
return false, admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject()))
}

// if this is an update, see if we are only updating the ownerRef/finalizers. Garbage collection does this
// and we should allow it in general, since you had the power to update and the power to delete.
// The worst that happens is that you delete something, but you aren't controlling the privileged object itself
if a.GetOperation() == admission.Update && rbacregistry.IsOnlyMutatingGCFields(a.GetObject(), a.GetOldObject(), apiequality.Semantic) {
return nil
return true, nil
}

return false, nil
}

// computeSecurityContext derives a valid security context while trying to avoid any changes to the given pod. I.e.
// if there is a matching policy with the same security context as given, it will be reused. If there is no
// matching policy the returned pod will be nil and the pspName empty.
func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes, pod *api.Pod, specMutationAllowed bool) (*api.Pod, string, field.ErrorList, error) {
// get all constraints that are usable by the user
glog.V(4).Infof("getting pod security policies for pod %s (generate: %s)", pod.Name, pod.GenerateName)
var saInfo user.Info
Expand All @@ -144,13 +208,13 @@ func (c *podSecurityPolicyPlugin) Admit(a admission.Attributes) error {

matchedPolicies, err := c.pspMatcher(c.lister, a.GetUserInfo(), saInfo, c.authz, a.GetNamespace())
if err != nil {
return admission.NewForbidden(a, err)
return nil, "", nil, err
}

// if we have no policies and want to succeed then return. Otherwise we'll end up with no
// providers and fail with "unable to validate against any pod security policy" below.
if len(matchedPolicies) == 0 && !c.failOnNoPolicies {
return nil
return pod, "", nil, nil
}

// sort by name to make order deterministic
Expand All @@ -163,20 +227,16 @@ func (c *podSecurityPolicyPlugin) Admit(a admission.Attributes) error {
logProviders(a, pod, providers, errs)

if len(providers) == 0 {
return admission.NewForbidden(a, fmt.Errorf("no providers available to validate pod request"))
return nil, "", nil, fmt.Errorf("no providers available to validate pod request")
}

// TODO(liggitt): allow spec mutation during initializing updates?
specMutationAllowed := a.GetOperation() == admission.Create

// all containers in a single pod must validate under a single provider or we will reject the request
validationErrs := field.ErrorList{}
var (
allowedPod *api.Pod
allowingProvider psp.Provider
allowedMutatedPod *api.Pod
allowingMutatingPSP string
)

loop:
for _, provider := range providers {
podCopy := pod.DeepCopy()

Expand All @@ -190,34 +250,21 @@ loop:
switch {
case apiequality.Semantic.DeepEqual(pod, podCopy):
// if it validated without mutating anything, use this result
allowedPod = podCopy
allowingProvider = provider
break loop
case specMutationAllowed && allowedPod == nil:
return podCopy, provider.GetPSPName(), nil, nil

case specMutationAllowed && allowedMutatedPod == nil:
// if mutation is allowed and this is the first PSP to allow the pod, remember it,
// but continue to see if another PSP allows without mutating
allowedPod = podCopy
allowingProvider = provider
glog.V(6).Infof("pod %s (generate: %s) in namespace %s validated against provider %s with mutation", pod.Name, pod.GenerateName, a.GetNamespace(), provider.GetPSPName())
case !specMutationAllowed:
glog.V(6).Infof("pod %s (generate: %s) in namespace %s validated against provider %s, but required mutation, skipping", pod.Name, pod.GenerateName, a.GetNamespace(), provider.GetPSPName())
allowedMutatedPod = podCopy
allowingMutatingPSP = provider.GetPSPName()
}
}

if allowedPod != nil {
*pod = *allowedPod
// annotate and accept the pod
glog.V(4).Infof("pod %s (generate: %s) in namespace %s validated against provider %s", pod.Name, pod.GenerateName, a.GetNamespace(), allowingProvider.GetPSPName())
if pod.ObjectMeta.Annotations == nil {
pod.ObjectMeta.Annotations = map[string]string{}
}
pod.ObjectMeta.Annotations[psputil.ValidatedPSPAnnotation] = allowingProvider.GetPSPName()
return nil
if allowedMutatedPod == nil {
return nil, "", validationErrs, nil
}

// we didn't validate against any provider, reject the pod and give the errors for each attempt
glog.V(4).Infof("unable to validate pod %s (generate: %s) in namespace %s against any pod security policy: %v", pod.Name, pod.GenerateName, a.GetNamespace(), validationErrs)
return admission.NewForbidden(a, fmt.Errorf("unable to validate against any pod security policy: %v", validationErrs))
return allowedMutatedPod, allowingMutatingPSP, nil, nil
}

// assignSecurityContext creates a security context for each container in the pod
Expand Down Expand Up @@ -265,7 +312,7 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P
}

// createProvidersFromPolicies creates providers from the constraints supplied.
func (c *podSecurityPolicyPlugin) createProvidersFromPolicies(psps []*extensions.PodSecurityPolicy, namespace string) ([]psp.Provider, []error) {
func (c *PodSecurityPolicyPlugin) createProvidersFromPolicies(psps []*extensions.PodSecurityPolicy, namespace string) ([]psp.Provider, []error) {
var (
// collected providers
providers []psp.Provider
Expand Down