bitwarden / clients

Bitwarden client apps (web, browser extension, desktop, and cli).
https://bitwarden.com
Other
8.69k stars 1.14k forks source link

Passkeys -> getPublicKey() function not implemented, passkeys feature not following W3C standards #6913

Closed wnelson03 closed 7 months ago

wnelson03 commented 7 months ago

Steps To Reproduce

  1. Go to https://webauthn.passwordless.id/demos/playground.html
  2. Click on Register Device button
  3. Scroll down to under that button you just clicked
  4. You will see that the "publicKey" field is empty.

Expected Result

A valid public key is retrieved

Actual Result

No valid public key is retrieved, empty string. Failed registration

Screenshots or Videos

No response

Additional Context

Supposedly this occurs according to the creator of this passwordless.id WebAuthn library because Bitwarden doesn't provide a getPublicKey method, which does not follow W3C standards

https://github.com/passwordless-id/webauthn/issues/37#issuecomment-1794726672

I look forward to this being resolved soon. It would be great if some of my website users who can't afford a physical security key can still benefit from WebAuthn stored in their Bitwarden.

I tested on Firefox, but the original poster of the GitHub issue I linked supposedly tested on Chrome. So it's a problem on all distributions of the Browser extension.

Operating System

Windows

Operating System Version

No response

Web Browser

Firefox

Browser Version

No response

Build Version

2023.10.2

Issue Tracking Info

Adedamola-Aina commented 7 months ago

Hi @wnelson03

I am unable to reproduce this issue, it has been escalated for further investigation. If you have more information that can help us, please add it below.

Thanks!

coroiu commented 7 months ago

I'm not sure it would be fair to say that we are not compatible with the WebAuthn standard. From the way I understand the specification getPublicKey() is optional and might return null. But maybe I've misunderstood or missed something?

https://www.w3.org/TR/webauthn-3/#dom-authenticatorattestationresponse-getpublickey

This operation returns the DER SubjectPublicKeyInfo of the new credential, or null if this is not available. See § 5.2.1.1 Easily accessing credential data.

Also I'm not saying that we shouldn't implement this! I'm just saying that the bug might not be our fault, and that this issue might appear in the future with other authenticators as well

holow29 commented 7 months ago

It seems like it might be allowably null in 2 cases (from § 5.2.1.1 Easily accessing credential data): 1) user-agent is not at level 2 of the spec

Note: getPublicKey() and getAuthenticatorData() were only added in level two of this spec. Relying Parties SHOULD use feature detection before using these functions by testing the value of 'getPublicKey' in AuthenticatorAttestationResponse.prototype. Relying Parties that require this function to exist may not interoperate with older user-agents.

2) User-agent doesn't understand public key algorithm requested

Use of getPublicKey() does impose some limitations: by using pubKeyCredParams, a Relying Party can negotiate with the authenticator to use public key algorithms that the user agent may not understand. However, if the Relying Party does so, the user agent will not be able to translate the resulting credential public key into SubjectPublicKeyInfo format and the return value of getPublicKey() will be null.

User agents MUST be able to return a non-null value for getPublicKey() when the credential public key has a COSEAlgorithmIdentifier value of: -7 (ES256), where kty is 2 (with uncompressed points) and crv is 1 (P-256). -257 (RS256). -8 (EdDSA), where crv is 6 (Ed25519).

A SubjectPublicKeyInfo does not include information about the signing algorithm (for example, which hash function to use) that is included in the COSE public key. To provide this, getPublicKeyAlgorithm() returns the COSEAlgorithmIdentifier for the credential public key.

It seems that adding support would therefore allow for better compatibility and be progress towards elevating Bitwarden to a higher level of the spec, but that this might require more care to make sure it supports the requisite algorithms.

coroiu commented 7 months ago

Yeah ok so without this we are technically only Level 1 compliant, thanks for providing additional information! We'll look into this as soon as we are able :)

trmartin4 commented 7 months ago

This will be resolved with the fix in https://github.com/bitwarden/clients/pull/6934 when it is released.

wnelson03 commented 7 months ago

@trmartin4 @gbubemismith this still doesn't seem to be fixed even with the latest Browser release.

I tested with Chrome Bitwarden browser extension, version 2023.12.0 image I see in the Javascript code that getPublicKey() function returns null. image I tested this in the devtools console as well, and I did in fact get a null response from the function. image I really hope this gets resolved soon, I was excited for this release but unfortunately it doesn't seem to have fixed the issue.

I also looked at the source code of the latest browser release, https://github.com/bitwarden/clients/archive/refs/tags/browser-v2023.12.0.zip, and I see return null; in there for the getPublicKey() function. So it looks like you guys didn't push the changes sadly, despite the description of the release saying "Bug fixes for creating passkeys".

gbubemismith commented 7 months ago

@wnelson03 the fix for this issue was not included in the latest release, and this was due to the timing of our release cycle and when the fix was implemented. However, I can confirm that the fix has been completed and is slated for inclusion in our next release.

wnelson03 commented 6 months ago

@wnelson03 the fix for this issue was not included in the latest release, and this was due to the timing of our release cycle and when the fix was implemented. However, I can confirm that the fix has been completed and is slated for inclusion in our next release.

Wow. Version 2023.12.1 just came out and the getPublicKey() function still does not work. What is the ETA for this? Will this ever release?

djsmith85 commented 6 months ago

@wnelson03 the fix for this issue was not included in the latest release, and this was due to the timing of our release cycle and when the fix was implemented. However, I can confirm that the fix has been completed and is slated for inclusion in our next release.

Wow. Version 2023.12.1 just came out and the getPublicKey() function still does not work. What is the ETA for this? Will this ever release?

@wnelson03: What @gbubemismith meant was the next major release which will be 2024.1. The 2023.12.1-release was cut to fix an issue with the newly available inline autofill