diff --git a/vault/external_tests/identity/oidc_provider_test.go b/vault/external_tests/identity/oidc_provider_test.go index a268d2d95863a..7a81977e0be5f 100644 --- a/vault/external_tests/identity/oidc_provider_test.go +++ b/vault/external_tests/identity/oidc_provider_test.go @@ -141,7 +141,7 @@ func TestOIDC_Auth_Code_Flow_CAP_Client(t *testing.T) { // Create the OIDC provider _, err = active.Logical().Write("identity/oidc/provider/test-provider", map[string]interface{}{ "allowed_client_ids": []string{clientID}, - "scopes": []string{"user", "groups"}, + "scopes_supported": []string{"user", "groups"}, }) require.NoError(t, err) diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index 3c46a7a431087..73845bfab2902 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -33,6 +33,9 @@ const ( scopesDelimiter = " " accessTokenScopesMeta = "scopes" accessTokenClientIDMeta = "client_id" + clientIDLength = 32 + clientSecretLength = 64 + clientSecretPrefix = "hvo_secret_" // Storage path constants oidcProviderPrefix = "oidc_provider/" @@ -41,10 +44,6 @@ const ( clientPath = oidcProviderPrefix + "client/" providerPath = oidcProviderPrefix + "provider/" - clientIDLength = 32 - clientSecretLength = 64 - clientSecretPrefix = "hvo_secret_" - // Error constants used in the Authorization Endpoint. See details at // https://openid.net/specs/openid-connect-core-1_0.html#AuthError. ErrAuthUnsupportedResponseType = "unsupported_response_type" @@ -108,7 +107,7 @@ type client struct { type provider struct { Issuer string `json:"issuer"` AllowedClientIDs []string `json:"allowed_client_ids"` - Scopes []string `json:"scopes"` + ScopesSupported []string `json:"scopes_supported"` // effectiveIssuer is a calculated field and will be either Issuer (if // that's set) or the Vault instance's api_addr. @@ -303,9 +302,9 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { Type: framework.TypeCommaStringSlice, Description: "The client IDs that are permitted to use the provider", }, - "scopes": { + "scopes_supported": { Type: framework.TypeCommaStringSlice, - Description: "The scopes available for requesting on the provider", + Description: "The scopes supported for requesting on the provider", }, }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -344,8 +343,10 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { Description: "Name of the provider", }, }, - Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.ReadOperation: i.pathOIDCProviderDiscovery, + Operations: map[logical.Operation]framework.OperationHandler{ + logical.ReadOperation: &framework.PathOperation{ + Callback: i.pathOIDCProviderDiscovery, + }, }, HelpSynopsis: "Query OIDC configurations", HelpDescription: "Query this path to retrieve the configured OIDC Issuer and Keys endpoints, response types, subject types, and signing algorithms used by the OIDC backend.", @@ -358,8 +359,10 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { Description: "Name of the provider", }, }, - Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.ReadOperation: i.pathOIDCReadProviderPublicKeys, + Operations: map[logical.Operation]framework.OperationHandler{ + logical.ReadOperation: &framework.PathOperation{ + Callback: i.pathOIDCReadProviderPublicKeys, + }, }, HelpSynopsis: "Retrieve public keys", HelpDescription: "Returns the public portion of keys for a named OIDC provider. Clients can use them to validate the authenticity of an ID token.", @@ -589,7 +592,7 @@ func (i *IdentityStore) providersReferencingTargetScopeName(ctx context.Context, if err := entry.DecodeJSON(&tempProvider); err != nil { return nil, err } - for _, a := range tempProvider.Scopes { + for _, a := range tempProvider.ScopesSupported { if a == targetScopeName { providers = append(providers, providerName) } @@ -620,13 +623,13 @@ func (i *IdentityStore) pathOIDCCreateUpdateAssignment(ctx context.Context, req if entitiesRaw, ok := d.GetOk("entity_ids"); ok { assignment.EntityIDs = entitiesRaw.([]string) } else if req.Operation == logical.CreateOperation { - assignment.EntityIDs = d.GetDefaultOrZero("entity_ids").([]string) + assignment.EntityIDs = d.Get("entity_ids").([]string) } if groupsRaw, ok := d.GetOk("group_ids"); ok { assignment.GroupIDs = groupsRaw.([]string) } else if req.Operation == logical.CreateOperation { - assignment.GroupIDs = d.GetDefaultOrZero("group_ids").([]string) + assignment.GroupIDs = d.Get("group_ids").([]string) } // remove duplicates and lowercase entities and groups @@ -748,13 +751,13 @@ func (i *IdentityStore) pathOIDCCreateUpdateScope(ctx context.Context, req *logi if descriptionRaw, ok := d.GetOk("description"); ok { scope.Description = descriptionRaw.(string) } else if req.Operation == logical.CreateOperation { - scope.Description = d.GetDefaultOrZero("description").(string) + scope.Description = d.Get("description").(string) } if templateRaw, ok := d.GetOk("template"); ok { scope.Template = templateRaw.(string) } else if req.Operation == logical.CreateOperation { - scope.Template = d.GetDefaultOrZero("template").(string) + scope.Template = d.Get("template").(string) } // Attempt to decode as base64 and use that if it works @@ -1093,24 +1096,24 @@ func (i *IdentityStore) pathOIDCCreateUpdateProvider(ctx context.Context, req *l if issuerRaw, ok := d.GetOk("issuer"); ok { provider.Issuer = issuerRaw.(string) } else if req.Operation == logical.CreateOperation { - provider.Issuer = d.GetDefaultOrZero("issuer").(string) + provider.Issuer = d.Get("issuer").(string) } if allowedClientIDsRaw, ok := d.GetOk("allowed_client_ids"); ok { provider.AllowedClientIDs = allowedClientIDsRaw.([]string) } else if req.Operation == logical.CreateOperation { - provider.AllowedClientIDs = d.GetDefaultOrZero("allowed_client_ids").([]string) + provider.AllowedClientIDs = d.Get("allowed_client_ids").([]string) } - if scopesRaw, ok := d.GetOk("scopes"); ok { - provider.Scopes = scopesRaw.([]string) + if scopesRaw, ok := d.GetOk("scopes_supported"); ok { + provider.ScopesSupported = scopesRaw.([]string) } else if req.Operation == logical.CreateOperation { - provider.Scopes = d.GetDefaultOrZero("scopes").([]string) + provider.ScopesSupported = d.Get("scopes_supported").([]string) } // remove duplicate allowed client IDs and scopes provider.AllowedClientIDs = strutil.RemoveDuplicates(provider.AllowedClientIDs, false) - provider.Scopes = strutil.RemoveDuplicates(provider.Scopes, false) + provider.ScopesSupported = strutil.RemoveDuplicates(provider.ScopesSupported, false) if provider.Issuer != "" { // verify that issuer is the correct format: @@ -1143,7 +1146,7 @@ func (i *IdentityStore) pathOIDCCreateUpdateProvider(ctx context.Context, req *l } scopeTemplateKeyNames := make(map[string]string) - for _, scopeName := range provider.Scopes { + for _, scopeName := range provider.ScopesSupported { scope, err := i.getOIDCScope(ctx, req.Storage, scopeName) if err != nil { return nil, err @@ -1223,7 +1226,7 @@ func (i *IdentityStore) pathOIDCReadProvider(ctx context.Context, req *logical.R Data: map[string]interface{}{ "issuer": provider.Issuer, "allowed_client_ids": provider.AllowedClientIDs, - "scopes": provider.Scopes, + "scopes_supported": provider.ScopesSupported, }, }, nil } @@ -1286,7 +1289,7 @@ func (i *IdentityStore) pathOIDCProviderDiscovery(ctx context.Context, req *logi } // the "openid" scope is reserved and is included for every provider - scopes := append(p.Scopes, openIDScope) + scopes := append(p.ScopesSupported, openIDScope) disc := providerDiscovery{ Issuer: p.effectiveIssuer, @@ -1455,7 +1458,7 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ // Scope values that are not supported by the provider should be ignored scopes := make([]string, 0) for _, scope := range requestedScopes { - if strutil.StrListContains(provider.Scopes, scope) && scope != openIDScope { + if strutil.StrListContains(provider.ScopesSupported, scope) && scope != openIDScope { scopes = append(scopes, scope) } } @@ -1513,12 +1516,15 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ } // Validate that there is an identity entity associated with the request + if req.EntityID == "" { + return authResponse("", state, ErrAuthAccessDenied, "identity entity must be associated with the request") + } entity, err := i.MemDBEntityByID(req.EntityID, false) if err != nil { return authResponse("", state, ErrAuthServerError, err.Error()) } if entity == nil { - return authResponse("", state, ErrAuthAccessDenied, "identity entity must be associated with the request") + return authResponse("", state, ErrAuthAccessDenied, "identity entity associated with the request not found") } // Validate that the entity is a member of the client's assignments @@ -1733,7 +1739,7 @@ func (i *IdentityStore) pathOIDCToken(ctx context.Context, req *logical.Request, return tokenResponse(nil, ErrTokenServerError, err.Error()) } if entity == nil { - return tokenResponse(nil, ErrTokenInvalidRequest, "identity entity associated with request not found") + return tokenResponse(nil, ErrTokenInvalidRequest, "identity entity associated with the request not found") } // Validate that the entity is a member of the client's assignments @@ -1935,13 +1941,16 @@ func (i *IdentityStore) pathOIDCUserInfo(ctx context.Context, req *logical.Reque return userInfoResponse(nil, ErrUserInfoAccessDenied, "client not found") } - // Get the entity associated with the request + // Validate that there is an identity entity associated with the request + if req.EntityID == "" { + return userInfoResponse(nil, ErrUserInfoAccessDenied, "identity entity must be associated with the request") + } entity, err := i.MemDBEntityByID(req.EntityID, false) if err != nil { return userInfoResponse(nil, ErrUserInfoServerError, err.Error()) } if entity == nil { - return userInfoResponse(nil, ErrUserInfoAccessDenied, "identity entity must be associated with the request") + return userInfoResponse(nil, ErrUserInfoAccessDenied, "identity entity associated with the request not found") } // Validate that the entity is a member of the client's assignments @@ -1959,14 +1968,22 @@ func (i *IdentityStore) pathOIDCUserInfo(ctx context.Context, req *logical.Reque } // Get the scopes for the access token - scopes, ok := te.InternalMeta[accessTokenScopesMeta] - if !ok || len(scopes) == 0 { + tokenScopes, ok := te.InternalMeta[accessTokenScopesMeta] + if !ok || len(tokenScopes) == 0 { return userInfoResponse(claims, "", "") } - parsedScopes := strutil.ParseStringSlice(scopes, scopesDelimiter) + parsedScopes := strutil.ParseStringSlice(tokenScopes, scopesDelimiter) + + // Scope values that are not supported by the provider should be ignored + scopes := make([]string, 0) + for _, scope := range parsedScopes { + if strutil.StrListContains(provider.ScopesSupported, scope) { + scopes = append(scopes, scope) + } + } // Populate each of the token's scope templates - templates, conflict, err := i.populateScopeTemplates(ctx, req.Storage, ns, entity, parsedScopes...) + templates, conflict, err := i.populateScopeTemplates(ctx, req.Storage, ns, entity, scopes...) if !conflict && err != nil { return userInfoResponse(nil, ErrUserInfoServerError, err.Error()) } diff --git a/vault/identity_store_oidc_provider_test.go b/vault/identity_store_oidc_provider_test.go index a39acd191249e..38871144ee716 100644 --- a/vault/identity_store_oidc_provider_test.go +++ b/vault/identity_store_oidc_provider_test.go @@ -576,6 +576,17 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { }, wantErr: ErrAuthRequestURINotSupported, }, + { + name: "invalid authorize request with identity entity not associated with the request", + args: args{ + entityID: "", + clientReq: testClientReq(s), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: testAuthorizeReq(s, clientID), + }, + wantErr: ErrAuthAccessDenied, + }, { name: "invalid authorize request with identity entity ID not found", args: args{ @@ -944,7 +955,7 @@ func testProviderReq(s logical.Storage, clientID string) *logical.Request { Operation: logical.CreateOperation, Data: map[string]interface{}{ "allowed_client_ids": []string{clientID}, - "scopes": []string{"test-scope", "conflict"}, + "scopes_supported": []string{"test-scope", "conflict"}, }, } } @@ -2111,7 +2122,7 @@ func TestOIDC_Path_OIDC_ProviderScope_DeleteWithExistingProvider(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope"}, + "scopes_supported": []string{"test-scope"}, }, Storage: storage, }) @@ -2465,7 +2476,7 @@ func TestOIDC_Path_OIDCProvider(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope"}, + "scopes_supported": []string{"test-scope"}, }, Storage: storage, }) @@ -2494,7 +2505,7 @@ func TestOIDC_Path_OIDCProvider(t *testing.T) { expected := map[string]interface{}{ "issuer": "", "allowed_client_ids": []string{}, - "scopes": []string{}, + "scopes_supported": []string{}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2518,7 +2529,7 @@ func TestOIDC_Path_OIDCProvider(t *testing.T) { Operation: logical.UpdateOperation, Data: map[string]interface{}{ "allowed_client_ids": []string{"test-client-id"}, - "scopes": []string{"test-scope"}, + "scopes_supported": []string{"test-scope"}, }, Storage: storage, }) @@ -2534,7 +2545,7 @@ func TestOIDC_Path_OIDCProvider(t *testing.T) { expected = map[string]interface{}{ "issuer": "", "allowed_client_ids": []string{"test-client-id"}, - "scopes": []string{"test-scope"}, + "scopes_supported": []string{"test-scope"}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2577,7 +2588,7 @@ func TestOIDC_Path_OIDCProvider(t *testing.T) { expected = map[string]interface{}{ "issuer": "https://example.com:8200", "allowed_client_ids": []string{"test-client-id"}, - "scopes": []string{"test-scope"}, + "scopes_supported": []string{"test-scope"}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2639,7 +2650,7 @@ func TestOIDC_Path_OIDCProvider_DuplicateTemplateKeys(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope1", "test-scope2"}, + "scopes_supported": []string{"test-scope1", "test-scope2"}, }, Storage: storage, }) @@ -2664,7 +2675,7 @@ func TestOIDC_Path_OIDCProvider_DuplicateTemplateKeys(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope1", "test-scope2"}, + "scopes_supported": []string{"test-scope1", "test-scope2"}, }, Storage: storage, }) @@ -2695,7 +2706,7 @@ func TestOIDC_Path_OIDCProvider_Deduplication(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope1", "test-scope1"}, + "scopes_supported": []string{"test-scope1", "test-scope1"}, "allowed_client_ids": []string{"test-id1", "test-id2", "test-id1"}, }, Storage: storage, @@ -2712,7 +2723,7 @@ func TestOIDC_Path_OIDCProvider_Deduplication(t *testing.T) { expected := map[string]interface{}{ "issuer": "", "allowed_client_ids": []string{"test-id1", "test-id2"}, - "scopes": []string{"test-scope1"}, + "scopes_supported": []string{"test-scope1"}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2747,7 +2758,7 @@ func TestOIDC_Path_OIDCProvider_Update(t *testing.T) { expected := map[string]interface{}{ "issuer": "https://example.com:8200", "allowed_client_ids": []string{"test-client-id"}, - "scopes": []string{}, + "scopes_supported": []string{}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2774,7 +2785,7 @@ func TestOIDC_Path_OIDCProvider_Update(t *testing.T) { expected = map[string]interface{}{ "issuer": "https://changedurl.com", "allowed_client_ids": []string{"test-client-id"}, - "scopes": []string{}, + "scopes_supported": []string{}, } if diff := deep.Equal(expected, resp.Data); diff != nil { t.Fatal(diff) @@ -2856,7 +2867,7 @@ func TestOIDC_Path_OpenIDProviderConfig(t *testing.T) { Path: "oidc/provider/test-provider", Operation: logical.CreateOperation, Data: map[string]interface{}{ - "scopes": []string{"test-scope-1"}, + "scopes_supported": []string{"test-scope-1"}, }, Storage: storage, }) @@ -2910,8 +2921,8 @@ func TestOIDC_Path_OpenIDProviderConfig(t *testing.T) { Operation: logical.UpdateOperation, Storage: storage, Data: map[string]interface{}{ - "issuer": testIssuer, - "scopes": []string{"test-scope-2"}, + "issuer": testIssuer, + "scopes_supported": []string{"test-scope-2"}, }, }) expectSuccess(t, resp, err)