Added support for JWT generated by IDPs like Keycloak #871
Conversation
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
|
|
WalkthroughThe recent updates introduce enhanced JWT handling capabilities, focusing on the management of JSON Web Key Sets (JWKS) for improved security and flexibility in authentication mechanisms. These changes include fetching JWKS from a designated URL, setting JWKS in the application's configuration, and expanding test coverage to ensure robust JWT functionality. Additionally, the middleware has been updated to utilize JWKS for token validation, replacing the previous algorithm-specific method. The infrastructure also sees an update with the Docker environment moving to a newer Golang version and adding PostgreSQL client tools. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files not summarized due to errors (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
go.modis excluded by:!**/*.modgo.sumis excluded by:!**/*.sumtestdata/docker-compose.ymlis excluded by:!**/*.yml
Files selected for processing (6)
- config/config.go (5 hunks)
- config/config_test.go (2 hunks)
- middlewares/config.go (1 hunks)
- middlewares/middlewares.go (3 hunks)
- middlewares/middlewares_test.go (2 hunks)
- testdata/Dockerfile (1 hunks)
Additional comments: 6
testdata/Dockerfile (1)
- 1-4: The update to
golang:1.20aligns with the PR's objectives to support thegolang.org/x/expmodule. The addition ofpostgresql-clientis logical for database interactions, but consider verifying if it's necessary for all users or if it could be optional.middlewares/config.go (1)
- 40-40: The switch to using
config.PrestConf.JWTJWKSaligns with the PR's objectives to support RSA encryption and JWKS for JWT validation. Consider adding comments or documentation to clarify the use ofJWTJWKSfor future maintainers.middlewares/middlewares_test.go (1)
- 100-100: The tests for RSA key handling and JWKS are crucial for the new JWT validation mechanism. It's good to see coverage for these scenarios. Consider adding tests for other key types as mentioned in the TODO comment to ensure comprehensive test coverage.
middlewares/middlewares.go (1)
- 148-188: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [126-185]
The modifications to
JwtMiddlewarealign with the PR's objectives to support RSA encryption and JWKS for JWT validation. The handling of different key types and the parsing of JWKSet JSON are well-implemented. Consider enhancing error handling and validation of the JWKS parsing logic to ensure robustness, especially in cases where the key is not found in the JWKS.config/config.go (2)
- 91-92: The addition of
JWTWellKnownandJWTJWKSfields to thePreststruct is a crucial part of supporting JWTs from IDPs. It's good to see these fields being introduced. However, it would be beneficial to add comments explaining their purpose and usage for future maintainers.Consider adding comments to these fields to improve code readability and maintainability.
- 182-183: Setting default values for
jwt.wellknownandjwt.jwksin the configuration is a good practice. However, ensure that these defaults are documented in the project's configuration documentation to inform users about the existence and purpose of these new settings.Update the project's documentation to include information about the new
jwt.wellknownandjwt.jwksconfiguration settings.
config/config_test.go
Outdated
| t.Run("PREST_JWT_WELLKNOWN", func(t *testing.T) { | ||
| //todo: Mock well-known config response | ||
| t.Setenv("PREST_JWT_WELLKNOWN", "https://accounts.google.com/.well-known/openid-configuration") | ||
| viperCfg() | ||
| cfg := &Prest{} | ||
| Parse(cfg) | ||
| require.Equal(t, "https://accounts.google.com/.well-known/openid-configuration", cfg.JWTWellKnown) | ||
| }) |
There was a problem hiding this comment.
The test case for PREST_JWT_WELLKNOWN sets an environment variable and verifies the configuration parsing. However, it lacks an assertion to ensure that the JWKS is fetched correctly based on the well-known URL. This is crucial for validating the new functionality introduced in the PR.
Consider adding an assertion to verify that the JWKS is fetched and correctly parsed into the configuration.
There was a problem hiding this comment.
I would have to mock a well-known URL response. Which I might need help with.
There was a problem hiding this comment.
@mcharest-mcn: To mock a well-known URL response in your tests, you can use the httptest package from the Go standard library. This package allows you to create a mock HTTP server, which you can configure to return the expected JWKS response for your test. Here's a basic example of how you could implement it:
package config_test
import (
"net/http"
"net/http/httptest"
"testing"
"github.com/stretchr/testify/require"
)
func TestParseWithMockWellKnown(t *testing.T) {
// Start a mock HTTP server
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Respond with a mock JWKS JSON when the well-known URL is called
w.WriteHeader(http.StatusOK)
w.Header().Set("Content-Type", "application/json")
w.Write([]byte(`{"keys": [{"kty": "RSA", ...}]}`)) // Simplified JWKS response
}))
defer server.Close()
// Use the mock server's URL as the well-known URL
t.Setenv("PREST_JWT_WELLKNOWN", server.URL)
// Continue with your test setup and assertions...
}This example demonstrates how to start a mock HTTP server that listens for requests and responds with a predefined JWKS JSON. You replace the ... in the JWKS JSON with the actual content you need for your tests. By setting the PREST_JWT_WELLKNOWN environment variable to the mock server's URL, your code under test will make requests to the mock server instead of the real well-known URL.
This approach allows you to test the functionality that fetches and parses the JWKS without making actual HTTP requests to external services, ensuring your tests are fast and reliable.
| t.Run("PREST_JWT_JWKS", func(t *testing.T) { | ||
| t.Setenv("PREST_JWT_JWKS", `{"keys":[{"kid":"lmjNOucrGdRiN7XlpWJbQRIzSeKBS7OD-92xrhch6kw","kty":"RSA","alg":"RS256","use":"sig","n":"9GPbUNJ_7dgq8k0eTbcCZtFMn-oTVpFHjzIi7nuyMm9TvIZNyu0q0O3buSIVTUWWhlakSgTp7hrRbldvxLmA4RSSs8oUw2Pm64q9oCdr0eXcnhL6mnfHASwpVed-aKMbM1Zlh1buDjPU0Ah_6D8sZaxqfOtMfrhT9LySbi91k2Hu16YJ6QK_RTj5BNjLZZSs2ns8-JdZKA-oL0RQwkEqO_QJrRvTWUhwguzpx4zACWc5zAQSWvDImbynH3N9L-rt2KoK3p2Zd0YZlCnZzK0iyYUHkVtTVixTFkYc-itceyZD64Z49q8vu478gIvu4dI8m3GIYeisZkKWBE5sjczvvw","e":"AQAB","x5c":["MIICmzCCAYMCBgGOLghSADANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0Y9tQ0n/t2CryTR5NtwJm0Uyf6hNWkUePMiLue7Iyb1O8hk3K7SrQ7du5IhVNRZaGVqRKBOnuGtFuV2/EuYDhFJKzyhTDY+brir2gJ2vR5dyeEvqad8cBLClV535ooxszVmWHVu4OM9TQCH/oPyxlrGp860x+uFP0vJJuL3WTYe7XpgnpAr9FOPkE2MtllKzaezz4l1koD6gvRFDCQSo79AmtG9NZSHCC7OnHjMAJZznMBBJa8MiZvKcfc30v6u3YqgrenZl3RhmUKdnMrSLJhQeRW1NWLFMWRhz6K1x7JkPrhnj2ry+7jvyAi+7h0jybcYhh6KxmQpYETmyNzO+/AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAAIDB54QwrWSQPou8UlGkpA8D3/Ws0ZGNiFutyIAQU0bzhzSB99AMsPl/4OJm5CGqpZMVyuLFgQHlMaArzeQJK7/8qN6piDZPP6A2lSRYuMJ/a8ciIVvjnepSUF+xx7PqeAnoarH8lxbdwhloBswnxn4iNcWTTMnxo73Ak9jpabj1m1a4e9+li6S8xCyA1AHxFXbjjAp5GxRvcUV2o3rMsDqdjM0IoU/+NNuCGtKApdTZNpFuk71AoKpM2/oxjuexEpOggyF30Pk5IdAgNtFMfD+pwcqzvSACbtKvk6VnSx4UtsFPWuizhWefWIkuV+7ml60NFMyD3eo28U9BQs2veU="],"x5t":"tUcTw0bM8ciXw9zIMlalEfyxdd8","x5t#S256":"eF-XsrHWa6gw8qC4W8RXJgA49xvac_7V-Tz7fdpS7ZM"},{"kid":"V3rRzf_j1beZjEmQnDeT8r8ZVnXpjW1Gk3635CTCEGk","kty":"RSA","alg":"RSA-OAEP","use":"enc","n":"1q1Iz-eyhnCWCBRKgq0xKm6cF2zHAi_a-L99OdwgnUgoGfut5bBTU2hGx9R1IGKn0loDjICtU64DVFpOaT7jY7oIG4BsQN3Et5H6O3XlVim5NQgMYVC6hKAreqnnVylUk-XfVvrQOotVkGfMFdARuBaLx1ubFxIHUONi2Mjgl2nZ8mmKg_GCsd5uKfJJ965zqSQu1CFn26YccTPp2doih4rykTGPVJdL5PVp3z4t9rTlahHbgCvv3E50yVK7LCNgtS9nmcZbD0meLqIZi3MoV0dBB_9C-qrEsevAIlPuXUmwtcbyDXOb1m7Xq_MPV_EASzoPYYjmk3k09zJ_p1EUTQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGOLghSlzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDWrUjP57KGcJYIFEqCrTEqbpwXbMcCL9r4v3053CCdSCgZ+63lsFNTaEbH1HUgYqfSWgOMgK1TrgNUWk5pPuNjuggbgGxA3cS3kfo7deVWKbk1CAxhULqEoCt6qedXKVST5d9W+tA6i1WQZ8wV0BG4FovHW5sXEgdQ42LYyOCXadnyaYqD8YKx3m4p8kn3rnOpJC7UIWfbphxxM+nZ2iKHivKRMY9Ul0vk9WnfPi32tOVqEduAK+/cTnTJUrssI2C1L2eZxlsPSZ4uohmLcyhXR0EH/0L6qsSx68AiU+5dSbC1xvINc5vWbter8w9X8QBLOg9hiOaTeTT3Mn+nURRNAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIKBZNe4GmyfqRW6Ee8ai1umbstAmyK3W1kP2i0xxINTlvY2rwblV8UCrdyi3laD7zvZy1midZmpKqtZqWpiNigeZ5aUt76paYvdSl5TAuvZGDGoEAhmmECbnDSQKLp36rCn7NlrgiTDfZZ2PvIKZ3cXClzqXLF/iC6uGiKOgY5yOFOa5QgsfItpJmmxHtTzrRF70RVsbZCexB1Lt4bcId6Y3x2w7JNUjKIhf1RZ3QZx8+3xBM4cJ83h2J4nE0+IlFeAJL3VLGdeOk+z+FGMu2mYkxJwkxd9Wl2ubqrRcNy0t61Bgp3s40BgD10pzvawTXl7lEgabc/jzN2R0lcXmLo="],"x5t":"n5Y_Obidr330txi13j50zHzVbfg","x5t#S256":"f-Hrw_t_qUq86Ux0J2EckWVycuM3L_IjdOK6DW0DFoc"}]}`) | ||
| viperCfg() | ||
| cfg := &Prest{} | ||
| Parse(cfg) | ||
| require.Equal(t, `{"keys":[{"kid":"lmjNOucrGdRiN7XlpWJbQRIzSeKBS7OD-92xrhch6kw","kty":"RSA","alg":"RS256","use":"sig","n":"9GPbUNJ_7dgq8k0eTbcCZtFMn-oTVpFHjzIi7nuyMm9TvIZNyu0q0O3buSIVTUWWhlakSgTp7hrRbldvxLmA4RSSs8oUw2Pm64q9oCdr0eXcnhL6mnfHASwpVed-aKMbM1Zlh1buDjPU0Ah_6D8sZaxqfOtMfrhT9LySbi91k2Hu16YJ6QK_RTj5BNjLZZSs2ns8-JdZKA-oL0RQwkEqO_QJrRvTWUhwguzpx4zACWc5zAQSWvDImbynH3N9L-rt2KoK3p2Zd0YZlCnZzK0iyYUHkVtTVixTFkYc-itceyZD64Z49q8vu478gIvu4dI8m3GIYeisZkKWBE5sjczvvw","e":"AQAB","x5c":["MIICmzCCAYMCBgGOLghSADANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0Y9tQ0n/t2CryTR5NtwJm0Uyf6hNWkUePMiLue7Iyb1O8hk3K7SrQ7du5IhVNRZaGVqRKBOnuGtFuV2/EuYDhFJKzyhTDY+brir2gJ2vR5dyeEvqad8cBLClV535ooxszVmWHVu4OM9TQCH/oPyxlrGp860x+uFP0vJJuL3WTYe7XpgnpAr9FOPkE2MtllKzaezz4l1koD6gvRFDCQSo79AmtG9NZSHCC7OnHjMAJZznMBBJa8MiZvKcfc30v6u3YqgrenZl3RhmUKdnMrSLJhQeRW1NWLFMWRhz6K1x7JkPrhnj2ry+7jvyAi+7h0jybcYhh6KxmQpYETmyNzO+/AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAAIDB54QwrWSQPou8UlGkpA8D3/Ws0ZGNiFutyIAQU0bzhzSB99AMsPl/4OJm5CGqpZMVyuLFgQHlMaArzeQJK7/8qN6piDZPP6A2lSRYuMJ/a8ciIVvjnepSUF+xx7PqeAnoarH8lxbdwhloBswnxn4iNcWTTMnxo73Ak9jpabj1m1a4e9+li6S8xCyA1AHxFXbjjAp5GxRvcUV2o3rMsDqdjM0IoU/+NNuCGtKApdTZNpFuk71AoKpM2/oxjuexEpOggyF30Pk5IdAgNtFMfD+pwcqzvSACbtKvk6VnSx4UtsFPWuizhWefWIkuV+7ml60NFMyD3eo28U9BQs2veU="],"x5t":"tUcTw0bM8ciXw9zIMlalEfyxdd8","x5t#S256":"eF-XsrHWa6gw8qC4W8RXJgA49xvac_7V-Tz7fdpS7ZM"},{"kid":"V3rRzf_j1beZjEmQnDeT8r8ZVnXpjW1Gk3635CTCEGk","kty":"RSA","alg":"RSA-OAEP","use":"enc","n":"1q1Iz-eyhnCWCBRKgq0xKm6cF2zHAi_a-L99OdwgnUgoGfut5bBTU2hGx9R1IGKn0loDjICtU64DVFpOaT7jY7oIG4BsQN3Et5H6O3XlVim5NQgMYVC6hKAreqnnVylUk-XfVvrQOotVkGfMFdARuBaLx1ubFxIHUONi2Mjgl2nZ8mmKg_GCsd5uKfJJ965zqSQu1CFn26YccTPp2doih4rykTGPVJdL5PVp3z4t9rTlahHbgCvv3E50yVK7LCNgtS9nmcZbD0meLqIZi3MoV0dBB_9C-qrEsevAIlPuXUmwtcbyDXOb1m7Xq_MPV_EASzoPYYjmk3k09zJ_p1EUTQ","e":"AQAB","x5c":["MIICmzCCAYMCBgGOLghSlzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDWrUjP57KGcJYIFEqCrTEqbpwXbMcCL9r4v3053CCdSCgZ+63lsFNTaEbH1HUgYqfSWgOMgK1TrgNUWk5pPuNjuggbgGxA3cS3kfo7deVWKbk1CAxhULqEoCt6qedXKVST5d9W+tA6i1WQZ8wV0BG4FovHW5sXEgdQ42LYyOCXadnyaYqD8YKx3m4p8kn3rnOpJC7UIWfbphxxM+nZ2iKHivKRMY9Ul0vk9WnfPi32tOVqEduAK+/cTnTJUrssI2C1L2eZxlsPSZ4uohmLcyhXR0EH/0L6qsSx68AiU+5dSbC1xvINc5vWbter8w9X8QBLOg9hiOaTeTT3Mn+nURRNAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIKBZNe4GmyfqRW6Ee8ai1umbstAmyK3W1kP2i0xxINTlvY2rwblV8UCrdyi3laD7zvZy1midZmpKqtZqWpiNigeZ5aUt76paYvdSl5TAuvZGDGoEAhmmECbnDSQKLp36rCn7NlrgiTDfZZ2PvIKZ3cXClzqXLF/iC6uGiKOgY5yOFOa5QgsfItpJmmxHtTzrRF70RVsbZCexB1Lt4bcId6Y3x2w7JNUjKIhf1RZ3QZx8+3xBM4cJ83h2J4nE0+IlFeAJL3VLGdeOk+z+FGMu2mYkxJwkxd9Wl2ubqrRcNy0t61Bgp3s40BgD10pzvawTXl7lEgabc/jzN2R0lcXmLo="],"x5t":"n5Y_Obidr330txi13j50zHzVbfg","x5t#S256":"f-Hrw_t_qUq86Ux0J2EckWVycuM3L_IjdOK6DW0DFoc"}]}`, cfg.JWTJWKS) |
There was a problem hiding this comment.
This test case for PREST_JWT_JWKS directly sets the JWKS in the configuration. It's good to see that the JWKS string is being tested for correct parsing. However, the test could be enhanced by verifying that the JWKS is correctly utilized in JWT validation logic, ensuring that the keys are properly loaded and can be used to validate tokens.
To improve test coverage, consider adding a test case that validates a JWT using the JWKS set through this configuration.
There was a problem hiding this comment.
This test is done in the middleware tests.
There was a problem hiding this comment.
@mcharest-mcn: Thank you for the clarification. I found the relevant middleware test files (utils_test.go, middlewares_test.go, and config_test.go) that likely cover JWT validation using JWKS as you mentioned. This approach ensures comprehensive testing by separating configuration parsing tests from middleware logic tests. If there's anything more specific within these tests you'd like to discuss or any other aspect of the PR, feel free to let me know!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to JWT middleware that might include JWT validation using JWKS.
rg --files-with-matches 'JWT' | grep 'middleware.*_test.go'
Length of output: 144
config/config.go
Outdated
| // fetchJWKS tries to get the JWKS from the URL in the config | ||
| func fetchJWKS(cfg *Prest) { | ||
| if cfg.JWTWellKnown == "" { | ||
| log.Debugln("no JWT WellKnown url found, skipping") | ||
| return | ||
| } | ||
| if cfg.JWTJWKS != "" { | ||
| log.Debugln("JWKS already set, skipping") | ||
| return | ||
| } | ||
|
|
||
| // Call provider to obtain .well-known config | ||
| r, err := http.Get(cfg.JWTWellKnown) | ||
| if err != nil { | ||
| log.Errorf("Cannot get .well-known configuration from '%s'. err: %v\n", cfg.JWTWellKnown, err) | ||
| return | ||
| } | ||
| defer r.Body.Close() | ||
|
|
||
| var wellKnown map[string]interface{} | ||
| err = json.NewDecoder(r.Body).Decode(&wellKnown) | ||
| if err != nil { | ||
| log.Errorf("Failed to decode JSON: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| //Retrieve the JWKS from the endpoint | ||
| JWKSet, err := jwk.Fetch(context.Background(), wellKnown["jwks_uri"].(string)) | ||
| if err != nil { | ||
| err := fmt.Errorf("failed to parse JWK: %s", err) | ||
| log.Errorf("Failed to fetch JWK: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| //Convert set to json string | ||
| jwkSetJSON, err := json.Marshal(JWKSet) | ||
| if err != nil { | ||
| log.Errorf("Failed to marshal JWKSet to JSON: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| cfg.JWTJWKS = string(jwkSetJSON) | ||
| } |
There was a problem hiding this comment.
The fetchJWKS function is a key addition to the project, enabling the fetching of JWKS from a well-known URL. The implementation correctly handles HTTP requests, JSON parsing, and error handling. However, there are a few areas for improvement:
- The use of
log.Errorffor logging errors is appropriate, but consider also implementing a retry mechanism for transient network errors. - The function directly modifies the
cfg.JWTJWKSfield. It might be more maintainable to return the JWKS as a result and let the caller decide how to use it, improving the function's reusability and testability.
Refactor the fetchJWKS function to include a retry mechanism for transient errors and return the JWKS instead of directly modifying the configuration.
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- config/config_test.go (3 hunks)
Files not summarized due to errors (1)
- config/config_test.go: Error: Message exceeds token limit
Additional comments: 2
config/config_test.go (2)
- 99-119: The test case for
PREST_JWT_WELLKNOWNeffectively simulates a JWKS server and a well-known configuration server, setting up the environment for testing the configuration parsing for JWT well-known URLs. This is a comprehensive approach to testing the new feature. However, it would be beneficial to also verify that the fetched JWKS is correctly utilized in JWT validation logic, ensuring that the keys are properly loaded and can be used to validate tokens. This could be part of integration tests rather than unit tests if not already covered.- 121-126: The test for
PREST_JWT_JWKSdirectly sets the JWKS in the configuration and verifies that it is correctly parsed. This is a straightforward and necessary test for ensuring the JWKS configuration works as expected. Given the previous discussion in the PR comments, it's clear that JWT validation using JWKS is tested elsewhere, specifically in middleware tests. It's good practice to separate configuration parsing tests from middleware logic tests, ensuring comprehensive testing coverage.
config/config_test.go
Outdated
| func Test_fetchJWKS(t *testing.T) { | ||
| serverJWKS := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(`{"keys":[{"alg":"RS256","e":"AQAB","kid":"lmjNOucrGdRiN7XlpWJbQRIzSeKBS7OD-92xrhch6kw","kty":"RSA","n":"9GPbUNJ_7dgq8k0eTbcCZtFMn-oTVpFHjzIi7nuyMm9TvIZNyu0q0O3buSIVTUWWhlakSgTp7hrRbldvxLmA4RSSs8oUw2Pm64q9oCdr0eXcnhL6mnfHASwpVed-aKMbM1Zlh1buDjPU0Ah_6D8sZaxqfOtMfrhT9LySbi91k2Hu16YJ6QK_RTj5BNjLZZSs2ns8-JdZKA-oL0RQwkEqO_QJrRvTWUhwguzpx4zACWc5zAQSWvDImbynH3N9L-rt2KoK3p2Zd0YZlCnZzK0iyYUHkVtTVixTFkYc-itceyZD64Z49q8vu478gIvu4dI8m3GIYeisZkKWBE5sjczvvw","use":"sig","x5c":["MIICmzCCAYMCBgGOLghSADANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0Y9tQ0n/t2CryTR5NtwJm0Uyf6hNWkUePMiLue7Iyb1O8hk3K7SrQ7du5IhVNRZaGVqRKBOnuGtFuV2/EuYDhFJKzyhTDY+brir2gJ2vR5dyeEvqad8cBLClV535ooxszVmWHVu4OM9TQCH/oPyxlrGp860x+uFP0vJJuL3WTYe7XpgnpAr9FOPkE2MtllKzaezz4l1koD6gvRFDCQSo79AmtG9NZSHCC7OnHjMAJZznMBBJa8MiZvKcfc30v6u3YqgrenZl3RhmUKdnMrSLJhQeRW1NWLFMWRhz6K1x7JkPrhnj2ry+7jvyAi+7h0jybcYhh6KxmQpYETmyNzO+/AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAAIDB54QwrWSQPou8UlGkpA8D3/Ws0ZGNiFutyIAQU0bzhzSB99AMsPl/4OJm5CGqpZMVyuLFgQHlMaArzeQJK7/8qN6piDZPP6A2lSRYuMJ/a8ciIVvjnepSUF+xx7PqeAnoarH8lxbdwhloBswnxn4iNcWTTMnxo73Ak9jpabj1m1a4e9+li6S8xCyA1AHxFXbjjAp5GxRvcUV2o3rMsDqdjM0IoU/+NNuCGtKApdTZNpFuk71AoKpM2/oxjuexEpOggyF30Pk5IdAgNtFMfD+pwcqzvSACbtKvk6VnSx4UtsFPWuizhWefWIkuV+7ml60NFMyD3eo28U9BQs2veU="],"x5t":"tUcTw0bM8ciXw9zIMlalEfyxdd8","x5t#S256":"eF-XsrHWa6gw8qC4W8RXJgA49xvac_7V-Tz7fdpS7ZM"},{"alg":"RSA-OAEP","e":"AQAB","kid":"V3rRzf_j1beZjEmQnDeT8r8ZVnXpjW1Gk3635CTCEGk","kty":"RSA","n":"1q1Iz-eyhnCWCBRKgq0xKm6cF2zHAi_a-L99OdwgnUgoGfut5bBTU2hGx9R1IGKn0loDjICtU64DVFpOaT7jY7oIG4BsQN3Et5H6O3XlVim5NQgMYVC6hKAreqnnVylUk-XfVvrQOotVkGfMFdARuBaLx1ubFxIHUONi2Mjgl2nZ8mmKg_GCsd5uKfJJ965zqSQu1CFn26YccTPp2doih4rykTGPVJdL5PVp3z4t9rTlahHbgCvv3E50yVK7LCNgtS9nmcZbD0meLqIZi3MoV0dBB_9C-qrEsevAIlPuXUmwtcbyDXOb1m7Xq_MPV_EASzoPYYjmk3k09zJ_p1EUTQ","use":"enc","x5c":["MIICmzCCAYMCBgGOLghSlzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDWrUjP57KGcJYIFEqCrTEqbpwXbMcCL9r4v3053CCdSCgZ+63lsFNTaEbH1HUgYqfSWgOMgK1TrgNUWk5pPuNjuggbgGxA3cS3kfo7deVWKbk1CAxhULqEoCt6qedXKVST5d9W+tA6i1WQZ8wV0BG4FovHW5sXEgdQ42LYyOCXadnyaYqD8YKx3m4p8kn3rnOpJC7UIWfbphxxM+nZ2iKHivKRMY9Ul0vk9WnfPi32tOVqEduAK+/cTnTJUrssI2C1L2eZxlsPSZ4uohmLcyhXR0EH/0L6qsSx68AiU+5dSbC1xvINc5vWbter8w9X8QBLOg9hiOaTeTT3Mn+nURRNAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIKBZNe4GmyfqRW6Ee8ai1umbstAmyK3W1kP2i0xxINTlvY2rwblV8UCrdyi3laD7zvZy1midZmpKqtZqWpiNigeZ5aUt76paYvdSl5TAuvZGDGoEAhmmECbnDSQKLp36rCn7NlrgiTDfZZ2PvIKZ3cXClzqXLF/iC6uGiKOgY5yOFOa5QgsfItpJmmxHtTzrRF70RVsbZCexB1Lt4bcId6Y3x2w7JNUjKIhf1RZ3QZx8+3xBM4cJ83h2J4nE0+IlFeAJL3VLGdeOk+z+FGMu2mYkxJwkxd9Wl2ubqrRcNy0t61Bgp3s40BgD10pzvawTXl7lEgabc/jzN2R0lcXmLo="],"x5t":"n5Y_Obidr330txi13j50zHzVbfg","x5t#S256":"f-Hrw_t_qUq86Ux0J2EckWVycuM3L_IjdOK6DW0DFoc"}]}`)) | ||
| })) | ||
| defer serverJWKS.Close() | ||
|
|
||
| serverWellKnown := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(`{"issuer":"http://127.0.0.1:8080/realms/master","authorization_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/auth","token_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/token","introspection_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/token/introspect","userinfo_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/userinfo","end_session_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/logout","frontchannel_logout_session_supported":true,"frontchannel_logout_supported":true,"jwks_uri":"` + serverJWKS.URL + `","check_session_iframe":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/login-status-iframe.html","grant_types_supported":["authorization_code","implicit","refresh_token","password","client_credentials","urn:openid:params:grant-type:ciba","urn:ietf:params:oauth:grant-type:device_code"],"acr_values_supported":["0","1"],"response_types_supported":["code","none","id_token","token","id_token token","code id_token","code token","code id_token token"],"subject_types_supported":["public","pairwise"],"id_token_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512"],"id_token_encryption_alg_values_supported":["RSA-OAEP","RSA-OAEP-256","RSA1_5"],"id_token_encryption_enc_values_supported":["A256GCM","A192GCM","A128GCM","A128CBC-HS256","A192CBC-HS384","A256CBC-HS512"],"userinfo_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512","none"],"userinfo_encryption_alg_values_supported":["RSA-OAEP","RSA-OAEP-256","RSA1_5"],"userinfo_encryption_enc_values_supported":["A256GCM","A192GCM","A128GCM","A128CBC-HS256","A192CBC-HS384","A256CBC-HS512"],"request_object_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512","none"],"request_object_encryption_alg_values_supported":["RSA-OAEP","RSA-OAEP-256","RSA1_5"],"request_object_encryption_enc_values_supported":["A256GCM","A192GCM","A128GCM","A128CBC-HS256","A192CBC-HS384","A256CBC-HS512"],"response_modes_supported":["query","fragment","form_post","query.jwt","fragment.jwt","form_post.jwt","jwt"],"registration_endpoint":"http://127.0.0.1:8080/realms/master/clients-registrations/openid-connect","token_endpoint_auth_methods_supported":["private_key_jwt","client_secret_basic","client_secret_post","tls_client_auth","client_secret_jwt"],"token_endpoint_auth_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512"],"introspection_endpoint_auth_methods_supported":["private_key_jwt","client_secret_basic","client_secret_post","tls_client_auth","client_secret_jwt"],"introspection_endpoint_auth_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512"],"authorization_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512"],"authorization_encryption_alg_values_supported":["RSA-OAEP","RSA-OAEP-256","RSA1_5"],"authorization_encryption_enc_values_supported":["A256GCM","A192GCM","A128GCM","A128CBC-HS256","A192CBC-HS384","A256CBC-HS512"],"claims_supported":["aud","sub","iss","auth_time","name","given_name","family_name","preferred_username","email","acr"],"claim_types_supported":["normal"],"claims_parameter_supported":true,"scopes_supported":["openid","roles","offline_access","email","microprofile-jwt","web-origins","acr","phone","profile","address"],"request_parameter_supported":true,"request_uri_parameter_supported":true,"require_request_uri_registration":true,"code_challenge_methods_supported":["plain","S256"],"tls_client_certificate_bound_access_tokens":true,"revocation_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/revoke","revocation_endpoint_auth_methods_supported":["private_key_jwt","client_secret_basic","client_secret_post","tls_client_auth","client_secret_jwt"],"revocation_endpoint_auth_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","HS256","HS512","ES256","RS256","HS384","ES512","PS256","PS512","RS512"],"backchannel_logout_supported":true,"backchannel_logout_session_supported":true,"device_authorization_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/auth/device","backchannel_token_delivery_modes_supported":["poll","ping"],"backchannel_authentication_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/ext/ciba/auth","backchannel_authentication_request_signing_alg_values_supported":["PS384","RS384","EdDSA","ES384","ES256","RS256","ES512","PS256","PS512","RS512"],"require_pushed_authorization_requests":false,"pushed_authorization_request_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/ext/par/request","mtls_endpoint_aliases":{"token_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/token","revocation_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/revoke","introspection_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/token/introspect","device_authorization_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/auth/device","registration_endpoint":"http://127.0.0.1:8080/realms/master/clients-registrations/openid-connect","userinfo_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/userinfo","pushed_authorization_request_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/ext/par/request","backchannel_authentication_endpoint":"http://127.0.0.1:8080/realms/master/protocol/openid-connect/ext/ciba/auth"},"authorization_response_iss_parameter_supported":true}`)) | ||
| })) | ||
| defer serverWellKnown.Close() | ||
|
|
||
| c := &Prest{JWTWellKnown: serverWellKnown.URL} | ||
| fetchJWKS(c) | ||
| require.Equal(t, `{"keys":[{"alg":"RS256","e":"AQAB","kid":"lmjNOucrGdRiN7XlpWJbQRIzSeKBS7OD-92xrhch6kw","kty":"RSA","n":"9GPbUNJ_7dgq8k0eTbcCZtFMn-oTVpFHjzIi7nuyMm9TvIZNyu0q0O3buSIVTUWWhlakSgTp7hrRbldvxLmA4RSSs8oUw2Pm64q9oCdr0eXcnhL6mnfHASwpVed-aKMbM1Zlh1buDjPU0Ah_6D8sZaxqfOtMfrhT9LySbi91k2Hu16YJ6QK_RTj5BNjLZZSs2ns8-JdZKA-oL0RQwkEqO_QJrRvTWUhwguzpx4zACWc5zAQSWvDImbynH3N9L-rt2KoK3p2Zd0YZlCnZzK0iyYUHkVtTVixTFkYc-itceyZD64Z49q8vu478gIvu4dI8m3GIYeisZkKWBE5sjczvvw","use":"sig","x5c":["MIICmzCCAYMCBgGOLghSADANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQD0Y9tQ0n/t2CryTR5NtwJm0Uyf6hNWkUePMiLue7Iyb1O8hk3K7SrQ7du5IhVNRZaGVqRKBOnuGtFuV2/EuYDhFJKzyhTDY+brir2gJ2vR5dyeEvqad8cBLClV535ooxszVmWHVu4OM9TQCH/oPyxlrGp860x+uFP0vJJuL3WTYe7XpgnpAr9FOPkE2MtllKzaezz4l1koD6gvRFDCQSo79AmtG9NZSHCC7OnHjMAJZznMBBJa8MiZvKcfc30v6u3YqgrenZl3RhmUKdnMrSLJhQeRW1NWLFMWRhz6K1x7JkPrhnj2ry+7jvyAi+7h0jybcYhh6KxmQpYETmyNzO+/AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAAIDB54QwrWSQPou8UlGkpA8D3/Ws0ZGNiFutyIAQU0bzhzSB99AMsPl/4OJm5CGqpZMVyuLFgQHlMaArzeQJK7/8qN6piDZPP6A2lSRYuMJ/a8ciIVvjnepSUF+xx7PqeAnoarH8lxbdwhloBswnxn4iNcWTTMnxo73Ak9jpabj1m1a4e9+li6S8xCyA1AHxFXbjjAp5GxRvcUV2o3rMsDqdjM0IoU/+NNuCGtKApdTZNpFuk71AoKpM2/oxjuexEpOggyF30Pk5IdAgNtFMfD+pwcqzvSACbtKvk6VnSx4UtsFPWuizhWefWIkuV+7ml60NFMyD3eo28U9BQs2veU="],"x5t":"tUcTw0bM8ciXw9zIMlalEfyxdd8","x5t#S256":"eF-XsrHWa6gw8qC4W8RXJgA49xvac_7V-Tz7fdpS7ZM"},{"alg":"RSA-OAEP","e":"AQAB","kid":"V3rRzf_j1beZjEmQnDeT8r8ZVnXpjW1Gk3635CTCEGk","kty":"RSA","n":"1q1Iz-eyhnCWCBRKgq0xKm6cF2zHAi_a-L99OdwgnUgoGfut5bBTU2hGx9R1IGKn0loDjICtU64DVFpOaT7jY7oIG4BsQN3Et5H6O3XlVim5NQgMYVC6hKAreqnnVylUk-XfVvrQOotVkGfMFdARuBaLx1ubFxIHUONi2Mjgl2nZ8mmKg_GCsd5uKfJJ965zqSQu1CFn26YccTPp2doih4rykTGPVJdL5PVp3z4t9rTlahHbgCvv3E50yVK7LCNgtS9nmcZbD0meLqIZi3MoV0dBB_9C-qrEsevAIlPuXUmwtcbyDXOb1m7Xq_MPV_EASzoPYYjmk3k09zJ_p1EUTQ","use":"enc","x5c":["MIICmzCCAYMCBgGOLghSlzANBgkqhkiG9w0BAQsFADARMQ8wDQYDVQQDDAZtYXN0ZXIwHhcNMjQwMzExMTQ1OTQxWhcNMzQwMzExMTUwMTIxWjARMQ8wDQYDVQQDDAZtYXN0ZXIwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDWrUjP57KGcJYIFEqCrTEqbpwXbMcCL9r4v3053CCdSCgZ+63lsFNTaEbH1HUgYqfSWgOMgK1TrgNUWk5pPuNjuggbgGxA3cS3kfo7deVWKbk1CAxhULqEoCt6qedXKVST5d9W+tA6i1WQZ8wV0BG4FovHW5sXEgdQ42LYyOCXadnyaYqD8YKx3m4p8kn3rnOpJC7UIWfbphxxM+nZ2iKHivKRMY9Ul0vk9WnfPi32tOVqEduAK+/cTnTJUrssI2C1L2eZxlsPSZ4uohmLcyhXR0EH/0L6qsSx68AiU+5dSbC1xvINc5vWbter8w9X8QBLOg9hiOaTeTT3Mn+nURRNAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAIKBZNe4GmyfqRW6Ee8ai1umbstAmyK3W1kP2i0xxINTlvY2rwblV8UCrdyi3laD7zvZy1midZmpKqtZqWpiNigeZ5aUt76paYvdSl5TAuvZGDGoEAhmmECbnDSQKLp36rCn7NlrgiTDfZZ2PvIKZ3cXClzqXLF/iC6uGiKOgY5yOFOa5QgsfItpJmmxHtTzrRF70RVsbZCexB1Lt4bcId6Y3x2w7JNUjKIhf1RZ3QZx8+3xBM4cJ83h2J4nE0+IlFeAJL3VLGdeOk+z+FGMu2mYkxJwkxd9Wl2ubqrRcNy0t61Bgp3s40BgD10pzvawTXl7lEgabc/jzN2R0lcXmLo="],"x5t":"n5Y_Obidr330txi13j50zHzVbfg","x5t#S256":"f-Hrw_t_qUq86Ux0J2EckWVycuM3L_IjdOK6DW0DFoc"}]}`, c.JWTJWKS) | ||
| } |
There was a problem hiding this comment.
The Test_fetchJWKS function tests the JWKS fetching functionality by simulating a JWKS server. This is crucial for ensuring that the application can dynamically load JWKS for JWT validation. The test is well-structured and covers the essential aspects of the JWKS fetching process. However, it would be beneficial to include negative test cases as well, such as handling server errors or invalid JWKS data, to ensure robust error handling in the JWKS fetching logic.
|
I received a comment by @arxdsilva about middlewares/config.go where I should have kept |
|
Just a check for the integration of this pull request. Are there any more work to do? |
config/config.go
Outdated
| } | ||
|
|
||
| //Retrieve the JWKS from the endpoint | ||
| JWKSet, err := jwk.Fetch(context.Background(), wellKnown["jwks_uri"].(string)) |
There was a problem hiding this comment.
can you split this conversion into a variable? just to make sure that it never panics:
uri, ok := wellKnown["jwks_uri"].(string)
...
JWKSet, err := jwk.Fetch(context.Background(),uri)There was a problem hiding this comment.
Done in latest commit.
fee30e7
config/config.go
Outdated
| @@ -271,7 +281,10 @@ func Parse(cfg *Prest) { | |||
| cfg.SingleDB = viper.GetBool("pg.single") | |||
| cfg.JWTKey = viper.GetString("jwt.key") | |||
| cfg.JWTAlgo = viper.GetString("jwt.algo") | |||
| cfg.JWTWellKnown = viper.GetString("jwt.wellknown") | |||
There was a problem hiding this comment.
could you rename this to JWTWellKnownURL ?
There was a problem hiding this comment.
Done in latest commit.
fee30e7
config/config.go
Outdated
| } | ||
|
|
||
| // Call provider to obtain .well-known config | ||
| r, err := http.Get(cfg.JWTWellKnown) |
There was a problem hiding this comment.
please add context to this call with a 5s timeout
There was a problem hiding this comment.
Done in latest commit.
fee30e7
|
@mcharest-mcn thanks for the contribution, I feel that once these points are addressed we can merge it. |
- Refactoring of logic in JwtMiddleware - Removing of comment in french that was a duplicate of correct comment in middlewares_test.go - Added timeout to WellKnown Config http call - Renaming of JWTWellKnown config parameter to JWTWellKnownURL for more precision
RSA Support
I was unable to use RSA256 to validate signed requests because the only supported encryption method was HMAC which is a symmetrical and I'm unable to exchange private key securely. In theory, it should also support the other asymmetrical key types, but I have not tested them.
To support RSA (and IDPs like Keycloak), I've added two new configuration parameters : PREST_JWT_WELLKNOWN and PREST_JWT_JWKS.
PREST_JWT_WELLKNOWN
Takes an URL to the well-known configuration of your IDP (ex: https://accounts.google.com/.well-known/openid-configuration) and is able to fetch the JWKS from your IDP. It will then verify if the key used to sign the received token is in the JWKS and verify the signature with said key.
PREST_JWT_JWKS
Instead of providing a well-known URL, you can set the JWKS yourself. It will have the same subsequent behavior as for the PREST_JWT_WELLKNOWN parameter.
Ex :
Unit tests fix
I changed the version of Go used in the docker image for the tests. It needed to be updated to at least 1.20 for the golang.org/x/exp module.
I also included tweaks to the docker-compose to be able to run it using podman instead of Docker.
Summary by CodeRabbit
golang:1.22and includedpostgresql-clientinstallation for an updated development environment.