From 21dc2c26e4a8a7da8e38f2ac9e9a45736bb0cbf2 Mon Sep 17 00:00:00 2001
From: Austin Gebauer <agebauer@hashicorp.com>
Date: Fri, 19 Nov 2021 15:21:48 -0800
Subject: [PATCH 1/3] identity/oidc: optional nonce parameter for authorize
 request

---
 go.sum                                     |  1 -
 vault/identity_store_oidc.go               |  9 +--
 vault/identity_store_oidc_provider.go      | 22 +++----
 vault/identity_store_oidc_provider_test.go | 77 +++++++++++++++++-----
 4 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/go.sum b/go.sum
index 2698877f7386..ad0cf3cd62f7 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 cc66e63194af..e9dbd18242c0 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 cff96505bfa7..6a8e6787a2aa 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 02499b3727c6..e279b21438bd 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)

From 84e50b99be252f2f68b458a38af7b8f6bb0a4382 Mon Sep 17 00:00:00 2001
From: Austin Gebauer <agebauer@hashicorp.com>
Date: Fri, 19 Nov 2021 15:46:34 -0800
Subject: [PATCH 2/3] Updates documentation

---
 .../content/api-docs/secret/identity/oidc-provider.mdx | 10 +++++-----
 website/content/docs/concepts/oidc-provider.mdx        |  4 ++--
 .../content/docs/secrets/identity/oidc-provider.mdx    |  5 +++--
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/website/content/api-docs/secret/identity/oidc-provider.mdx b/website/content/api-docs/secret/identity/oidc-provider.mdx
index d7af7f1ff5ad..4de357852bc9 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: <required>)` - A value used to maintain state between the authentication request and client.
 
-- `nonce` `(string: <required>)` - A value that is returned in the ID token nonce claim. It is used to mitigate replay attacks.
+- `nonce` `(string: <optional>)` - 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 b9cc30778263..ab6b5836941d 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 75db03e3a4c1..05225109d01b 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

From 6c647613dfb15da23e9be127f00d82fe0439a301 Mon Sep 17 00:00:00 2001
From: Austin Gebauer <agebauer@hashicorp.com>
Date: Fri, 19 Nov 2021 15:59:28 -0800
Subject: [PATCH 3/3] Adds changelog entry

---
 changelog/13231.txt | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 changelog/13231.txt

diff --git a/changelog/13231.txt b/changelog/13231.txt
new file mode 100644
index 000000000000..3af52338134e
--- /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.
+```