From d5f4fbecc1055dfe7f2378754a32068aa8c47b6b Mon Sep 17 00:00:00 2001 From: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> Date: Mon, 22 Nov 2021 09:42:22 -0800 Subject: [PATCH] identity/oidc: optional nonce parameter for authorize request (#13231) --- changelog/13231.txt | 3 + go.sum | 1 - vault/identity_store_oidc.go | 9 ++- vault/identity_store_oidc_provider.go | 22 +++--- vault/identity_store_oidc_provider_test.go | 77 +++++++++++++++---- .../secret/identity/oidc-provider.mdx | 10 +-- .../content/docs/concepts/oidc-provider.mdx | 4 +- .../docs/secrets/identity/oidc-provider.mdx | 5 +- 8 files changed, 87 insertions(+), 44 deletions(-) create mode 100644 changelog/13231.txt diff --git a/changelog/13231.txt b/changelog/13231.txt new file mode 100644 index 000000000..3af523381 --- /dev/null +++ b/changelog/13231.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity/oidc: Make the `nonce` parameter optional for the Authorization Endpoint of OIDC providers. +``` diff --git a/go.sum b/go.sum index 2698877f7..ad0cf3cd6 100644 --- a/go.sum +++ b/go.sum @@ -830,7 +830,6 @@ github.com/hashicorp/go-kms-wrapping/entropy v0.1.0/go.mod h1:d1g9WGtAunDNpek8jU github.com/hashicorp/go-memdb v1.3.2 h1:RBKHOsnSszpU6vxq80LzC2BaQjuuvoyaQbkLTf7V7g8= github.com/hashicorp/go-memdb v1.3.2/go.mod h1:Mluclgwib3R93Hk5fxEfiRhB+6Dar64wWh71LpNSe3g= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= -github.com/hashicorp/go-msgpack v0.5.5 h1:i9R9JSrqIz0QVLz3sz+i3YJdT7TTSLcfLLzJi9aZTuI= github.com/hashicorp/go-msgpack v0.5.5/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= github.com/hashicorp/go-msgpack v1.1.5 h1:9byZdVjKTe5mce63pRVNP1L7UAmdHOTEMGehn6KvJWs= github.com/hashicorp/go-msgpack v1.1.5/go.mod h1:gWVc3sv/wbDmR3rQsj1CAktEZzoz1YNK9NfGLXJ69/4= diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index cc66e6319..e9dbd1824 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -111,7 +111,7 @@ const ( ) var ( - requiredClaims = []string{ + reservedClaims = []string{ "iat", "aud", "exp", "iss", "sub", "namespace", "nonce", "auth_time", "at_hash", "c_hash", @@ -970,6 +970,7 @@ func (tok *idToken) generatePayload(logger hclog.Logger, templates ...string) ([ "iat": tok.IssuedAt, } + // Copy optional claims into output if len(tok.Nonce) > 0 { output["nonce"] = tok.Nonce } @@ -1009,7 +1010,7 @@ func mergeJSONTemplates(logger hclog.Logger, output map[string]interface{}, temp } for k, v := range parsed { - if !strutil.StrListContains(requiredClaims, k) { + if !strutil.StrListContains(reservedClaims, k) { output[k] = v } else { logger.Warn("invalid top level OIDC template key", "template", template, "key", k) @@ -1114,9 +1115,9 @@ func (i *IdentityStore) pathOIDCCreateUpdateRole(ctx context.Context, req *logic } for key := range tmp { - if strutil.StrListContains(requiredClaims, key) { + if strutil.StrListContains(reservedClaims, key) { return logical.ErrorResponse(`top level key %q not allowed. Restricted keys: %s`, - key, strings.Join(requiredClaims, ", ")), nil + key, strings.Join(reservedClaims, ", ")), nil } } } diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index cff96505b..6a8e6787a 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -403,7 +403,6 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { "nonce": { Type: framework.TypeString, Description: "The value that will be returned in the ID token nonce claim after a token exchange.", - Required: true, }, "max_age": { Type: framework.TypeInt, @@ -793,9 +792,9 @@ func (i *IdentityStore) pathOIDCCreateUpdateScope(ctx context.Context, req *logi } for key := range tmp { - if strutil.StrListContains(requiredClaims, key) { + if strutil.StrListContains(reservedClaims, key) { return logical.ErrorResponse(`top level key %q not allowed. Restricted keys: %s`, - key, strings.Join(requiredClaims, ", ")), nil + key, strings.Join(reservedClaims, ", ")), nil } } } @@ -1518,12 +1517,6 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ return authResponse("", state, ErrAuthInvalidRedirectURI, "redirect_uri is not allowed for the client") } - // Validate the nonce - nonce := d.Get("nonce").(string) - if nonce == "" { - return authResponse("", state, ErrAuthInvalidRequest, "nonce parameter is required") - } - // We don't support the request or request_uri parameters. If they're provided, // the appropriate errors must be returned. For details, see the spec at: // https://openid.net/specs/openid-connect-core-1_0.html#RequestObject @@ -1556,6 +1549,10 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ return authResponse("", state, ErrAuthAccessDenied, "identity entity not authorized by client assignment") } + // A nonce is optional for the authorization code flow. If not + // provided, the nonce claim will be omitted from the ID token. + nonce := d.Get("nonce").(string) + // Create the auth code cache entry authCodeEntry := &authCodeCacheEntry{ provider: name, @@ -1567,10 +1564,9 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ } // Validate the optional max_age parameter to check if an active re-authentication - // of the user should occur. Re-authentication will be requested if max_age=0 or the - // last time the token actively authenticated exceeds the given max_age requirement. - // Returning ErrAuthMaxAgeReAuthenticate will enforce the user to re-authenticate via - // the user agent. + // of the user should occur. Re-authentication will be requested if the last time + // the token actively authenticated exceeds the given max_age requirement. Returning + // ErrAuthMaxAgeReAuthenticate will enforce the user to re-authenticate via the user agent. if maxAgeRaw, ok := d.GetOk("max_age"); ok { maxAge := maxAgeRaw.(int) if maxAge < 1 { diff --git a/vault/identity_store_oidc_provider_test.go b/vault/identity_store_oidc_provider_test.go index 02499b372..e279b2143 100644 --- a/vault/identity_store_oidc_provider_test.go +++ b/vault/identity_store_oidc_provider_test.go @@ -287,6 +287,20 @@ func TestOIDC_Path_OIDC_Token(t *testing.T) { }, }, }, + { + name: "valid token request with empty nonce in authorize request", + args: args{ + clientReq: testClientReq(s), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + delete(req.Data, "nonce") + return req + }(), + tokenReq: testTokenReq(s, "", clientID, clientSecret), + }, + }, { name: "valid token request", args: args{ @@ -404,9 +418,39 @@ func TestOIDC_Path_OIDC_Token(t *testing.T) { require.Equal(t, "Bearer", tokenRes.TokenType) require.NotEmpty(t, tokenRes.AccessToken) require.NotEmpty(t, tokenRes.IDToken) - require.NotEmpty(t, tokenRes.ExpiresIn) + require.Equal(t, int64(86400), tokenRes.ExpiresIn) require.Empty(t, tokenRes.Error) require.Empty(t, tokenRes.ErrorDescription) + + // Parse the claims from the ID token payload + parts := strings.Split(tokenRes.IDToken, ".") + require.Equal(t, 3, len(parts)) + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + require.NoError(t, err) + claims := make(map[string]interface{}) + require.NoError(t, json.Unmarshal(payload, &claims)) + + // Assert that reserved claims are present in the ID token. + // Optional reserved claims are asserted on conditionally. + for _, c := range reservedClaims { + switch c { + case "nonce": + // nonce must equal the nonce provided in the authorize request (including empty) + require.EqualValues(t, tt.args.authorizeReq.Data[c], claims[c]) + + case "auth_time": + // auth_time must exist if max_age provided in the authorize request + if _, ok := tt.args.authorizeReq.Data["max_age"]; ok { + require.EqualValues(t, creationTime.Unix(), claims[c]) + } else { + require.Empty(t, claims[c]) + } + + default: + // other reserved claims must be present in all cases + require.NotEmpty(t, claims[c]) + } + } }) } } @@ -579,21 +623,6 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { }, wantErr: ErrAuthInvalidRequest, }, - { - name: "invalid authorize request with missing nonce", - args: args{ - entityID: entityID, - clientReq: testClientReq(s), - providerReq: testProviderReq(s, clientID), - assignmentReq: testAssignmentReq(s, entityID, groupID), - authorizeReq: func() *logical.Request { - req := testAuthorizeReq(s, clientID) - req.Data["nonce"] = "" - return req - }(), - }, - wantErr: ErrAuthInvalidRequest, - }, { name: "invalid authorize request with request parameter provided", args: args{ @@ -683,6 +712,20 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { }, wantErr: ErrAuthInvalidRequest, }, + { + name: "valid authorize request with empty nonce", + args: args{ + entityID: entityID, + clientReq: testClientReq(s), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + delete(req.Data, "nonce") + return req + }(), + }, + }, { name: "active re-authentication required with token creation time exceeding max_age requirement", args: args{ @@ -842,7 +885,7 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { } // setupOIDCCommon creates all of the resources needed to test a Vault OIDC provider. -// Returns the entity ID, group ID, and client ID to be used in tests. +// Returns the entity ID, group ID, client ID, client secret to be used in tests. func setupOIDCCommon(t *testing.T, c *Core, s logical.Storage) (string, string, string, string) { t.Helper() ctx := namespace.RootContext(nil) diff --git a/website/content/api-docs/secret/identity/oidc-provider.mdx b/website/content/api-docs/secret/identity/oidc-provider.mdx index d7af7f1ff..4de357852 100644 --- a/website/content/api-docs/secret/identity/oidc-provider.mdx +++ b/website/content/api-docs/secret/identity/oidc-provider.mdx @@ -37,7 +37,7 @@ This endpoint creates or updates a Provider. ### Sample Request ```shell-session -$ curl \ +$ curl \ --header "X-Vault-Token: ..." \ --request POST \ --data @payload.json \ @@ -154,7 +154,7 @@ This endpoint creates or updates a scope. ### Sample Request ```shell-session -$ curl \ +$ curl \ --header "X-Vault-Token: ..." \ --request POST \ --data @payload.json \ @@ -281,7 +281,7 @@ This endpoint creates or updates a client. ### Sample Request ```shell-session -$ curl \ +$ curl \ --header "X-Vault-Token: ..." \ --request POST \ --data @payload.json \ @@ -402,7 +402,7 @@ This endpoint creates or updates an assignment. ### Sample Request ```shell-session -$ curl \ +$ curl \ --header "X-Vault-Token: ..." \ --request POST \ --data @payload.json \ @@ -620,7 +620,7 @@ to be used for the [Authorization Code Flow](https://openid.net/specs/openid-con - `state` `(string: )` - A value used to maintain state between the authentication request and client. -- `nonce` `(string: )` - A value that is returned in the ID token nonce claim. It is used to mitigate replay attacks. +- `nonce` `(string: )` - A value that is returned in the ID token nonce claim. It is used to mitigate replay attacks, so we *strongly encourage* providing this optional parameter. ### Sample Request diff --git a/website/content/docs/concepts/oidc-provider.mdx b/website/content/docs/concepts/oidc-provider.mdx index b9cc30778..ab6b58369 100644 --- a/website/content/docs/concepts/oidc-provider.mdx +++ b/website/content/docs/concepts/oidc-provider.mdx @@ -137,11 +137,11 @@ Each provider offers an unauthenticated endpoint that provides the public portio ### Authorization Endpoint -Each provider offers an authenticated [authorization endpoint](https://openid.net/specs/openid-connect-core-1_0.html#AuthorizationEndpoint). The authorization endpoint for each provider is added to Vault's [default policy](/docs/concepts/policies#default-policy) using the `identity/oidc/provider/+/authorize` path. The endpoint incorporates all required [authentication request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest) parameters as input. Additionally, the `state` and `nonce` parameters are required. +Each provider offers an authenticated [authorization endpoint](https://openid.net/specs/openid-connect-core-1_0.html#AuthorizationEndpoint). The authorization endpoint for each provider is added to Vault's [default policy](/docs/concepts/policies#default-policy) using the `identity/oidc/provider/+/authorize` path. The endpoint incorporates all required [authentication request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest) parameters as input. Additionally, the `state` parameter is required. The endpoint [validates](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequestValidation) client requests and ensures that all required parameters are present and valid. The `redirect_uri` of the request is validated against the client's `redirect_uris`. The requesting Vault entity will be validated against the client's `assignments`. An appropriate [error code](https://openid.net/specs/openid-connect-core-1_0.html#AuthError) is returned for invalid requests. -An authorization code is generated with a successful validation of the request. The authorization code is single-use and cached with a lifetime of approximately 5 minutes, which mitigates the risk of leaks. A response including the original `state` presented by the client and `code` will be returned to the Vault UI which initiated the request. Vault will issue an HTTP 302 redirect to the `redirect_uri` of the request, which includes the `code`, `state`, and `nonce` as query parameters. +An authorization code is generated with a successful validation of the request. The authorization code is single-use and cached with a lifetime of approximately 5 minutes, which mitigates the risk of leaks. A response including the original `state` presented by the client and `code` will be returned to the Vault UI which initiated the request. Vault will issue an HTTP 302 redirect to the `redirect_uri` of the request, which includes the `code` and `state` as query parameters. ### Token Endpoint diff --git a/website/content/docs/secrets/identity/oidc-provider.mdx b/website/content/docs/secrets/identity/oidc-provider.mdx index 75db03e3a..05225109d 100644 --- a/website/content/docs/secrets/identity/oidc-provider.mdx +++ b/website/content/docs/secrets/identity/oidc-provider.mdx @@ -16,9 +16,10 @@ identity. Clients can configure their authentication logic to talk to Vault. Once enabled, Vault will act as the bridge to identity providers via its existing authentication methods. Clients will also obtain identity information for their end-users by leveraging custom templating of Vault identity -information. +information. For more information on the configuration resources and OIDC endpoints, +please visit the [OIDC provider](/docs/concepts/oidc-provider) concepts page. -The Vault OIDC provider feature currently only supports the +The Vault OIDC provider feature currently only supports the [authorization code flow](https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth). ## OIDC Provider Configuration