Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

55 move low level webauthn logic to primitives directory #95

Merged
merged 13 commits into from
Sep 29, 2023
Merged

55 move low level webauthn logic to primitives directory #95

merged 13 commits into from
Sep 29, 2023

Conversation

JohnGuilding
Copy link
Contributor

@JohnGuilding JohnGuilding commented Sep 18, 2023 •
edited
Loading

This PR:

  1. Adds a new foundry project inside the primitives directory
  2. Moves webauthn contract and accompanying libraries to primitives directory (including test)
  3. Symlinks primitives to account-integrations/safe
  4. Deletes old files in the safe directory
  5. Adds a github workflow for primitives

Base automatically changed from 92-clean-up-use-of-FCL-lib-with-calldata to main September 18, 2023 17:26
@JohnGuilding JohnGuilding force-pushed the 55-move-low-level-webauthn-logic-to-primitives-directory branch from 0ef0eb1 to 0914e57 Compare September 25, 2023 11:50
@JohnGuilding JohnGuilding marked this pull request as ready for review September 27, 2023 14:42
@JohnGuilding JohnGuilding linked an issue Sep 27, 2023 that may be closed by this pull request
Move low-level webauthn logic to primitives directory #55
Closed
blakecduncan
blakecduncan approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@blakecduncan blakecduncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This lgtm!

@JohnGuilding JohnGuilding merged commit b84a486 into main Sep 29, 2023
3 checks passed
@JohnGuilding JohnGuilding deleted the 55-move-low-level-webauthn-logic-to-primitives-directory branch September 29, 2023 17:49
jzaki
jzaki reviewed Oct 2, 2023
View reviewed changes
primitives/src/WebAuthn.sol
function verifySignature(
bytes calldata authenticatorData,
bytes1 authenticatorDataFlagMask,
bytes calldata clientData,
bytes32 messageHash,
bytes32 clientChallenge,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the cryptographic side (before here) is client challenge always guaranteed to be 32 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I've seen bytes32 used elsewhere, but looking at the spec itself, it looks like the challenge should be a minimum of 16 bytes, so it could be different to 32 bytes:

In order to prevent replay attacks, the challenges MUST contain enough entropy to make guessing them infeasible. Challenges SHOULD therefore be at least 16 bytes long.

https://www.w3.org/TR/webauthn-2/#sctn-cryptographic-challenges

FCL assumes bytes32 but perhaps this is wrong according to the spec. Added an issue to look into this - #107

@JohnGuilding JohnGuilding mentioned this pull request Oct 2, 2023
Open
@jzaki
Copy link
Contributor

jzaki commented Oct 2, 2023

Would be good to know how much of account-integrations/safe/SafeWebAuthnPlugin can make its way into primitives/WebAuthn and remain generic to 4337.
@blakecduncan this could be explored in bringing WebAuthn into account-integrations/kernel (lower priority than current integration work)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jzaki jzaki jzaki left review comments

@blakecduncan blakecduncan blakecduncan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move low-level webauthn logic to primitives directory
3 participants
@JohnGuilding @jzaki @blakecduncan