feat (accounts/base): Add passkey support to base account type. #21755
Conversation
WalkthroughWalkthroughThe changes introduce new types and functions for handling authentication and client data in the Cosmos SDK. Key additions include structures for token binding and client data, along with functions for generating authentication keys and client data representations. A new file for managing authentication public keys is created, implementing signature verification. Additionally, a protocol buffer definition for authentication public keys is introduced, facilitating secure management of public keys associated with passkeys. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| func (pubKey *AuthnPubKey) Address() cometcrypto.Address { | ||
| if len(pubKey.Key) != AuthnPubKeySize { | ||
| panic("length of pubkey is incorrect") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
baryon2
requested review from
testinginprod,
sontrinh16 and
a team
as code owners
![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
Outside diff range and nitpick comments (3)
x/accounts/proto/cosmos/accounts/defaults/base/v1/keys/authn_pubkey.proto (1)
9-22: Consider renaming the message to follow PascalCase convention.The message structure and fields look good, but the message name "authnpubkey" doesn't follow the typical PascalCase convention for protocol buffer messages. Consider renaming it to "AuthnPubKey" for consistency with common protobuf naming practices.
Here's the suggested change:
- message authnpubkey { + message AuthnPubKey {The rest of the message structure, including field definitions and comments, looks good and provides clear information about the purpose of each field.
x/accounts/defaults/base/keys/authn/authn_pubkey.go (1)
62-123: Add unit tests for theVerifySignaturemethod.Unit tests are crucial for ensuring the correctness and security of cryptographic functions like
VerifySignature. Please add comprehensive tests covering various scenarios, including valid and invalid signatures, incorrect encodings, and edge cases.Do you want me to help generate unit test code or open a GitHub issue to track this task?
x/accounts/defaults/base/account_test.go (1)
64-64: Nitpick: Consider generatingKeyIddynamically to avoid static analysis alertsThe hardcoded
KeyIdat line 64 may trigger static analysis tools to flag a potential API key exposure. Even though this is test code, generating theKeyIddynamically can help avoid unnecessary alerts and adhere to best practices.Consider applying this change:
- KeyId: "f482bd153df0ca2ea17d7b1e0178c14ff959628f", + KeyId: generateKeyId(pkBytes),And define a
generateKeyIdfunction to compute theKeyIdfrompkBytes, such as by hashing the public key bytes.Tools
Gitleaks
64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
x/accounts/defaults/base/keys/authn/authn_pubkey.pb.gois excluded by!**/*.pb.go
Files selected for processing (3)
- x/accounts/defaults/base/account_test.go (4 hunks)
- x/accounts/defaults/base/keys/authn/authn_pubkey.go (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/base/v1/keys/authn_pubkey.proto (1 hunks)
Additional context used
Path-based instructions (2)
x/accounts/defaults/base/account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/base/keys/authn/authn_pubkey.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Gitleaks
x/accounts/defaults/base/account_test.go
64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
GitHub Check: CodeQL
x/accounts/defaults/base/keys/authn/authn_pubkey.go
[warning] 40-40: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
buf
x/accounts/proto/cosmos/accounts/defaults/base/v1/keys/authn_pubkey.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
Additional comments not posted (4)
x/accounts/proto/cosmos/accounts/defaults/base/v1/keys/authn_pubkey.proto (3)
2-2: LGTM: Package declaration and Go option are correct.The package declaration and Go-specific option are properly set and follow the Cosmos SDK conventions.
Also applies to: 7-7
11-14: LGTM: Amino and GoGo options are correctly set.The Amino and GoGo options are properly configured:
- The Amino name follows the Cosmos SDK naming convention.
- The Amino message encoding is appropriately set to "key_field".
- The GoGo option to disable the default stringer is correctly set, which is common in Cosmos SDK for custom string representations.
These options ensure proper integration with the Cosmos SDK's serialization and encoding systems.
1-5: Verify the existence of the "amino/amino.proto" file.The static analysis tool indicates that the file "amino/amino.proto" does not exist. Please ensure that this file is present in the correct location within your project or dependency tree.
Run the following script to check for the existence of the file:
If the file is not found, consider updating the import statement or adding the missing file to your project.
Verification successful
Confirmed the existence of "proto/amino/amino.proto".
The file "amino/amino.proto" was found at "proto/amino/amino.proto", ensuring it is present in the codebase. The errors encountered when searching "vendor" and "go.mod" can be disregarded as these paths are not directories.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the existence of amino/amino.proto file # Test: Search for amino/amino.proto file fd -t f 'amino.proto' | grep 'amino/amino.proto' # If the above command doesn't find the file, check in common dependency locations echo "Checking in common dependency locations:" fd -t f 'amino.proto' vendor go.modLength of output: 344
Tools
buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
x/accounts/defaults/base/account_test.go (1)
341-479: Unit testTestAuthenticateAuthnprovides comprehensive coverageThe test function
TestAuthenticateAuthneffectively covers both successful authentication and failure scenarios with invalid signatures. This ensures that the new authentication mechanism is thoroughly tested for correctness and robustness.
| func (pubKey *AuthnPubKey) Address() cometcrypto.Address { | ||
| if len(pubKey.Key) != AuthnPubKeySize { | ||
| panic("length of pubkey is incorrect") | ||
| } | ||
|
|
||
| return address.Hash(proto.MessageName(pubKey), pubKey.Key) | ||
| } |
There was a problem hiding this comment.
Avoid using panic in library code.
According to the Uber Go Style Guide, panics should be avoided in library code. Instead of panicking when the key length is incorrect, consider validating the key length upon creation of the AuthnPubKey instance or redesigning the method to return an error.
Tools
GitHub Check: CodeQL
[warning] 40-40: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
| func (pubKey *AuthnPubKey) VerifySignature(msg, sigStr []byte) bool { | ||
| sig := Signature{} | ||
| err := json.Unmarshal(sigStr, &sig) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| clientDataJSON, err := hex.DecodeString(sig.ClientDataJSON) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| clientData := make(map[string]interface{}) | ||
| err = json.Unmarshal(clientDataJSON, &clientData) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| challengeBase64, ok := clientData["challenge"].(string) | ||
| if !ok { | ||
| return false | ||
| } | ||
| challenge, err := base64.RawURLEncoding.DecodeString(challengeBase64) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Check challenge == msg | ||
| if !bytes.Equal(challenge, msg) { | ||
| return false | ||
| } | ||
|
|
||
| publicKey := &ecdsa.PublicKey{Curve: elliptic.P256()} | ||
|
|
||
| publicKey.X, publicKey.Y = elliptic.UnmarshalCompressed(elliptic.P256(), pubKey.Key) | ||
| if publicKey.X == nil || publicKey.Y == nil { | ||
| return false | ||
| } | ||
|
|
||
| signatureBytes, err := hex.DecodeString(sig.Signature) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| authenticatorData, err := hex.DecodeString(sig.AuthenticatorData) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // check authenticatorData length | ||
| if len(authenticatorData) < 37 { | ||
| return false | ||
| } | ||
|
|
||
| clientDataHash := sha256.Sum256(clientDataJSON) | ||
| payload := append(authenticatorData, clientDataHash[:]...) | ||
|
|
||
| h := crypto.SHA256.New() | ||
| h.Write(payload) | ||
|
|
||
| return ecdsa.VerifyASN1(publicKey, h.Sum(nil), signatureBytes) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring VerifySignature for better readability.
The VerifySignature method is lengthy and handles multiple steps such as unmarshalling, decoding, and verification processes. Breaking it into smaller helper functions would improve readability and maintainability.
| clientDataJSON, err := hex.DecodeString(sig.ClientDataJSON) | ||
| if err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Verify that the correct decoding functions are used for ClientDataJSON and AuthenticatorData.
Currently, hex.DecodeString is used to decode sig.ClientDataJSON and sig.AuthenticatorData. Ensure that these fields are actually hex-encoded. Often, these fields are base64 or base64url encoded in authentication protocols like WebAuthn. Using the incorrect decoding method may cause signature verification to fail.
Also applies to: 106-109
| clientData := make(map[string]interface{}) | ||
| err = json.Unmarshal(clientDataJSON, &clientData) | ||
| if err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typed structs instead of map[string]interface{} for JSON unmarshalling.
Employing a typed struct for unmarshalling clientDataJSON can enhance type safety and reduce reliance on type assertions, leading to clearer and more maintainable code.
| WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error { | ||
| return nil | ||
| }), |
There was a problem hiding this comment.
Implement validation logic in WithPubKeyWithValidationFunc
The validation function for authn.AuthnPubKey at lines 99-101 currently returns nil without performing any checks. Implementing validation logic ensures that public keys meet expected criteria and helps detect invalid keys during testing.
Apply this diff to add basic validation:
WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error {
- return nil
+ if pt == nil || len(pt.Key) == 0 {
+ return errors.New("invalid AuthnPubKey: key is nil or empty")
+ }
+ // Additional validation logic can be added here
+ return nil
}),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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error { | |
| return nil | |
| }), | |
| WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error { | |
| if pt == nil || len(pt.Key) == 0 { | |
| return errors.New("invalid AuthnPubKey: key is nil or empty") | |
| } | |
| // Additional validation logic can be added here | |
| return nil | |
| }), |
testinginprod
added
the
backport/v0.52.x
|
converting to draft as some changes need to happen before its ready for review. @baryon2 should the sdk team take it over? |
|
@tac0turtle apologies for the late reply. Yes the sdk team can take over this PR. |
|
Is there anything left here? |
we need to redo the pr, there may be conflicts with recent changes in accounts as well |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
we should pick this up |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Any chance this is planned for |
|
ill reopen it, maybe someone can help getting it over the finish line but as it currently stands we wont have capacity for it |
Description
This PR adds authn pubkey type to the base account that will make it possible to verify transactions signed using passkeys.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes