feat: top origin verification #217
Conversation
This adds top origin verification options to the library.
|
@coderabbitai please review this |
|
Tip For best results, initiate chat on the files or code changes. @james-d-elliott: Could you please provide more details about the changes made in this PR or specify if there are particular files or aspects of the changes that you're concerned about? This will help me to tailor the review to your needs more effectively. |
|
@coderabbitai please review this pull request and provide comprehensive feedback |
|
@james-d-elliott, I've identified the files changed in the pull request. Here's the list:
Given the number of files involved, I'll proceed with a high-level review focusing on key aspects such as code quality, security, performance, and adherence to best practices. This will include reviewing the implementation logic, checking for any potential security issues, assessing the efficiency of the code, and ensuring that the changes align with common best practices in Go programming and the specific standards of the I'll start with the files under the Expect my comprehensive feedback shortly. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# List the files changed in the PR using GitHub CLI
gh pr view https://github.com/go-webauthn/webauthn/pull/217 --json files --jq '.files[].path'
Length of output: 328 Actions performedReview triggered.
|
WalkthroughThe updates focus on enhancing verification processes in a WebAuthn library, integrating new parameters for top origin validation in both assertion and credential verification. This includes the introduction of Changes
Assessment against linked issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 selected for processing (10)
- protocol/assertion.go (1 hunks)
- protocol/assertion_test.go (1 hunks)
- protocol/attestation_test.go (1 hunks)
- protocol/client.go (5 hunks)
- protocol/client_test.go (4 hunks)
- protocol/credential.go (1 hunks)
- protocol/credential_test.go (1 hunks)
- webauthn/login.go (1 hunks)
- webauthn/registration.go (1 hunks)
- webauthn/types.go (2 hunks)
Additional comments: 9
protocol/client_test.go (1)
- 29-29: The
Verifyfunction call now includesnilandTopOriginIgnoreVerificationModeas new parameters. Ensure that the introduction of these parameters aligns with the updated verification logic, particularly the handling ofnilfor RP Top Origins and the explicit choice of ignoring top origin verification in these tests.protocol/assertion.go (1)
- 127-127: The
Verifymethod has been updated to includerpOrigins,rpTopOrigins,rpTopOriginsVerifyas parameters for enhanced verification. This is a significant change that aligns with the objective of verifying bothoriginandtopOrigin. Ensure that all calls to this method throughout the codebase have been updated to pass the correct arguments according to the new signature.webauthn/registration.go (1)
- 186-186: The
Verifyfunction call withinCreateCredentialhas been updated to include additional configuration parameters. This change is crucial for integrating the new top origin verification logic. Ensure that the configuration passed toVerifyis correctly set up in theConfigstruct and that all necessary fields are populated before this call.webauthn/types.go (1)
- 165-172: The validation logic for
RPTopOriginVerificationModecorrectly sets a default mode and enforces the presence ofRPTopOriginswhen the mode is set toTopOriginImplicitVerificationMode. This is a good practice to ensure that the configuration is consistent and secure. However, ensure that the default mode (TopOriginIgnoreVerificationMode) is appropriate for all use cases and consider documenting the implications of each mode clearly for users of the library.protocol/client.go (2)
- 23-24: Adding
TopOriginandCrossOriginfields to theCollectedClientDatastruct is a necessary step to support the verification of top origins as per the PR objectives. This change correctly extends the data model to include the new information required for enhanced security checks. Ensure that these fields are properly documented to clarify their usage and impact on the verification process.- 198-221: The introduction of
TopOriginVerificationModeconstants provides a clear and flexible way to specify the mode of top origin verification. This design allows for easy extension and adjustment as the specifications evolve. Ensure that each mode is thoroughly tested, especially theTopOriginAutoVerificationModeandTopOriginExplicitVerificationMode, to validate their behavior in real-world scenarios.protocol/attestation_test.go (1)
- 39-39: The addition of
TopOriginIgnoreVerificationModeas a parameter in theVerifymethod call withinTestAttestationVerifyintroduces a new verification mode. This change is consistent with the PR's objective to enhance security by verifyingtopOrigin. However, it's important to ensure that this new mode is thoroughly tested across different scenarios to validate its behavior and impact comprehensively.Ensure that tests covering the new verification mode (
TopOriginIgnoreVerificationMode) are comprehensive and cover various scenarios, including positive and negative test cases. This is crucial for validating the behavior and impact of the new verification mode on the authentication process.protocol/credential.go (1)
- 143-145: The addition of
rpOrigins,rpTopOrigins, andrpTopOriginsVerifyparameters to theVerifyfunction enhances the library's capability to verifytopOriginin addition toorigin. This change is crucial for improving security and aligns with the PR's objectives. However, it's important to ensure that the implementation correctly handles various scenarios, including edge cases and potential error conditions, to maintain robustness and reliability in the verification process.Verify that the implementation of the new parameters in the
Verifyfunction correctly handles various scenarios, including edge cases and potential error conditions. This is essential for maintaining the robustness and reliability of the verification process.protocol/credential_test.go (1)
- 367-367: The addition of
nilandTopOriginIgnoreVerificationModeas arguments to theVerifymethod call in theTestParsedCredentialCreationData_Verifyfunction aligns with the PR's objective to enhance top origin verification. However, it's crucial to ensure that thenilargument, which likely represents the absence of a specific parameter (possiblyrpTopOrigins), is handled appropriately within theVerifymethod. This handling should be verified, especially in the context of different top origin verification modes.
- Ensure that the
Verifymethod is designed to gracefully handlenilvalues for the newly added parameter. This is important for maintaining robustness and avoiding potentialnilpointer dereferences.- The use of
TopOriginIgnoreVerificationModeindicates a specific verification mode is being tested. It would be beneficial to include tests for other verification modes (AllowAll,AllowRPOrigins,AllowNone) to ensure comprehensive coverage and validation of the new top origin verification logic.
| @@ -180,7 +180,7 @@ func TestParsedCredentialAssertionData_Verify(t *testing.T) { | |||
| Raw: tt.fields.Raw, | |||
| } | |||
|
|
|||
| if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, "", tt.args.verifyUser, tt.args.credentialBytes); (err != nil) != tt.wantErr { | |||
| if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, "", tt.args.verifyUser, tt.args.credentialBytes); (err != nil) != tt.wantErr { | |||
There was a problem hiding this comment.
The addition of nil and TopOriginIgnoreVerificationMode as parameters in the Verify method call within TestParsedCredentialAssertionData_Verify aligns with the new verification logic introduced for topOrigin. However, it's crucial to ensure that corresponding test cases are added or updated to specifically test the behavior of topOrigin verification under different scenarios, including the TopOriginIgnoreVerificationMode. This will help in validating the correctness and robustness of the new feature.
| WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", rpOrigins, fqOrigin)) | ||
| } | ||
|
|
||
| if rpTopOriginsVerify != TopOriginIgnoreVerificationMode { | ||
| switch len(c.TopOrigin) { | ||
| case 0: | ||
| break | ||
| default: | ||
| if !c.CrossOrigin { | ||
| return ErrVerification. | ||
| WithDetails("Error validating topOrigin"). | ||
| WithInfo("The topOrigin can't have values unless crossOrigin is true.") | ||
| } | ||
|
|
||
| var ( | ||
| fqTopOrigin string | ||
| possibleTopOrigins []string | ||
| ) | ||
|
|
||
| if fqTopOrigin, err = FullyQualifiedOrigin(c.TopOrigin); err != nil { | ||
| return ErrParsingData.WithDetails("Error decoding clientData topOrigin as URL") | ||
| } | ||
|
|
||
| switch rpTopOriginsVerify { | ||
| case TopOriginExplicitVerificationMode: | ||
| possibleTopOrigins = rpTopOrigins | ||
| case TopOriginAutoVerificationMode: | ||
| possibleTopOrigins = append(rpTopOrigins, rpOrigins...) | ||
| case TopOriginImplicitVerificationMode: | ||
| possibleTopOrigins = rpOrigins | ||
| default: | ||
| return ErrNotImplemented.WithDetails("Error handling unknown Top Origin verification mode") | ||
| } | ||
|
|
||
| found = false | ||
|
|
||
| for _, origin := range possibleTopOrigins { | ||
| if strings.EqualFold(fqTopOrigin, origin) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| return ErrVerification. | ||
| WithDetails("Error validating top origin"). | ||
| WithInfo(fmt.Sprintf("Expected Values: %s, Received: %s", possibleTopOrigins, fqTopOrigin)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Registration Step 6 and Assertion Step 10. Verify that the value of C.tokenBinding.status | ||
| // matches the state of Token Binding for the TLS connection over which the assertion was | ||
| // obtained. If Token Binding was used on that TLS connection, also verify that C.tokenBinding.id |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [82-177]
The modifications in the Verify method of CollectedClientData to include rpTopOrigins and rpTopOriginsVerify parameters, and the updated logic to handle TopOrigin verification based on different verification modes, are well-structured and align with the objectives of enhancing origin verification. However, it's important to ensure comprehensive testing of this logic under various scenarios to guarantee its correctness and robustness. Additionally, consider adding more detailed error messages to aid in debugging and troubleshooting.
| // RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully | ||
| // qualified origins. | ||
| RPTopOrigins []string | ||
|
|
||
| // RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the | ||
| // TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3 | ||
| // becomes recommended, implementers should explicitly set this value if they want stability. | ||
| RPTopOriginVerificationMode protocol.TopOriginVerificationMode |
There was a problem hiding this comment.
The addition of RPTopOrigins and RPTopOriginVerificationMode fields to the Config struct is a significant enhancement for configuring top origin verification. Ensure that the documentation for these fields is clear and provides guidance on how to use them effectively. Additionally, consider validating the contents of RPTopOrigins to ensure they are well-formed URLs.
+ // Ensure RPTopOrigins contains well-formed URLs.
+ for _, origin := range config.RPTopOrigins {
+ if _, err := url.ParseRequestURI(origin); err != nil {
+ return fmt.Errorf("RPTopOrigins contains an invalid URL: %s", origin)
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully | |
| // qualified origins. | |
| RPTopOrigins []string | |
| // RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the | |
| // TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3 | |
| // becomes recommended, implementers should explicitly set this value if they want stability. | |
| RPTopOriginVerificationMode protocol.TopOriginVerificationMode | |
| // RPTopOrigins configures the list of Relying Party Server Top Origins that are permitted. These should be fully | |
| // qualified origins. | |
| RPTopOrigins []string | |
| // Ensure RPTopOrigins contains well-formed URLs. | |
| for _, origin := range config.RPTopOrigins { | |
| if _, err := url.ParseRequestURI(origin); err != nil { | |
| return fmt.Errorf("RPTopOrigins contains an invalid URL: %s", origin) | |
| } | |
| } | |
| // RPTopOriginVerificationMode determines the verification mode for the Top Origin value. By default the | |
| // TopOriginIgnoreVerificationMode is used however this is going to change at such a time as WebAuthn Level 3 | |
| // becomes recommended, implementers should explicitly set this value if they want stability. | |
| RPTopOriginVerificationMode protocol.TopOriginVerificationMode |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This adds top origin verification options to the library.
Closes #205
Summary by CodeRabbit