From 33f45fa9653d6504cd46ffc1c7a409a12fa0b56e Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 1 Apr 2026 13:23:59 +0200 Subject: [PATCH] feat(multi-tenancy): verify tenant via OIDC claim When multi-tenancy is enable we now allow to specify an OIDC claim against which the tenantid of the user resolved via CS3 apis is matched. Partial: #2310 --- services/proxy/pkg/command/server.go | 1 + services/proxy/pkg/config/config.go | 1 + .../proxy/pkg/middleware/account_resolver.go | 26 ++- .../pkg/middleware/account_resolver_test.go | 172 +++++++++++++----- services/proxy/pkg/middleware/options.go | 10 + 5 files changed, 160 insertions(+), 50 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 9209ec30fb..ff77d25ed4 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -366,6 +366,7 @@ func loadMiddlewares(logger log.Logger, cfg *config.Config, middleware.SkipUserInfo(cfg.OIDC.SkipUserInfo), middleware.UserOIDCClaim(cfg.UserOIDCClaim), middleware.UserCS3Claim(cfg.UserCS3Claim), + middleware.TenantOIDCClaim(cfg.TenantOIDCClaim), middleware.AutoprovisionAccounts(cfg.AutoprovisionAccounts), middleware.MultiTenantEnabled(cfg.Commons.MultiTenantEnabled), middleware.EventsPublisher(publisher), diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 27d382c41d..1b82bf7a7e 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -34,6 +34,7 @@ type Config struct { AccountBackend string `yaml:"account_backend" env:"PROXY_ACCOUNT_BACKEND_TYPE" desc:"Account backend the PROXY service should use. Currently only 'cs3' is possible here." introductionVersion:"1.0.0"` UserOIDCClaim string `yaml:"user_oidc_claim" env:"PROXY_USER_OIDC_CLAIM" desc:"The name of an OpenID Connect claim that is used for resolving users with the account backend. The value of the claim must hold a per user unique, stable and non re-assignable identifier. The availability of claims depends on your Identity Provider. There are common claims available for most Identity providers like 'email' or 'preferred_username' but you can also add your own claim." introductionVersion:"1.0.0"` UserCS3Claim string `yaml:"user_cs3_claim" env:"PROXY_USER_CS3_CLAIM" desc:"The name of a CS3 user attribute (claim) that should be mapped to the 'user_oidc_claim'. Supported values are 'username', 'mail' and 'userid'." introductionVersion:"1.0.0"` + TenantOIDCClaim string `yaml:"tenant_oidc_claim" env:"PROXY_TENANT_OIDC_CLAIM" desc:"JMESPath expression to extract the tenant ID from the OIDC token claims. When set, the extracted value is verified against the tenant ID returned by the user backend, rejecting requests where they do not match. Only relevant when multi-tenancy is enabled." introductionVersion:"%%NEXT%%"` MachineAuthAPIKey string `yaml:"machine_auth_api_key" env:"OC_MACHINE_AUTH_API_KEY;PROXY_MACHINE_AUTH_API_KEY" desc:"Machine auth API key used to validate internal requests necessary to access resources from other services." introductionVersion:"1.0.0" mask:"password"` AutoprovisionAccounts bool `yaml:"auto_provision_accounts" env:"PROXY_AUTOPROVISION_ACCOUNTS" desc:"Set this to 'true' to automatically provision users that do not yet exist in the users service on-demand upon first sign-in. To use this a write-enabled libregraph user backend needs to be setup an running." introductionVersion:"1.0.0"` AutoProvisionClaims AutoProvisionClaims `yaml:"auto_provision_claims"` diff --git a/services/proxy/pkg/middleware/account_resolver.go b/services/proxy/pkg/middleware/account_resolver.go index ae55d5521d..68c388bc2f 100644 --- a/services/proxy/pkg/middleware/account_resolver.go +++ b/services/proxy/pkg/middleware/account_resolver.go @@ -42,6 +42,7 @@ func AccountResolver(optionSetters ...Option) func(next http.Handler) http.Handl userProvider: options.UserProvider, userOIDCClaim: options.UserOIDCClaim, userCS3Claim: options.UserCS3Claim, + tenantOIDCClaim: options.TenantOIDCClaim, userRoleAssigner: options.UserRoleAssigner, autoProvisionAccounts: options.AutoprovisionAccounts, multiTenantEnabled: options.MultiTenantEnabled, @@ -61,6 +62,7 @@ type accountResolver struct { multiTenantEnabled bool userOIDCClaim string userCS3Claim string + tenantOIDCClaim string // lastGroupSyncCache is used to keep track of when the last sync of group // memberships was done for a specific user. This is used to trigger a sync // with every single request. @@ -68,7 +70,7 @@ type accountResolver struct { eventsPublisher events.Publisher } -func readUserIDClaim(path string, claims map[string]interface{}) (string, error) { +func readStringClaim(path string, claims map[string]interface{}) (string, error) { // happy path value, _ := claims[path].(string) if value != "" { @@ -118,7 +120,7 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { } if user == nil && claims != nil { - value, err := readUserIDClaim(m.userOIDCClaim, claims) + value, err := readStringClaim(m.userOIDCClaim, claims) if err != nil { m.logger.Error().Err(err).Msg("could not read user id claim") w.WriteHeader(http.StatusInternalServerError) @@ -169,6 +171,15 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { return } + // if a tenant claim is configured, verify it matches the tenant id on the resolved user + if m.tenantOIDCClaim != "" { + if err = m.verifyTenantClaim(user.GetId().GetTenantId(), claims); err != nil { + m.logger.Error().Err(err).Str("userid", user.GetId().GetOpaqueId()).Msg("Tenant claim mismatch") + w.WriteHeader(http.StatusUnauthorized) + return + } + } + // update user if needed if m.autoProvisionAccounts { if err = m.userProvider.UpdateUserIfNeeded(req.Context(), user, claims); err != nil { @@ -248,3 +259,14 @@ func (m accountResolver) ServeHTTP(w http.ResponseWriter, req *http.Request) { span.End() m.next.ServeHTTP(w, req) } + +func (m accountResolver) verifyTenantClaim(userTenantID string, claims map[string]interface{}) error { + claimTenantID, err := readStringClaim(m.tenantOIDCClaim, claims) + if err != nil { + return fmt.Errorf("could not read tenant claim: %w", err) + } + if claimTenantID != userTenantID { + return fmt.Errorf("tenant id from claim %q does not match user tenant id %q", claimTenantID, userTenantID) + } + return nil +} diff --git a/services/proxy/pkg/middleware/account_resolver_test.go b/services/proxy/pkg/middleware/account_resolver_test.go index b279e869fb..79bb6cca06 100644 --- a/services/proxy/pkg/middleware/account_resolver_test.go +++ b/services/proxy/pkg/middleware/account_resolver_test.go @@ -20,14 +20,21 @@ import ( "github.com/stretchr/testify/mock" ) +const ( + testIdP = "https://idx.example.com" + testTenantA = "tenant-a" + testTenantB = "tenant-b" + testJWTSecret = "change-me" +) + func TestTokenIsAddedWithMailClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Id: &userv1beta1.UserId{Idp: testIdP, OpaqueId: "123"}, Mail: "foo@example.com", }, nil, oidc.Email, "mail", false) req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.Email: "foo@example.com", }) @@ -40,12 +47,12 @@ func TestTokenIsAddedWithMailClaim(t *testing.T) { func TestTokenIsAddedWithUsernameClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Id: &userv1beta1.UserId{Idp: testIdP, OpaqueId: "123"}, Mail: "foo@example.com", }, nil, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.PreferredUsername: "foo", }) @@ -59,13 +66,13 @@ func TestTokenIsAddedWithUsernameClaim(t *testing.T) { func TestTokenIsAddedWithDotUsernamePathClaim(t *testing.T) { sut := newMockAccountResolver(&userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Id: &userv1beta1.UserId{Idp: testIdP, OpaqueId: "123"}, Mail: "foo@example.com", }, nil, "li.un", "username", false) // This is how lico adds the username to the access token req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, "li": map[string]interface{}{ "un": "foo", }, @@ -79,44 +86,44 @@ func TestTokenIsAddedWithDotUsernamePathClaim(t *testing.T) { assert.Contains(t, token, "eyJ") } -func TestTokenIsAddedWithDotEscapedUsernameClaim(t *testing.T) { - sut := newMockAccountResolver(&userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, - Mail: "foo@example.com", - }, nil, "li\\.un", "username", false) +func TestTokenIsAddedWithDottedUsernameClaim(t *testing.T) { + tests := []struct { + name string + oidcClaim string + // comment describing what the claim exercises + desc string + }{ + { + name: "escaped dot treated as literal key", + oidcClaim: "li\\.un", + desc: "li\\.un escapes the dot so the claim is looked up as the literal key \"li.un\"", + }, + { + name: "dotted path falls back to literal key", + oidcClaim: "li.un", + desc: "li.un is first tried as a nested path; when \"un\" is absent under \"li\", it falls back to the literal key \"li.un\"", + }, + } - // This tests the . escaping of the readUserIDClaim - req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", - "li.un": "foo", - }) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + sut := newMockAccountResolver(&userv1beta1.User{ + Id: &userv1beta1.UserId{Idp: testIdP, OpaqueId: "123"}, + Mail: "foo@example.com", + }, nil, tc.oidcClaim, "username", false) - sut.ServeHTTP(rw, req) + req, rw := mockRequest(map[string]interface{}{ + oidc.Iss: testIdP, + "li.un": "foo", + }) - token := req.Header.Get(revactx.TokenHeader) - assert.NotEmpty(t, token) + sut.ServeHTTP(rw, req) - assert.Contains(t, token, "eyJ") -} - -func TestTokenIsAddedWithDottedUsernameClaimFallback(t *testing.T) { - sut := newMockAccountResolver(&userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, - Mail: "foo@example.com", - }, nil, "li.un", "username", false) - - // This tests the . escaping fallback of the readUserIDClaim - req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", - "li.un": "foo", - }) - - sut.ServeHTTP(rw, req) - - token := req.Header.Get(revactx.TokenHeader) - assert.NotEmpty(t, token) - - assert.Contains(t, token, "eyJ") + token := req.Header.Get(revactx.TokenHeader) + assert.NotEmpty(t, token) + assert.Contains(t, token, "eyJ") + }) + } } func TestNSkipOnNoClaims(t *testing.T) { @@ -133,7 +140,7 @@ func TestNSkipOnNoClaims(t *testing.T) { func TestUnauthorizedOnUserNotFound(t *testing.T) { sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.PreferredUsername: "foo", }) @@ -147,7 +154,7 @@ func TestUnauthorizedOnUserNotFound(t *testing.T) { func TestUnauthorizedOnUserDisabled(t *testing.T) { sut := newMockAccountResolver(nil, backend.ErrAccountDisabled, oidc.PreferredUsername, "username", false) req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.PreferredUsername: "foo", }) @@ -161,7 +168,7 @@ func TestUnauthorizedOnUserDisabled(t *testing.T) { func TestInternalServerErrorOnMissingMailAndUsername(t *testing.T) { sut := newMockAccountResolver(nil, backend.ErrAccountNotFound, oidc.Email, "mail", false) req, rw := mockRequest(map[string]interface{}{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, }) sut.ServeHTTP(rw, req) @@ -174,12 +181,12 @@ func TestInternalServerErrorOnMissingMailAndUsername(t *testing.T) { func TestUnauthorizedOnMissingTenantId(t *testing.T) { sut := newMockAccountResolver( &userv1beta1.User{ - Id: &userv1beta1.UserId{Idp: "https://idx.example.com", OpaqueId: "123"}, + Id: &userv1beta1.UserId{Idp: testIdP, OpaqueId: "123"}, Username: "foo", }, nil, oidc.PreferredUsername, "username", true) req, rw := mockRequest(map[string]any{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.PreferredUsername: "foo", }) @@ -194,7 +201,7 @@ func TestTokenIsAddedWhenUserHasTenantId(t *testing.T) { sut := newMockAccountResolver( &userv1beta1.User{ Id: &userv1beta1.UserId{ - Idp: "https://idx.example.com", + Idp: testIdP, OpaqueId: "123", TenantId: "tenant1", }, @@ -202,7 +209,7 @@ func TestTokenIsAddedWhenUserHasTenantId(t *testing.T) { }, nil, oidc.PreferredUsername, "username", true) req, rw := mockRequest(map[string]any{ - oidc.Iss: "https://idx.example.com", + oidc.Iss: testIdP, oidc.PreferredUsername: "foo", }) @@ -213,9 +220,78 @@ func TestTokenIsAddedWhenUserHasTenantId(t *testing.T) { assert.Contains(t, token, "eyJ") } +func TestTenantClaimValidation(t *testing.T) { + tests := []struct { + name string + requestTenant string + wantToken bool + wantStatusCode int + }{ + { + name: "token added when tenant claim matches", + requestTenant: testTenantA, + wantToken: true, + wantStatusCode: http.StatusOK, + }, + { + name: "unauthorized when tenant claim does not match", + requestTenant: testTenantB, + wantToken: false, + wantStatusCode: http.StatusUnauthorized, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + user := &userv1beta1.User{ + Id: &userv1beta1.UserId{ + Idp: testIdP, + OpaqueId: "123", + TenantId: testTenantA, + }, + Username: "foo", + } + + tokenManager, _ := jwt.New(map[string]interface{}{"secret": testJWTSecret, "expires": int64(60)}) + s, _ := scope.AddOwnerScope(nil) + token, _ := tokenManager.MintToken(context.Background(), user, s) + + ub := mocks.UserBackend{} + ub.On("GetUserByClaims", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(user, token, nil) + ra := userRoleMocks.UserRoleAssigner{} + ra.On("UpdateUserRoleAssignment", mock.Anything, mock.Anything, mock.Anything).Return(user, nil) + + sut := AccountResolver( + Logger(log.NewLogger()), + UserProvider(&ub), + UserRoleAssigner(&ra), + UserOIDCClaim(oidc.PreferredUsername), + UserCS3Claim("username"), + TenantOIDCClaim("tenant_id"), + MultiTenantEnabled(true), + )(mockHandler{}) + + req, rw := mockRequest(map[string]interface{}{ + oidc.Iss: testIdP, + oidc.PreferredUsername: "foo", + "tenant_id": tc.requestTenant, + }) + + sut.ServeHTTP(rw, req) + + if tc.wantToken { + assert.NotEmpty(t, req.Header.Get(revactx.TokenHeader)) + } else { + assert.Empty(t, req.Header.Get(revactx.TokenHeader)) + } + assert.Equal(t, tc.wantStatusCode, rw.Code) + }) + } +} + func newMockAccountResolver(userBackendResult *userv1beta1.User, userBackendErr error, oidcclaim, cs3claim string, multiTenant bool) http.Handler { tokenManager, _ := jwt.New(map[string]interface{}{ - "secret": "change-me", + "secret": testJWTSecret, "expires": int64(60), }) diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index e87afeaffc..9d434bbbbf 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -49,6 +49,9 @@ type Options struct { UserOIDCClaim string // UserCS3Claim to use when looking up a user in the CS3 API UserCS3Claim string + // TenantOIDCClaim is a JMESPath expression to extract the tenant ID from the OIDC claims. + // When set, the extracted value is verified against the tenant ID on the resolved user. + TenantOIDCClaim string // AutoprovisionAccounts when an accountResolver does not exist. AutoprovisionAccounts bool // EnableBasicAuth to allow basic auth @@ -171,6 +174,13 @@ func UserCS3Claim(val string) Option { } } +// TenantOIDCClaim provides a function to set the TenantOIDCClaim config +func TenantOIDCClaim(val string) Option { + return func(o *Options) { + o.TenantOIDCClaim = val + } +} + // AutoprovisionAccounts provides a function to set the AutoprovisionAccounts config func AutoprovisionAccounts(val bool) Option { return func(o *Options) {