From 6aa80d9172d72055539c99da3794a2ac0fc767f8 Mon Sep 17 00:00:00 2001 From: Michael Taufen Date: Thu, 28 Jan 2021 17:03:34 -0800 Subject: [PATCH] Graduate ServiceAccountIssuerDiscovery to GA Waiting on KEP updates first: https://github.com/kubernetes/enhancements/pull/2363 --- cmd/kube-apiserver/app/BUILD | 1 - cmd/kube-apiserver/app/server.go | 25 ++++----- pkg/controlplane/instance.go | 56 +++++++++---------- pkg/features/kube_features.go | 3 +- pkg/kubeapiserver/options/authentication.go | 18 +++--- .../authorizer/rbac/bootstrappolicy/policy.go | 46 +++++++-------- test/integration/auth/svcaccttoken_test.go | 4 -- 7 files changed, 68 insertions(+), 85 deletions(-) diff --git a/cmd/kube-apiserver/app/BUILD b/cmd/kube-apiserver/app/BUILD index bf45bdb09a3..5b9457d8437 100644 --- a/cmd/kube-apiserver/app/BUILD +++ b/cmd/kube-apiserver/app/BUILD @@ -17,7 +17,6 @@ go_library( "//pkg/controlplane/controller/crdregistration:go_default_library", "//pkg/controlplane/reconcilers:go_default_library", "//pkg/controlplane/tunneler:go_default_library", - "//pkg/features:go_default_library", "//pkg/generated/openapi:go_default_library", "//pkg/kubeapiserver:go_default_library", "//pkg/kubeapiserver/admission:go_default_library", diff --git a/cmd/kube-apiserver/app/server.go b/cmd/kube-apiserver/app/server.go index 715e289f701..74e33065290 100644 --- a/cmd/kube-apiserver/app/server.go +++ b/cmd/kube-apiserver/app/server.go @@ -73,7 +73,6 @@ import ( "k8s.io/kubernetes/pkg/controlplane" "k8s.io/kubernetes/pkg/controlplane/reconcilers" "k8s.io/kubernetes/pkg/controlplane/tunneler" - "k8s.io/kubernetes/pkg/features" generatedopenapi "k8s.io/kubernetes/pkg/generated/openapi" "k8s.io/kubernetes/pkg/kubeapiserver" kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission" @@ -422,21 +421,19 @@ func CreateKubeAPIServerConfig( config.ExtraConfig.ProxyTransport = c } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountIssuerDiscovery) { - // Load the public keys. - var pubKeys []interface{} - for _, f := range s.Authentication.ServiceAccounts.KeyFiles { - keys, err := keyutil.PublicKeysFromFile(f) - if err != nil { - return nil, nil, nil, fmt.Errorf("failed to parse key file %q: %v", f, err) - } - pubKeys = append(pubKeys, keys...) + // Load the public keys. + var pubKeys []interface{} + for _, f := range s.Authentication.ServiceAccounts.KeyFiles { + keys, err := keyutil.PublicKeysFromFile(f) + if err != nil { + return nil, nil, nil, fmt.Errorf("failed to parse key file %q: %v", f, err) } - // Plumb the required metadata through ExtraConfig. - config.ExtraConfig.ServiceAccountIssuerURL = s.Authentication.ServiceAccounts.Issuer - config.ExtraConfig.ServiceAccountJWKSURI = s.Authentication.ServiceAccounts.JWKSURI - config.ExtraConfig.ServiceAccountPublicKeys = pubKeys + pubKeys = append(pubKeys, keys...) } + // Plumb the required metadata through ExtraConfig. + config.ExtraConfig.ServiceAccountIssuerURL = s.Authentication.ServiceAccounts.Issuer + config.ExtraConfig.ServiceAccountJWKSURI = s.Authentication.ServiceAccounts.JWKSURI + config.ExtraConfig.ServiceAccountPublicKeys = pubKeys return config, serviceResolver, pluginInitializers, nil } diff --git a/pkg/controlplane/instance.go b/pkg/controlplane/instance.go index dfd5744e0e9..de7d674a7f0 100644 --- a/pkg/controlplane/instance.go +++ b/pkg/controlplane/instance.go @@ -366,37 +366,35 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget) routes.Logs{}.Install(s.Handler.GoRestfulContainer) } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountIssuerDiscovery) { - // Metadata and keys are expected to only change across restarts at present, - // so we just marshal immediately and serve the cached JSON bytes. - md, err := serviceaccount.NewOpenIDMetadata( - c.ExtraConfig.ServiceAccountIssuerURL, - c.ExtraConfig.ServiceAccountJWKSURI, - c.GenericConfig.ExternalAddress, - c.ExtraConfig.ServiceAccountPublicKeys, - ) - if err != nil { - // If there was an error, skip installing the endpoints and log the - // error, but continue on. We don't return the error because the - // metadata responses require additional, backwards incompatible - // validation of command-line options. - msg := fmt.Sprintf("Could not construct pre-rendered responses for"+ - " ServiceAccountIssuerDiscovery endpoints. Endpoints will not be"+ - " enabled. Error: %v", err) - if c.ExtraConfig.ServiceAccountIssuerURL != "" { - // The user likely expects this feature to be enabled if issuer URL is - // set and the feature gate is enabled. In the future, if there is no - // longer a feature gate and issuer URL is not set, the user may not - // expect this feature to be enabled. We log the former case as an Error - // and the latter case as an Info. - klog.Error(msg) - } else { - klog.Info(msg) - } + // Metadata and keys are expected to only change across restarts at present, + // so we just marshal immediately and serve the cached JSON bytes. + md, err := serviceaccount.NewOpenIDMetadata( + c.ExtraConfig.ServiceAccountIssuerURL, + c.ExtraConfig.ServiceAccountJWKSURI, + c.GenericConfig.ExternalAddress, + c.ExtraConfig.ServiceAccountPublicKeys, + ) + if err != nil { + // If there was an error, skip installing the endpoints and log the + // error, but continue on. We don't return the error because the + // metadata responses require additional, backwards incompatible + // validation of command-line options. + msg := fmt.Sprintf("Could not construct pre-rendered responses for"+ + " ServiceAccountIssuerDiscovery endpoints. Endpoints will not be"+ + " enabled. Error: %v", err) + if c.ExtraConfig.ServiceAccountIssuerURL != "" { + // The user likely expects this feature to be enabled if issuer URL is + // set and the feature gate is enabled. In the future, if there is no + // longer a feature gate and issuer URL is not set, the user may not + // expect this feature to be enabled. We log the former case as an Error + // and the latter case as an Info. + klog.Error(msg) } else { - routes.NewOpenIDMetadataServer(md.ConfigJSON, md.PublicKeysetJSON). - Install(s.Handler.GoRestfulContainer) + klog.Info(msg) } + } else { + routes.NewOpenIDMetadataServer(md.ConfigJSON, md.PublicKeysetJSON). + Install(s.Handler.GoRestfulContainer) } m := &Instance{ diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index f7fe6a7ec82..2afc95e3f85 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -194,6 +194,7 @@ const ( // owner: @mtaufen // alpha: v1.18 // beta: v1.20 + // stable: v1.21 // // Enable OIDC discovery endpoints (issuer and JWKS URLs) for the service // account issuer in the API server. @@ -699,7 +700,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS 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.Alpha}, - ServiceAccountIssuerDiscovery: {Default: true, PreRelease: featuregate.Beta}, + ServiceAccountIssuerDiscovery: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.22 CRIContainerLogRotation: {Default: true, PreRelease: featuregate.Beta}, CSIMigration: {Default: true, PreRelease: featuregate.Beta}, CSIMigrationGCE: {Default: false, PreRelease: featuregate.Beta}, // Off by default (requires GCE PD CSI Driver) diff --git a/pkg/kubeapiserver/options/authentication.go b/pkg/kubeapiserver/options/authentication.go index aac123cdc43..3e01c30fc08 100644 --- a/pkg/kubeapiserver/options/authentication.go +++ b/pkg/kubeapiserver/options/authentication.go @@ -213,18 +213,14 @@ func (o *BuiltInAuthenticationOptions) Validate() []error { allErrors = append(allErrors, errors.New("service-account-key-file is a required flag")) } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountIssuerDiscovery) { - // Validate the JWKS URI when it is explicitly set. - // When unset, it is later derived from ExternalHost. - if o.ServiceAccounts.JWKSURI != "" { - if u, err := url.Parse(o.ServiceAccounts.JWKSURI); err != nil { - allErrors = append(allErrors, fmt.Errorf("service-account-jwks-uri must be a valid URL: %v", err)) - } else if u.Scheme != "https" { - allErrors = append(allErrors, fmt.Errorf("service-account-jwks-uri requires https scheme, parsed as: %v", u.String())) - } + // Validate the JWKS URI when it is explicitly set. + // When unset, it is later derived from ExternalHost. + if o.ServiceAccounts.JWKSURI != "" { + if u, err := url.Parse(o.ServiceAccounts.JWKSURI); err != nil { + allErrors = append(allErrors, fmt.Errorf("service-account-jwks-uri must be a valid URL: %v", err)) + } else if u.Scheme != "https" { + allErrors = append(allErrors, fmt.Errorf("service-account-jwks-uri requires https scheme, parsed as: %v", u.String())) } - } else if len(o.ServiceAccounts.JWKSURI) > 0 { - allErrors = append(allErrors, fmt.Errorf("service-account-jwks-uri may only be set when the ServiceAccountIssuerDiscovery feature gate is enabled")) } } diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go index 2af3f501a09..44787de5904 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go @@ -494,18 +494,16 @@ func ClusterRoles() []rbacv1.ClusterRole { }, } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountIssuerDiscovery) { - // Add the cluster role for reading the ServiceAccountIssuerDiscovery endpoints - roles = append(roles, rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{Name: "system:service-account-issuer-discovery"}, - Rules: []rbacv1.PolicyRule{ - rbacv1helpers.NewRule("get").URLs( - "/.well-known/openid-configuration", - "/openid/v1/jwks", - ).RuleOrDie(), - }, - }) - } + // Add the cluster role for reading the ServiceAccountIssuerDiscovery endpoints + roles = append(roles, rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Name: "system:service-account-issuer-discovery"}, + Rules: []rbacv1.PolicyRule{ + rbacv1helpers.NewRule("get").URLs( + "/.well-known/openid-configuration", + "/openid/v1/jwks", + ).RuleOrDie(), + }, + }) // node-proxier role is used by kube-proxy. nodeProxierRules := []rbacv1.PolicyRule{ @@ -590,19 +588,17 @@ func ClusterRoleBindings() []rbacv1.ClusterRoleBinding { }, } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceAccountIssuerDiscovery) { - // Allow all in-cluster workloads (via their service accounts) to read the OIDC discovery endpoints. - // Users with certain forms of write access (create pods, create secrets, create service accounts, etc) - // can gain access to a service account identity which would allow them to access this information. - // This includes the issuer URL, which is already present in the SA token JWT. Similarly, SAs can - // already gain this same info via introspection of their own token. Since this discovery endpoint - // points to what issued all service account tokens, it seems fitting for SAs to have this access. - // Defer to the cluster admin with regard to binding directly to all authenticated and/or - // unauthenticated users. - rolebindings = append(rolebindings, - rbacv1helpers.NewClusterBinding("system:service-account-issuer-discovery").Groups(serviceaccount.AllServiceAccountsGroup).BindingOrDie(), - ) - } + // Allow all in-cluster workloads (via their service accounts) to read the OIDC discovery endpoints. + // Users with certain forms of write access (create pods, create secrets, create service accounts, etc) + // can gain access to a service account identity which would allow them to access this information. + // This includes the issuer URL, which is already present in the SA token JWT. Similarly, SAs can + // already gain this same info via introspection of their own token. Since this discovery endpoint + // points to what issued all service account tokens, it seems fitting for SAs to have this access. + // Defer to the cluster admin with regard to binding directly to all authenticated and/or + // unauthenticated users. + rolebindings = append(rolebindings, + rbacv1helpers.NewClusterBinding("system:service-account-issuer-discovery").Groups(serviceaccount.AllServiceAccountsGroup).BindingOrDie(), + ) addClusterRoleBindingLabel(rolebindings) diff --git a/test/integration/auth/svcaccttoken_test.go b/test/integration/auth/svcaccttoken_test.go index 70dca7ccb9b..6499ba93424 100644 --- a/test/integration/auth/svcaccttoken_test.go +++ b/test/integration/auth/svcaccttoken_test.go @@ -43,16 +43,13 @@ import ( "k8s.io/apiserver/pkg/authentication/request/bearertoken" apiserverserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount" "k8s.io/apiserver/pkg/authorization/authorizerfactory" - utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" v1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/keyutil" - featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/apis/core" serviceaccountgetter "k8s.io/kubernetes/pkg/controller/serviceaccount" - "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/serviceaccount" "k8s.io/kubernetes/test/integration/framework" ) @@ -64,7 +61,6 @@ AwEHoUQDQgAEH6cuzP8XuD5wal6wf9M6xDljTOPLX2i8uIp/C/ASqiIGUeeKQtX0 -----END EC PRIVATE KEY-----` func TestServiceAccountTokenCreate(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServiceAccountIssuerDiscovery, true)() // Build client config, clientset, and informers sk, err := keyutil.ParsePrivateKeyPEM([]byte(ecdsaPrivateKey))