router/pkg/authentication/jwks_token_decoder.go (1)

41-49: Add documentation for the new fields and clarify mutual exclusivity.

The new fields lack documentation explaining their purpose and usage. Additionally, the mutual exclusivity between URL-based and secret-based configurations should be documented at the struct level.

Add documentation to clarify the usage:

 type JWKSConfig struct {
+	// URL specifies the HTTP endpoint to fetch JWK sets from.
+	// Either URL or Secret must be provided, but not both.
 	URL               string
 	RefreshInterval   time.Duration
 	AllowedAlgorithms []string
 
+	// Secret is the symmetric key for HMAC-based JWT validation.
+	// When provided, Algorithm and KeyId must also be specified.
 	Secret    string
+	// Algorithm specifies the signing algorithm (e.g., HS256, HS384, HS512).
+	// Required when Secret is provided.
 	Algorithm string
+	// KeyId is the key identifier for the secret-based JWK.
+	// Required when Secret is provided.
 	KeyId     string
 }

router/pkg/config/config_test.go (1)

1202-1284: Well-structured test with good coverage of JWKS configuration scenarios.

The test function effectively validates the new secret-based JWKS authentication feature with comprehensive subtests covering both valid and invalid configurations.

Minor suggestions for improvement:

1. Line 1244: Consider using a realistic algorithm array instead of empty array:

-        algorithms: []
+        algorithms: ["RS256"]

2. Test naming consistency: Consider using more descriptive test names that clearly indicate the expected behavior, e.g., "should_reject_mixed_url_and_secret_config".

router/pkg/config/config.schema.json (2)

1642-1653: Incorrect description for key_id.

Line 1652 still reads “The secret of the JWKs”, copy-pasted from the secret field. This should describe the key identifier instead, otherwise generated docs (and UX) will mislead users.

 "key_id": {
   "type": "string",
-  "description": "The secret of the JWKs"
+  "description": "Key ID (kid) that will be set on the synthesized JWK"
 }

---

1685-1694: One-of guard blocks look good – just ask for self-documentation.

The new mutual-exclusivity rules are well-formed; however, adding a brief comment above each branch clarifying “remote JWKS via URL” vs “local shared-secret JWKS” would aid future maintainers reading this ~3 000-line schema.

router/pkg/authentication/jwks_token_decoder.go (1)

97-97: Consider adding validation for secret strength.

The code accepts any string as a secret without validating its strength. For HMAC algorithms, weak secrets can compromise security.

Consider adding a minimum length validation for secrets:

 	}
+	// Validate secret strength based on algorithm
+	minSecretLength := 32 // 256 bits minimum
+	if alg == jwkset.AlgHS384 {
+		minSecretLength = 48 // 384 bits
+	} else if alg == jwkset.AlgHS512 {
+		minSecretLength = 64 // 512 bits
+	}
+	if len(c.Secret) < minSecretLength {
+		return nil, fmt.Errorf("secret length (%d bytes) is below recommended minimum (%d bytes) for algorithm %s", 
+			len(c.Secret), minSecretLength, c.Algorithm)
+	}
 	jwk, err := jwkset.NewJWKFromKey([]byte(c.Secret), jwkOptions)

router-tests/authentication_test.go (3)

757-804: LGTM! Consider adding test coverage for tokens from different sources.

The test correctly validates authentication with multiple JWKS configurations. However, it only tests with a token from authServer2.

Consider extending the test to verify tokens from all three sources work correctly:

 	t.Run("authenticate when multiple jwks are present", func(t *testing.T) {
 		authServer1, err := jwks.NewServer(t)
 		t.Cleanup(authServer1.Close)
 		require.NoError(t, err)
 
 		authServer2, err := jwks.NewServer(t)
 		t.Cleanup(authServer2.Close)
 		require.NoError(t, err)
 
-		token, err := authServer2.Token(nil)
+		token1, err := authServer1.Token(nil)
+		require.NoError(t, err)
+		token2, err := authServer2.Token(nil)
+		require.NoError(t, err)
 
 		t.Parallel()
 
+		secret := "example secret"
+		kid := "givenKID"
 		authenticators := ConfigureAuthWithJwksConfig(t, []authentication.JWKSConfig{
 			{
-				Secret:    "example secret",
+				Secret:    secret,
 				Algorithm: string(jwkset.AlgHS256),
-				KeyId:     "givenKID",
+				KeyId:     kid,
 			},
 			{
 				URL:             authServer1.JWKSURL(),
 				RefreshInterval: time.Second * 5,
 			},
 			{
 				URL:             authServer2.JWKSURL(),
 				RefreshInterval: time.Second * 5,
 			},
 		})
+		
+		token3 := generateToken(t, kid, secret, jwt.SigningMethodHS256)
 
 		testenv.Run(t, &testenv.Config{
 			RouterOptions: []core.Option{
 				core.WithAccessController(core.NewAccessController(authenticators, true)),
 			},
 		}, func(t *testing.T, xEnv *testenv.Environment) {
-			// Operations with a token should succeed
-			header := http.Header{
-				"Authorization": []string{"Bearer " + token},
+			// Test all three token sources
+			for _, token := range []string{token1, token2, token3} {
+				header := http.Header{
+					"Authorization": []string{"Bearer " + token},
+				}
+				res, err := xEnv.MakeRequest(http.MethodPost, "/graphql", header, strings.NewReader(employeesQuery))
+				require.NoError(t, err)
+				defer res.Body.Close()
+				require.Equal(t, http.StatusOK, res.StatusCode)
+				require.Equal(t, JwksName, res.Header.Get(xAuthenticatedByHeader))
+				data, err := io.ReadAll(res.Body)
+				require.NoError(t, err)
+				require.Equal(t, employeesExpectedData, string(data))
 			}
-			res, err := xEnv.MakeRequest(http.MethodPost, "/graphql", header, strings.NewReader(employeesQuery))
-			require.NoError(t, err)
-			defer res.Body.Close()
-			require.Equal(t, http.StatusOK, res.StatusCode)
-			require.Equal(t, JwksName, res.Header.Get(xAuthenticatedByHeader))
-			data, err := io.ReadAll(res.Body)
-			require.NoError(t, err)
-			require.Equal(t, employeesExpectedData, string(data))
 		})
 	})

---

2038-2044: Make the signing method parameter more flexible.

The function parameter is currently typed as *jwt.SigningMethodHMAC, but it could be more generic to support all signing methods.

-func generateToken(t *testing.T, kid string, secret string, signingMethod *jwt.SigningMethodHMAC) string {
+func generateToken(t *testing.T, kid string, secret string, signingMethod jwt.SigningMethod) string {
 	token := jwt.New(signingMethod)
 	token.Header[jwkset.HeaderKID] = kid
 	jwtValue, err := token.SignedString([]byte(secret))
 	require.NoError(t, err)
 	return jwtValue
 }

---

808-924: Consider adding tests for other HMAC algorithms and edge cases.

The current tests focus on HS256. Consider adding test coverage for:

• HS384 and HS512 algorithms
• Invalid/unsupported algorithms
• Empty or weak secrets
• Missing required fields (Algorithm or KeyId when Secret is provided)

Would you like me to generate additional test cases for these scenarios?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro