Infer the JWS signing algorithm name by looking at the provided key (#…

…247) * Infer the JWS signing algorithm name by looking at the provided key This handles cases where the JWS message or the key do not have a proper `alg` header. The `alg` header is optional[0], so some identity providers may not supply it (such as Microsoft Identity[1]) [0]: https://datatracker.ietf.org/doc/html/rfc7517#section-4.4 [1]: https://login.microsoftonline.com/common/discovery/v2.0/keys Signed-off-by: Erik Haugrud <[email protected]> * Fix linter issues Signed-off-by: Erik Haugrud <[email protected]> * Add comment re: why key algorithm inference is necessary Signed-off-by: Erik Haugrud <[email protected]> * Add comment in test re: key algorithm inference Signed-off-by: Erik Haugrud <[email protected]> --------- Signed-off-by: Erik Haugrud <[email protected]>
istio-ecosystem · Apr 15, 2024 · 11a2002 · 11a2002
1 parent 8e2e849
commit 11a2002
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 4 deletions.
diff --git a/internal/authz/oidc.go b/internal/authz/oidc.go
@@ -621,7 +621,9 @@ func (o *oidcHandler) isValidIDToken(ctx context.Context, log telemetry.Logger,
 		return false, codes.Internal
 	}
 
-	if _, err := jws.Verify([]byte(idTokenString), jws.WithKeySet(jwtSet)); err != nil {
+	// We use jws.WithInferAlgorithmFromKey(true) in case the keys are missing the "alg" value;
+	// some providers (e.g. Microsoft Identity) exclude the "alg" value from their keys.
+	if _, err := jws.Verify([]byte(idTokenString), jws.WithKeySet(jwtSet, jws.WithInferAlgorithmFromKey(true))); err != nil {
 		log.Error("error verifying id token with fetched jwks", err)
 		return false, codes.Internal
 	}

diff --git a/internal/authz/oidc_test.go b/internal/authz/oidc_test.go
@@ -194,7 +194,18 @@ func TestOIDCProcess(t *testing.T) {
 
 	unknownJWKPriv, _ := newKeyPair(t)
 	jwkPriv, jwkPub := newKeyPair(t)
-	bytes, err := json.Marshal(newKeySet(t, jwkPub))
+	// We remove the optional "alg" field from this key to test that we can
+	// properly validate against them. Some providers (e.g. Microsoft Identity)
+	// exclude the "alg" field from their keys.
+	noAlgJwkPriv, noAlgJwkPub := newKeyPair(t)
+	err := noAlgJwkPriv.Set(jwk.KeyIDKey, noAlgKeyID)
+	require.NoError(t, err)
+	err = noAlgJwkPub.Set(jwk.KeyIDKey, noAlgKeyID)
+	require.NoError(t, err)
+	err = noAlgJwkPub.Remove(jwk.AlgorithmKey)
+	require.NoError(t, err)
+
+	bytes, err := json.Marshal(newKeySet(t, jwkPub, noAlgJwkPub))
 	require.NoError(t, err)
 	basicOIDCConfig.JwksConfig = &oidcv1.OIDCConfig_Jwks{
 		Jwks: string(bytes),
@@ -360,6 +371,23 @@ func TestOIDCProcess(t *testing.T) {
 				requireStoredTokens(t, store, newSessionID, false)
 			},
 		},
+		{
+			name:            "successfully retrieve new tokens when 'alg' is not specified in JWK",
+			req:             callbackRequest,
+			storedAuthState: validAuthState,
+			mockTokensResponse: &idpTokensResponse{
+				IDToken:     newJWT(t, noAlgJwkPriv, jwt.NewBuilder().Audience([]string{"test-client-id"}).Claim("nonce", newNonce)),
+				AccessToken: "access-token",
+				TokenType:   "Bearer",
+			},
+			responseVerify: func(t *testing.T, resp *envoy.CheckResponse) {
+				require.Equal(t, int32(codes.Unauthenticated), resp.GetStatus().GetCode())
+				requireStandardResponseHeaders(t, resp)
+				requireRedirectResponse(t, resp.GetDeniedResponse(), requestedAppURL, nil)
+				requireStoredTokens(t, store, sessionID, true)
+				requireStoredTokens(t, store, newSessionID, false)
+			},
+		},
 		{
 			name: "request is invalid, query parameters are missing",
 			req:  modifyCallbackRequestPath("/callback?"),
@@ -1405,8 +1433,9 @@ func modifyCallbackRequestPath(path string) *envoy.CheckRequest {
 }
 
 const (
-	keyID  = "test"
-	keyAlg = jwa.RS256
+	keyID      = "test"
+	keyAlg     = jwa.RS256
+	noAlgKeyID = "noAlgTest"
 )
 
 func newKeySet(t *testing.T, keys ...jwk.Key) jwk.Set {