From 9d2c2dda875e62d72294ff5821ad9d6c2e3bef35 Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Wed, 5 Jun 2024 12:03:05 -0700 Subject: [PATCH] Add validation of iss claim parameter --- cmd/server/app/serve.go | 12 +++++++-- config/server-config.yaml.example | 3 ++- docker-compose.yaml | 1 + internal/auth/jwauth_test.go | 42 ++++++++++++++++++++++++++---- internal/auth/jwtauth.go | 5 +++- internal/config/server/identity.go | 15 ++++++++++- 6 files changed, 68 insertions(+), 10 deletions(-) diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index 92546e0b8a..decd771103 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -87,11 +87,19 @@ var serveCmd = &cobra.Command{ } // Identity - jwksUrl, err := cfg.Identity.Server.Path("realms/stacklok/protocol/openid-connect/certs") + // TODO: cfg.Identity.Server.IssuerUrl _should_ be a URL to an issuer that has an + // .../.well-known/jwks.json or .../.well-known/openid-configuration endpoint. Right + // now it's just a hostname. When we have this, we can consolidate the jwksUrl and issUrl, + // and remove the Keycloak-specific paths. + jwksUrl, err := cfg.Identity.Server.Path("/realms/stacklok/protocol/openid-connect/certs") if err != nil { return fmt.Errorf("failed to create JWKS URL: %w\n", err) } - jwt, err := auth.NewJwtValidator(ctx, jwksUrl.String(), cfg.Identity.Server.Audience) + issUrl, err := cfg.Identity.Server.JwtUrl() + if err != nil { + return fmt.Errorf("failed to create issuer URL: %w\n", err) + } + jwt, err := auth.NewJwtValidator(ctx, jwksUrl.String(), issUrl.String(), cfg.Identity.Server.Audience) if err != nil { return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err) } diff --git a/config/server-config.yaml.example b/config/server-config.yaml.example index a8395aa73d..d77f11f757 100644 --- a/config/server-config.yaml.example +++ b/config/server-config.yaml.example @@ -46,7 +46,8 @@ database: identity: server: - issuer_url: http://keycloak:8080 # Use http://localhost:8081 instead for running minder outside of docker compose + issuer_url: http://localhost:8081 + issuer_claim: http://localhost:8081/realms/stacklok client_id: minder-server client_secret: secret audience: minder diff --git a/docker-compose.yaml b/docker-compose.yaml index e18049fa23..260cc7796c 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -78,6 +78,7 @@ services: condition: service_completed_successfully keycloak-config: condition: service_completed_successfully + migrate: container_name: minder_migrate_up build: diff --git a/internal/auth/jwauth_test.go b/internal/auth/jwauth_test.go index baa1d81e0d..b51f6f484a 100644 --- a/internal/auth/jwauth_test.go +++ b/internal/auth/jwauth_test.go @@ -50,6 +50,8 @@ func TestParseAndValidate(t *testing.T) { err = jwks.AddKey(publicJwk) require.NoError(t, err, "failed to setup JWK set") + issUrl := "https://localhost/realm/foo" + testCases := []struct { name string buildToken func() string @@ -58,7 +60,7 @@ func TestParseAndValidate(t *testing.T) { { name: "Valid token", buildToken: func() string { - token, _ := jwt.NewBuilder().Subject("123").Audience([]string{"minder"}).Expiration(time.Now().Add(time.Duration(1) * time.Minute)).Build() + token, _ := jwtBuilder("123", issUrl, "minder").Expiration(time.Now().Add(time.Duration(1) * time.Minute)).Build() signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, privateJwk)) return string(signed) }, @@ -71,7 +73,7 @@ func TestParseAndValidate(t *testing.T) { { name: "Expired token", buildToken: func() string { - token, _ := jwt.NewBuilder().Subject("123").Audience([]string{"minder"}).Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() + token, _ := jwtBuilder("123", issUrl, "minder").Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, privateJwk)) return string(signed) }, @@ -88,7 +90,7 @@ func TestParseAndValidate(t *testing.T) { otherJwk, _ := jwk.FromRaw(otherKey) err = otherJwk.Set(jwk.KeyIDKey, `otherKey`) require.NoError(t, err, "failed to setup signing key ID") - token, _ := jwt.NewBuilder().Subject("123").Expiration(time.Now().Add(time.Duration(1) * time.Minute)).Build() + token, _ := jwtBuilder("123", issUrl, "minder").Expiration(time.Now().Add(time.Duration(1) * time.Minute)).Build() signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, otherJwk)) return string(signed) }, @@ -112,7 +114,20 @@ func TestParseAndValidate(t *testing.T) { { name: "Missing subject claim", buildToken: func() string { - token, _ := jwt.NewBuilder().Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() + token, _ := jwtBuilder("", issUrl, "minder").Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() + signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, privateJwk)) + return string(signed) + }, + checkError: func(t *testing.T, err error) { + t.Helper() + + assert.Error(t, err) + }, + }, + { + name: "Missing issuer claim", + buildToken: func() string { + token, _ := jwtBuilder("123", "", "minder").Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, privateJwk)) return string(signed) }, @@ -125,7 +140,7 @@ func TestParseAndValidate(t *testing.T) { { name: "Missing audience claim", buildToken: func() string { - token, _ := jwt.NewBuilder().Subject("123").Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() + token, _ := jwtBuilder("123", issUrl, "").Expiration(time.Now().Add(-time.Duration(1) * time.Minute)).Build() signed, _ := jwt.Sign(token, jwt.WithKey(jwa.RS256, privateJwk)) return string(signed) }, @@ -149,6 +164,7 @@ func TestParseAndValidate(t *testing.T) { jwtValidator := JwkSetJwtValidator{ jwksFetcher: mockKeyFetcher, + iss: issUrl, aud: "minder", } _, err := jwtValidator.ParseAndValidate(tc.buildToken()) @@ -167,3 +183,19 @@ func randomKeypair(length int) (*rsa.PrivateKey, *rsa.PublicKey) { return privateKey, publicKey } + +func jwtBuilder(sub, iss, aud string) *jwt.Builder { + r := jwt.NewBuilder() + + if sub != "" { + r = r.Subject(sub) + } + if iss != "" { + r = r.Issuer(iss) + } + if aud != "" { + r = r.Audience([]string{aud}) + } + + return r +} diff --git a/internal/auth/jwtauth.go b/internal/auth/jwtauth.go index 43fc2cea70..98bdd02f48 100644 --- a/internal/auth/jwtauth.go +++ b/internal/auth/jwtauth.go @@ -34,6 +34,7 @@ type JwtValidator interface { // JwkSetJwtValidator is a JWT validator that uses a JWK set URL to validate the tokens type JwkSetJwtValidator struct { jwksFetcher KeySetFetcher + iss string aud string } @@ -64,6 +65,7 @@ func (j *JwkSetJwtValidator) ParseAndValidate(tokenString string) (openid.Token, token, err := jwt.ParseString( tokenString, jwt.WithKeySet(set), + jwt.WithIssuer(j.iss), jwt.WithValidate(true), jwt.WithToken(openid.New()), jwt.WithAudience(j.aud)) @@ -84,7 +86,7 @@ func (j *JwkSetJwtValidator) ParseAndValidate(tokenString string) (openid.Token, } // NewJwtValidator creates a new JWT validator that uses a JWK set URL to validate the tokens -func NewJwtValidator(ctx context.Context, jwksUrl string, aud string) (JwtValidator, error) { +func NewJwtValidator(ctx context.Context, jwksUrl string, issUrl string, aud string) (JwtValidator, error) { // Cache the JWK set // The cache will refresh every 15 minutes by default jwks := jwk.NewCache(ctx) @@ -106,6 +108,7 @@ func NewJwtValidator(ctx context.Context, jwksUrl string, aud string) (JwtValida } return &JwkSetJwtValidator{ jwksFetcher: &keySetCache, + iss: issUrl, aud: aud, }, nil } diff --git a/internal/config/server/identity.go b/internal/config/server/identity.go index 347be7d53f..8c0fd63733 100644 --- a/internal/config/server/identity.go +++ b/internal/config/server/identity.go @@ -36,8 +36,12 @@ type IdentityConfigWrapper struct { // IdentityConfig is the configuration for the identity provider in minder server type IdentityConfig struct { - // IssuerUrl is the base URL where the identity server is running + // IssuerUrl is the base URL for calling APIs on the identity server. Note that this URL + // ised for direct communication with the identity server, and is not the URL that + // is included in the JWT tokens. It is named 'issuer_url' for historical compatibility. IssuerUrl string `mapstructure:"issuer_url" default:"http://localhost:8081"` + // IssuerClaim is the claim in the JWT token that identifies the issuer + IssuerClaim string `mapstructure:"issuer_claim" default:"http://localhost:8081/realms/stacklok"` // ClientId is the client ID that identifies the minder server ClientId string `mapstructure:"client_id" default:"minder-server"` // ClientSecret is the client secret for the minder server @@ -59,6 +63,15 @@ func RegisterIdentityFlags(v *viper.Viper, flags *pflag.FlagSet) error { "The base URL where the identity server is running", flags.String) } +// JwtUrl returns the base `iss` claim as a URL. +func (sic *IdentityConfig) JwtUrl(elem ...string) (*url.URL, error) { + parsedUrl, err := url.Parse(sic.IssuerClaim) + if err != nil { + return nil, err + } + return parsedUrl.JoinPath(elem...), nil +} + // Path returns a URL for the given path on the identity server func (sic *IdentityConfig) Path(path string) (*url.URL, error) { parsedUrl, err := url.Parse(sic.IssuerUrl)