Backport 1.9.x: identity/oidc: optional nonce parameter for authorize…

… request (#13231) (#13242)

changelog/13231.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+identity/oidc: Make the `nonce` parameter optional for the Authorization Endpoint of OIDC providers.
+```

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=

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
 			}
 		}
 	}

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 {

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)