From 452a379d0306641f35b53ac6d65a1c7a024328b5 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Thu, 30 Jan 2025 17:20:29 -0800 Subject: [PATCH] Address feedback from eleftherias --- cmd/server/app/serve.go | 4 +- internal/auth/context.go | 2 +- internal/auth/githubactions/githubactions.go | 16 +------ .../auth/githubactions/githubactions_test.go | 16 +------ internal/auth/jwt/dynamic/dynamic_fetch.go | 45 ++++++++++-------- .../auth/jwt/dynamic/dynamic_fetch_test.go | 47 ++++++++++++------- internal/auth/jwt/merged/merged_jwt.go | 16 +------ internal/controlplane/handlers_authz.go | 2 +- pkg/config/server/identity.go | 3 +- 9 files changed, 68 insertions(+), 83 deletions(-) diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index 1bc2daf938..d351285594 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -96,7 +96,9 @@ var serveCmd = &cobra.Command{ if err != nil { return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err) } - dynamicJwt := dynamic.NewDynamicValidator(ctx, cfg.Identity.Server.Audience) + allowedIssuers := []string{issUrl.String()} + allowedIssuers = append(allowedIssuers, cfg.Identity.AdditionalIssuers...) + dynamicJwt := dynamic.NewDynamicValidator(ctx, cfg.Identity.Server.Audience, allowedIssuers) jwt := merged.Validator{Validators: []jwt.Validator{staticJwt, dynamicJwt}} authzc, err := authz.NewAuthzClient(&cfg.Authz, l) diff --git a/internal/auth/context.go b/internal/auth/context.go index 62eb39a26f..de5dd56a3e 100644 --- a/internal/auth/context.go +++ b/internal/auth/context.go @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors // SPDX-License-Identifier: Apache-2.0 package auth diff --git a/internal/auth/githubactions/githubactions.go b/internal/auth/githubactions/githubactions.go index 6d280349e0..f68ae658a6 100644 --- a/internal/auth/githubactions/githubactions.go +++ b/internal/auth/githubactions/githubactions.go @@ -1,17 +1,5 @@ -// -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 // Package githubactions provides an implementation of the GitHub IdentityProvider. package githubactions diff --git a/internal/auth/githubactions/githubactions_test.go b/internal/auth/githubactions/githubactions_test.go index a7ef6c4c51..8955f7f66e 100644 --- a/internal/auth/githubactions/githubactions_test.go +++ b/internal/auth/githubactions/githubactions_test.go @@ -1,17 +1,5 @@ -// -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 // Package githubactions provides an implementation of the GitHub IdentityProvider. package githubactions diff --git a/internal/auth/jwt/dynamic/dynamic_fetch.go b/internal/auth/jwt/dynamic/dynamic_fetch.go index 387e62d34b..3119fb0503 100644 --- a/internal/auth/jwt/dynamic/dynamic_fetch.go +++ b/internal/auth/jwt/dynamic/dynamic_fetch.go @@ -1,20 +1,8 @@ -// -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 // Package dynamic provides the logic for reading and validating JWT tokens -// using a JWKS URL from the token's +// using a JWKS URL from the token's `iss` claim. package dynamic import ( @@ -24,6 +12,7 @@ import ( "fmt" "io" "net/http" + "slices" "strings" "sync" "time" @@ -45,19 +34,21 @@ type openIdConfig struct { } var cachedIssuers metric.Int64Counter +var deniedIssuers metric.Int64Counter var dynamicAuths metric.Int64Counter var metricsInit sync.Once // Validator dynamically validates JWTs by fetching the key from the well-known OIDC issuer URL. type Validator struct { - jwks *jwk.Cache - aud string + jwks *jwk.Cache + aud string + allowedIssuers []string } var _ stacklok_jwt.Validator = (*Validator)(nil) // NewDynamicValidator creates a new instance of the dynamic JWT validator -func NewDynamicValidator(ctx context.Context, aud string) *Validator { +func NewDynamicValidator(ctx context.Context, aud string, issuers []string) *Validator { metricsInit.Do(func() { meter := otel.Meter("minder") var err error @@ -68,6 +59,13 @@ func NewDynamicValidator(ctx context.Context, aud string) *Validator { if err != nil { zerolog.Ctx(context.Background()).Warn().Err(err).Msg("Creating gauge for cached issuers failed") } + deniedIssuers, err = meter.Int64Counter("dynamic_jwt.denied_issuers", + metric.WithDescription("Number of denied issuers for dynamic JWT validation"), + metric.WithUnit("count"), + ) + if err != nil { + zerolog.Ctx(context.Background()).Warn().Err(err).Msg("Creating gauge for denied issuers failed") + } dynamicAuths, err = meter.Int64Counter("dynamic_jwt.auths", metric.WithDescription("Number of dynamic JWT authentications"), metric.WithUnit("count"), @@ -77,8 +75,9 @@ func NewDynamicValidator(ctx context.Context, aud string) *Validator { } }) return &Validator{ - jwks: jwk.NewCache(ctx), - aud: aud, + jwks: jwk.NewCache(ctx), + aud: aud, + allowedIssuers: issuers, } } @@ -122,6 +121,12 @@ func (m Validator) ParseAndValidate(tokenString string) (openid.Token, error) { } func (m Validator) getKeySet(issuer string) (jwk.Set, error) { + if !slices.Contains(m.allowedIssuers, issuer) { + if deniedIssuers != nil { + deniedIssuers.Add(context.Background(), 1) + } + return nil, fmt.Errorf("issuer %s is not allowed", issuer) + } jwksUrl, err := getJWKSUrlForOpenId(issuer) if err != nil { return nil, fmt.Errorf("failed to fetch JWKS URL from openid: %w", err) diff --git a/internal/auth/jwt/dynamic/dynamic_fetch_test.go b/internal/auth/jwt/dynamic/dynamic_fetch_test.go index 2c66058bc3..1967b75ebe 100644 --- a/internal/auth/jwt/dynamic/dynamic_fetch_test.go +++ b/internal/auth/jwt/dynamic/dynamic_fetch_test.go @@ -1,20 +1,8 @@ -// -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 // Package dynamic provides the logic for reading and validating JWT tokens -// using a JWKS URL from the token's +// using a JWKS URL from the token's `iss` claim. package dynamic import ( @@ -95,10 +83,12 @@ func TestValidator_ParseAndValidate(t *testing.T) { tests := []struct { name string + issuers []string getToken func(t *testing.T) (string, openid.Token) wantErr string }{{ name: "valid token", + issuers: []string{server.URL}, getToken: func(t *testing.T) (string, openid.Token) { t.Helper() token, err := openid.NewBuilder(). @@ -115,6 +105,7 @@ func TestValidator_ParseAndValidate(t *testing.T) { }, }, { name: "valid token, other issuer", + issuers: []string{server.URL + "/other"}, getToken: func(t *testing.T) (string, openid.Token) { t.Helper() token, err := openid.NewBuilder(). @@ -131,6 +122,7 @@ func TestValidator_ParseAndValidate(t *testing.T) { }, }, { name: "invalid signature", + issuers: []string{server.URL}, getToken: func(_ *testing.T) (string, openid.Token) { t.Helper() return "invalid", nil @@ -138,10 +130,11 @@ func TestValidator_ParseAndValidate(t *testing.T) { wantErr: `failed to split compact JWT: invalid number of segments`, }, { name: "expired jwt", + issuers: []string{server.URL}, getToken: func(_ *testing.T) (string, openid.Token) { t.Helper() token, err := openid.NewBuilder(). - Issuer(server.URL + "/elsewhere"). + Issuer(server.URL). Subject("test"). Audience([]string{"minder"}). Expiration(time.Now().Add(-1 * time.Minute)). @@ -155,6 +148,7 @@ func TestValidator_ParseAndValidate(t *testing.T) { wantErr: `failed to parse JWT payload: "exp" not satisfied`, }, { name: "bad well-known URL", + issuers: []string{server.URL, server.URL + "/elsewhere"}, getToken: func(t *testing.T) (string, openid.Token) { t.Helper() token, err := openid.NewBuilder(). @@ -172,6 +166,7 @@ func TestValidator_ParseAndValidate(t *testing.T) { wantErr: `non-200 response code "404 Not Found"`, }, { name: "bad issuer", + issuers: []string{server.URL, server.URL + "/nothing"}, getToken: func(t *testing.T) (string, openid.Token) { t.Helper() token, err := openid.NewBuilder(). @@ -187,12 +182,30 @@ func TestValidator_ParseAndValidate(t *testing.T) { return string(signed), token }, wantErr: `failed to fetch JWKS URL`, + }, { + name: "prohibited issuer", + issuers: []string{server.URL}, + getToken: func(t *testing.T) (string, openid.Token) { + t.Helper() + token, err := openid.NewBuilder(). + Issuer(server.URL + "/nothing"). + Subject("test"). + Audience([]string{"minder"}). + Expiration(time.Now().Add(time.Minute)). + IssuedAt(time.Now()). + Build() + require.NoError(t, err) + signed, err := jwt.Sign(token, jwt.WithKey(jwa.RS256, jwkKey)) + require.NoError(t, err) + return string(signed), token + }, + wantErr: `is not allowed`, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - validator := NewDynamicValidator(ctx, "minder") + validator := NewDynamicValidator(ctx, "minder", tt.issuers) token, want := tt.getToken(t) got, err := validator.ParseAndValidate(string(token)) diff --git a/internal/auth/jwt/merged/merged_jwt.go b/internal/auth/jwt/merged/merged_jwt.go index 5066e23828..a2ca77bca0 100644 --- a/internal/auth/jwt/merged/merged_jwt.go +++ b/internal/auth/jwt/merged/merged_jwt.go @@ -1,17 +1,5 @@ -// -// Copyright 2024 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 // Package merged provides the logic for reading and validating JWT tokens package merged diff --git a/internal/controlplane/handlers_authz.go b/internal/controlplane/handlers_authz.go index ddf9444f2e..026834da57 100644 --- a/internal/controlplane/handlers_authz.go +++ b/internal/controlplane/handlers_authz.go @@ -467,7 +467,7 @@ func (s *Server) UpdateRole(ctx context.Context, req *minder.UpdateRoleRequest) // isUserSelfUpdating is used to prevent if the user is trying to update their own role func isUserSelfUpdating(ctx context.Context, subject, inviteeEmail string) error { if subject != "" { - if auth.IdentityFromContext(ctx).Human() == subject { + if auth.IdentityFromContext(ctx).String() == subject { return util.UserVisibleError(codes.InvalidArgument, "cannot update your own role") } } diff --git a/pkg/config/server/identity.go b/pkg/config/server/identity.go index 6466ee86ce..c4b6ca3d9d 100644 --- a/pkg/config/server/identity.go +++ b/pkg/config/server/identity.go @@ -19,7 +19,8 @@ import ( // IdentityConfigWrapper is the configuration for the identity provider type IdentityConfigWrapper struct { - Server IdentityConfig `mapstructure:"server"` + Server IdentityConfig `mapstructure:"server"` + AdditionalIssuers []string `mapstructure:"additional_issuers"` } // IdentityConfig is the configuration for the identity provider in minder server