From eac517b7d3bb10a3c0bd6728740ee7b8edacba84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 30 Apr 2021 21:15:15 +0200 Subject: [PATCH 1/2] UPSTREAM: : WIP: Use the still-created token secrets to provide service-ca.crt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... with BoundServiceAccountTokenVolume . Signed-off-by: Miloslav Trmač --- .../podsecuritypolicy/provider_test.go | 4 +- pkg/security/podsecuritypolicy/util/util.go | 12 +++- .../podsecuritypolicy/util/util_test.go | 43 +++++++++++++-- .../pkg/admission/serviceaccount/admission.go | 55 +++++++++++++------ .../serviceaccount/admission_test.go | 20 ++++++- 5 files changed, 107 insertions(+), 27 deletions(-) diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index 76c5a753d75a6..8bbb945878c8a 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -1444,7 +1444,7 @@ func TestValidateProjectedVolume(t *testing.T) { { desc: "deny if secret is not allowed", allowedFSTypes: []policy.FSType{policy.EmptyDir}, - projectedVolumeSource: serviceaccount.TokenVolumeSource(), + projectedVolumeSource: serviceaccount.TokenVolumeSource("service-account-token-secret"), wantAllow: false, }, { @@ -1471,7 +1471,7 @@ func TestValidateProjectedVolume(t *testing.T) { { desc: "allow if secret is allowed and the projected volume sources equals to the ones injected by service account admission plugin", allowedFSTypes: []policy.FSType{policy.Secret}, - projectedVolumeSource: serviceaccount.TokenVolumeSource(), + projectedVolumeSource: serviceaccount.TokenVolumeSource("service-account-token-secret"), wantAllow: true, }, } diff --git a/pkg/security/podsecuritypolicy/util/util.go b/pkg/security/podsecuritypolicy/util/util.go index 1405714f8bbcb..8fb0b41f404b2 100644 --- a/pkg/security/podsecuritypolicy/util/util.go +++ b/pkg/security/podsecuritypolicy/util/util.go @@ -23,6 +23,7 @@ import ( policy "k8s.io/api/policy/v1beta1" "k8s.io/apimachinery/pkg/util/sets" api "k8s.io/kubernetes/pkg/apis/core" + controllerserviceaccount "k8s.io/kubernetes/pkg/controller/serviceaccount" ) const ( @@ -253,7 +254,7 @@ func EqualStringSlices(a, b []string) bool { func IsOnlyServiceAccountTokenSources(v *api.ProjectedVolumeSource) bool { for _, s := range v.Sources { // reject any projected source that does not match any of our expected source types - if s.ServiceAccountToken == nil && s.ConfigMap == nil && s.DownwardAPI == nil { + if s.ServiceAccountToken == nil && s.ConfigMap == nil && s.DownwardAPI == nil && s.Secret == nil { return false } if t := s.ServiceAccountToken; t != nil && (t.Path != "token" || t.Audience != "") { @@ -271,6 +272,15 @@ func IsOnlyServiceAccountTokenSources(v *api.ProjectedVolumeSource) bool { } } } + + if s.Secret != nil { // FIXME? We don’t check the secret projection’s LocalObjectReference because the caller is allowed to use Secret volume type directly anyway + for _, d := range s.Secret.Items { + if d.Path != controllerserviceaccount.ServiceServingCASecretKey { + return false + } + } + + } } return true } diff --git a/pkg/security/podsecuritypolicy/util/util_test.go b/pkg/security/podsecuritypolicy/util/util_test.go index ceef7dc0dc9f9..d4d39c428a7c9 100644 --- a/pkg/security/podsecuritypolicy/util/util_test.go +++ b/pkg/security/podsecuritypolicy/util/util_test.go @@ -285,6 +285,19 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { }, }, } + secret := api.VolumeProjection{ + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{ + Name: "some-secret-that-is-allowed-anyway", + }, + Items: []api.KeyToPath{ + { + Key: "service-ca.crt", + Path: "service-ca.crt", + }, + }, + }, + } tests := []struct { desc string @@ -300,6 +313,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { ExpirationSeconds: serviceaccount.WarnOnlyBoundTokenExpirationSeconds, }}, configMap, + secret, downwardAPI, }, }, @@ -314,6 +328,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { ExpirationSeconds: serviceaccount.WarnOnlyBoundTokenExpirationSeconds, }}, configMap, + secret, downwardAPI, }, }, @@ -336,6 +351,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { }, }, }, + secret, downwardAPI, }, }, @@ -346,6 +362,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { Sources: []api.VolumeProjection{ serviceAccountToken, configMap, + secret, { DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ @@ -368,6 +385,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { Sources: []api.VolumeProjection{ serviceAccountToken, configMap, + secret, { DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ @@ -386,6 +404,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { Sources: []api.VolumeProjection{ serviceAccountToken, configMap, + secret, { DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ @@ -408,6 +427,7 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { Sources: []api.VolumeProjection{ serviceAccountToken, configMap, + secret, { DownwardAPI: &api.DownwardAPIProjection{ Items: []api.DownwardAPIVolumeFile{ @@ -425,20 +445,30 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { }, }, { - desc: "deny if Secret exists", + desc: "deny if Secret has wrong field path", volume: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ - { - Secret: &api.SecretProjection{}, - }, configMap, downwardAPI, serviceAccountToken, + { + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{ + Name: "some-secret-that-is-allowed-anyway", + }, + Items: []api.KeyToPath{ + { + Key: "not-service-ca.crt", + Path: "not-service-ca.crt", + }, + }, + }, + }, }, }, }, { - desc: "deny if none of ServiceAccountToken, ConfigMap and DownwardAPI exist", + desc: "deny if none of ServiceAccountToken, ConfigMap, DownwardAPI and Secret exist", volume: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ {}, @@ -446,12 +476,13 @@ func TestIsOnlyServiceAccountTokenSources(t *testing.T) { }, }, { - desc: "allow if any of ServiceAccountToken, ConfigMap and DownwardAPI matches", + desc: "allow if any of ServiceAccountToken, ConfigMap, DownwardAPI and Secret matches", volume: &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ configMap, downwardAPI, serviceAccountToken, + secret, }, }, want: true, diff --git a/plugin/pkg/admission/serviceaccount/admission.go b/plugin/pkg/admission/serviceaccount/admission.go index 6a003f75e0a63..3a080fa886988 100644 --- a/plugin/pkg/admission/serviceaccount/admission.go +++ b/plugin/pkg/admission/serviceaccount/admission.go @@ -41,6 +41,7 @@ import ( "k8s.io/component-base/featuregate" podutil "k8s.io/kubernetes/pkg/api/pod" api "k8s.io/kubernetes/pkg/apis/core" + controllerserviceaccount "k8s.io/kubernetes/pkg/controller/serviceaccount" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/serviceaccount" ) @@ -328,29 +329,32 @@ func (s *Plugin) getServiceAccount(namespace string, name string) (*corev1.Servi return nil, errors.NewNotFound(api.Resource("serviceaccount"), name) } -// getReferencedServiceAccountToken returns the name of the first referenced secret which is a ServiceAccountToken for the service account -func (s *Plugin) getReferencedServiceAccountToken(serviceAccount *corev1.ServiceAccount) (string, error) { +// getReferencedServiceAccountToken returns the name of the first referenced secret which is a ServiceAccountToken for the service account, and whether it contains service-ca.crt. +func (s *Plugin) getReferencedServiceAccountToken(serviceAccount *corev1.ServiceAccount) (string, bool, error) { if len(serviceAccount.Secrets) == 0 { - return "", nil + return "", false, nil } tokens, err := s.getServiceAccountTokens(serviceAccount) if err != nil { - return "", err + return "", false, err } accountTokens := sets.NewString() + hasServiceServingCA := map[string]bool{} for _, token := range tokens { accountTokens.Insert(token.Name) + _, ok := token.Data[controllerserviceaccount.ServiceServingCASecretKey] // This may be false during bootstrap. + hasServiceServingCA[token.Name] = ok } // Prefer secrets in the order they're referenced. for _, secret := range serviceAccount.Secrets { if accountTokens.Has(secret.Name) { - return secret.Name, nil + return secret.Name, hasServiceServingCA[secret.Name], nil } } - return "", nil + return "", false, nil } // getServiceAccountTokens returns all ServiceAccountToken secrets for the given ServiceAccount @@ -430,12 +434,10 @@ func (s *Plugin) mountServiceAccountToken(serviceAccount *corev1.ServiceAccount, serviceAccountToken string err error ) - if !s.boundServiceAccountTokenVolume { - // Find the name of a referenced ServiceAccountToken secret we can mount - serviceAccountToken, err = s.getReferencedServiceAccountToken(serviceAccount) - if err != nil { - return fmt.Errorf("Error looking up service account token for %s/%s: %v", serviceAccount.Namespace, serviceAccount.Name, err) - } + // Find the name of a referenced ServiceAccountToken secret we can mount + serviceAccountToken, hasServiceServingCA, err := s.getReferencedServiceAccountToken(serviceAccount) + if err != nil { + return fmt.Errorf("Error looking up service account token for %s/%s: %v", serviceAccount.Namespace, serviceAccount.Name, err) } if len(serviceAccountToken) == 0 && !s.boundServiceAccountTokenVolume { // We don't have an API token to mount, so return @@ -516,17 +518,20 @@ func (s *Plugin) mountServiceAccountToken(serviceAccount *corev1.ServiceAccount, // Add the volume if a container needs it if !hasTokenVolume && needsTokenVolume { - pod.Spec.Volumes = append(pod.Spec.Volumes, s.createVolume(tokenVolumeName, serviceAccountToken)) + pod.Spec.Volumes = append(pod.Spec.Volumes, s.createVolume(tokenVolumeName, serviceAccountToken, hasServiceServingCA)) } return nil } -func (s *Plugin) createVolume(tokenVolumeName, secretName string) api.Volume { +func (s *Plugin) createVolume(tokenVolumeName, secretName string, hasServiceServingCA bool) api.Volume { if s.boundServiceAccountTokenVolume { + if !hasServiceServingCA { + secretName = "" + } return api.Volume{ Name: tokenVolumeName, VolumeSource: api.VolumeSource{ - Projected: TokenVolumeSource(), + Projected: TokenVolumeSource(secretName), }, } } @@ -541,8 +546,8 @@ func (s *Plugin) createVolume(tokenVolumeName, secretName string) api.Volume { } // TokenVolumeSource returns the projected volume source for service account token. -func TokenVolumeSource() *api.ProjectedVolumeSource { - return &api.ProjectedVolumeSource{ +func TokenVolumeSource(serviceAccountTokenSecretName string) *api.ProjectedVolumeSource { + res := &api.ProjectedVolumeSource{ Sources: []api.VolumeProjection{ { ServiceAccountToken: &api.ServiceAccountTokenProjection{ @@ -578,4 +583,20 @@ func TokenVolumeSource() *api.ProjectedVolumeSource { }, }, } + if serviceAccountTokenSecretName != "" { + res.Sources = append(res.Sources, api.VolumeProjection{ + Secret: &api.SecretProjection{ + LocalObjectReference: api.LocalObjectReference{ + Name: serviceAccountTokenSecretName, + }, + Items: []api.KeyToPath{ + { + Key: controllerserviceaccount.ServiceServingCASecretKey, + Path: controllerserviceaccount.ServiceServingCASecretKey, + }, + }, + }, + }) + } + return res } diff --git a/plugin/pkg/admission/serviceaccount/admission_test.go b/plugin/pkg/admission/serviceaccount/admission_test.go index 14fe7f223b3ec..a2ec5fa698504 100644 --- a/plugin/pkg/admission/serviceaccount/admission_test.go +++ b/plugin/pkg/admission/serviceaccount/admission_test.go @@ -220,6 +220,7 @@ func TestAssignsDefaultServiceAccountAndRejectsMissingAPIToken(t *testing.T) { func TestAssignsDefaultServiceAccountAndBoundTokenWithNoSecretTokens(t *testing.T) { ns := "myns" + serviceAccountTokenSecretName := "some-service-account-token-secret" admit := NewServiceAccount() informerFactory := informers.NewSharedInformerFactory(nil, controller.NoResyncPeriodFunc()) @@ -234,6 +235,22 @@ func TestAssignsDefaultServiceAccountAndBoundTokenWithNoSecretTokens(t *testing. Name: DefaultServiceAccountName, Namespace: ns, }, + Secrets: []corev1.ObjectReference{ + { + Name: serviceAccountTokenSecretName, + }, + }, + }) + informerFactory.Core().V1().Secrets().Informer().GetStore().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceAccountTokenSecretName, + Namespace: ns, + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: DefaultServiceAccountName, + }, + }, + Type: corev1.SecretTypeServiceAccountToken, + Data: map[string][]byte{"service-ca.crt": []byte("contents")}, }) pod := &api.Pod{ @@ -255,6 +272,7 @@ func TestAssignsDefaultServiceAccountAndBoundTokenWithNoSecretTokens(t *testing. {ServiceAccountToken: &api.ServiceAccountTokenProjection{ExpirationSeconds: 3607, Path: "token"}}, {ConfigMap: &api.ConfigMapProjection{LocalObjectReference: api.LocalObjectReference{Name: "kube-root-ca.crt"}, Items: []api.KeyToPath{{Key: "ca.crt", Path: "ca.crt"}}}}, {DownwardAPI: &api.DownwardAPIProjection{Items: []api.DownwardAPIVolumeFile{{Path: "namespace", FieldRef: &api.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.namespace"}}}}}, + {Secret: &api.SecretProjection{LocalObjectReference: api.LocalObjectReference{Name: serviceAccountTokenSecretName}, Items: []api.KeyToPath{{Key: "service-ca.crt", Path: "service-ca.crt"}}}}, }, }, }, @@ -351,7 +369,7 @@ func TestAutomountsAPIToken(t *testing.T) { tokenName = generatedVolumeName } - expectedVolume := admit.createVolume(tokenName, tokenName) + expectedVolume := admit.createVolume(tokenName, tokenName, false) expectedVolumeMount := api.VolumeMount{ Name: tokenName, ReadOnly: true, From 68ba35eacfde6e070d500f1ac224ea68084e01b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 30 Apr 2021 21:16:34 +0200 Subject: [PATCH 2/2] UPSTREAM: : Revert "UPSTREAM: : disable BoundServiceAccountTokenVolume by default" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit d101b9e32c9d3b2173985332c88798014a87c249. Signed-off-by: Miloslav Trmač --- pkg/features/kube_features.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index bdb3588f760c8..05dee386236a4 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -786,7 +786,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS StorageObjectInUseProtection: {Default: true, PreRelease: featuregate.GA}, SupportPodPidsLimit: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23 SupportNodePidsLimit: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.23 - BoundServiceAccountTokenVolume: {Default: false, PreRelease: featuregate.Beta}, // TODO(auth): investigate the impact of enabling this feature (https://bugzilla.redhat.com/show_bug.cgi?id=1946479) + BoundServiceAccountTokenVolume: {Default: true, PreRelease: featuregate.Beta}, ServiceAccountIssuerDiscovery: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22 CRIContainerLogRotation: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22 CSIMigration: {Default: true, PreRelease: featuregate.Beta},