Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add validation of iss claim parameter #3552

Merged
merged 2 commits into from
Jun 12, 2024
Merged

Add validation of iss claim parameter #3552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d2c2dd
Add validation of iss claim parameter
evankanderson Jun 5, 2024
5c4244f
Merge branch 'main' into validate-issuer
dmjb Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions cmd/server/app/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this comment is accurate. Maybe it's better to rename the issuerUrl to hostnameUrl. We intentionally use the hostname for deriving the user deletion URL as well, which is also Keycloak specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... so, in an ideal world, the URL that Keycloak includes in the iss claim would be reachable by the minder server and then we could use the standard OpenID JWKS discovery.

Unfortunately, the current docker-compose setup produces an iss of http://localhost:8081/realms/stacklok, but the minder container can only reach Keycloak via keycloak:8080. This isn't a problem in staging or production, where Minder can reach the external interface, and we just happen to fetch some keys from an entirely different URL and use them to validate the JWT.

Let me think on this, and see if I can come up with a better way to handle this.

And yes, we should rename issuer_url to identity_host or identity_url, but I didn't want to gratuitously break existing configuration (including our own).

That also reminds me that I need to set these fields in staging and production before we push this code there...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields are now set in production.

// .../.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)
}
Expand Down
3 changes: 2 additions & 1 deletion config/server-config.yaml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -49 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want to have the default be the all-in-docker setup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can set this whichever way -- we override the config value with a flag in docker-compose for the all-in-docker setup.

client_id: minder-server
client_secret: secret
audience: minder
Expand Down
1 change: 1 addition & 0 deletions docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ services:
condition: service_completed_successfully
keycloak-config:
condition: service_completed_successfully

migrate:
container_name: minder_migrate_up
build:
Expand Down
42 changes: 37 additions & 5 deletions internal/auth/jwauth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
},
Expand All @@ -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)
},
Expand All @@ -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)
},
Expand All @@ -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)
},
Expand All @@ -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)
},
Expand All @@ -149,6 +164,7 @@ func TestParseAndValidate(t *testing.T) {

jwtValidator := JwkSetJwtValidator{
jwksFetcher: mockKeyFetcher,
iss: issUrl,
aud: "minder",
}
_, err := jwtValidator.ParseAndValidate(tc.buildToken())
Expand All @@ -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
}
5 changes: 4 additions & 1 deletion internal/auth/jwtauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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))
Expand All @@ -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)
Expand All @@ -106,6 +108,7 @@ func NewJwtValidator(ctx context.Context, jwksUrl string, aud string) (JwtValida
}
return &JwkSetJwtValidator{
jwksFetcher: &keySetCache,
iss: issUrl,
aud: aud,
}, nil
}
Expand Down
15 changes: 14 additions & 1 deletion internal/config/server/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down