Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[passkey] initial draft #474

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

[passkey] initial draft #474

wants to merge 16 commits into from

Conversation

alex4506
Copy link

Description

This submission adds basic support for passkey to the SDK, and also includes corresponding tests.

Test Plan

There still some errors in api tests, especially the submission part of passkey transactions

Related Links

This submission mainly referenced the passkeySigning branch.

src/core/crypto/passkey.ts Outdated Show resolved Hide resolved
});
}

export async function registerCredential(
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the registerCredential, there needs to be more methods verifying whether or not the credential was actually backedup (parse the registration response, verify backup eligibility and backup state), otherwise people will shoot themselves in the foot and create non-backedup credentials.

See https://github.com/aptos-foundation/AIPs/blob/main/aips/aip-66.md#authenticator-data

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally we need support for identifying incompetent / malicious authenticators via AAGUID. see https://github.com/aptos-foundation/AIPs/blob/main/aips/aip-66.md#incompetent--malicious-authenticators

Copy link
Author

Choose a reason for hiding this comment

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

  1. Can I just expose this method verifyAuthenticationResponse to user? Its return type VerifiedAuthenticationResponse include credentialDeviceType and credentialBackedUp

  2. Should we implement a policy that only allows the use of password managers on the list https://github.com/passkeydeveloper/passkey-authenticator-aaguids/blob/main/aaguid.json ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For context on my general thought process here, overall I'd like to provide strong / safe defaults on passkey registration for the user with the ability to override them if a developer wants to.

  1. Yes you're on the right track, the verifyAuthenticationResponse method parses BE and BS and returns the VerifiedAuthenticationResponse. To make it easier for the user we can just check to make sure both are set to true to determine if the credential is backed up.
  2. Per the point above, yes, I think it may make sense to have a strong default for users that allows for authenticators that are well known and guaranteed to be backed up. iCloud Keychain (Managed), iCloud Keychain, and Google Password Manager for example. 1Password and Dashlane may also be acceptable but I'm not confident on the rest of them at this moment.

@@ -0,0 +1,3 @@
module.exports = {
presets: [["@babel/preset-env", { targets: { node: "current" } }]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the reason for adding babel? the sdk uses tsup as the bundle tool

Copy link
Author

@alex4506 alex4506 Jul 23, 2024

Choose a reason for hiding this comment

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

It is for babel-jest

Some imports from @simplewebauthn/browser broke jest, and I couldn't find a way to successfully run it using only ts-jest, so I introduced bable-jest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I'd prefer not to introduce another bundle tool and keep the existing one. Also, to reduce potential errors, and to not increase the package size.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, but I have spent two days, still cannot run jest with only ts-jest, looking forward to any suggestions

Copy link
Author

Choose a reason for hiding this comment

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

BTW, the solution I found is from MasterKale/SimpleWebAuthn#293

* @returns {Promise<*>}
*
*/
const balance = async (aptos: Aptos, name: string, address: AccountAddress) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we keep this example in a node env - same as all the other examples on the SDK? https://github.com/aptos-labs/aptos-ts-sdk/blob/main/examples/typescript/keyless.ts

Copy link
Author

Choose a reason for hiding this comment

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

I will have a try, but it maybe hard. Passkey (webauthn) requires browser API for interactive authentication process.

src/api/passkey.ts Show resolved Hide resolved
src/core/crypto/passkey.ts Outdated Show resolved Hide resolved
src/core/crypto/passkey.ts Outdated Show resolved Hide resolved
src/core/crypto/passkey.ts Outdated Show resolved Hide resolved
@hariria
Copy link
Contributor

hariria commented Jul 23, 2024

@alex4506
Copy link
Author

Comment on lines +17 to +29
export async function recoverPublicKey(
authenticatorData: Uint8Array,
clientDataJSON: Uint8Array,
signature: Uint8Array,
) {
const shaClientDataJSON = await sha256(clientDataJSON);
const message = concatenateUint8Arrays(authenticatorData, shaClientDataJSON);

const result = await recoverPublicKeyFromMessageAndSignature(message, signature);
return result;
}

export async function recoverPublicKeyFromMessageAndSignature(message: Uint8Array, signature: Uint8Array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this in.

  1. Can we add tests for these methods that use the Passkey classes below? We want to make sure users can recover the public key associated with their passkey credential from one or more valid ECDSA signatures in the event that that the relying party loses or misplaces the user's public key.
  2. Also lets include a button in the UI for the examples/typescript/passkey folder that tests this

tests/e2e/api/passkey.test.ts Show resolved Hide resolved
});
}

export async function registerCredential(
Copy link
Contributor

Choose a reason for hiding this comment

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

For context on my general thought process here, overall I'd like to provide strong / safe defaults on passkey registration for the user with the ability to override them if a developer wants to.

  1. Yes you're on the right track, the verifyAuthenticationResponse method parses BE and BS and returns the VerifiedAuthenticationResponse. To make it easier for the user we can just check to make sure both are set to true to determine if the credential is backed up.
  2. Per the point above, yes, I think it may make sense to have a strong default for users that allows for authenticators that are well known and guaranteed to be backed up. iCloud Keychain (Managed), iCloud Keychain, and Google Password Manager for example. 1Password and Dashlane may also be acceptable but I'm not confident on the rest of them at this moment.

@hariria
Copy link
Contributor

hariria commented Jul 27, 2024

Also I'm not sure if SimpleWebAuthn provides this but I think it would be good to provide a method like checkPlatformBackupable that checks

  1. browser version (should be Chrome 118+ for backupable passkeys on MacOS iirc, safari, firefox)
  2. operating system

And this method should be checked in the happy path for passkey registration.

See https://passkeys.dev/device-support/ for more info

@hariria hariria force-pushed the passkey branch 7 times, most recently from 390d5bc to fa185dd Compare August 22, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants