fix(protocol): credential public key match limited to ECDSA - #732
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAndroid Key and Apple attestation validation now use generic credential public-key matching. The matcher supports EC2, RSA, and Ed25519 COSE keys. Tests cover matches, mismatches, malformed keys, and key-type differences. ChangesAttestation key verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AndroidKeyAttestation
participant verifyAttestationPublicKeyMatch
participant webauthncose.VerifySignature
AndroidKeyAttestation->>verifyAttestationPublicKeyMatch: verify credential key against certificate key
verifyAttestationPublicKeyMatch-->>AndroidKeyAttestation: return parsed credential key
AndroidKeyAttestation->>webauthncose.VerifySignature: verify attestation signature
webauthncose.VerifySignature-->>AndroidKeyAttestation: return verification result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #732 +/- ##
==========================================
- Coverage 86.23% 86.08% -0.16%
==========================================
Files 49 49
Lines 4286 4296 +10
==========================================
+ Hits 3696 3698 +2
- Misses 379 384 +5
- Partials 211 214 +3
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@protocol/attestation_androidkey.go`:
- Around line 100-103: Update the verification handling around
webauthncose.VerifySignature in the Android key attestation flow by checking and
reporting a non-nil verification error separately from a false valid result.
Preserve the existing error cause only for actual verification errors, and
return a distinct invalid-signature detail without formatting or attaching a nil
error when valid is false.
- Around line 97-103: Update the second signature verification in the Android
key attestation flow to use the attestation statement’s alg value by passing
webauthncose.COSEAlgorithmIdentifier(alg) to VerifySignature, or remove this
redundant check. Preserve the existing invalid-attestation error handling while
ensuring RSA/PSS and ECDSA attestations are verified with the statement
algorithm rather than the credential key’s algorithm.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16ec60ee-5fcd-4032-a295-8c722eeb1d70
📒 Files selected for processing (4)
protocol/attestation_androidkey.goprotocol/attestation_apple.goprotocol/utils.goprotocol/utils_test.go
§8.4 and §8.8 only require the attestation certificate public key to match the credentialPublicKey, but the check refused anything but ECDSA on either side, rejecting the RSA keys Android Keystore attests. It now converts the parsed COSE key to its standard library equivalent and compares with Equal, so a differing key type is a mismatch rather than a refusal. The android-key handler's second signature verification now goes through webauthncose.VerifySignature. It isn't redundant with the certificate check above it, as each takes its digest from the algorithm it carries, so passing both requires the two to agree.
james-d-elliott
force-pushed
the
fix-androidkey-ec2
branch
from
August 8, 2026 05:31
ae8fff8 to
bc43f08
Compare
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
§8.4 and §8.8 only require the attestation certificate public key to match the credentialPublicKey, but the check refused anything but ECDSA on either side, rejecting the RSA keys Android Keystore attests. It now converts the parsed COSE key to its standard library equivalent and compares with Equal, so a differing key type is a mismatch rather than a refusal.
The android-key handler's second signature verification now goes through webauthncose.VerifySignature. It isn't redundant with the certificate check above it, as each takes its digest from the algorithm it carries, so passing both requires the two to agree.