Repository: istio-ecosystem/authservice
Pull request #247 (2024-04-12 23:26:06)

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](https://datatracker.ietf.org/doc/html/rfc7517#section-4.4), so some identity providers may not supply it (such as [Microsoft Identity](https://login.microsoftonline.com/common/discovery/v2.0/keys)).

istio-testing: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/istio-ecosystem/authservice/pull/247#" title="Author self-approved">erik-h</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign [brenodemedeiros](https://github.com/brenodemedeiros) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=istio-ecosystem%2Fauthservice).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/istio-ecosystem/authservice/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["brenodemedeiros"]} -->
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/istio-ecosystem/authservice/pull/247?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=istio-ecosystem) Report
All modified and coverable lines are covered by tests :white_check_mark:
> :exclamation: No coverage uploaded for pull request base (`main@8e2e849`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=istio-ecosystem#section-missing-base-commit).

:exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main     #247   +/-   ##
=======================================
  Coverage        ?   90.31%           
=======================================
  Files           ?       24           
  Lines           ?     1569           
  Branches        ?        0           
=======================================
  Hits            ?     1417           
  Misses          ?      108           
  Partials        ?       44           
```

| [Files](https://app.codecov.io/gh/istio-ecosystem/authservice/pull/247?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=istio-ecosystem) | Coverage Δ | |
|---|---|---|
| [internal/authz/oidc.go](https://app.codecov.io/gh/istio-ecosystem/authservice/pull/247?src=pr&el=tree&filepath=internal%2Fauthz%2Foidc.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=istio-ecosystem#diff-aW50ZXJuYWwvYXV0aHovb2lkYy5nbw==) | `93.55% <100.00%> (ø)` | |

</details>
erik-h: @nacx thanks for the review! I've added a couple comments and fixed the linter issues.
nacx: Thx!

Patch:
diff --git a/internal/authz/oidc.go b/internal/authz/oidc.go
index 5164691..7fa5407 100644
--- 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
 	}


Test patch:
diff --git a/internal/authz/oidc_test.go b/internal/authz/oidc_test.go
index 0331f3a..2cfe65e 100644
--- 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 {