fix(configuration): incorrect sector_identifier_uri validation #7037
Conversation
ArtifactsThese changes are published for testing on Buildkite, DockerHub and GitHub Container Registry. Docker Container
|
|
Warning Rate Limit Exceeded@james-d-elliott has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThese updates focus on refining the validation and configuration processes for OpenID Connect (OIDC) clients, particularly around sector identifier URI requirements and error message formatting. The changes introduce new utility functions for string joining, adjust the handling of TLS configurations, and improve the validation logic across various aspects of the configuration, including identity providers, session management, and storage. Additionally, enhancements to OIDC and URL utilities support more robust handling of sector identifiers and URL validations. Changes
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 (
|
✅ Deploy Preview for authelia-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
e674312
to
1cf62ae
Compare
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
2 times, most recently
from
00ebcb1
to
a164364
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
config.template.ymlis excluded by!**/*.ymlinternal/configuration/config.template.ymlis excluded by!**/*.yml
Files selected for processing (6)
- docs/content/configuration/identity-providers/openid-connect/clients.md (2 hunks)
- internal/configuration/validator/const.go (1 hunks)
- internal/configuration/validator/identity_providers.go (1 hunks)
- internal/configuration/validator/identity_providers_test.go (4 hunks)
- internal/utils/strings.go (2 hunks)
- internal/utils/strings_test.go (1 hunks)
Additional Context Used
GitHub Check Runs (1)
golangci failure (2)
internal/configuration/validator/identity_providers_test.go: [failure] 472-472: [golangci] internal/configuration/validator/identity_providers_test.go#L472
use offmt.Printlnforbidden because "Do not commit print statements." (forbidigo)
internal/configuration/validator/identity_providers_test.go: [failure] 383-383: [golangci] internal/configuration/validator/identity_providers_test.go#L383
string://has 2 occurrences, make it a constant (goconst)
Additional comments not posted (17)
internal/utils/strings.go (8)
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [10-18]
The
IsStringAbsURLfunction is correctly implemented with clear error handling. It effectively checks if a given string is an absolute URL and provides informative error messages if not.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [20-28]
The
IsStringAlphaNumericfunction is implemented efficiently to check if a string is alphanumeric, using theunicodepackage for character classification.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [30-82]
The variations of the
IsStringInSlicefunction are well-implemented, catering to different string comparison needs efficiently. This modular approach enhances code reusability and maintainability.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [84-98]
The
SliceStringfunction is correctly implemented to split a string into an array of substrings of a specified maximum length. It efficiently handles the division and remainder to ensure all characters are included.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [100-124]
The
IsStringSlicesDifferentand related functions are well-designed, using a helper method to abstract common logic. This approach promotes code reuse and maintainability while efficiently comparing two slices of strings for differences.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [126-166]
The URL and string conversion functions, including
StringSliceFromURLs,URLsFromStringSlice, andOriginFromURL, are correctly implemented. They efficiently handle parsing, stringification, and extraction of URL origins, with appropriate error handling for unparseable URLs.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [168-213]
The string manipulation functions related to HTML escaping and delimited string joining/splitting are correctly implemented. They efficiently handle special character escaping and round-trip encoding/decoding of delimited strings, including escaped delimiters.
3-8: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [215-225]
The
JoinAndCanonicalizeHeadersfunction is correctly implemented to join and canonicalize HTTP header names usingfasthttp.AppendNormalizedHeaderKey. This approach is efficient and suitable for the intended use case.internal/utils/strings_test.go (1)
331-333: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [5-234]
The test cases in
internal/utils/strings_test.goare well-structured and comprehensive, covering both positive and negative scenarios for the utility functions. They follow best practices by using table-driven tests and theassertandrequirepackages for assertions. Ensure all utility functions have corresponding test cases for complete coverage.docs/content/configuration/identity-providers/openid-connect/clients.md (1)
158-159: Clarify the future validation of the JSON document in thesector_identifier_urito ensure readers understand this is a planned feature.Consider rephrasing to explicitly state that content validation of the JSON document at the
sector_identifier_uriis a feature planned for future implementation, to avoid any confusion about current capabilities.internal/configuration/validator/const.go (1)
270-275: The new error format constants for validating thesector_identifier_uriare clear and follow the existing naming and formatting conventions in the file. However, it's important to ensure that these error messages are used in a context where the placeholders (e.g.,%s) are appropriately replaced with actual values to provide meaningful feedback to the user.internal/configuration/validator/identity_providers.go (1)
584-606: The validation logic forsector_identifier_uricorrectly handles several important aspects:
- Sets the URI to
nilif it's an empty string, ensuring that empty strings are ignored as intended.- Validates that the URI is absolute, which is crucial for ensuring that the URI can be resolved properly.
- Enforces the HTTPS scheme, enhancing security by ensuring that the JSON document is served over a secure connection.
- Excludes URIs with fragments, usernames, or passwords, which helps in avoiding potential security issues and ensures the URI points directly to a JSON document without unnecessary components.
Overall, this validation logic aligns well with the PR objectives and best practices for handling URIs in a secure and predictable manner.
internal/configuration/validator/identity_providers_test.go (5)
383-383: ThemustParseURLfunction is used to parse URLs but does not handle potential errors directly within the function. It's a good practice to handle errors where they occur or to rename the function to indicate it can panic, e.g.,mustParseURL.Consider adding error handling within the function or renaming it to
mustParseURLto clearly indicate it panics on error.
383-383: The construction of URLs using string concatenation is prone to errors and can lead to hard-to-find bugs, especially when dealing with complex URLs. It's better to use thenet/urlpackage to construct URLs safely.Consider using the
net/urlpackage to construct URLs instead of string concatenation. This approach is safer and more maintainable.Also applies to: 397-397, 415-417, 434-436, 452-452, 465-465
383-383: The tests make extensive use of hardcoded strings for URLs, which can lead to duplication and make the tests harder to maintain.Consider defining common URL parts as constants or using helper functions to generate URLs. This will reduce duplication and improve maintainability.
Also applies to: 397-397, 415-417, 434-436, 452-452, 465-465
383-383: The test cases cover various scenarios for URL validation, which is good. However, it seems like there could be more tests around edge cases, such as URLs with unusual characters or internationalized domain names.Consider adding more test cases to cover edge cases, such as URLs with unusual characters or internationalized domain names. This will ensure the validation logic is robust against a wider range of inputs.
Also applies to: 397-397, 415-417, 434-436, 452-452, 465-465
383-383: The test functions are well-structured and follow a clear naming convention, which is good for readability. However, there's an opportunity to further improve code readability by adding comments that describe the purpose of each test case.Consider adding comments to each test case to describe its purpose and what specific behavior it's testing. This will make the tests more readable and easier to understand for future maintainers.
Also applies to: 397-397, 415-417, 434-436, 452-452, 465-465
docs/content/configuration/identity-providers/openid-connect/clients.md
Outdated
Show resolved
Hide resolved
This fixes a number of validation errors with the sector_identifier_uri for clients. For starters empty strings should be ignored, secondly a sector_identifier_uri must point to a JSON document on a secure protocol i.e. HTTPS but this was not reflected in the tests. We still need to add a check to ensure the JSON document is valid for the client (i.e. includes all of the registered redirect_uris). Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
…lients.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
3bb5469
to
b12be58
Compare
There was a problem hiding this comment.
Actionable comments posted: 45
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
config.template.ymlis excluded by!**/*.ymlinternal/configuration/config.template.ymlis excluded by!**/*.yml
Files selected for processing (6)
- docs/content/configuration/identity-providers/openid-connect/clients.md (2 hunks)
- internal/configuration/validator/const.go (1 hunks)
- internal/configuration/validator/identity_providers.go (2 hunks)
- internal/configuration/validator/identity_providers_test.go (6 hunks)
- internal/utils/strings.go (2 hunks)
- internal/utils/strings_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- internal/configuration/validator/const.go
- internal/configuration/validator/identity_providers.go
- internal/utils/strings.go
- internal/utils/strings_test.go
Additional Context Used
GitHub Check Runs (1)
golangci failure (2)
internal/configuration/validator/identity_providers_test.go: [failure] 479-479: [golangci] internal/configuration/validator/identity_providers_test.go#L479
use offmt.Printlnforbidden because "Do not commit print statements." (forbidigo)
internal/configuration/validator/identity_providers_test.go: [failure] 384-384: [golangci] internal/configuration/validator/identity_providers_test.go#L384
string://has 2 occurrences, make it a constant (goconst)
Additional comments not posted (1)
docs/content/configuration/identity-providers/openid-connect/clients.md (1)
155-162: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]
The word "integration" is spelled correctly in this context.
Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
b12be58
to
1747456
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7037 +/- ##
==========================================
- Coverage 73.31% 73.28% -0.04%
==========================================
Files 345 345
Lines 29913 29996 +83
Branches 839 839
==========================================
+ Hits 21932 21983 +51
- Misses 7106 7143 +37
+ Partials 875 870 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
5 times, most recently
from
716e1ca
to
3967f3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (4)
config.template.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.mod,!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/configuration/config.template.ymlis excluded by!**/*.yml
Files selected for processing (26)
- docs/content/configuration/identity-providers/openid-connect/clients.md (2 hunks)
- internal/commands/context.go (2 hunks)
- internal/configuration/provider_test.go (1 hunks)
- internal/configuration/validator/access_control.go (4 hunks)
- internal/configuration/validator/authentication.go (7 hunks)
- internal/configuration/validator/configuration.go (4 hunks)
- internal/configuration/validator/const.go (3 hunks)
- internal/configuration/validator/identity_providers.go (42 hunks)
- internal/configuration/validator/identity_providers_test.go (26 hunks)
- internal/configuration/validator/identity_validation.go (1 hunks)
- internal/configuration/validator/log.go (1 hunks)
- internal/configuration/validator/server.go (2 hunks)
- internal/configuration/validator/session.go (2 hunks)
- internal/configuration/validator/storage.go (1 hunks)
- internal/configuration/validator/theme.go (1 hunks)
- internal/configuration/validator/totp.go (1 hunks)
- internal/configuration/validator/util.go (1 hunks)
- internal/configuration/validator/util_test.go (1 hunks)
- internal/configuration/validator/webauthn.go (1 hunks)
- internal/oidc/const.go (1 hunks)
- internal/oidc/types.go (1 hunks)
- internal/oidc/util.go (3 hunks)
- internal/utils/strings.go (3 hunks)
- internal/utils/strings_test.go (2 hunks)
- internal/utils/url.go (1 hunks)
- internal/utils/url_test.go (1 hunks)
Files not summarized due to errors (1)
- internal/configuration/validator/identity_providers.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (22)
- internal/commands/context.go
- internal/configuration/provider_test.go
- internal/configuration/validator/access_control.go
- internal/configuration/validator/authentication.go
- internal/configuration/validator/const.go
- internal/configuration/validator/identity_providers_test.go
- internal/configuration/validator/identity_validation.go
- internal/configuration/validator/log.go
- internal/configuration/validator/server.go
- internal/configuration/validator/session.go
- internal/configuration/validator/storage.go
- internal/configuration/validator/theme.go
- internal/configuration/validator/totp.go
- internal/configuration/validator/util.go
- internal/configuration/validator/util_test.go
- internal/configuration/validator/webauthn.go
- internal/oidc/const.go
- internal/oidc/types.go
- internal/oidc/util.go
- internal/utils/strings_test.go
- internal/utils/url.go
- internal/utils/url_test.go
Additional Context Used
Additional comments not posted (27)
internal/configuration/validator/configuration.go (4)
1-28: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-66]
The introduction of variadic parameters in
ValidateConfigurationfor additional options and the use of a newValidateCtxstruct to manage context and HTTP client configuration are good practices for flexibility and maintainability. However, ensure that all callers ofValidateConfigurationare updated to utilize these new features effectively.
107-110: Creating a new validation context withNewValidateCtxthat defaults to a background context is a sensible default. It's important to ensure that this context is appropriately used in downstream calls, especially in asynchronous or long-running operations where cancellation might be needed.
123-145: TheGetHTTPClientmethod inValidateCtxis designed to lazily initialize the HTTP client with a custom transport configuration. This approach is efficient but ensure that the TLS configuration (ctx.tlsconfig) is thread-safe if it can be modified after the client's first use. Additionally, consider exposing a way to refresh or reset the HTTP client if the TLS configuration changes at runtime.
147-149: TheWithTLSConfigfunction is a clean way to apply TLS configuration to the validation context. This design supports a flexible and modular approach to setting up the validation context. Ensure that the TLS configuration provided here is validated for correctness and security (e.g., ensuring strong ciphers and TLS versions) before being used.docs/content/configuration/identity-providers/openid-connect/clients.md (4)
155-172: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]
The word "integration" might be misspelled. Please verify.
Please double-check the spelling of "integration" in the context of your documentation to ensure accuracy.
158-160: Clarify the future validation of the JSON document in thesector_identifier_urisection.Consider adding a note or a separate section detailing the planned future validation of the JSON document referenced by
sector_identifier_urito align with the PR objectives.
158-160: The documentation should emphasize the HTTPS requirement forsector_identifier_uri.Highlight the requirement that
sector_identifier_urimust use the HTTPS protocol for enhanced security, as per the PR objectives.
158-160: Mention ignoring empty strings forsector_identifier_uri.Include a note or clarification that empty strings for
sector_identifier_uriwill be ignored during validation, aligning with the PR objectives.internal/configuration/validator/identity_providers.go (19)
57-57: The addition ofvalidateOIDCClientswithinvalidateOIDCensures that client-specific validations are performed when OIDC clients are configured. This change aligns with the PR's objective to enhance validation logic, particularly focusing on thesector_identifier_uriand other client-related configurations.
73-73: The use ofutils.StringJoinAndfor constructing error messages invalidateOIDCAuthorizationPoliciesenhances the readability of error messages when invalid policy names are encountered. This improvement aligns with the PR's goal of refining validation messages.
84-84: The validation for default policies invalidateOIDCAuthorizationPoliciesnow includes a more descriptive error message usingutils.StringJoinAnd, which is a positive change for clarity and user feedback.
98-98: The addition of detailed error messages for invalid policy rules invalidateOIDCAuthorizationPoliciesusingutils.StringJoinAndenhances the clarity of validation feedback, which is beneficial for debugging and configuration management.
146-146: The validation logic invalidateOIDDIssuerSigningAlgsDiscoveryforDiscoverySignedResponseAlgnow provides a more informative error message usingutils.StringJoinOr, which aids in identifying configuration issues related to signing algorithms.
151-151: The addition of a detailed error message for invalidDiscoverySignedResponseKeyIDinvalidateOIDDIssuerSigningAlgsDiscoveryusingutils.StringJoinOrimproves the clarity of validation feedback, which is beneficial for administrators configuring OIDC.
226-226: The validation for theUseproperty of JSON Web Keys invalidateOIDCIssuerPrivateKeysUseAlgnow includes a more descriptive error message usingutils.StringJoinOr, which enhances the clarity of feedback provided to the user.
241-241: The addition of detailed error messages for invalidAlgorithmconfigurations invalidateOIDCIssuerPrivateKeysUseAlgusingutils.StringJoinOris a positive change for improving the clarity of validation feedback.
588-588: Clearing theSectorIdentifierURIif it's an empty string is a sensible approach to handle potentially misconfigured URIs. This change ensures that further validation logic is skipped for such cases, aligning with the PR's objective to refine validation processes.
596-605: The validation for theSectorIdentifierURIto be an absolute URL and to use the HTTPS scheme is crucial for security. This change ensures that sector identifiers are securely hosted, aligning with the PR's emphasis on enhancing security aspects of the validation process.
607-612: Prohibiting the use of fragments inSectorIdentifierURIis a good practice, as fragments can lead to unpredictable behavior in the context of URL processing. This validation step enhances the robustness of the configuration.
614-624: The validation to ensure that theSectorIdentifierURIdoes not contain user information (username or password) is an important security measure. This prevents potential leakage of sensitive information through the URI.
627-628: The call tooidc.ValidateSectorIdentifierURIfor validating the sector identifier URI against the client's redirect URIs is a crucial addition. It ensures that the sector identifier URI is not only correctly formatted but also valid in the context of the client's configuration, aligning with the PR's objectives to enhance validation mechanisms.
549-549: The validation for the presence of a signing algorithm when the client's JSON Web Keys URI is not specified is a necessary addition. It ensures that the configuration is complete and secure, aligning with the PR's focus on enhancing validation logic.
560-560: The addition of detailed error messages for invalidUseconfigurations in client JSON Web Keys usingutils.StringJoinOrimproves the clarity of validation feedback, which is beneficial for administrators configuring OIDC clients.
569-569: The validation for theAlgorithmproperty of client JSON Web Keys now includes a more descriptive error message usingutils.StringJoinOr, which enhances the clarity of feedback provided to the user.
1054-1054: The validation for the presence of a signing algorithm invalidateOIDCClientTokenEndpointAuthPublicKeyJWTis crucial for ensuring that the token endpoint authentication configuration is complete and secure. This change aligns with the PR's focus on enhancing validation logic.
1061-1061: The addition of a detailed error message for invalid signing algorithm configurations invalidateOIDCClientTokenEndpointAuthPublicKeyJWTusingutils.StringJoinOrimproves the clarity of validation feedback, which is beneficial for administrators configuring OIDC clients.
1045-1045: The validation for the token endpoint authentication signing algorithm invalidateOIDCClientTokenEndpointAuthClientSecretJWTnow includes a more informative error message usingutils.StringJoinOr, which aids in identifying configuration issues related to signing algorithms.
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
3967f3e
to
b02bddc
Compare
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
b02bddc
to
24b1146
Compare
Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
james-d-elliott
force-pushed
the
fix-oidc-sector
branch
from
24b1146
to
b5ca391
Compare
…lia#7037) This fixes a number of validation errors with the sector_identifier_uri for clients. For starters empty strings should be ignored, secondly a sector_identifier_uri must point to a JSON document on a secure protocol i.e. HTTPS but this was not reflected in the tests. We still need to add a check to ensure the JSON document is valid for the client (i.e. includes all of the registered redirect_uris). Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This fixes a number of validation errors with the sector_identifier_uri for clients. For starters empty strings should be ignored, secondly a sector_identifier_uri must point to a JSON document on a secure protocol i.e. HTTPS but this was not reflected in the validations. We've also added proper validation for the
redirect_uriswhen this option is correctly configured.Summary by CodeRabbit
Summary by CodeRabbit
Contextinterface to include HTTP client configuration.