From a3e914528a1c16af9797ff9e0e070a84e1842ba0 Mon Sep 17 00:00:00 2001 From: Jean Rouge Date: Thu, 16 May 2019 15:32:59 -0700 Subject: [PATCH] API changes for Windows GMSA support This patch comprises the API changes outlined in the Windows GMSA KEP (https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md) to add GMSA support to Windows workloads. It includes validation, as well as dropping fields if the `WindowsGMSA` feature flag is not set, both with unit tests. Signed-off-by: Jean Rouge --- pkg/api/pod/util.go | 73 +++++++ pkg/api/pod/util_test.go | 202 ++++++++++++++++++++ pkg/apis/core/types.go | 12 +- pkg/apis/core/validation/validation.go | 40 +++- pkg/apis/core/validation/validation_test.go | 74 +++++++ staging/src/k8s.io/api/core/v1/types.go | 12 +- 6 files changed, 410 insertions(+), 3 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 3bffd1bf292..e2a7cb566ea 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -368,6 +368,8 @@ func dropDisabledFields( dropDisabledRunAsGroupField(podSpec, oldPodSpec) + dropDisabledGMSAFields(podSpec, oldPodSpec) + if !utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClass) && !runtimeClassInUse(oldPodSpec) { // Set RuntimeClassName to nil only if feature is disabled and it is not used podSpec.RuntimeClassName = nil @@ -399,6 +401,39 @@ func dropDisabledRunAsGroupField(podSpec, oldPodSpec *api.PodSpec) { } } +// dropDisabledGMSAFields removes disabled fields related to Windows GMSA +// from the given PodSpec. +func dropDisabledGMSAFields(podSpec, oldPodSpec *api.PodSpec) { + if utilfeature.DefaultFeatureGate.Enabled(features.WindowsGMSA) || + gMSAFieldsInUse(oldPodSpec) { + return + } + + if podSpec.SecurityContext != nil { + dropDisabledGMSAFieldsFromWindowsSecurityOptions(podSpec.SecurityContext.WindowsOptions) + } + dropDisabledGMSAFieldsFromContainers(podSpec.Containers) + dropDisabledGMSAFieldsFromContainers(podSpec.InitContainers) +} + +// dropDisabledGMSAFieldsFromWindowsSecurityOptions removes disabled fields +// related to Windows GMSA from the given WindowsSecurityContextOptions. +func dropDisabledGMSAFieldsFromWindowsSecurityOptions(windowsOptions *api.WindowsSecurityContextOptions) { + if windowsOptions != nil { + windowsOptions.GMSACredentialSpecName = nil + windowsOptions.GMSACredentialSpec = nil + } +} + +// dropDisabledGMSAFieldsFromContainers removes disabled fields +func dropDisabledGMSAFieldsFromContainers(containers []api.Container) { + for i := range containers { + if containers[i].SecurityContext != nil { + dropDisabledGMSAFieldsFromWindowsSecurityOptions(containers[i].SecurityContext.WindowsOptions) + } + } +} + // dropDisabledProcMountField removes disabled fields from PodSpec related // to ProcMount only if it is not already used by the old spec func dropDisabledProcMountField(podSpec, oldPodSpec *api.PodSpec) { @@ -612,6 +647,44 @@ func runAsGroupInUse(podSpec *api.PodSpec) bool { return false } +// gMSAFieldsInUse returns true if the pod spec is non-nil and has one of any +// SecurityContext's GMSACredentialSpecName or GMSACredentialSpec fields set. +func gMSAFieldsInUse(podSpec *api.PodSpec) bool { + if podSpec == nil { + return false + } + + if podSpec.SecurityContext != nil && gMSAFieldsInUseInWindowsSecurityOptions(podSpec.SecurityContext.WindowsOptions) { + return true + } + + return gMSAFieldsInUseInAnyContainer(podSpec.Containers) || + gMSAFieldsInUseInAnyContainer(podSpec.InitContainers) +} + +// gMSAFieldsInUseInWindowsSecurityOptions returns true if the given WindowsSecurityContextOptions is +// non-nil and one of its GMSACredentialSpecName or GMSACredentialSpec fields is set. +func gMSAFieldsInUseInWindowsSecurityOptions(windowsOptions *api.WindowsSecurityContextOptions) bool { + if windowsOptions == nil { + return false + } + + return windowsOptions.GMSACredentialSpecName != nil || + windowsOptions.GMSACredentialSpec != nil +} + +// gMSAFieldsInUseInAnyContainer returns true if any of the given Containers has its +// SecurityContext's GMSACredentialSpecName or GMSACredentialSpec fields set. +func gMSAFieldsInUseInAnyContainer(containers []api.Container) bool { + for _, container := range containers { + if container.SecurityContext != nil && gMSAFieldsInUseInWindowsSecurityOptions(container.SecurityContext.WindowsOptions) { + return true + } + } + + return false +} + // subpathExprInUse returns true if the pod spec is non-nil and has a volume mount that makes use of the subPathExpr feature func subpathExprInUse(podSpec *api.PodSpec) bool { if podSpec == nil { diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 0533a8861c7..f3523c5ada9 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1359,6 +1359,208 @@ func TestDropRunAsGroup(t *testing.T) { } } +func TestDropGMSAFields(t *testing.T) { + defaultContainerSecurityContextFactory := func() *api.SecurityContext { + defaultProcMount := api.DefaultProcMount + return &api.SecurityContext{ProcMount: &defaultProcMount} + } + podWithoutWindowsOptionsFactory := func() *api.Pod { + return &api.Pod{ + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicyNever, + SecurityContext: &api.PodSecurityContext{}, + Containers: []api.Container{{Name: "container1", Image: "testimage", SecurityContext: defaultContainerSecurityContextFactory()}}, + InitContainers: []api.Container{{Name: "initContainer1", Image: "testimage", SecurityContext: defaultContainerSecurityContextFactory()}}, + }, + } + } + + type podFactoryInfo struct { + description string + hasGMSAField bool + // this factory should generate the input pod whose spec will be fed to dropDisabledFields + podFactory func() *api.Pod + // this factory should generate the expected pod after the GMSA fields have been dropped + // we can't just use podWithoutWindowsOptionsFactory as is for this, since in some cases + // we'll be left with a WindowsSecurityContextOptions struct with no GMSA field set, as opposed + // to a nil pointer in the pod generated by podWithoutWindowsOptionsFactory + // if this field is not set, it will default to the podFactory + strippedPodFactory func() *api.Pod + } + podFactoryInfos := []podFactoryInfo{ + { + description: "does not have any GMSA field set", + hasGMSAField: false, + podFactory: podWithoutWindowsOptionsFactory, + }, + { + description: "has a pod-level WindowsSecurityContextOptions struct with no GMSA field set", + hasGMSAField: false, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }, + { + description: "has a WindowsSecurityContextOptions struct with no GMSA field set on a container", + hasGMSAField: false, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.Containers[0].SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }, + { + description: "has a WindowsSecurityContextOptions struct with no GMSA field set on an init container", + hasGMSAField: false, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.InitContainers[0].SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }, + { + description: "is nil", + hasGMSAField: false, + podFactory: func() *api.Pod { return nil }, + }, + } + + toPtr := func(s string) *string { + return &s + } + addGMSACredentialSpecName := func(windowsOptions *api.WindowsSecurityContextOptions) { + windowsOptions.GMSACredentialSpecName = toPtr("dummy-gmsa-cred-spec-name") + } + addGMSACredentialSpec := func(windowsOptions *api.WindowsSecurityContextOptions) { + windowsOptions.GMSACredentialSpec = toPtr("dummy-gmsa-cred-spec-contents") + } + addBothGMSAFields := func(windowsOptions *api.WindowsSecurityContextOptions) { + addGMSACredentialSpecName(windowsOptions) + addGMSACredentialSpec(windowsOptions) + } + + for fieldName, windowsOptionsTransformingFunc := range map[string]func(*api.WindowsSecurityContextOptions){ + "GMSACredentialSpecName field": addGMSACredentialSpecName, + "GMSACredentialSpec field": addGMSACredentialSpec, + "both GMSA fields": addBothGMSAFields, + } { + // yes, these variables are indeed needed for the closure to work + // properly, please do NOT remove them + name := fieldName + transformingFunc := windowsOptionsTransformingFunc + + windowsOptionsWithGMSAFieldFactory := func() *api.WindowsSecurityContextOptions { + windowsOptions := &api.WindowsSecurityContextOptions{} + transformingFunc(windowsOptions) + return windowsOptions + } + + podFactoryInfos = append(podFactoryInfos, + podFactoryInfo{ + description: fmt.Sprintf("has %s in Pod", name), + hasGMSAField: true, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.SecurityContext.WindowsOptions = windowsOptionsWithGMSAFieldFactory() + return pod + }, + strippedPodFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }, + podFactoryInfo{ + description: fmt.Sprintf("has %s in Container", name), + hasGMSAField: true, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.Containers[0].SecurityContext.WindowsOptions = windowsOptionsWithGMSAFieldFactory() + return pod + }, + strippedPodFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.Containers[0].SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }, + podFactoryInfo{ + description: fmt.Sprintf("has %s in InitContainer", name), + hasGMSAField: true, + podFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.InitContainers[0].SecurityContext.WindowsOptions = windowsOptionsWithGMSAFieldFactory() + return pod + }, + strippedPodFactory: func() *api.Pod { + pod := podWithoutWindowsOptionsFactory() + pod.Spec.InitContainers[0].SecurityContext.WindowsOptions = &api.WindowsSecurityContextOptions{} + return pod + }, + }) + } + + for _, enabled := range []bool{true, false} { + for _, oldPodFactoryInfo := range podFactoryInfos { + for _, newPodFactoryInfo := range podFactoryInfos { + newPodHasGMSAField, newPod := newPodFactoryInfo.hasGMSAField, newPodFactoryInfo.podFactory() + if newPod == nil { + continue + } + oldPodHasGMSAField, oldPod := oldPodFactoryInfo.hasGMSAField, oldPodFactoryInfo.podFactory() + + t.Run(fmt.Sprintf("feature enabled=%v, old pod %s, new pod %s", enabled, oldPodFactoryInfo.description, newPodFactoryInfo.description), func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.WindowsGMSA, enabled)() + + var oldPodSpec *api.PodSpec + if oldPod != nil { + oldPodSpec = &oldPod.Spec + } + dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil) + + // old pod should never be changed + if !reflect.DeepEqual(oldPod, oldPodFactoryInfo.podFactory()) { + t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodFactoryInfo.podFactory())) + } + + switch { + case enabled || oldPodHasGMSAField: + // new pod should not be changed if the feature is enabled, or if the old pod had any GMSA field set + if !reflect.DeepEqual(newPod, newPodFactoryInfo.podFactory()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodFactoryInfo.podFactory())) + } + case newPodHasGMSAField: + // new pod should be changed + if reflect.DeepEqual(newPod, newPodFactoryInfo.podFactory()) { + t.Errorf("%v", oldPod) + t.Errorf("%v", newPod) + t.Errorf("new pod was not changed") + } + // new pod should not have any GMSA field set + var expectedStrippedPod *api.Pod + if newPodFactoryInfo.strippedPodFactory == nil { + expectedStrippedPod = newPodFactoryInfo.podFactory() + } else { + expectedStrippedPod = newPodFactoryInfo.strippedPodFactory() + } + + if !reflect.DeepEqual(newPod, expectedStrippedPod) { + t.Errorf("new pod had some GMSA field set: %v", diff.ObjectReflectDiff(newPod, expectedStrippedPod)) + } + default: + // new pod should not need to be changed + if !reflect.DeepEqual(newPod, newPodFactoryInfo.podFactory()) { + t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodFactoryInfo.podFactory())) + } + } + }) + } + } + } +} + func TestDropPodSysctls(t *testing.T) { podWithSysctls := func() *api.Pod { return &api.Pod{ diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 5b88d570706..fb0755e019c 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -4739,7 +4739,17 @@ type SELinuxOptions struct { // WindowsSecurityContextOptions contain Windows-specific options and credentials. type WindowsSecurityContextOptions struct { - // intentionally left empty for now + // GMSACredentialSpecName is the name of the GMSA credential spec to use. + // This field is alpha-level and is only honored by servers that enable the WindowsGMSA feature flag. + // +optional + GMSACredentialSpecName *string + + // GMSACredentialSpec is where the GMSA admission webhook + // (https://github.com/kubernetes-sigs/windows-gmsa) inlines the contents of the + // GMSA credential spec named by the GMSACredentialSpecName field. + // This field is alpha-level and is only honored by servers that enable the WindowsGMSA feature flag. + // +optional + GMSACredentialSpec *string } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index bd54e385ac2..1f92208cd5a 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3446,6 +3446,8 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec * if len(securityContext.Sysctls) != 0 { allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...) } + + allErrs = append(allErrs, validateWindowsSecurityContextOptions(securityContext.WindowsOptions, fldPath.Child("windowsOptions"))...) } return allErrs @@ -5156,7 +5158,7 @@ func ValidateEndpointsUpdate(newEndpoints, oldEndpoints *core.Endpoints) field.E return allErrs } -// ValidateSecurityContext ensure the security context contains valid settings +// ValidateSecurityContext ensures the security context contains valid settings func ValidateSecurityContext(sc *core.SecurityContext, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} //this should only be true for testing since SecurityContext is defaulted by the core @@ -5202,6 +5204,42 @@ func ValidateSecurityContext(sc *core.SecurityContext, fldPath *field.Path) fiel } } + allErrs = append(allErrs, validateWindowsSecurityContextOptions(sc.WindowsOptions, fldPath.Child("windowsOptions"))...) + + return allErrs +} + +// maxGMSACredentialSpecLength is the max length, in bytes, for the actual contents +// of a GMSA cred spec. In general, those shouldn't be more than a few hundred bytes, +// so we want to give plenty of room here while still providing an upper bound. +const ( + maxGMSACredentialSpecLengthInKiB = 64 + maxGMSACredentialSpecLength = maxGMSACredentialSpecLengthInKiB * 1024 +) + +func validateWindowsSecurityContextOptions(windowsOptions *core.WindowsSecurityContextOptions, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if windowsOptions == nil { + return allErrs + } + + if windowsOptions.GMSACredentialSpecName != nil { + // gmsaCredentialSpecName must be the name of a custom resource + for _, msg := range validation.IsDNS1123Subdomain(*windowsOptions.GMSACredentialSpecName) { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("gmsaCredentialSpecName"), windowsOptions.GMSACredentialSpecName, msg)) + } + } + + if windowsOptions.GMSACredentialSpec != nil { + if l := len(*windowsOptions.GMSACredentialSpec); l == 0 { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("gmsaCredentialSpec"), windowsOptions.GMSACredentialSpec, "gmsaCredentialSpec cannot be an empty string")) + } else if l > maxGMSACredentialSpecLength { + errMsg := fmt.Sprintf("gmsaCredentialSpec size must be under %d KiB", maxGMSACredentialSpecLengthInKiB) + allErrs = append(allErrs, field.Invalid(fieldPath.Child("gmsaCredentialSpec"), windowsOptions.GMSACredentialSpec, errMsg)) + } + } + return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 57b5a8866a7..2feccddb9b2 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -13205,3 +13205,77 @@ func TestValidateOrSetClientIPAffinityConfig(t *testing.T) { } } } + +func TestValidateWindowsSecurityContextOptions(t *testing.T) { + toPtr := func(s string) *string { + return &s + } + + testCases := []struct { + testName string + + windowsOptions *core.WindowsSecurityContextOptions + expectedErrorSubstring string + }{ + { + testName: "a nil pointer", + }, + { + testName: "an empty struct", + windowsOptions: &core.WindowsSecurityContextOptions{}, + }, + { + testName: "a valid input", + windowsOptions: &core.WindowsSecurityContextOptions{ + GMSACredentialSpecName: toPtr("dummy-gmsa-crep-spec-name"), + GMSACredentialSpec: toPtr("dummy-gmsa-crep-spec-contents"), + }, + }, + { + testName: "a GMSA cred spec name that is not a valid resource name", + windowsOptions: &core.WindowsSecurityContextOptions{ + // invalid because of the underscore + GMSACredentialSpecName: toPtr("not_a-valid-gmsa-crep-spec-name"), + }, + expectedErrorSubstring: dnsSubdomainLabelErrMsg, + }, + { + testName: "empty GMSA cred spec contents", + windowsOptions: &core.WindowsSecurityContextOptions{ + GMSACredentialSpec: toPtr(""), + }, + expectedErrorSubstring: "gmsaCredentialSpec cannot be an empty string", + }, + { + testName: "GMSA cred spec contents that are too long", + windowsOptions: &core.WindowsSecurityContextOptions{ + GMSACredentialSpec: toPtr(strings.Repeat("a", maxGMSACredentialSpecLength+1)), + }, + expectedErrorSubstring: "gmsaCredentialSpec size must be under", + }, + } + + for _, testCase := range testCases { + t.Run("validateWindowsSecurityContextOptions with"+testCase.testName, func(t *testing.T) { + errs := validateWindowsSecurityContextOptions(testCase.windowsOptions, field.NewPath("field")) + + switch len(errs) { + case 0: + if testCase.expectedErrorSubstring != "" { + t.Errorf("expected a failure containing the substring: %q", testCase.expectedErrorSubstring) + } + case 1: + if testCase.expectedErrorSubstring == "" { + t.Errorf("didn't expect a failure, got: %q", errs[0].Error()) + } else if !strings.Contains(errs[0].Error(), testCase.expectedErrorSubstring) { + t.Errorf("expected a failure with the substring %q, got %q instead", testCase.expectedErrorSubstring, errs[0].Error()) + } + default: + t.Errorf("got %d failures", len(errs)) + for i, err := range errs { + t.Errorf("error %d: %q", i, err.Error()) + } + } + }) + } +} diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index d40e1033a96..394fb4d6563 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -5355,7 +5355,17 @@ type SELinuxOptions struct { // WindowsSecurityContextOptions contain Windows-specific options and credentials. type WindowsSecurityContextOptions struct { - // intentionally left empty for now + // GMSACredentialSpecName is the name of the GMSA credential spec to use. + // This field is alpha-level and is only honored by servers that enable the WindowsGMSA feature flag. + // +optional + GMSACredentialSpecName *string `json:"gmsaCredentialSpecName,omitempty" protobuf:"bytes,1,opt,name=gmsaCredentialSpecName"` + + // GMSACredentialSpec is where the GMSA admission webhook + // (https://github.com/kubernetes-sigs/windows-gmsa) inlines the contents of the + // GMSA credential spec named by the GMSACredentialSpecName field. + // This field is alpha-level and is only honored by servers that enable the WindowsGMSA feature flag. + // +optional + GMSACredentialSpec *string `json:"gmsaCredentialSpec,omitempty" protobuf:"bytes,2,opt,name=gmsaCredentialSpec"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object