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

1Password Fails to Register #419

Open
natewiebe13 opened this issue Feb 27, 2024 · 5 comments
Open

1Password Fails to Register #419

natewiebe13 opened this issue Feb 27, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@natewiebe13
Copy link

Version(s) affected

N/A

Description

While implementing the web-auth/webauthn-lib lib on a custom PHP application, we ran into 1Password not being able to generate passkeys and noticed the demo is affected as well. From our testing, if pubKeyCredParams is empty, 1Password is unable to generate passkeys. Once we added algorithms, it works. Since 1Password is quite popular, it's probably worth revisiting if a default algorithm list should be included by default, instead of being empty. Or at least providing documentation that it's likely required for some services, such as 1Password.

How to reproduce

  1. Have the 1Password extension installed (tested with Chrome and Firefox)
  2. Navigate to https://webauthn.spomky-labs.com/register
  3. Fill out form and press Register
  4. Click on Save when 1Password asks to save the passkey
    image
  5. The following should appear in the console.
    Port Opener: passkey-save-prompt/1qcm68e received error: "create-passkey-failed"
    The screenshot below is what the dialog should look like.
    image

Possible Solution

As mentioned in the description, supported algorithms should likely be included by default.

Additional Context

No response

@natewiebe13
Copy link
Author

Looking a bit further into this, https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create#pubkeycredparams is a required property, and is an array of algorithms supported by the RP.

Here's the options coming from the demo:

{
    "rp": {
        "name": "demo",
        "id": "webauthn.spomky-labs.com"
    },
    "user": {
        "name": "gborsHd4mEMyyf89IFck7pG4buc8eYhjBOp36AJnmCg",
        "id": "MDFIUVAzSFpDQllFUUI0SDY2Rko2OFdYTTU",
        "displayName": "gborsHd4mEMyyf89IFck7pG4buc8eYhjBOp36AJnmCg"
    },
    "challenge": "5aU_Bx1J58fBNrgZUEjxvW1qhMJrt0s378DllM1Vji8",
    "pubKeyCredParams": [],
    "authenticatorSelection": {
        "requireResidentKey": false,
        "userVerification": "preferred",
        "residentKey": "preferred"
    },
    "attestation": "none",
    "extensions": {
        "uvm": true
    },
    "status": "ok",
    "errorMessage": ""
}

This appears that the RP in this case doesn't support any algorithms. So in this case, I feel like 1Password's implementation is correct.

@Spomky
Copy link
Contributor

Spomky commented Feb 27, 2024

Hi,

Many thanks for your feedback. That's really interesting 🤔. As per spec, when the algorithm list is missing, default algs that are namely ES256 and RS256 should be used.

@natewiebe13
Copy link
Author

I must have missed that earlier when I was reviewing the spec. But I agree. So in this case, 1Password isn't following the spec properly. So I'm going to see if I can bring it up with their team. In the meantime, I don't know if it's worth updating the demo to include algos so it at least functions for people seeing if the lib works with 1Password.

@Spomky
Copy link
Contributor

Spomky commented Feb 28, 2024

I don't know if it's worth updating the demo to include algos so it at least functions for people seeing if the lib works with 1Password.

Indeed. I then it is a good idea to set a curated list of algs.

@Spomky Spomky added enhancement New feature or request good first issue Good for newcomers labels Feb 28, 2024
@natewiebe13
Copy link
Author

This was fixed in 1Password/passkey-rs#15, which should already be released on the nightly build. So the next version of the 1Password extension should work with an empty array.

I'm thinking this can probably be closed then, but I'll leave it open in case you still want it done @Spomky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants