From d41c6d7c5d43bce94e1d5b6fbdb9553430be6849 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Wed, 4 Dec 2024 16:45:37 -0500 Subject: [PATCH 1/6] Dynamically retrieve the cluster audiences --- lib/kube/token/validator.go | 50 +++++++++++++++++++++++--------- lib/kube/token/validator_test.go | 10 +++++-- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index cff66b95f2454..23786c5abf1df 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -46,8 +46,8 @@ const ( // Kubernetes should support bound tokens on 1.20 and 1.21, // but we can have an apiserver running 1.21 and kubelets running 1.19. kubernetesBoundTokenSupportVersion = "1.22.0" - // kubernetesAudience is the Kubernetes default audience put on SA tokens if we don't specify one. - kubernetesAudience = "https://kubernetes.default.svc" + // kubernetesAudience is the Kubernetes default audience put on SA tokens if we can't get the default audiences. + kubernetesAudience = "https://kubernetes.default.svc.cluster.local" ) type ValidationResult struct { @@ -85,35 +85,59 @@ func (c *ValidationResult) JoinAuditAttributes() (map[string]interface{}, error) // Kubernetes TokenRequest API endpoint. type TokenReviewValidator struct { mu sync.Mutex - // client is protected by mu and should only be accessed via the getClient - // method. - client kubernetes.Interface + // client and clusterAudiences are protected by mu and should only be + // accessed via the getClient method. + client kubernetes.Interface + clusterAudiences []string } // getClient allows the lazy initialisation of the Kubernetes client -func (v *TokenReviewValidator) getClient() (kubernetes.Interface, error) { +func (v *TokenReviewValidator) getClient(ctx context.Context) (kubernetes.Interface, []string, error) { v.mu.Lock() defer v.mu.Unlock() - if v.client != nil { - return v.client, nil + if v.client != nil && v.clusterAudiences != nil { + return v.client, v.clusterAudiences, nil } config, err := rest.InClusterConfig() if err != nil { - return nil, trace.WrapWithMessage(err, "failed to initialize in-cluster Kubernetes config") + return nil, nil, trace.WrapWithMessage(err, "failed to initialize in-cluster Kubernetes config") } client, err := kubernetes.NewForConfig(config) if err != nil { - return nil, trace.WrapWithMessage(err, "failed to initialize in-cluster Kubernetes client") + return nil, nil, trace.WrapWithMessage(err, "failed to initialize in-cluster Kubernetes client") + } + + // We do a self-review to recover the default cluster audience + audiences, err := getTokenAudiences(ctx, client, config.BearerToken) + if err != nil { + audiences = []string{kubernetesAudience} } v.client = client - return client, nil + v.clusterAudiences = audiences + return client, audiences, nil +} + +func getTokenAudiences(ctx context.Context, client kubernetes.Interface, token string) ([]string, error) { + review := &v1.TokenReview{ + Spec: v1.TokenReviewSpec{ + Token: token, + }, + } + options := metav1.CreateOptions{} + + reviewResult, err := client.AuthenticationV1().TokenReviews().Create(ctx, review, options) + if err != nil { + return nil, trace.Wrap(err, "reviewing token and retrieving audience") + } + + return reviewResult.Status.Audiences, nil } // Validate uses the Kubernetes TokenReview API to validate a token and return its UserInfo func (v *TokenReviewValidator) Validate(ctx context.Context, token, clusterName string) (*ValidationResult, error) { - client, err := v.getClient() + client, audiences, err := v.getClient(ctx) if err != nil { return nil, trace.Wrap(err) } @@ -125,7 +149,7 @@ func (v *TokenReviewValidator) Validate(ctx context.Context, token, clusterName // But people kept confusing it with JWKS and set the cluster name // as the audience/. To avoid his common footgun we now allow tokens // whose audience is the teleport cluster name. - Audiences: []string{kubernetesAudience, clusterName}, + Audiences: append(audiences, clusterName), }, } options := metav1.CreateOptions{} diff --git a/lib/kube/token/validator_test.go b/lib/kube/token/validator_test.go index 823ca40214b22..9f385946231f6 100644 --- a/lib/kube/token/validator_test.go +++ b/lib/kube/token/validator_test.go @@ -41,7 +41,10 @@ import ( "github.com/gravitational/teleport/lib/cryptosuites" ) -const testClusterName = "teleport.example.com" +const ( + testClusterName = "teleport.example.com" + testAudience = "test-audience" +) var userGroups = []string{"system:serviceaccounts", "system:serviceaccounts:namespace", "system:authenticated"} @@ -67,7 +70,7 @@ func tokenReviewMock(t *testing.T, reviewResult *v1.TokenReview) func(ctest.Acti require.True(t, ok) require.Equal(t, reviewResult.Spec.Token, reviewRequest.Spec.Token) - require.ElementsMatch(t, reviewRequest.Spec.Audiences, []string{kubernetesAudience, testClusterName}) + require.ElementsMatch(t, reviewRequest.Spec.Audiences, []string{testAudience, testClusterName}) return true, reviewResult, nil } } @@ -239,7 +242,8 @@ func TestIDTokenValidator_Validate(t *testing.T) { client := newFakeClientset(tt.kubeVersion) client.AddReactor("create", "tokenreviews", tokenReviewMock(t, tt.review)) v := TokenReviewValidator{ - client: client, + client: client, + clusterAudiences: []string{testAudience}, } result, err := v.Validate(context.Background(), tt.token, testClusterName) if tt.expectedError != nil { From 714613a5e1f115d4b4161c2c5d30c19cc26e50f2 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Wed, 4 Dec 2024 17:02:25 -0500 Subject: [PATCH 2/6] hard fail --- lib/kube/token/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index 23786c5abf1df..7a78b06fa1d42 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -111,7 +111,7 @@ func (v *TokenReviewValidator) getClient(ctx context.Context) (kubernetes.Interf // We do a self-review to recover the default cluster audience audiences, err := getTokenAudiences(ctx, client, config.BearerToken) if err != nil { - audiences = []string{kubernetesAudience} + return nil, nil, trace.Wrap(err, "doing a self-review") } v.client = client From 64435092aa0fd8bdd0c1586732d8a8fb205a0445 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Wed, 4 Dec 2024 18:31:45 -0500 Subject: [PATCH 3/6] remove unused variable --- lib/kube/token/validator.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index 7a78b06fa1d42..3d4b1d902aa7f 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -46,8 +46,6 @@ const ( // Kubernetes should support bound tokens on 1.20 and 1.21, // but we can have an apiserver running 1.21 and kubelets running 1.19. kubernetesBoundTokenSupportVersion = "1.22.0" - // kubernetesAudience is the Kubernetes default audience put on SA tokens if we can't get the default audiences. - kubernetesAudience = "https://kubernetes.default.svc.cluster.local" ) type ValidationResult struct { From ac0e811beb31785f959f6515cd577d29834806e5 Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Thu, 5 Dec 2024 11:02:40 -0500 Subject: [PATCH 4/6] Address edoardo's feedback + ensure support of older kube versions --- lib/kube/token/validator.go | 51 ++++++++----- lib/kube/token/validator_test.go | 121 ++++++++++++++++++++++++++++--- 2 files changed, 142 insertions(+), 30 deletions(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index 3d4b1d902aa7f..d5234cbc1bd26 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -85,15 +85,19 @@ type TokenReviewValidator struct { mu sync.Mutex // client and clusterAudiences are protected by mu and should only be // accessed via the getClient method. - client kubernetes.Interface + client kubernetes.Interface + // clusterAudiences contains the default Kubernetes cluster audiences. + // This field is populated when getting the Kube client and returned by + // getClient. + // A nil value indicates that the cluster doesn't support audiences. clusterAudiences []string } -// getClient allows the lazy initialisation of the Kubernetes client -func (v *TokenReviewValidator) getClient(ctx context.Context) (kubernetes.Interface, []string, error) { +// getClient allows the lazy initialisation of the Kubernetes client and clusterAudiences +func (v *TokenReviewValidator) getClient(_ context.Context) (kubernetes.Interface, []string, error) { v.mu.Lock() defer v.mu.Unlock() - if v.client != nil && v.clusterAudiences != nil { + if v.client != nil { return v.client, v.clusterAudiences, nil } @@ -107,7 +111,7 @@ func (v *TokenReviewValidator) getClient(ctx context.Context) (kubernetes.Interf } // We do a self-review to recover the default cluster audience - audiences, err := getTokenAudiences(ctx, client, config.BearerToken) + audiences, err := getTokenAudiences(config.BearerToken) if err != nil { return nil, nil, trace.Wrap(err, "doing a self-review") } @@ -117,20 +121,19 @@ func (v *TokenReviewValidator) getClient(ctx context.Context) (kubernetes.Interf return client, audiences, nil } -func getTokenAudiences(ctx context.Context, client kubernetes.Interface, token string) ([]string, error) { - review := &v1.TokenReview{ - Spec: v1.TokenReviewSpec{ - Token: token, - }, +// getTokenAudiences extracts the audience from +func getTokenAudiences(token string) ([]string, error) { + jwt, err := josejwt.ParseSigned(token) + if err != nil { + return nil, trace.Wrap(err) } - options := metav1.CreateOptions{} - - reviewResult, err := client.AuthenticationV1().TokenReviews().Create(ctx, review, options) + claims := &ServiceAccountClaims{} + err = jwt.UnsafeClaimsWithoutVerification(claims) if err != nil { - return nil, trace.Wrap(err, "reviewing token and retrieving audience") + return nil, trace.Wrap(err) } - return reviewResult.Status.Audiences, nil + return claims.Audience, nil } // Validate uses the Kubernetes TokenReview API to validate a token and return its UserInfo @@ -143,13 +146,21 @@ func (v *TokenReviewValidator) Validate(ctx context.Context, token, clusterName review := &v1.TokenReview{ Spec: v1.TokenReviewSpec{ Token: token, - // In-cluster used to only allow tokens with the kubernetes audience - // But people kept confusing it with JWKS and set the cluster name - // as the audience/. To avoid his common footgun we now allow tokens - // whose audience is the teleport cluster name. - Audiences: append(audiences, clusterName), }, } + + // In-cluster used to only allow tokens with the kubernetes audience but people + // kept confusing it with the JWKS kube join method and set the cluster name + // as the audience. To avoid his common footgun we now allow tokens whose + // audience is the teleport cluster name. + // + // We do this only if the Kubernetes cluster supports audiences. + // Earlier Kube versions don't have audience + // support, in this case, we just do a regular token review. + if audiences != nil { + review.Spec.Audiences = append([]string{clusterName}, audiences...) + } + options := metav1.CreateOptions{} reviewResult, err := client.AuthenticationV1().TokenReviews().Create(ctx, review, options) diff --git a/lib/kube/token/validator_test.go b/lib/kube/token/validator_test.go index 9f385946231f6..b949ba057904d 100644 --- a/lib/kube/token/validator_test.go +++ b/lib/kube/token/validator_test.go @@ -43,7 +43,6 @@ import ( const ( testClusterName = "teleport.example.com" - testAudience = "test-audience" ) var userGroups = []string{"system:serviceaccounts", "system:serviceaccounts:namespace", "system:authenticated"} @@ -61,7 +60,7 @@ var legacyTokenKubernetesVersion = version.Info{ } // tokenReviewMock creates a testing.ReactionFunc validating the tokenReview request and answering it -func tokenReviewMock(t *testing.T, reviewResult *v1.TokenReview) func(ctest.Action) (bool, runtime.Object, error) { +func tokenReviewMock(t *testing.T, reviewResult *v1.TokenReview, expectedAudiences []string) func(ctest.Action) (bool, runtime.Object, error) { return func(action ctest.Action) (bool, runtime.Object, error) { createAction, ok := action.(ctest.CreateAction) require.True(t, ok) @@ -70,7 +69,7 @@ func tokenReviewMock(t *testing.T, reviewResult *v1.TokenReview) func(ctest.Acti require.True(t, ok) require.Equal(t, reviewResult.Spec.Token, reviewRequest.Spec.Token) - require.ElementsMatch(t, reviewRequest.Spec.Audiences, []string{testAudience, testClusterName}) + require.ElementsMatch(t, expectedAudiences, reviewRequest.Spec.Audiences) return true, reviewResult, nil } } @@ -96,13 +95,82 @@ func (c *fakeClientSet) Discovery() discovery.DiscoveryInterface { return &c.discovery } +const ( + // The tokens below are test data to validate that we can extract audiences properly from tokens coming from + // different Kubernetes clusters versions. + // Those tokens are only test data and don't provide access to anything. The Kube clusters who generated them + // are long gone. + // If your scanner brought you here, please save everyone's time and DO NOT REPORT accidentally commited tokens. + + // this token has no audience + testDataLegacyToken = "eyJhbGciOiJSUzI1NiIsImtpZCI6IkRVdTJXUGNPLUthZjk2c3ZJcXhFWlBsRHUyUUx5cWxrUm1qT0s2VG9XQTAifQ.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6InRlc3RzYS10b2tlbi1namZxYiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJ0ZXN0c2EiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC51aWQiOiIwYWY0YTc4Zi04ZjEwLTQ2ODUtYWMyOS1jYWUwODY1OWJjYWUiLCJzdWIiOiJzeXN0ZW06c2VydmljZWFjY291bnQ6ZGVmYXVsdDp0ZXN0c2EifQ.Y683lBdmc5e0wvL3BPXj4hVkYXiu7M4bn2w1tvqIdrXdat3Fnqlv5Qgih3RS1VRETXDOMp3_CA8Jv7Nqe_PSIrBPvLStRYvvRByWDWY3PvYDfFxeARKj0E_AQnFkXQxEN21eXkJi3k0i93uhGGNvUr5bNpk7buMbD2UKt-y8N7sQmTIQ6nPdjaC3YHn32-MHpAquqwbTqXhETPrvk9RjSzE4jszcd_P1Gi7BdZoHPErnCc5XaEVBJnHiWsZCsJDL00nAaAS_Ru8TsIdX_z1pyp91sigxfvFrGwhns3oXq54rrhpBuloPwmDW0gWDAV-qjje5MPFuaiov431ocKrOsg\n" + // audience is defaultKubeAudiences + testDataBoundTokenKubeAudiences = "eyJhbGciOiJSUzI1NiIsImtpZCI6InY2VnNnUmdpVS1DNlZWaUVjLV9Wb1dCR2dpZVEtblV0RDFXQjVfY3JOckEifQ.eyJhdWQiOlsiaHR0cHM6Ly9rdWJlcm5ldGVzLmRlZmF1bHQuc3ZjLmNsdXN0ZXIubG9jYWwiLCJrM3MiXSwiZXhwIjoxNzY0OTQ3Mzc3LCJpYXQiOjE3MzM0MTEzNzcsImlzcyI6Imh0dHBzOi8va3ViZXJuZXRlcy5kZWZhdWx0LnN2Yy5jbHVzdGVyLmxvY2FsIiwianRpIjoiMjE4Y2I0ZGQtMzQyNS00Yzc0LThiODktNTRiNzIzNGRiMDE4Iiwia3ViZXJuZXRlcy5pbyI6eyJuYW1lc3BhY2UiOiJkZWZhdWx0Iiwibm9kZSI6eyJuYW1lIjoiazNkLWszcy1kZWZhdWx0LXNlcnZlci0wIiwidWlkIjoiZWMwMTUzZGUtMThiOC00OTk3LTg4ZGQtNjFiYzVmZTkzYThiIn0sInBvZCI6eyJuYW1lIjoiaHVnby10ZXN0LXBvZCIsInVpZCI6ImIxNjliZTdkLWNkYTItNGNhNy04ODVmLWMxNzJiMzZhNjUzNyJ9LCJzZXJ2aWNlYWNjb3VudCI6eyJuYW1lIjoiZGVmYXVsdCIsInVpZCI6IjQxMzgxNWFiLWNjZjctNDI4YS1iNjA4LTllNGUyYmU1OTc2ZiJ9LCJ3YXJuYWZ0ZXIiOjE3MzM0MTQ5ODR9LCJuYmYiOjE3MzM0MTEzNzcsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZWZhdWx0OmRlZmF1bHQifQ.Cs3mekFZzEOk1Gj_w0seURuYC92aY5Xy9WdNz5LtyL0L0eKNnzTV5MNWHgAas--t8ABcvHtcbdS1-XSemqyDfn_GcNJXeZa88bX1PKyG-XdDuqfn40DRxrBXR_sim_2WUGJM2oNh6C6irHzUOQFU0Wmx4oWY3pZ_BSFUlDi3xKnPv-TFWroBVmtc_wLAbCBl5gZF1KngAgMlbdX0szBEwzewkeoFhDTh3OoNWRaRpJL7_YeZsBkKPGY107fFMDXIKmZtd6qyU8-yp3Wwn_1qwucfllNmru8_bncqN18RuDOoQyFej4R93NwntyfzGy1wQexR363QFd7veSgtBS7nJQ" + // audience is customKubeAudiences + testDataBoundTokenCustomAudiences = "eyJhbGciOiJSUzI1NiIsImtpZCI6InY2VnNnUmdpVS1DNlZWaUVjLV9Wb1dCR2dpZVEtblV0RDFXQjVfY3JOckEifQ.eyJhdWQiOlsidGVsZXBvcnQuZXhhbXBsZS5jb20iXSwiZXhwIjoxNzMzNDE2MDEyLCJpYXQiOjE3MzM0MTI0MDUsImlzcyI6Imh0dHBzOi8va3ViZXJuZXRlcy5kZWZhdWx0LnN2Yy5jbHVzdGVyLmxvY2FsIiwianRpIjoiZDE5Yjk2ZjctMTgyYy00ODVjLThkYWYtNzdkMTRhYzA4NmNlIiwia3ViZXJuZXRlcy5pbyI6eyJuYW1lc3BhY2UiOiJkZWZhdWx0Iiwibm9kZSI6eyJuYW1lIjoiazNkLWszcy1kZWZhdWx0LXNlcnZlci0wIiwidWlkIjoiZWMwMTUzZGUtMThiOC00OTk3LTg4ZGQtNjFiYzVmZTkzYThiIn0sInBvZCI6eyJuYW1lIjoiaHVnby10ZXN0LXBvZCIsInVpZCI6Ijc1ZTIwMmFjLTAwZWMtNDVmZC05ZGViLTgwM2JkODA0YjMxNSJ9LCJzZXJ2aWNlYWNjb3VudCI6eyJuYW1lIjoiZGVmYXVsdCIsInVpZCI6IjQxMzgxNWFiLWNjZjctNDI4YS1iNjA4LTllNGUyYmU1OTc2ZiJ9fSwibmJmIjoxNzMzNDEyNDA1LCJzdWIiOiJzeXN0ZW06c2VydmljZWFjY291bnQ6ZGVmYXVsdDpkZWZhdWx0In0.QwhCSQzDrXBNjZU6X642FC_fzglaT80ldCFTXQEwR6IdPvPntjbSAZq8yN6arngctgM6wEh3buc7kq0awmytgF2hbeSRN6PEeRbVvKaAClnCPTzlJYnDq4FOYezqSBZ7jVCW3cNxeU0QCNwj5w8Xy1uxCJu24iWov-ElyxqiCkpa9FjiquOu4kHq9OErXe5ZmXmTDOILnQBzsZnbg-sBKTX-mNAHro8DwQCfFtPmW27iySaScIegqwZNHXbZJDWZYDB2uj3xuHzX75amgPWpfqUq4JaNkf-xlldaH3SdZa5hlL9zvV9e9Dwgqlnergq6EpEZNHmCPb9birQFn46n_w" +) + +var ( + defaultKubeAudiences = []string{"https://kubernetes.default.svc.cluster.local", "k3s"} + customKubeAudiences = []string{testClusterName} +) + +func TestGetTokenAudiences(t *testing.T) { + tests := []struct { + name string + token string + expectedAudiences []string + expectErr require.ErrorAssertionFunc + }{ + { + name: "legacy token with no audience", + token: testDataLegacyToken, + expectedAudiences: nil, + expectErr: require.NoError, + }, + { + name: "modern bound token with default kube audience", + token: testDataBoundTokenKubeAudiences, + expectedAudiences: defaultKubeAudiences, + expectErr: require.NoError, + }, + { + name: "modern bound token with custom audience", + token: testDataBoundTokenCustomAudiences, + expectedAudiences: customKubeAudiences, + expectErr: require.NoError, + }, + { + name: "broken token", + token: "asdfghjkl", + expectedAudiences: nil, + expectErr: require.Error, + }, + { + name: "no token", + token: "", + expectedAudiences: nil, + expectErr: require.Error, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := getTokenAudiences(tt.token) + tt.expectErr(t, err) + assert.Equal(t, tt.expectedAudiences, result) + }) + } +} + func TestIDTokenValidator_Validate(t *testing.T) { tests := []struct { - token string - review *v1.TokenReview - kubeVersion *version.Info - wantResult *ValidationResult - expectedError error + token string + review *v1.TokenReview + kubeVersion *version.Info + wantResult *ValidationResult + clusterAudiences []string + expectedAudiences []string + expectedError error }{ { token: "valid", @@ -130,6 +198,39 @@ func TestIDTokenValidator_Validate(t *testing.T) { }, kubeVersion: &boundTokenKubernetesVersion, expectedError: nil, + // As the cluster doesn't have default audiences, we should not set + // the cluster name in the tokenReview request audiences. + expectedAudiences: nil, + }, + { + token: "valid-with-cluster-audiences", + review: &v1.TokenReview{ + Spec: v1.TokenReviewSpec{ + Token: "valid-with-cluster-audiences", + }, + Status: v1.TokenReviewStatus{ + Authenticated: true, + User: v1.UserInfo{ + Username: "system:serviceaccount:namespace:my-service-account", + UID: "sa-uuid", + Groups: userGroups, + Extra: map[string]v1.ExtraValue{ + "authentication.kubernetes.io/pod-name": {"podA"}, + "authentication.kubernetes.io/pod-uid": {"podA-uuid"}, + }, + }, + }, + }, + wantResult: &ValidationResult{ + Type: types.KubernetesJoinTypeInCluster, + Username: "system:serviceaccount:namespace:my-service-account", + // Raw will be filled in during test run to value of review + }, + kubeVersion: &boundTokenKubernetesVersion, + expectedError: nil, + clusterAudiences: defaultKubeAudiences, + // We check that the cluster name got added to the default kube cluster audiences + expectedAudiences: append([]string{testClusterName}, defaultKubeAudiences...), }, { token: "valid-not-bound", @@ -240,10 +341,10 @@ func TestIDTokenValidator_Validate(t *testing.T) { } client := newFakeClientset(tt.kubeVersion) - client.AddReactor("create", "tokenreviews", tokenReviewMock(t, tt.review)) + client.AddReactor("create", "tokenreviews", tokenReviewMock(t, tt.review, tt.expectedAudiences)) v := TokenReviewValidator{ client: client, - clusterAudiences: []string{testAudience}, + clusterAudiences: tt.clusterAudiences, } result, err := v.Validate(context.Background(), tt.token, testClusterName) if tt.expectedError != nil { From 480d459cd0fbabb7d1dcc5deeb22319f27626d2e Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Thu, 5 Dec 2024 11:57:57 -0500 Subject: [PATCH 5/6] Address edoardo and tiago's feedback + dedup audiences --- lib/kube/token/validator.go | 18 ++++++++++++------ lib/kube/token/validator_test.go | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index d5234cbc1bd26..5e31185e17157 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -21,6 +21,7 @@ package token import ( "context" "encoding/json" + "github.com/gravitational/teleport/api/utils" "slices" "strings" "sync" @@ -110,8 +111,8 @@ func (v *TokenReviewValidator) getClient(_ context.Context) (kubernetes.Interfac return nil, nil, trace.WrapWithMessage(err, "failed to initialize in-cluster Kubernetes client") } - // We do a self-review to recover the default cluster audience - audiences, err := getTokenAudiences(config.BearerToken) + // We extract the audiences from our own token. This allows us to detect the default Kubernetes audiences. + audiences, err := unsafeGetTokenAudiences(config.BearerToken) if err != nil { return nil, nil, trace.Wrap(err, "doing a self-review") } @@ -121,8 +122,11 @@ func (v *TokenReviewValidator) getClient(_ context.Context) (kubernetes.Interfac return client, audiences, nil } -// getTokenAudiences extracts the audience from -func getTokenAudiences(token string) ([]string, error) { +// unsafeGetTokenAudiences extracts the audience from the mounted token. +// THIS FUNCTION DOES NOT VALIDATE THE TOKEN SIGNATURE. +// Bound tokens always have audiences and the list will not be empty. +// Legacy tokens don't have audiences, the result will be an empty list and no error. +func unsafeGetTokenAudiences(token string) ([]string, error) { jwt, err := josejwt.ParseSigned(token) if err != nil { return nil, trace.Wrap(err) @@ -157,8 +161,10 @@ func (v *TokenReviewValidator) Validate(ctx context.Context, token, clusterName // We do this only if the Kubernetes cluster supports audiences. // Earlier Kube versions don't have audience // support, in this case, we just do a regular token review. - if audiences != nil { - review.Spec.Audiences = append([]string{clusterName}, audiences...) + if len(audiences) > 0 { + // We deduplicate because the Teleport cluster name could be one of the default audiences + // And I really don't want to discover if sending the same audience multiple times is valid for Kubernetes. + review.Spec.Audiences = utils.Deduplicate(append([]string{clusterName}, audiences...)) } options := metav1.CreateOptions{} diff --git a/lib/kube/token/validator_test.go b/lib/kube/token/validator_test.go index b949ba057904d..0884bfed37ed0 100644 --- a/lib/kube/token/validator_test.go +++ b/lib/kube/token/validator_test.go @@ -155,7 +155,7 @@ func TestGetTokenAudiences(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := getTokenAudiences(tt.token) + result, err := unsafeGetTokenAudiences(tt.token) tt.expectErr(t, err) assert.Equal(t, tt.expectedAudiences, result) }) From 6d917668fc5511ee7a2d803a9348e6439ec7917c Mon Sep 17 00:00:00 2001 From: hugoShaka Date: Thu, 5 Dec 2024 13:34:14 -0500 Subject: [PATCH 6/6] lint --- lib/kube/token/validator.go | 2 +- lib/kube/token/validator_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/kube/token/validator.go b/lib/kube/token/validator.go index 5e31185e17157..056b5ee1def0d 100644 --- a/lib/kube/token/validator.go +++ b/lib/kube/token/validator.go @@ -21,7 +21,6 @@ package token import ( "context" "encoding/json" - "github.com/gravitational/teleport/api/utils" "slices" "strings" "sync" @@ -38,6 +37,7 @@ import ( "k8s.io/client-go/rest" "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/api/utils" ) const ( diff --git a/lib/kube/token/validator_test.go b/lib/kube/token/validator_test.go index 0884bfed37ed0..49054df1e47ee 100644 --- a/lib/kube/token/validator_test.go +++ b/lib/kube/token/validator_test.go @@ -100,7 +100,7 @@ const ( // different Kubernetes clusters versions. // Those tokens are only test data and don't provide access to anything. The Kube clusters who generated them // are long gone. - // If your scanner brought you here, please save everyone's time and DO NOT REPORT accidentally commited tokens. + // If your scanner brought you here, please save everyone's time and DO NOT REPORT accidentally committed tokens. // this token has no audience testDataLegacyToken = "eyJhbGciOiJSUzI1NiIsImtpZCI6IkRVdTJXUGNPLUthZjk2c3ZJcXhFWlBsRHUyUUx5cWxrUm1qT0s2VG9XQTAifQ.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6InRlc3RzYS10b2tlbi1namZxYiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJ0ZXN0c2EiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC51aWQiOiIwYWY0YTc4Zi04ZjEwLTQ2ODUtYWMyOS1jYWUwODY1OWJjYWUiLCJzdWIiOiJzeXN0ZW06c2VydmljZWFjY291bnQ6ZGVmYXVsdDp0ZXN0c2EifQ.Y683lBdmc5e0wvL3BPXj4hVkYXiu7M4bn2w1tvqIdrXdat3Fnqlv5Qgih3RS1VRETXDOMp3_CA8Jv7Nqe_PSIrBPvLStRYvvRByWDWY3PvYDfFxeARKj0E_AQnFkXQxEN21eXkJi3k0i93uhGGNvUr5bNpk7buMbD2UKt-y8N7sQmTIQ6nPdjaC3YHn32-MHpAquqwbTqXhETPrvk9RjSzE4jszcd_P1Gi7BdZoHPErnCc5XaEVBJnHiWsZCsJDL00nAaAS_Ru8TsIdX_z1pyp91sigxfvFrGwhns3oXq54rrhpBuloPwmDW0gWDAV-qjje5MPFuaiov431ocKrOsg\n"