From efa71843a8ed6fba092d1e4f06cafae430fd530c Mon Sep 17 00:00:00 2001 From: zerox80 Date: Sat, 5 Sep 2026 03:28:56 +0200 Subject: [PATCH 1/4] feat(oidc): add access token audience validation --- pkg/oidc/access_token_test.go | 196 ++++++++++++++++++++++++++++++++++ pkg/oidc/client.go | 12 ++- pkg/oidc/options.go | 11 ++ 3 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 pkg/oidc/access_token_test.go diff --git a/pkg/oidc/access_token_test.go b/pkg/oidc/access_token_test.go new file mode 100644 index 0000000000..9fce9beb45 --- /dev/null +++ b/pkg/oidc/access_token_test.go @@ -0,0 +1,196 @@ +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 TestAccessTokenAudiences(t *testing.T) { + key := newRSAKey(t) + tests := []struct { + name string + audiences []string + aud any + missing bool + wantErr error + }{ + {name: "disabled missing", missing: true}, + {name: "disabled foreign", aud: "immich"}, + {name: "disabled null", aud: nil}, + {name: "disabled empty array", aud: []string{}}, + {name: "explicitly empty configuration", audiences: []string{}, aud: "immich"}, + {name: "single audience", audiences: []string{"opencloud"}, aud: "opencloud"}, + {name: "array first match", audiences: []string{"opencloud"}, aud: []string{"opencloud", "immich"}}, + {name: "array last match", audiences: []string{"opencloud"}, aud: []string{"immich", "opencloud"}}, + {name: "any allowed audience", audiences: []string{"opencloud", "opencloud-api"}, aud: "opencloud-api"}, + {name: "any allowed audience in array", audiences: []string{"opencloud", "opencloud-api"}, aud: []string{"immich", "opencloud-api"}}, + {name: "duplicate audiences", audiences: []string{"opencloud", "opencloud"}, aud: []string{"opencloud", "opencloud"}}, + {name: "URI audience", audiences: []string{"https://cloud.example/api"}, aud: "https://cloud.example/api"}, + {name: "foreign", audiences: []string{"opencloud"}, aud: "immich", wantErr: jwt.ErrTokenInvalidAudience}, + {name: "foreign array", audiences: []string{"opencloud"}, aud: []string{"immich", "account"}, wantErr: jwt.ErrTokenInvalidAudience}, + {name: "case sensitive", audiences: []string{"opencloud"}, aud: "OpenCloud", wantErr: jwt.ErrTokenInvalidAudience}, + {name: "exact match", audiences: []string{"opencloud"}, aud: "opencloud-api", wantErr: jwt.ErrTokenInvalidAudience}, + {name: "no token normalization", audiences: []string{"opencloud"}, aud: " opencloud ", wantErr: jwt.ErrTokenInvalidAudience}, + {name: "no wildcard matching", audiences: []string{"*"}, aud: "opencloud", wantErr: jwt.ErrTokenInvalidAudience}, + {name: "missing", audiences: []string{"opencloud"}, missing: true, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "null", audiences: []string{"opencloud"}, aud: nil, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "empty string", audiences: []string{"opencloud"}, aud: "", wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "empty array", audiences: []string{"opencloud"}, aud: []string{}, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "array empty string", audiences: []string{"opencloud"}, aud: []string{""}, wantErr: jwt.ErrTokenRequiredClaimMissing}, + {name: "number", audiences: []string{"opencloud"}, aud: 123, wantErr: jwt.ErrTokenMalformed}, + {name: "object", audiences: []string{"opencloud"}, aud: map[string]string{"aud": "opencloud"}, wantErr: jwt.ErrTokenMalformed}, + {name: "mixed array", audiences: []string{"opencloud"}, aud: []any{"opencloud", 123}, wantErr: jwt.ErrTokenMalformed}, + {name: "null array entry", audiences: []string{"opencloud"}, aud: []any{"opencloud", nil}, wantErr: jwt.ErrTokenMalformed}, + {name: "disabled still rejects invalid type", aud: 123, wantErr: jwt.ErrTokenMalformed}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + claims := jwt.MapClaims{ + "iss": "https://issuer.example", + "sub": "alice", + "sid": "session", + "exp": time.Now().Add(time.Hour).Unix(), + } + if !tt.missing { + claims["aud"] = tt.aud + } + client := newAccessTokenTestClient(key, tt.audiences, &oidc.ProviderMetadata{}) + 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, "unverified claims must not be returned") + return + } + require.NoError(t, err) + require.Equal(t, "alice", registered.Subject) + require.Equal(t, "session", registered.SessionID) + require.Equal(t, "alice", all["sub"]) + }) + } +} + +func TestAccessTokenValidationWithAudiences(t *testing.T) { + key, otherKey := newRSAKey(t), newRSAKey(t) + tests := []struct { + name string + issuer string + provider *oidc.ProviderMetadata + signingKey *signingKey + exp time.Time + nbf time.Time + wantErr error + }{ + {name: "invalid signature", signingKey: otherKey, wantErr: jwt.ErrTokenSignatureInvalid}, + {name: "invalid issuer", issuer: "https://other.example", wantErr: jwt.ErrTokenInvalidIssuer}, + {name: "expired", exp: time.Now().Add(-time.Hour), wantErr: jwt.ErrTokenExpired}, + {name: "not yet valid", nbf: time.Now().Add(time.Hour), wantErr: jwt.ErrTokenNotValidYet}, + {name: "AD FS access token issuer", issuer: "https://adfs.example", provider: &oidc.ProviderMetadata{AccessTokenIssuer: "https://adfs.example"}}, + {name: "AD FS rejects discovery issuer", provider: &oidc.ProviderMetadata{AccessTokenIssuer: "https://adfs.example"}, wantErr: jwt.ErrTokenInvalidIssuer}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.issuer == "" { + tt.issuer = "https://issuer.example" + } + if tt.provider == nil { + tt.provider = &oidc.ProviderMetadata{} + } + if tt.signingKey == nil { + tt.signingKey = key + } + if tt.exp.IsZero() { + tt.exp = time.Now().Add(time.Hour) + } + claims := jwt.MapClaims{"iss": tt.issuer, "sub": "alice", "aud": "opencloud", "exp": tt.exp.Unix()} + if !tt.nbf.IsZero() { + claims["nbf"] = tt.nbf.Unix() + } + client := newAccessTokenTestClient(key, []string{"opencloud"}, tt.provider) + _, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, tt.signingKey, claims)) + require.ErrorIs(t, err, tt.wantErr) + }) + } +} + +func TestAccessTokenAudienceConfiguration(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 := oidc.NewOIDCClient( + oidc.WithAccessTokenVerifyMethod(method), + oidc.WithAccessTokenAudiences([]string{"opencloud"}), + ) + _, _, err := client.VerifyAccessToken(context.Background(), "opaque-token") + require.ErrorContains(t, err, "requires the jwt verification method") + }) + } + for _, audiences := range [][]string{{""}, {" \t"}, {"opencloud", ""}} { + client := oidc.NewOIDCClient( + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + oidc.WithAccessTokenAudiences(audiences), + ) + _, _, err := client.VerifyAccessToken(context.Background(), "token") + require.ErrorContains(t, err, "empty or whitespace-only") + } + t.Run("none remains compatible when disabled", func(t *testing.T) { + client := oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationNone), + oidc.WithProviderMetadata(&oidc.ProviderMetadata{}), + ) + _, _, err := client.VerifyAccessToken(context.Background(), "opaque-token") + require.NoError(t, err) + }) + t.Run("caller cannot mutate the policy", func(t *testing.T) { + key := newRSAKey(t) + audiences := []string{"opencloud"} + client := newAccessTokenTestClient(key, audiences, &oidc.ProviderMetadata{}) + audiences[0] = "immich" + _, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, + jwt.MapClaims{"iss": "https://issuer.example", "aud": "immich"})) + require.ErrorIs(t, err, jwt.ErrTokenInvalidAudience) + }) +} + +func TestAccessTokenAudiencesDoNotApplyToLogoutTokens(t *testing.T) { + key := newRSAKey(t) + client := newAccessTokenTestClient(key, []string{"opencloud"}, &oidc.ProviderMetadata{}) + token := signAccessToken(t, key, jwt.MapClaims{ + "iss": "https://issuer.example", + "sub": "alice", + "aud": "web-client", + "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 newAccessTokenTestClient(key *signingKey, audiences []string, provider *oidc.ProviderMetadata) oidc.OIDCClient { + return oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), + oidc.WithOidcIssuer("https://issuer.example"), + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + oidc.WithAccessTokenAudiences(audiences), + oidc.WithJWKS(key.jwks), + oidc.WithProviderMetadata(provider), + ) +} + +func signAccessToken(t *testing.T, key *signingKey, claims jwt.MapClaims) string { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + token.Header["kid"] = "1" + signed, err := token.SignedString(key.priv) + require.NoError(t, err) + return signed +} diff --git a/pkg/oidc/client.go b/pkg/oidc/client.go index b6065ec66f..aa16abd9a8 100644 --- a/pkg/oidc/client.go +++ b/pkg/oidc/client.go @@ -57,6 +57,7 @@ type oidcClient struct { providerLock *sync.Mutex skipIssuerValidation bool accessTokenVerifyMethod string + accessTokenAudiences []string remoteKeySet KeySet algorithms []string @@ -91,6 +92,7 @@ func NewOIDCClient(opts ...Option) OIDCClient { issuer: options.OIDCIssuer, httpClient: options.HTTPClient, accessTokenVerifyMethod: options.AccessTokenVerifyMethod, + accessTokenAudiences: options.AccessTokenAudiences, JWKSOptions: options.JWKSOptions, // TODO I don't like that we pass down config options ... JWKS: options.JWKS, providerLock: &sync.Mutex{}, @@ -270,6 +272,14 @@ func (c *oidcClient) UserInfo(ctx context.Context, tokenSource oauth2.TokenSourc } func (c *oidcClient) VerifyAccessToken(ctx context.Context, token string) (RegClaimsWithSID, jwt.MapClaims, error) { + if len(c.accessTokenAudiences) > 0 && c.accessTokenVerifyMethod != config.AccessTokenVerificationJWT { + return RegClaimsWithSID{}, jwt.MapClaims{}, errors.New("access token audience validation requires the jwt verification method") + } + for _, audience := range c.accessTokenAudiences { + if strings.TrimSpace(audience) == "" { + return RegClaimsWithSID{}, jwt.MapClaims{}, errors.New("access token audiences must not contain empty or whitespace-only entries") + } + } if err := c.lookupWellKnownOpenidConfiguration(ctx); err != nil { return RegClaimsWithSID{}, jwt.MapClaims{}, err } @@ -301,7 +311,7 @@ func (c *oidcClient) verifyAccessTokenJWT(token string) (RegClaimsWithSID, jwt.M issuer = c.provider.AccessTokenIssuer } - _, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc, jwt.WithIssuer(issuer)) + _, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc, jwt.WithIssuer(issuer), jwt.WithAudience(c.accessTokenAudiences...)) if err != nil { return claims, mapClaims, err } diff --git a/pkg/oidc/options.go b/pkg/oidc/options.go index bf025e7670..9d27d95657 100644 --- a/pkg/oidc/options.go +++ b/pkg/oidc/options.go @@ -35,6 +35,9 @@ type Options struct { // AccessTokenVerifyMethod to use when verifying access tokens // TODO pass a function or interface to verify? an AccessTokenVerifier? AccessTokenVerifyMethod string + // AccessTokenAudiences requires at least one matching audience in access tokens. + // An empty list disables audience validation. + AccessTokenAudiences []string // Config to use Config *goidc.Config @@ -74,6 +77,14 @@ func WithAccessTokenVerifyMethod(val string) Option { } } +// WithAccessTokenAudiences sets the allowed audiences for access tokens only. +// An empty list disables audience validation. +func WithAccessTokenAudiences(val []string) Option { + return func(o *Options) { + o.AccessTokenAudiences = append([]string(nil), val...) + } +} + // WithHTTPClient provides a function to set the httpClient option. func WithHTTPClient(val *http.Client) Option { return func(o *Options) { From 23dcbdce2521662e8a0ac2cb1ce329bae431777b Mon Sep 17 00:00:00 2001 From: zerox80 Date: Sat, 5 Sep 2026 03:28:56 +0200 Subject: [PATCH 2/4] feat(proxy): configure and enforce OIDC access token audiences --- services/proxy/pkg/command/oidc.go | 36 ++ services/proxy/pkg/command/oidc_test.go | 420 ++++++++++++++++++ services/proxy/pkg/command/server.go | 17 +- services/proxy/pkg/config/config.go | 15 +- services/proxy/pkg/config/parser/parse.go | 9 + .../proxy/pkg/config/parser/parse_test.go | 115 +++++ services/proxy/pkg/middleware/oidc_auth.go | 58 ++- .../proxy/pkg/middleware/oidc_cache_test.go | 64 +++ services/proxy/pkg/middleware/options.go | 14 +- 9 files changed, 702 insertions(+), 46 deletions(-) create mode 100644 services/proxy/pkg/command/oidc.go create mode 100644 services/proxy/pkg/command/oidc_test.go create mode 100644 services/proxy/pkg/config/parser/parse_test.go create mode 100644 services/proxy/pkg/middleware/oidc_cache_test.go diff --git a/services/proxy/pkg/command/oidc.go b/services/proxy/pkg/command/oidc.go new file mode 100644 index 0000000000..84e29456b5 --- /dev/null +++ b/services/proxy/pkg/command/oidc.go @@ -0,0 +1,36 @@ +package command + +import ( + "net/http" + + "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/opencloud-eu/opencloud/services/proxy/pkg/middleware" + "go-micro.dev/v4/store" +) + +func newOIDCAuthenticator(logger log.Logger, cfg *config.Config, userInfoCache store.Store, httpClient *http.Client) *middleware.OIDCAuthenticator { + if cfg.OIDC.Issuer != "" && len(cfg.OIDC.Audiences) == 0 { + logger.Warn().Msg("OIDC access token audience validation is disabled. Configure PROXY_OIDC_AUDIENCES to enable it; this is recommended for production.") + } + + return middleware.NewOIDCAuthenticator( + middleware.Logger(logger), + middleware.UserInfoCache(userInfoCache), + middleware.DefaultAccessTokenTTL(cfg.OIDC.UserinfoCache.TTL), + middleware.HTTPClient(httpClient), + middleware.OIDCIss(cfg.OIDC.Issuer), + middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), + middleware.ValidateAccessTokenOnCacheHit(len(cfg.OIDC.Audiences) > 0), + middleware.OIDCClient(oidc.NewOIDCClient( + oidc.WithAccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), + oidc.WithAccessTokenAudiences(cfg.OIDC.Audiences), + oidc.WithLogger(logger), + oidc.WithHTTPClient(httpClient), + oidc.WithOidcIssuer(cfg.OIDC.Issuer), + oidc.WithJWKSOptions(cfg.OIDC.JWKS), + )), + middleware.SkipUserInfo(cfg.OIDC.SkipUserInfo), + ) +} diff --git a/services/proxy/pkg/command/oidc_test.go b/services/proxy/pkg/command/oidc_test.go new file mode 100644 index 0000000000..b3f9642687 --- /dev/null +++ b/services/proxy/pkg/command/oidc_test.go @@ -0,0 +1,420 @@ +package command + +import ( + "bytes" + "crypto/rand" + "crypto/rsa" + "encoding/base64" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "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/opencloud-eu/opencloud/services/proxy/pkg/config/defaults" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/middleware" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/router" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes" + bcl "github.com/opencloud-eu/opencloud/services/proxy/pkg/staticroutes/backchannellogout" + "github.com/rs/zerolog" + "github.com/stretchr/testify/require" + "github.com/vmihailenco/msgpack/v5" + "go-micro.dev/v4/store" + "golang.org/x/crypto/sha3" +) + +func TestOIDCAudienceAuthentication(t *testing.T) { + for _, skipUserInfo := range []bool{false, true} { + t.Run(fmt.Sprintf("skip_user_info=%t", skipUserInfo), func(t *testing.T) { + idp := newAudienceTestIDP(t, "opencloud") + for _, tt := range []struct { + name string + audiences []string + aud any + want int + }{ + {name: "matching string", audiences: []string{"opencloud"}, aud: "opencloud", want: http.StatusOK}, + {name: "matching array", audiences: []string{"opencloud", "opencloud-api"}, aud: []string{"immich", "opencloud-api"}, want: http.StatusOK}, + {name: "foreign despite matching userinfo", audiences: []string{"opencloud"}, aud: "immich", want: http.StatusUnauthorized}, + {name: "missing despite matching userinfo", audiences: []string{"opencloud"}, want: http.StatusUnauthorized}, + {name: "disabled accepts foreign", aud: "immich", want: http.StatusOK}, + {name: "disabled accepts missing", want: http.StatusOK}, + } { + t.Run(tt.name, func(t *testing.T) { + cache := newAudienceTestCache() + cfg := audienceTestConfig(idp, tt.audiences, skipUserInfo) + auth := newOIDCAuthenticator(log.NopLogger(), cfg, cache, idp.server.Client()) + token := idp.accessToken(t, jwt.MapClaims{"aud": tt.aud}) + before := idp.userinfoRequests.Load() + response := audienceRequest(auth, token) + require.Equal(t, tt.want, response.status) + if tt.want == http.StatusUnauthorized { + require.Nil(t, response.claims, "the protected handler must not run") + require.Equal(t, before, idp.userinfoRequests.Load(), "reject before requesting userinfo") + require.Empty(t, cache.writes, "rejected tokens must not be cached") + return + } + require.Equal(t, "alice", response.claims["sub"]) + require.True(t, response.newSession) + cache.waitForSession(t) + expectedRequests := before + if !skipUserInfo { + expectedRequests++ + } + require.Equal(t, expectedRequests, idp.userinfoRequests.Load()) + response = audienceRequest(auth, token) + require.Equal(t, http.StatusOK, response.status) + require.False(t, response.newSession) + require.Equal(t, expectedRequests, idp.userinfoRequests.Load(), "reuse cached userinfo") + }) + } + }) + } +} + +func TestOIDCAudiencePolicyChangesWithCachedUserinfo(t *testing.T) { + for _, skipUserInfo := range []bool{false, true} { + for _, initialAudiences := range [][]string{nil, {"immich"}} { + t.Run(fmt.Sprintf("skip_user_info=%t/initial=%v", skipUserInfo, initialAudiences), func(t *testing.T) { + idp := newAudienceTestIDP(t, "opencloud") + cache := newAudienceTestCache() + cfg := audienceTestConfig(idp, initialAudiences, skipUserInfo) + token := idp.accessToken(t, jwt.MapClaims{"aud": "immich"}) + auth := newOIDCAuthenticator(log.NopLogger(), cfg, cache, idp.server.Client()) + require.Equal(t, http.StatusOK, audienceRequest(auth, token).status) + cache.waitForSession(t) + requests := idp.userinfoRequests.Load() + + // Reuse the store just like a persistent/shared cache after a proxy restart. + cfg.OIDC.Audiences = []string{"opencloud"} + auth = newOIDCAuthenticator(log.NopLogger(), cfg, cache, idp.server.Client()) + response := audienceRequest(auth, token) + require.Equal(t, http.StatusUnauthorized, response.status) + require.Nil(t, response.claims) + require.Equal(t, requests, idp.userinfoRequests.Load()) + + // Disabling the opt-in restores the existing cache behavior. + cfg.OIDC.Audiences = nil + auth = newOIDCAuthenticator(log.NopLogger(), cfg, cache, idp.server.Client()) + response = audienceRequest(auth, token) + require.Equal(t, http.StatusOK, response.status) + require.False(t, response.newSession) + }) + } + } +} + +func TestOIDCAudienceUsesAccessTokenInsteadOfUserinfo(t *testing.T) { + idp := newAudienceTestIDP(t, "different-userinfo-audience") + cache := newAudienceTestCache() + auth := newOIDCAuthenticator(log.NopLogger(), audienceTestConfig(idp, []string{"opencloud"}, false), cache, idp.server.Client()) + token := idp.accessToken(t, jwt.MapClaims{"aud": "opencloud"}) + require.Equal(t, http.StatusOK, audienceRequest(auth, token).status) + cache.waitForSession(t) + response := audienceRequest(auth, token) + require.Equal(t, http.StatusOK, response.status) + require.Equal(t, "different-userinfo-audience", response.claims["aud"]) + require.EqualValues(t, 1, idp.userinfoRequests.Load()) + require.EqualValues(t, 1, idp.discoveryRequests.Load()) + require.EqualValues(t, 1, idp.jwksRequests.Load()) +} + +func TestOIDCAudienceValidatesTokensBeforeCache(t *testing.T) { + idp := newAudienceTestIDP(t, "opencloud") + for _, tt := range []struct { + name string + claims jwt.MapClaims + mangle bool + }{ + {name: "expired", claims: jwt.MapClaims{"exp": time.Now().Add(-time.Hour).Unix()}}, + {name: "not yet valid", claims: jwt.MapClaims{"nbf": time.Now().Add(time.Hour).Unix()}}, + {name: "wrong issuer", claims: jwt.MapClaims{"iss": "https://other.example"}}, + {name: "missing audience", claims: jwt.MapClaims{"aud": nil}}, + {name: "invalid signature", mangle: true}, + } { + t.Run(tt.name, func(t *testing.T) { + cache := newAudienceTestCache() + token := idp.accessToken(t, tt.claims) + if tt.mangle { + parts := strings.Split(token, ".") + sig, err := base64.RawURLEncoding.DecodeString(parts[2]) + require.NoError(t, err) + sig[0] ^= 1 + parts[2] = base64.RawURLEncoding.EncodeToString(sig) + token = strings.Join(parts, ".") + } + cached, err := msgpack.Marshal(map[string]any{ + "sub": "alice", "aud": "opencloud", "exp": time.Now().Add(time.Hour).Unix(), + }) + require.NoError(t, err) + require.NoError(t, cache.Store.Write(&store.Record{Key: audienceTokenCacheKey(token), Value: cached, Expiry: time.Hour})) + auth := newOIDCAuthenticator(log.NopLogger(), audienceTestConfig(idp, []string{"opencloud"}, false), cache, idp.server.Client()) + require.Equal(t, http.StatusUnauthorized, audienceRequest(auth, token).status) + require.Zero(t, idp.userinfoRequests.Load()) + }) + } +} + +func TestOIDCAudienceRefreshesExpiredOrCorruptCachedClaims(t *testing.T) { + for _, skipUserInfo := range []bool{false, true} { + for _, corrupt := range []bool{false, true} { + t.Run(fmt.Sprintf("skip_user_info=%t/corrupt=%t", skipUserInfo, corrupt), func(t *testing.T) { + idp := newAudienceTestIDP(t, "opencloud") + cache := newAudienceTestCache() + token := idp.accessToken(t, nil) + cached, err := msgpack.Marshal(map[string]any{"sub": "stale", "exp": time.Now().Add(-time.Hour).Unix()}) + require.NoError(t, err) + if corrupt { + cached = []byte{0xc1} // Reserved/invalid MessagePack marker. + } + require.NoError(t, cache.Store.Write(&store.Record{Key: audienceTokenCacheKey(token), Value: cached, Expiry: time.Hour})) + auth := newOIDCAuthenticator(log.NopLogger(), audienceTestConfig(idp, []string{"opencloud"}, skipUserInfo), cache, idp.server.Client()) + response := audienceRequest(auth, token) + require.Equal(t, http.StatusOK, response.status) + require.Equal(t, "alice", response.claims["sub"]) + require.True(t, response.newSession) + cache.waitForSession(t) + require.False(t, audienceRequest(auth, token).newSession) + }) + } + } +} + +func TestOIDCAudiencePreservesBackchannelLogout(t *testing.T) { + for _, skipUserInfo := range []bool{false, true} { + t.Run(fmt.Sprintf("skip_user_info=%t", skipUserInfo), func(t *testing.T) { + idp := newAudienceTestIDP(t, "opencloud") + cache := newAudienceTestCache() + cfg := audienceTestConfig(idp, []string{"opencloud"}, skipUserInfo) + auth := newOIDCAuthenticator(log.NopLogger(), cfg, cache, idp.server.Client()) + token := idp.accessToken(t, nil) + require.Equal(t, http.StatusOK, audienceRequest(auth, token).status) + cache.waitForSession(t) + + sessionKey, err := bcl.NewKey("alice", "session") + require.NoError(t, err) + records, err := cache.Read(sessionKey) + require.NoError(t, err) + require.Len(t, records, 1) + require.Equal(t, audienceTokenCacheKey(token), string(records[0].Value)) + + logoutClient := oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), + oidc.WithOidcIssuer(idp.server.URL), + oidc.WithHTTPClient(idp.server.Client()), + oidc.WithAccessTokenAudiences([]string{"opencloud"}), + ) + routes := &staticroutes.StaticRouteHandler{ + Prefix: "/", Config: *cfg, Logger: log.NopLogger(), OidcClient: logoutClient, + UserInfoCache: cache, Proxy: http.NotFoundHandler(), + } + // Subject logout invalidates all sessions, without requiring a user/event backend. + logoutToken := idp.sign(t, jwt.MapClaims{ + "iss": idp.server.URL, "sub": "alice", "aud": "web-client", + "events": map[string]any{"http://schemas.openid.net/event/backchannel-logout": map[string]any{}}, + }) + form := url.Values{"logout_token": {logoutToken}} + req := httptest.NewRequest(http.MethodPost, "/backchannel_logout", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + response := httptest.NewRecorder() + routes.Handler().ServeHTTP(response, req) + require.Equal(t, http.StatusOK, response.Code, response.Body.String()) + _, err = cache.Read(sessionKey) + require.ErrorIs(t, err, store.ErrNotFound) + _, err = cache.Read(audienceTokenCacheKey(token)) + require.ErrorIs(t, err, store.ErrNotFound) + }) + } +} + +func TestOIDCAudienceStartupWarning(t *testing.T) { + // Match log.NewLogger's global level while testing the per-service filter. + previousLevel := zerolog.GlobalLevel() + zerolog.SetGlobalLevel(zerolog.TraceLevel) + t.Cleanup(func() { zerolog.SetGlobalLevel(previousLevel) }) + idp := newAudienceTestIDP(t, "opencloud") + for _, tt := range []struct { + name string + audiences []string + level zerolog.Level + inactive bool + want int + }{ + {name: "disabled", level: zerolog.WarnLevel, want: 1}, + {name: "enabled", audiences: []string{"opencloud"}, level: zerolog.WarnLevel}, + {name: "filtered", level: zerolog.ErrorLevel}, + {name: "OIDC inactive", inactive: true, level: zerolog.WarnLevel}, + } { + t.Run(tt.name, func(t *testing.T) { + var output bytes.Buffer + logger := log.Logger{Logger: zerolog.New(&output).Level(tt.level)} + cfg := audienceTestConfig(idp, tt.audiences, true) + if tt.inactive { + cfg.OIDC.Issuer = "" + } + cache := newAudienceTestCache() + auth := newOIDCAuthenticator(logger, cfg, cache, idp.server.Client()) + if !tt.inactive { + token := idp.accessToken(t, nil) + require.Equal(t, http.StatusOK, audienceRequest(auth, token).status) + cache.waitForSession(t) + for range 3 { + require.Equal(t, http.StatusOK, audienceRequest(auth, token).status) + } + } + require.Equal(t, tt.want, strings.Count(output.String(), "PROXY_OIDC_AUDIENCES")) + if tt.want == 1 { + require.Contains(t, output.String(), "\"level\":\"warn\"") + } else { + require.Empty(t, output.String()) + } + }) + } +} + +type audienceTestIDP struct { + server *httptest.Server + key *rsa.PrivateKey + discoveryRequests atomic.Int32 + jwksRequests atomic.Int32 + userinfoRequests atomic.Int32 +} + +func newAudienceTestIDP(t *testing.T, userinfoAudience string) *audienceTestIDP { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idp := &audienceTestIDP{key: key} + mux := http.NewServeMux() + idp.server = httptest.NewServer(mux) + t.Cleanup(idp.server.Close) + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + idp.discoveryRequests.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": idp.server.URL, "jwks_uri": idp.server.URL + "/jwks", + "userinfo_endpoint": idp.server.URL + "/userinfo", + "id_token_signing_alg_values_supported": []string{"RS256"}, + }) + }) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, r *http.Request) { + idp.jwksRequests.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"keys": []any{map[string]any{ + "kty": "RSA", "kid": "test", "alg": "RS256", "use": "sig", + "n": base64.RawURLEncoding.EncodeToString(key.N.Bytes()), "e": "AQAB", + }}}) + }) + mux.HandleFunc("/userinfo", func(w http.ResponseWriter, r *http.Request) { + idp.userinfoRequests.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"sub": "alice", "preferred_username": "alice", "aud": userinfoAudience}) + }) + return idp +} + +func (idp *audienceTestIDP) sign(t *testing.T, claims jwt.MapClaims) string { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + token.Header["kid"] = "test" + signed, err := token.SignedString(idp.key) + require.NoError(t, err) + return signed +} + +func (idp *audienceTestIDP) accessToken(t *testing.T, overrides jwt.MapClaims) string { + t.Helper() + claims := jwt.MapClaims{ + "iss": idp.server.URL, "sub": "alice", "sid": "session", "aud": "opencloud", + "exp": time.Now().Add(time.Hour).Unix(), + } + for key, value := range overrides { + if value == nil { + delete(claims, key) + } else { + claims[key] = value + } + } + return idp.sign(t, claims) +} + +func audienceTestConfig(idp *audienceTestIDP, audiences []string, skipUserInfo bool) *config.Config { + cfg := defaults.FullDefaultConfig() + cfg.OIDC.Issuer = idp.server.URL + cfg.OIDC.Audiences = audiences + cfg.OIDC.SkipUserInfo = skipUserInfo + cfg.OIDC.JWKS = config.JWKS{} // No background refresh goroutines in tests. + return cfg +} + +type audienceTestResponse struct { + status int + newSession bool + claims map[string]any +} + +func audienceRequest(auth middleware.Authenticator, token string) audienceTestResponse { + result := audienceTestResponse{} + handler := middleware.Authentication([]middleware.Authenticator{auth})(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + result.newSession = oidc.NewSessionFlagFromContext(r.Context()) + result.claims = oidc.FromContext(r.Context()) + w.WriteHeader(http.StatusOK) + })) + req := httptest.NewRequest(http.MethodGet, "/protected", http.NoBody) + req = req.WithContext(router.SetRoutingInfo(req.Context(), router.RoutingInfo{})) + req.Header.Set("Authorization", "Bearer "+token) + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + result.status = recorder.Code + return result +} + +// Wait for the asynchronous session write instead of sleeping or racing the cache. +type audienceTestCache struct { + store.Store + writes chan string +} + +func newAudienceTestCache() *audienceTestCache { + return &audienceTestCache{Store: store.NewMemoryStore(), writes: make(chan string, 16)} +} + +func (cache *audienceTestCache) Write(record *store.Record, opts ...store.WriteOption) error { + err := cache.Store.Write(record, opts...) + if err == nil { + cache.writes <- record.Key + } + return err +} + +func (cache *audienceTestCache) waitForSession(t *testing.T) { + t.Helper() + key, err := bcl.NewKey("alice", "session") + require.NoError(t, err) + timer := time.NewTimer(5 * time.Second) + defer timer.Stop() + for { + select { + case written := <-cache.writes: + if written == key { + return + } + case <-timer.C: + t.Fatal("timed out waiting for session cache write") + } + } +} + +func audienceTokenCacheKey(token string) string { + hash := make([]byte, 64) + sha3.ShakeSum256(hash, []byte(token)) + return base64.URLEncoding.EncodeToString(hash) +} diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 2a9a5cc0ba..a35c1fe4a0 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -299,22 +299,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, UserRoleAssigner: roleAssigner, }) } - authenticators = append(authenticators, middleware.NewOIDCAuthenticator( - middleware.Logger(logger), - middleware.UserInfoCache(userInfoCache), - middleware.DefaultAccessTokenTTL(cfg.OIDC.UserinfoCache.TTL), - middleware.HTTPClient(oidcHTTPClient), - middleware.OIDCIss(cfg.OIDC.Issuer), - middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), - middleware.OIDCClient(oidc.NewOIDCClient( - oidc.WithAccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), - oidc.WithLogger(logger), - oidc.WithHTTPClient(oidcHTTPClient), - oidc.WithOidcIssuer(cfg.OIDC.Issuer), - oidc.WithJWKSOptions(cfg.OIDC.JWKS), - )), - middleware.SkipUserInfo(cfg.OIDC.SkipUserInfo), - )) + authenticators = append(authenticators, newOIDCAuthenticator(logger, cfg, userInfoCache, oidcHTTPClient)) authenticators = append(authenticators, middleware.PublicShareAuthenticator{ Logger: logger, RevaGatewaySelector: gatewaySelector, diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 05344049a6..2c952a0a46 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -116,13 +116,14 @@ const ( // OIDC is the config for the OpenID-Connect middleware. If set the proxy will try to authenticate every request // with the configured oidc-provider type OIDC struct { - 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"` - SkipUserInfo bool `yaml:"skip_user_info" env:"PROXY_OIDC_SKIP_USER_INFO" desc:"Do not look up user claims at the userinfo endpoint and directly read them from the access token. Incompatible with 'PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=none'." introductionVersion:"1.0.0"` - UserinfoCache *Cache `yaml:"user_info_cache"` - JWKS JWKS `yaml:"jwks"` - RewriteWellKnown bool `yaml:"rewrite_well_known" env:"PROXY_OIDC_REWRITE_WELLKNOWN" desc:"Enables rewriting the /.well-known/openid-configuration to the configured OIDC issuer. Needed by the Desktop Client, Android Client and iOS Client to discover the OIDC provider." introductionVersion:"1.0.0"` + 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:"%%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"` + SkipUserInfo bool `yaml:"skip_user_info" env:"PROXY_OIDC_SKIP_USER_INFO" desc:"Do not look up user claims at the userinfo endpoint and directly read them from the access token. Incompatible with 'PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=none'." introductionVersion:"1.0.0"` + UserinfoCache *Cache `yaml:"user_info_cache"` + JWKS JWKS `yaml:"jwks"` + RewriteWellKnown bool `yaml:"rewrite_well_known" env:"PROXY_OIDC_REWRITE_WELLKNOWN" desc:"Enables rewriting the /.well-known/openid-configuration to the configured OIDC issuer. Needed by the Desktop Client, Android Client and iOS Client to discover the OIDC provider." introductionVersion:"1.0.0"` } type JWKS struct { diff --git a/services/proxy/pkg/config/parser/parse.go b/services/proxy/pkg/config/parser/parse.go index 7a8be8c7a8..9cae495b6d 100644 --- a/services/proxy/pkg/config/parser/parse.go +++ b/services/proxy/pkg/config/parser/parse.go @@ -3,6 +3,7 @@ package parser import ( "errors" "fmt" + "strings" occfg "github.com/opencloud-eu/opencloud/pkg/config" "github.com/opencloud-eu/opencloud/pkg/shared" @@ -56,6 +57,14 @@ func Validate(cfg *config.Config) error { cfg.OIDC.SkipUserInfo, cfg.Service.Name, ) } + if len(cfg.OIDC.Audiences) > 0 && cfg.OIDC.AccessTokenVerifyMethod != config.AccessTokenVerificationJWT { + return fmt.Errorf("OIDC audiences (PROXY_OIDC_AUDIENCES) in service %s require access_token_verify_method to be 'jwt'", cfg.Service.Name) + } + for _, audience := range cfg.OIDC.Audiences { + if strings.TrimSpace(audience) == "" { + return fmt.Errorf("OIDC audiences (PROXY_OIDC_AUDIENCES) 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 new file mode 100644 index 0000000000..b955ac6c6d --- /dev/null +++ b/services/proxy/pkg/config/parser/parse_test.go @@ -0,0 +1,115 @@ +package parser_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/opencloud-eu/opencloud/pkg/shared" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config/defaults" + "github.com/opencloud-eu/opencloud/services/proxy/pkg/config/parser" + "github.com/stretchr/testify/require" +) + +func TestParseOIDCAudiences(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 audiences: [opencloud, opencloud-api]\n", want: []string{"opencloud", "opencloud-api"}}, + {name: "empty YAML list", yaml: "oidc:\n audiences: []\n"}, + {name: "null YAML list", yaml: "oidc:\n audiences: null\n"}, + {name: "ENV", setEnv: true, env: "opencloud,opencloud-api", want: []string{"opencloud", "opencloud-api"}}, + {name: "ENV trims list entries", setEnv: true, env: " opencloud, opencloud-api ", want: []string{"opencloud", "opencloud-api"}}, + {name: "ENV precedence", yaml: "oidc:\n audiences: [yaml-audience]\n", setEnv: true, env: "env-audience", want: []string{"env-audience"}}, + {name: "empty ENV disables YAML", yaml: "oidc:\n audiences: [opencloud]\n", setEnv: true}, + {name: "existing ENV empty segment handling", setEnv: true, env: "opencloud,,opencloud-api", want: []string{"opencloud", "opencloud-api"}}, + {name: "YAML blank entry", yaml: "oidc:\n audiences: ['']\n", wantErr: "empty or whitespace-only"}, + {name: "YAML whitespace entry", yaml: "oidc:\n audiences: [' ']\n", wantErr: "empty or whitespace-only"}, + {name: "ENV whitespace entry", setEnv: true, env: "opencloud, ", wantErr: "empty or whitespace-only"}, + {name: "ENV whitespace only", setEnv: true, env: " ", wantErr: "empty or whitespace-only"}, + {name: "YAML preserves case", yaml: "oidc:\n audiences: [OpenCloud]\n", want: []string{"OpenCloud"}}, + {name: "YAML incompatible verification", yaml: "oidc:\n audiences: [opencloud]\n access_token_verify_method: none\n", wantErr: "require access_token_verify_method to be 'jwt'"}, + {name: "ENV incompatible verification", setEnv: true, env: "opencloud", method: "none", wantErr: "require access_token_verify_method to be 'jwt'"}, + {name: "ENV enables JWT over YAML none", yaml: "oidc:\n audiences: [opencloud]\n access_token_verify_method: none\n", method: "jwt", want: []string{"opencloud"}}, + {name: "empty ENV restores none compatibility", yaml: "oidc:\n audiences: [opencloud]\n access_token_verify_method: none\n", setEnv: true}, + } + 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")) + if tt.setEnv { + t.Setenv("PROXY_OIDC_AUDIENCES", 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_AUDIENCES") + return + } + require.NoError(t, err) + if len(tt.want) == 0 { + require.Empty(t, cfg.OIDC.Audiences) + } else { + require.Equal(t, tt.want, cfg.OIDC.Audiences) + } + }) + } +} + +func TestValidateOIDCAudiences(t *testing.T) { + for _, tt := range []struct { + name string + audiences []string + method string + wantErr string + }{ + {name: "disabled JWT", method: "jwt"}, + {name: "disabled none", method: "none"}, + {name: "enabled JWT", audiences: []string{"opencloud"}, method: "jwt"}, + {name: "enabled none", audiences: []string{"opencloud"}, method: "none", wantErr: "require access_token_verify_method to be 'jwt'"}, + {name: "blank", audiences: []string{""}, method: "jwt", wantErr: "empty or whitespace-only"}, + {name: "whitespace", audiences: []string{" \t"}, method: "jwt", wantErr: "empty or whitespace-only"}, + {name: "mixed valid and blank", audiences: []string{"opencloud", ""}, method: "jwt", wantErr: "empty or whitespace-only"}, + } { + t.Run(tt.name, func(t *testing.T) { + cfg := validProxyConfig() + cfg.OIDC.Audiences = tt.audiences + cfg.OIDC.AccessTokenVerifyMethod = tt.method + err := parser.Validate(cfg) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + require.ErrorContains(t, err, "PROXY_OIDC_AUDIENCES") + } else { + require.NoError(t, err) + } + }) + } +} + +func validProxyConfig() *config.Config { + cfg := defaults.FullDefaultConfig() + cfg.MachineAuthAPIKey = "test-machine-key" + cfg.TransferSecret = "test-transfer-secret" + cfg.ServiceAccount.ServiceAccountID = "test-service-account" + cfg.ServiceAccount.ServiceAccountSecret = "test-service-secret" + cfg.Commons = &shared.Commons{URLSigningSecret: "test-url-secret"} + return cfg +} diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index b80f7fcb7a..1899dba7f6 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -30,32 +30,45 @@ func NewOIDCAuthenticator(opts ...Option) *OIDCAuthenticator { options := newOptions(opts...) return &OIDCAuthenticator{ - Logger: options.Logger, - userInfoCache: options.UserInfoCache, - HTTPClient: options.HTTPClient, - OIDCIss: options.OIDCIss, - DefaultTokenCacheTTL: options.DefaultAccessTokenTTL, - oidcClient: options.OIDCClient, - AccessTokenVerifyMethod: options.AccessTokenVerifyMethod, - skipUserInfo: options.SkipUserInfo, - TimeFunc: time.Now, + Logger: options.Logger, + userInfoCache: options.UserInfoCache, + HTTPClient: options.HTTPClient, + OIDCIss: options.OIDCIss, + DefaultTokenCacheTTL: options.DefaultAccessTokenTTL, + oidcClient: options.OIDCClient, + AccessTokenVerifyMethod: options.AccessTokenVerifyMethod, + validateAccessTokenOnCacheHit: options.ValidateAccessTokenOnCacheHit, + skipUserInfo: options.SkipUserInfo, + TimeFunc: time.Now, } } // OIDCAuthenticator is an authenticator responsible for OIDC authentication. type OIDCAuthenticator struct { - Logger log.Logger - HTTPClient *http.Client - OIDCIss string - userInfoCache store.Store - DefaultTokenCacheTTL time.Duration - oidcClient oidc.OIDCClient - AccessTokenVerifyMethod string - skipUserInfo bool - TimeFunc func() time.Time + Logger log.Logger + HTTPClient *http.Client + OIDCIss string + userInfoCache store.Store + DefaultTokenCacheTTL time.Duration + oidcClient oidc.OIDCClient + AccessTokenVerifyMethod string + validateAccessTokenOnCacheHit bool + skipUserInfo bool + TimeFunc func() time.Time } func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[string]any, bool, error) { + var aClaims oidc.RegClaimsWithSID + var tokenClaims map[string]any + var err error + if m.validateAccessTokenOnCacheHit { + // Cache entries may predate the current audience policy. Verify the signed + // access token, not the userinfo claims, before trusting a cached result. + aClaims, tokenClaims, err = m.oidcClient.VerifyAccessToken(req.Context(), token) + if err != nil { + return nil, false, errors.Wrap(err, "failed to verify access token") + } + } var claims map[string]any // use a 64 bytes long hash to have 256-bit collision resistance. @@ -79,10 +92,13 @@ func (m *OIDCAuthenticator) getClaims(token string, req *http.Request) (map[stri } } - aClaims, claims, err := m.oidcClient.VerifyAccessToken(req.Context(), token) - if err != nil { - return nil, false, errors.Wrap(err, "failed to verify access token") + if !m.validateAccessTokenOnCacheHit { + aClaims, tokenClaims, err = m.oidcClient.VerifyAccessToken(req.Context(), token) + if err != nil { + return nil, false, errors.Wrap(err, "failed to verify access token") + } } + claims = tokenClaims if !m.skipUserInfo { oauth2Token := &oauth2.Token{ diff --git a/services/proxy/pkg/middleware/oidc_cache_test.go b/services/proxy/pkg/middleware/oidc_cache_test.go new file mode 100644 index 0000000000..7f0ba5d182 --- /dev/null +++ b/services/proxy/pkg/middleware/oidc_cache_test.go @@ -0,0 +1,64 @@ +package middleware + +import ( + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/vmihailenco/msgpack/v5" + "go-micro.dev/v4/store" + "golang.org/x/crypto/sha3" + + "github.com/opencloud-eu/opencloud/pkg/log" + "github.com/opencloud-eu/opencloud/pkg/oidc" + oidcmocks "github.com/opencloud-eu/opencloud/pkg/oidc/mocks" +) + +func TestOIDCCacheTokenVerification(t *testing.T) { + for _, validateCacheHit := range []bool{false, true} { + for _, cached := range []bool{false, true} { + t.Run(fmt.Sprintf("validate_cache_hit=%t/cached=%t", validateCacheHit, cached), func(t *testing.T) { + client := &oidcmocks.OIDCClient{} + expiresAt := time.Now().Add(time.Hour) + claims := jwt.MapClaims{"sub": "alice", "exp": expiresAt.Unix()} + if validateCacheHit || !cached { + client.On("VerifyAccessToken", mock.Anything, "token").Return(oidc.RegClaimsWithSID{ + SessionID: "session", + RegisteredClaims: jwt.RegisteredClaims{ + Subject: "alice", ExpiresAt: jwt.NewNumericDate(expiresAt), + }, + }, claims, nil).Once() + } + cache := store.NewMemoryStore() + if cached { + hash := make([]byte, 64) + sha3.ShakeSum256(hash, []byte("token")) + data, err := msgpack.Marshal(claims) + require.NoError(t, err) + require.NoError(t, cache.Write(&store.Record{ + Key: base64.URLEncoding.EncodeToString(hash), Value: data, + })) + } + authenticator := NewOIDCAuthenticator( + Logger(log.NopLogger()), OIDCClient(client), UserInfoCache(cache), + SkipUserInfo(true), ValidateAccessTokenOnCacheHit(validateCacheHit), + ) + got, newSession, err := authenticator.getClaims("token", httptest.NewRequest(http.MethodGet, "/", http.NoBody)) + require.NoError(t, err) + require.Equal(t, "alice", got["sub"]) + require.Equal(t, !cached, newSession) + client.AssertExpectations(t) + if !validateCacheHit && cached { + client.AssertNotCalled(t, "VerifyAccessToken", mock.Anything, mock.Anything) + } + client.AssertNotCalled(t, "UserInfo", mock.Anything, mock.Anything) + }) + } + } +} diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index 7e57d13ba2..28b222bea5 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -65,6 +65,8 @@ type Options struct { // AccessTokenVerifyMethod configures how access_tokens should be verified but the oidc_auth middleware. // Possible values currently: "jwt" and "none" AccessTokenVerifyMethod string + // ValidateAccessTokenOnCacheHit also verifies tokens before accepting cached userinfo. + ValidateAccessTokenOnCacheHit bool // JWKS sets the options for fetching the JWKS from the IDP JWKS config.JWKS // RoleQuotas hold userid:quota mappings. These will be used when provisioning new users. @@ -80,8 +82,8 @@ type Options struct { // tenant ID in the OIDC claims via the gateway's TenantAPI before comparing it to the user's stored tenant ID. TenantIDMappingEnabled bool // ServiceAccount holds credentials used to authenticate internal service calls (e.g. TenantAPI lookups). - ServiceAccount config.ServiceAccount - EventsPublisher events.Publisher + ServiceAccount config.ServiceAccount + EventsPublisher events.Publisher } // newOptions initializes the available default options. @@ -235,6 +237,14 @@ func AccessTokenVerifyMethod(method string) Option { } } +// ValidateAccessTokenOnCacheHit verifies access tokens even when userinfo is cached. +// Use this when cached claims may have been accepted under a different validation policy. +func ValidateAccessTokenOnCacheHit(val bool) Option { + return func(o *Options) { + o.ValidateAccessTokenOnCacheHit = val + } +} + // RoleQuotas sets the role quota mapping setting func RoleQuotas(roleQuotas map[string]uint64) Option { return func(o *Options) { From 9715305d4ae247815ef06c6430345992398c704b Mon Sep 17 00:00:00 2001 From: zerox80 Date: Sat, 5 Sep 2026 03:28:56 +0200 Subject: [PATCH 3/4] docs(proxy): document OIDC access token audience validation --- .../unreleased/feature-oidc-audiences.md | 19 +++++++ services/proxy/README.md | 57 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 changelog/unreleased/feature-oidc-audiences.md diff --git a/changelog/unreleased/feature-oidc-audiences.md b/changelog/unreleased/feature-oidc-audiences.md new file mode 100644 index 0000000000..ca51dac6fb --- /dev/null +++ b/changelog/unreleased/feature-oidc-audiences.md @@ -0,0 +1,19 @@ +Enhancement: Optional OIDC access token audience validation + +The proxy can now restrict OIDC access tokens to configured audiences using +PROXY_OIDC_AUDIENCES or oidc.audiences in proxy.yaml. For example, setting +PROXY_OIDC_AUDIENCES=opencloud,opencloud-api requires at least one of these values +in the access token's aud claim. Matching is exact and case-sensitive, and both +string and array claims are supported. + +The list defaults to empty to preserve existing deployments. When OIDC is active +and audience validation is disabled, the proxy emits one startup warning. +Enabling validation is recommended for production, especially when an identity +provider serves multiple applications. Administrators must configure the selected +audience in their identity provider's access tokens before enabling the check. + +Configured audiences require JWT verification. Missing, empty, malformed or +nonmatching token audiences are rejected, including when Userinfo is already +cached. Changing the configuration requires restarting the proxy. + +https://github.com/opencloud-eu/opencloud/issues/3456 diff --git a/services/proxy/README.md b/services/proxy/README.md index 16030f7530..105fb875b7 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -13,6 +13,60 @@ The following request authentication schemes are implemented: - Signed URL - Public Share Token +### OIDC Access Token Audiences + +For production deployments, **enable audience validation** so that OpenCloud only +accepts access tokens intended for it. This is especially relevant when the same +identity provider serves several applications: without this check, an otherwise +valid token issued for another application can also be accepted by OpenCloud. + +Set the allowed audiences as a comma-separated environment variable: + +```console +PROXY_OIDC_AUDIENCES=opencloud,opencloud-api +PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt +``` + +Alternatively, configure the list in `proxy.yaml`: + +```yaml +oidc: + audiences: + - opencloud + - opencloud-api + access_token_verify_method: jwt +``` + +These audience values are examples. Configure your identity provider to include +the intended OpenCloud resource audience in the **access tokens** issued to all +relevant clients, including web, desktop and mobile clients. Adding an audience +only to an ID token or a Userinfo response does not satisfy this check. + +An access token must contain at least one exactly matching, case-sensitive value +in its `aud` claim. Both strings, such as `"aud": "opencloud"`, and arrays, such as +`"aud": ["another-api", "opencloud"]`, are supported. Tokens with missing, empty, +malformed or exclusively nonmatching audiences receive HTTP 401 on protected +routes. Configured audiences require JWT verification; combining a nonempty list +with `access_token_verify_method: none` prevents startup. List entries must not be +empty or consist only of whitespace. + +The default list is empty, which disables audience validation to preserve +compatibility with existing identity provider configurations. An explicitly empty +`PROXY_OIDC_AUDIENCES` overrides any YAML list and disables the check; `audiences: []` +does the same in YAML. When OIDC is active and the check is disabled, the proxy +logs one startup warning, subject to the configured log level. + +Restart the proxy after changing the configuration and apply the same policy to +all proxy instances. When enabled, the signed access token is verified before +every cache lookup, so the current policy also applies to existing cache entries. +Userinfo remains cached, and successful cache hits do not require an additional +Userinfo request. + +The disabled default is a compatibility decision. It does not relax the +[audience validation requirement in RFC 9068, Section 4](https://www.rfc-editor.org/rfc/rfc9068.html#name-validating-jwt-access-token): +a resource server following that JWT access token profile must reject tokens +whose audience does not identify the resource server. + ## 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. @@ -231,6 +285,9 @@ The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The In a production deployment, you want to have basic authentication (`PROXY_ENABLE_BASIC_AUTH`) disabled which is the default state. You also want to setup a firewall to only allow requests to the proxy service or the reverse proxy if you have one. Requests to the other services should be blocked by the firewall. +Configure `PROXY_OIDC_AUDIENCES` as described in [OIDC Access Token Audiences](#oidc-access-token-audiences). +Enabling this check is strongly recommended for production deployments. + ### Content Security Policy For OpenCloud, external resources like an IDP (e.g. Keycloak) or when using web office documents or web apps, require defining a CSP. If not defined, the referenced services will not work. From 1947e483077fb1ef0ed0885d0ee9e28895e04b1a Mon Sep 17 00:00:00 2001 From: zerox80 Date: Sat, 5 Sep 2026 17:35:14 +0200 Subject: [PATCH 4/4] perf(oidc): avoid reparsing verified token headers and signatures --- pkg/oidc/access_token_test.go | 44 +++++++++++++++++++++++++++++++++++ pkg/oidc/client.go | 14 ++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/pkg/oidc/access_token_test.go b/pkg/oidc/access_token_test.go index 9fce9beb45..7a5c164b34 100644 --- a/pkg/oidc/access_token_test.go +++ b/pkg/oidc/access_token_test.go @@ -2,6 +2,7 @@ package oidc_test import ( "context" + "encoding/json" "testing" "time" @@ -175,6 +176,49 @@ func TestAccessTokenAudiencesDoNotApplyToLogoutTokens(t *testing.T) { require.NoError(t, err) } +func TestAccessTokenClaimExtraction(t *testing.T) { + key := newRSAKey(t) + client := newAccessTokenTestClient(key, []string{"opencloud"}, &oidc.ProviderMetadata{}) + t.Run("preserves arbitrary claims and numeric types", func(t *testing.T) { + claims := jwt.MapClaims{ + "iss": "https://issuer.example", "aud": "opencloud", "sub": "alice", "sid": "session", + "exp": 4102444800.75, "groups": []any{"users", "engineering"}, + "profile": map[string]any{"enabled": true, "score": 1.25}, "custom": nil, + } + registered, all, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, claims)) + require.NoError(t, err) + require.Equal(t, "alice", registered.Subject) + require.Equal(t, "session", registered.SessionID) + require.EqualValues(t, 4102444800, registered.ExpiresAt.Unix()) + require.Equal(t, claims, all) + }) + t.Run("preserves malformed map claim errors", func(t *testing.T) { + // Typed claims ignore this custom field; decoding MapClaims must still + // reject its overflowing number and preserve the JWT and JSON errors. + _, _, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, jwt.MapClaims{ + "iss": "https://issuer.example", "aud": "opencloud", "custom": json.Number("1e1000"), + })) + require.ErrorIs(t, err, jwt.ErrTokenMalformed) + var jsonError *json.UnmarshalTypeError + require.ErrorAs(t, err, &jsonError) + require.Equal(t, "number 1e1000", jsonError.Value) + }) + t.Run("retains empty map for null payload", func(t *testing.T) { + // With issuer/audience checks omitted, the client API previously accepted + // a signed null payload and returned an initialized, non-nil empty map. + client := oidc.NewOIDCClient( + oidc.WithLogger(log.NopLogger()), oidc.WithJWKS(key.jwks), + oidc.WithProviderMetadata(&oidc.ProviderMetadata{}), + oidc.WithAccessTokenVerifyMethod(config.AccessTokenVerificationJWT), + ) + registered, all, err := client.VerifyAccessToken(context.Background(), signAccessToken(t, key, nil)) + require.NoError(t, err) + require.Equal(t, oidc.RegClaimsWithSID{}, registered) + require.NotNil(t, all) + require.Empty(t, all) + }) +} + func newAccessTokenTestClient(key *signingKey, audiences []string, provider *oidc.ProviderMetadata) oidc.OIDCClient { return oidc.NewOIDCClient( oidc.WithLogger(log.NopLogger()), diff --git a/pkg/oidc/client.go b/pkg/oidc/client.go index aa16abd9a8..2e1687f593 100644 --- a/pkg/oidc/client.go +++ b/pkg/oidc/client.go @@ -315,10 +315,22 @@ func (c *oidcClient) verifyAccessTokenJWT(token string) (RegClaimsWithSID, jwt.M if err != nil { return claims, mapClaims, err } - _, _, err = new(jwt.Parser).ParseUnverified(token, mapClaims) + // The token's structure, encoding and signature have already been verified. + // Decode only the payload to retain arbitrary claims without parsing the + // header and signature again. Keep typed claims above for validation. + _, payloadAndSignature, _ := strings.Cut(token, ".") + payload, _, _ := strings.Cut(payloadAndSignature, ".") + claimBytes, err := new(jwt.Parser).DecodeSegment(payload) + if err != nil { + return claims, mapClaims, fmt.Errorf("%w: could not base64 decode claim: %w", jwt.ErrTokenMalformed, err) + } + // Match ParseUnverified's map value semantics, including a null payload. + decodedMapClaims := mapClaims + err = json.Unmarshal(claimBytes, &decodedMapClaims) // TODO: decode mapClaims to sth readable c.Logger.Debug().Interface("access token", &claims).Msg("parsed access token") if err != nil { + err = fmt.Errorf("%w: could not JSON decode claim: %w", jwt.ErrTokenMalformed, err) c.Logger.Info().Err(err).Msg("Failed to parse/verify the access token.") return claims, mapClaims, err }