diff --git a/pkg/oidc/client.go b/pkg/oidc/client.go index 220a4d3c57..dfdfe367a9 100644 --- a/pkg/oidc/client.go +++ b/pkg/oidc/client.go @@ -8,6 +8,7 @@ import ( "io" "mime" "net/http" + "slices" "strconv" "strings" "sync" @@ -58,6 +59,7 @@ type oidcClient struct { skipIssuerValidation bool accessTokenVerifyMethod string accessTokenAudiences []string + accessTokenClientIDs []string remoteKeySet KeySet algorithms []string @@ -95,6 +97,14 @@ func NewOIDCClient(opts ...Option) (OIDCClient, error) { return nil, errors.New("access token audiences must not contain empty or whitespace-only entries") } } + if len(options.AccessTokenClientIDs) > 0 && options.AccessTokenVerifyMethod != config.AccessTokenVerificationJWT { + return nil, errors.New("access token client id validation requires the jwt verification method") + } + for _, clientID := range options.AccessTokenClientIDs { + if strings.TrimSpace(clientID) == "" { + return nil, errors.New("access token client ids must not contain empty or whitespace-only entries") + } + } return &oidcClient{ Logger: options.Logger, @@ -102,6 +112,7 @@ func NewOIDCClient(opts ...Option) (OIDCClient, error) { httpClient: options.HTTPClient, accessTokenVerifyMethod: options.AccessTokenVerifyMethod, accessTokenAudiences: options.AccessTokenAudiences, + accessTokenClientIDs: options.AccessTokenClientIDs, JWKSOptions: options.JWKSOptions, // TODO I don't like that we pass down config options ... JWKS: options.JWKS, providerLock: &sync.Mutex{}, @@ -323,10 +334,50 @@ func (c *oidcClient) verifyAccessTokenJWT(token string) (RegClaimsWithSID, jwt.M c.Logger.Info().Err(err).Msg("Failed to parse/verify the access token.") return claims, mapClaims, err } + if err := c.verifyClientID(mapClaims); err != nil { + c.Logger.Info().Err(err).Msg("Rejected the access token's client.") + return RegClaimsWithSID{}, jwt.MapClaims{}, err + } return claims, mapClaims, nil } +// ErrTokenInvalidClientID is returned when an access token was issued to a +// client that is not in the configured list of allowed client ids. +var ErrTokenInvalidClientID = errors.New("token was issued to a client that is not allowed") + +// verifyClientID checks the client the access token was issued to against the +// configured allow list. Audiences cannot do this: a client can usually ask for +// any audience, so a token minted through another client of the same IDP, with +// that client's weaker login policy (MFA, allowed users, networks), carries the +// expected aud just as well. The client id is assigned by the IDP. +// +// The client is read from client_id (RFC 9068, JWT access tokens) and, if that +// is absent, from azp (OpenID Connect, e.g. Keycloak). +func (c *oidcClient) verifyClientID(claims jwt.MapClaims) error { + if len(c.accessTokenClientIDs) == 0 { + return nil + } + raw, ok := claims["client_id"] + if !ok { + raw, ok = claims["azp"] + } + if !ok || raw == nil { + return fmt.Errorf("%w: client_id/azp", jwt.ErrTokenRequiredClaimMissing) + } + clientID, ok := raw.(string) + if !ok { + return fmt.Errorf("%w: client_id/azp is not a string", jwt.ErrTokenMalformed) + } + if clientID == "" { + return fmt.Errorf("%w: client_id/azp", jwt.ErrTokenRequiredClaimMissing) + } + if !slices.Contains(c.accessTokenClientIDs, clientID) { + return ErrTokenInvalidClientID + } + return nil +} + func (c *oidcClient) VerifyLogoutToken(ctx context.Context, rawToken string) (*LogoutToken, error) { var claims LogoutToken if err := c.lookupWellKnownOpenidConfiguration(ctx); err != nil { diff --git a/pkg/oidc/client_id_test.go b/pkg/oidc/client_id_test.go new file mode 100644 index 0000000000..ff8166064d --- /dev/null +++ b/pkg/oidc/client_id_test.go @@ -0,0 +1,148 @@ +package oidc_test + +import ( + "context" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/pkg/oidc" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestAccessTokenClientIDs(t *testing.T) { + key := newRSAKey(t) + tests := []struct { + name string + clientIDs []string + claims jwt.MapClaims + wantErr error + }{ + {name: "disabled accepts any client", claims: jwt.MapClaims{"client_id": "other"}}, + {name: "disabled accepts no client", claims: jwt.MapClaims{}}, + {name: "explicitly empty configuration", clientIDs: []string{}, claims: jwt.MapClaims{"azp": "other"}}, + {name: "client_id", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "web"}}, + {name: "azp", clientIDs: []string{"web"}, claims: jwt.MapClaims{"azp": "web"}}, + {name: "any allowed client", clientIDs: []string{"web", "desktop"}, claims: jwt.MapClaims{"client_id": "desktop"}}, + {name: "client_id wins over azp", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "web", "azp": "other"}}, + {name: "client_id not allowed despite allowed azp", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "other", "azp": "web"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "foreign client", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "other"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "foreign azp", clientIDs: []string{"web"}, claims: jwt.MapClaims{"azp": "other"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "allowed audience does not help", clientIDs: []string{"web"}, claims: jwt.MapClaims{"aud": "web", "azp": "other"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "case sensitive", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "Web"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "exact match", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": "web-legacy"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "no wildcard matching", clientIDs: []string{"*"}, claims: jwt.MapClaims{"client_id": "web"}, wantErr: oidc.ErrTokenInvalidClientID}, + {name: "missing", clientIDs: []string{"web"}, claims: jwt.MapClaims{}, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "null", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": nil}, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "empty", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": ""}, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "number", clientIDs: []string{"web"}, claims: jwt.MapClaims{"client_id": 123}, wantErr: jwt.ErrTokenMalformed}, + {name: "array", clientIDs: []string{"web"}, claims: jwt.MapClaims{"azp": []string{"web"}}, wantErr: jwt.ErrTokenMalformed}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + claims := jwt.MapClaims{ + "iss": "https://issuer.example", + "sub": "alice", + "exp": time.Now().Add(time.Hour).Unix(), + } + for k, v := range tt.claims { + claims[k] = v + } + client := newClientIDTestClient(t, key, tt.clientIDs) + registered, all, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, claims)) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + require.Empty(t, all, "claims of a rejected token must not be returned") + require.Empty(t, registered.Subject, "claims of a rejected token must not be returned") + return + } + require.NoError(t, err) + require.Equal(t, "alice", registered.Subject) + require.Equal(t, "alice", all["sub"]) + }) + } +} + +func TestAccessTokenClientIDsWithAudiences(t *testing.T) { + key := newRSAKey(t) + client, err := oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), + oidc.WithOidcIssuer("https://issuer.example"), + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + oidc.WithAccessTokenAudiences([]string{"opencloud"}), + oidc.WithAccessTokenClientIDs([]string{"web"}), + oidc.WithJWKS(key.jwks), + oidc.WithProviderMetadata(&oidc.ProviderMetadata{}), + ) + require.NoError(t, err) + verify := func(claims jwt.MapClaims) error { + claims["iss"] = "https://issuer.example" + _, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, claims)) + return err + } + require.NoError(t, verify(jwt.MapClaims{"aud": "opencloud", "azp": "web"})) + require.ErrorIs(t, verify(jwt.MapClaims{"aud": "opencloud", "azp": "other"}), oidc.ErrTokenInvalidClientID) + require.ErrorIs(t, verify(jwt.MapClaims{"aud": "other", "azp": "web"}), jwt.ErrTokenInvalidAudience) +} + +func TestAccessTokenClientIDConfiguration(t *testing.T) { + for _, method := range []string{config.AccessTokenVerificationNone, ""} { + t.Run("incompatible method "+method, func(t *testing.T) { + // No HTTP client is supplied: invalid configuration must fail before discovery. + client, err := oidc.NewOIDCClient( + oidc.WithAccessTokenVerifyMethod(method), + oidc.WithAccessTokenClientIDs([]string{"web"}), + ) + require.ErrorContains(t, err, "requires the jwt verification method") + require.Nil(t, client) + }) + } + for _, clientIDs := range [][]string{{""}, {" \t"}, {"web", ""}} { + client, err := oidc.NewOIDCClient( + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + oidc.WithAccessTokenClientIDs(clientIDs), + ) + require.ErrorContains(t, err, "empty or whitespace-only") + require.Nil(t, client) + } + t.Run("caller cannot mutate the policy", func(t *testing.T) { + key := newRSAKey(t) + clientIDs := []string{"web"} + client := newClientIDTestClient(t, key, clientIDs) + clientIDs[0] = "other" + _, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, + jwt.MapClaims{"iss": "https://issuer.example", "client_id": "other"})) + require.ErrorIs(t, err, oidc.ErrTokenInvalidClientID) + }) +} + +func TestAccessTokenClientIDsDoNotApplyToLogoutTokens(t *testing.T) { + key := newRSAKey(t) + client := newClientIDTestClient(t, key, []string{"web"}) + token := signAccessToken(t, key, jwt.MapClaims{ + "iss": "https://issuer.example", + "sub": "alice", + "aud": "other", + "events": map[string]any{ + "http://schemas.openid.net/event/backchannel-logout": map[string]any{}, + }, + }) + _, err := client.VerifyLogoutToken(context.Background(), token) + require.NoError(t, err) +} + +func newClientIDTestClient(t *testing.T, key *signingKey, clientIDs []string) oidc.OIDCClient { + t.Helper() + client, err := oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), + oidc.WithOidcIssuer("https://issuer.example"), + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + oidc.WithAccessTokenClientIDs(clientIDs), + oidc.WithJWKS(key.jwks), + oidc.WithProviderMetadata(&oidc.ProviderMetadata{}), + ) + require.NoError(t, err) + return client +} diff --git a/pkg/oidc/options.go b/pkg/oidc/options.go index 9d27d95657..5157575a58 100644 --- a/pkg/oidc/options.go +++ b/pkg/oidc/options.go @@ -38,6 +38,9 @@ type Options struct { // AccessTokenAudiences requires at least one matching audience in access tokens. // An empty list disables audience validation. AccessTokenAudiences []string + // AccessTokenClientIDs requires the client an access token was issued to + // (client_id, else azp) to be one of these. An empty list disables the check. + AccessTokenClientIDs []string // Config to use Config *goidc.Config @@ -85,6 +88,14 @@ func WithAccessTokenAudiences(val []string) Option { } } +// WithAccessTokenClientIDs sets the clients whose access tokens are accepted. +// An empty list disables client validation. +func WithAccessTokenClientIDs(val []string) Option { + return func(o *Options) { + o.AccessTokenClientIDs = append([]string(nil), val...) + } +} + // WithHTTPClient provides a function to set the httpClient option. func WithHTTPClient(val *http.Client) Option { return func(o *Options) { diff --git a/services/proxy/README.md b/services/proxy/README.md index adf64fb85f..ea4c36509a 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -79,6 +79,42 @@ The disabled default is a compatibility decision. It does not relax the a resource server following that JWT access token profile must reject tokens whose audience does not identify the resource server. +### OIDC Allowed Client IDs + +An audience check alone does not keep out tokens minted through *other clients* +of the same identity provider. Many IDPs let a client request any audience, and +with dynamic client registration anyone can create such a client. Identity +providers attach their login policy to the client: who may sign in, MFA, +allowed networks and so on. A token obtained through a weaker client carries +the expected audience just as well and bypasses that policy. + +To accept only tokens issued to your OpenCloud clients, list their client ids: + +```console +PROXY_OIDC_ALLOWED_CLIENT_IDS=web,OpenCloudDesktop,OpenCloudAndroid,OpenCloudIOS +PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt +``` + +or in `proxy.yaml`: + +```yaml +oidc: + allowed_client_ids: + - web + - OpenCloudDesktop + access_token_verify_method: jwt +``` + +The client is read from the access token's `client_id` claim +([RFC 9068](https://www.rfc-editor.org/rfc/rfc9068.html#name-data-structure)) and, +if that is absent, from `azp` (OpenID Connect; Keycloak, for example). The value +must match one entry exactly and case-sensitively. Tokens without a client, with +a malformed one or with a client that is not listed receive HTTP 401 on +protected routes. Like audiences, this requires JWT verification, entries must +not be blank, an empty list disables the check, and it is evaluated on Userinfo +cache misses. The check works together with `PROXY_OIDC_AUDIENCES`; both must +pass when both are configured. + ## Configuring Routes The proxy handles routing to all endpoints that OpenCloud offers. The currently availabe default routes can be found [in the code](https://github.com/opencloud-eu/opencloud/blob/main/services/proxy/pkg/config/defaults/defaultconfig.go). Changing or adding routes can be necessary when writing own OpenCloud extensions. diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 4beba8f79f..ed42369263 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -265,6 +265,7 @@ func newOIDCAuthenticator(logger log.Logger, cfg *config.Config, userInfoCache m oidcClient, err := oidc.NewOIDCClient( oidc.WithAccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), oidc.WithAccessTokenAudiences(cfg.OIDC.Audiences), + oidc.WithAccessTokenClientIDs(cfg.OIDC.AllowedClientIDs), oidc.WithLogger(logger), oidc.WithHTTPClient(httpClient), oidc.WithOidcIssuer(cfg.OIDC.Issuer), diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 838cf48595..ad538bbf9c 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -118,6 +118,7 @@ const ( // with the configured oidc-provider type OIDC struct { Audiences []string `yaml:"audiences" env:"PROXY_OIDC_AUDIENCES" desc:"Optional comma-separated list of allowed audiences for OIDC access tokens. Empty disables audience validation for compatibility. Configuring audiences is recommended for production and requires PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt. Tokens must contain at least one exactly matching, case-sensitive audience in their aud claim." introductionVersion:"8.1.0"` + AllowedClientIDs []string `yaml:"allowed_client_ids" env:"PROXY_OIDC_ALLOWED_CLIENT_IDS" desc:"Optional comma-separated list of OIDC client ids whose access tokens are accepted. The client is read from the token's client_id claim, or azp if client_id is absent. Empty disables client validation for compatibility. Requires PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt. Unlike audiences, the client id is assigned by the IDP, so this keeps tokens minted through other clients of the same IDP (with their own login policies) out." introductionVersion:"%%NEXT%%"` Issuer string `yaml:"issuer" env:"OC_URL;OC_OIDC_ISSUER;PROXY_OIDC_ISSUER" desc:"URL of the OIDC issuer. It defaults to URL of the builtin IDP." introductionVersion:"1.0.0"` Insecure bool `yaml:"insecure" env:"OC_INSECURE;PROXY_OIDC_INSECURE" desc:"Disable TLS certificate validation for connections to the IDP. Note that this is not recommended for production environments." introductionVersion:"1.0.0"` AccessTokenVerifyMethod string `yaml:"access_token_verify_method" env:"PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD" desc:"Sets how OIDC access tokens should be verified. Possible values are 'none' and 'jwt'. When using 'none', no special validation apart from using it for accessing the IDP's userinfo endpoint will be done. When using 'jwt', it tries to parse the access token as a jwt token and verifies the signature using the keys published on the IDP's 'jwks_uri'." introductionVersion:"1.0.0"` diff --git a/services/proxy/pkg/config/parser/parse.go b/services/proxy/pkg/config/parser/parse.go index 9cae495b6d..49e32a9d16 100644 --- a/services/proxy/pkg/config/parser/parse.go +++ b/services/proxy/pkg/config/parser/parse.go @@ -65,6 +65,14 @@ func Validate(cfg *config.Config) error { return fmt.Errorf("OIDC audiences (PROXY_OIDC_AUDIENCES) in service %s must not contain empty or whitespace-only entries", cfg.Service.Name) } } + if len(cfg.OIDC.AllowedClientIDs) > 0 && cfg.OIDC.AccessTokenVerifyMethod != config.AccessTokenVerificationJWT { + return fmt.Errorf("OIDC allowed client ids (PROXY_OIDC_ALLOWED_CLIENT_IDS) in service %s require access_token_verify_method to be 'jwt'", cfg.Service.Name) + } + for _, clientID := range cfg.OIDC.AllowedClientIDs { + if strings.TrimSpace(clientID) == "" { + return fmt.Errorf("OIDC allowed client ids (PROXY_OIDC_ALLOWED_CLIENT_IDS) in service %s must not contain empty or whitespace-only entries", cfg.Service.Name) + } + } if cfg.ServiceAccount.ServiceAccountID == "" { return shared.MissingServiceAccountID(cfg.Service.Name) diff --git a/services/proxy/pkg/config/parser/parse_test.go b/services/proxy/pkg/config/parser/parse_test.go index 4ef3694ddc..7d3fe4e6fc 100644 --- a/services/proxy/pkg/config/parser/parse_test.go +++ b/services/proxy/pkg/config/parser/parse_test.go @@ -104,6 +104,63 @@ func TestValidateOIDCAudiences(t *testing.T) { } } +func TestParseOIDCAllowedClientIDs(t *testing.T) { + tests := []struct { + name string + yaml string + env string + method string + setEnv bool + want []string + wantErr string + }{ + {name: "unset defaults to disabled"}, + {name: "YAML", yaml: "oidc:\n allowed_client_ids: [web, desktop]\n", method: "jwt", want: []string{"web", "desktop"}}, + {name: "ENV", setEnv: true, env: "web,desktop", method: "jwt", want: []string{"web", "desktop"}}, + {name: "ENV trims list entries", setEnv: true, env: " web, desktop ", method: "jwt", want: []string{"web", "desktop"}}, + {name: "ENV precedence", yaml: "oidc:\n allowed_client_ids: [yaml-client]\n", setEnv: true, env: "env-client", method: "jwt", want: []string{"env-client"}}, + {name: "empty ENV disables YAML", yaml: "oidc:\n allowed_client_ids: [web]\n", setEnv: true}, + {name: "YAML preserves case", yaml: "oidc:\n allowed_client_ids: [Web]\n", method: "jwt", want: []string{"Web"}}, + {name: "YAML blank entry", yaml: "oidc:\n allowed_client_ids: ['']\n", method: "jwt", wantErr: "empty or whitespace-only"}, + {name: "ENV whitespace entry", setEnv: true, env: "web, ", method: "jwt", wantErr: "empty or whitespace-only"}, + {name: "YAML incompatible verification", yaml: "oidc:\n allowed_client_ids: [web]\n access_token_verify_method: none\n", wantErr: "require access_token_verify_method to be 'jwt'"}, + {name: "ENV incompatible verification", setEnv: true, env: "web", method: "none", wantErr: "require access_token_verify_method to be 'jwt'"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + t.Setenv("OC_CONFIG_DIR", dir) + t.Setenv("PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD", "") + require.NoError(t, os.Unsetenv("PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD")) + if tt.method != "" { + t.Setenv("PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD", tt.method) + } + t.Setenv("PROXY_OIDC_SKIP_USER_INFO", "false") + t.Setenv("PROXY_OIDC_AUDIENCES", "") + require.NoError(t, os.Unsetenv("PROXY_OIDC_AUDIENCES")) + t.Setenv("PROXY_OIDC_ALLOWED_CLIENT_IDS", "") + require.NoError(t, os.Unsetenv("PROXY_OIDC_ALLOWED_CLIENT_IDS")) + if tt.setEnv { + t.Setenv("PROXY_OIDC_ALLOWED_CLIENT_IDS", tt.env) + } + require.NoError(t, os.WriteFile(filepath.Join(dir, "proxy.yaml"), []byte(tt.yaml), 0600)) + cfg := validProxyConfig() + err := parser.ParseConfig(cfg) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + require.ErrorContains(t, err, "PROXY_OIDC_ALLOWED_CLIENT_IDS") + return + } + require.NoError(t, err) + if len(tt.want) == 0 { + require.Empty(t, cfg.OIDC.AllowedClientIDs) + } else { + require.Equal(t, tt.want, cfg.OIDC.AllowedClientIDs) + } + }) + } +} + func validProxyConfig() *config.Config { cfg := defaults.FullDefaultConfig() cfg.MachineAuthAPIKey = "test-machine-key"