From a2bc8cfb96c199f77dab2004a79d094d22ea0e7b Mon Sep 17 00:00:00 2001 From: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> Date: Fri, 5 Aug 2022 11:37:24 -0700 Subject: [PATCH] identity/oidc: change the state parameter to optional (#16599) * identity/oidc: change the state parameter to optional * adds changelog * update docs --- changelog/16599.txt | 3 ++ vault/identity_store_oidc_provider.go | 5 --- vault/identity_store_oidc_provider_test.go | 35 +++++++++---------- .../secret/identity/oidc-provider.mdx | 2 +- .../content/docs/concepts/oidc-provider.mdx | 2 +- 5 files changed, 22 insertions(+), 25 deletions(-) create mode 100644 changelog/16599.txt diff --git a/changelog/16599.txt b/changelog/16599.txt new file mode 100644 index 000000000..dd3239527 --- /dev/null +++ b/changelog/16599.txt @@ -0,0 +1,3 @@ +```release-note:bug +identity/oidc: Change the `state` parameter of the Authorization Endpoint to optional. +``` diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index fc32017a6..cd935a8ef 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -448,7 +448,6 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { "state": { Type: framework.TypeString, Description: "The value used to maintain state between the authentication request and client.", - Required: true, }, "nonce": { Type: framework.TypeString, @@ -1609,11 +1608,7 @@ func (i *IdentityStore) keyIDsReferencedByTargetClientIDs(ctx context.Context, s } func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - // Validate the state state := d.Get("state").(string) - if state == "" { - return authResponse("", "", ErrAuthInvalidRequest, "state parameter is required") - } // Get the namespace ns, err := namespace.FromContext(ctx) diff --git a/vault/identity_store_oidc_provider_test.go b/vault/identity_store_oidc_provider_test.go index 24b1b7d36..b75b78b7d 100644 --- a/vault/identity_store_oidc_provider_test.go +++ b/vault/identity_store_oidc_provider_test.go @@ -55,7 +55,7 @@ func TestOIDC_Path_OIDC_Cross_Provider_Exchange(t *testing.T) { require.NoError(t, err) require.NoError(t, json.Unmarshal(resp.Data["http_raw_body"].([]byte), &authRes)) require.Regexp(t, authCodeRegex, authRes.Code) - require.NotEmpty(t, authRes.State) + require.Equal(t, req.Data["state"], authRes.State) // Assert that the authorization code cannot be exchanged using the second provider var tokenRes struct { @@ -477,7 +477,7 @@ func TestOIDC_Path_OIDC_Token(t *testing.T) { } require.NoError(t, json.Unmarshal(resp.Data["http_raw_body"].([]byte), &authRes)) require.Regexp(t, authCodeRegex, authRes.Code) - require.NotEmpty(t, authRes.State) + require.Equal(t, tt.args.authorizeReq.Data["state"], authRes.State) // Update the assignment tt.args.assignmentReq.Operation = logical.UpdateOperation @@ -740,21 +740,6 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { }, wantErr: ErrAuthInvalidRedirectURI, }, - { - name: "invalid authorize request with missing state", - 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["state"] = "" - return req - }(), - }, - wantErr: ErrAuthInvalidRequest, - }, { name: "invalid authorize request with request parameter provided", args: args{ @@ -910,6 +895,20 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { }(), }, }, + { + name: "valid authorize request with empty state", + 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["state"] = "" + return req + }(), + }, + }, { name: "active re-authentication required with token creation time exceeding max_age requirement", args: args{ @@ -1143,7 +1142,7 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { expectSuccess(t, resp, err) require.Equal(t, http.StatusOK, resp.Data[logical.HTTPStatusCode].(int)) require.Regexp(t, authCodeRegex, authRes.Code) - require.NotEmpty(t, authRes.State) + require.Equal(t, tt.args.authorizeReq.Data["state"], authRes.State) require.Empty(t, authRes.Error) require.Empty(t, authRes.ErrorDescription) }) diff --git a/website/content/api-docs/secret/identity/oidc-provider.mdx b/website/content/api-docs/secret/identity/oidc-provider.mdx index 1deffc385..0e896e46a 100644 --- a/website/content/api-docs/secret/identity/oidc-provider.mdx +++ b/website/content/api-docs/secret/identity/oidc-provider.mdx @@ -672,7 +672,7 @@ to be used for the [Authorization Code Flow](https://openid.net/specs/openid-con - `redirect_uri` `(string: )` - The redirection URI to which the response will be sent. -- `state` `(string: )` - A value used to maintain state between the authentication request and client. +- `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, so we *strongly encourage* providing this optional parameter. diff --git a/website/content/docs/concepts/oidc-provider.mdx b/website/content/docs/concepts/oidc-provider.mdx index baa8b9715..c7c7a014a 100644 --- a/website/content/docs/concepts/oidc-provider.mdx +++ b/website/content/docs/concepts/oidc-provider.mdx @@ -199,7 +199,7 @@ 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` parameter is 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. 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.