Verify Multiple P-256 Verifiers Are Supported #307
Merged
Conversation
This file contains 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
nlordell
requested review from
akshay-ap,
mmv08 and
remedcu
and removed request for
a team
nlordell
commented
Mar 6, 2024
| 'preferred', | ||
| 'discouraged', | ||
| } | ||
| export type UserVerificationRequirement = 'required' | 'preferred' | 'discouraged' |
There was a problem hiding this comment.
To match the web interface.
nlordell
commented
Mar 6, 2024
| credential.id, | ||
| credential.cosePublicKey(), | ||
| ], | ||
| authData: b2ab( |
There was a problem hiding this comment.
This was a small bug that existed before but went unnoticed as the previous version of the CBOR decoding library used in the tests accepted both types here. Essentially, we need the field to be an ArrayBuffer and not a Uint8Array. We got slightly different versions of the library when installing the dependencies in this package, which are more pedantic than their predecessors, and led to this bug being found.
nlordell
commented
Mar 6, 2024
| /** | ||
| * Extract the x and y coordinates of the public key from a created public key credential. | ||
| * Inspired from <https://webauthn.guide/#registration>. | ||
| */ | ||
| export function extractPublicKey(response: AuthenticatorAttestationResponse): { x: bigint; y: bigint } { | ||
| const attestationObject = CBOR.decode(response.attestationObject) | ||
| const authDataView = new DataView(attestationObject.authData.buffer) | ||
| const credentialIdLength = authDataView.getUint16(53) | ||
| const authData = new DataView( |
There was a problem hiding this comment.
Also small bug that went unnoticed - we can potentially have a Uint8Array with an offset and length, so we can't blindly create a view to its underlying buffer.
nlordell
force-pushed
the
285-support-multiple-verifiers
branch
from
f5680bb to
22f1097
Compare
Pull Request Test Coverage Report for Build 8187306793Details
💛 - Coveralls |
remedcu
approved these changes
Mar 7, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Partially addresses #285
This PR adds new tests that verify multiple P-256 verifiers are supported by our code. The
webauthnshim code now moved to thepasskeypackage and is imported by the4337package, as this made more sense to me. @mmv08 moved E2E tests for Passkey+4337 to thepasskeyproject, so once they both merge, the dependency can be removed.Note that for now, we use a
TestWebAuthnSignerFactorcontract just for implementing the tests, but it should be switched to using the canonical signing factory from #306 once merged.The additional verifier that is being used is the one from Daimo-eth. The artifact was vendored into the project, as it is a Foundry project, and this is the easiest way to include the dependency without building it (giving us identical bytecode to what is deployed on-chain).
Adding a test for using the precompile will be done in a separate PR.
Note that this PR contains some unrelated changes to the deployment and E2E setup. This was a result of rebasing onto changes introduced in #306 to get things working as well as some nits leftover from the aforementioned PR.