Closed Bug 1381190 Opened 2 years ago Closed 2 years ago

Web Authentication - Change to COSE Algorithm Identifier types

Categories

(Core :: DOM: Device Interfaces, enhancement, P1)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
Future
Tracking Status
firefox58 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [webauthn][webauthn-wd07] )

Attachments

(2 files)

There's a TODO in WebAuthnManager.cpp:GetAlgorithmName to coerce JS objects to strings and get their names, similar to what WebCrypto does. This is a spec compliance requirement.

This will require piping JS objects to the GetAlgorithmName function.
WD-07 changes this to COSE types
Blocks: 1384776
No longer blocks: webauthn
Priority: P2 → P1
Summary: Support WebCrypto Algorithm object types for WebAuthn algorithms → Web Authentication - Change to COSE Algorithm Identifier types
Whiteboard: [webauthn] [webauthn-interop] → [webauthn][webauthn-wd07]
Target Milestone: --- → Future
Version: 55 Branch → 58 Branch
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Comment on attachment 8918030 [details]
Bug 1381190 - Remove WebAuthnRequest dead code

https://reviewboard.mozilla.org/r/188928/#review194432

Sweet.
Attachment #8918030 - Flags: review?(ttaubert) → review+
Comment on attachment 8918031 [details]
Bug 1381190 - Change to COSE Algorithm identifiers for WebAuthn

https://reviewboard.mozilla.org/r/188930/#review194822

::: dom/webauthn/WebAuthnManager.cpp:21
(Diff revision 1)
>  #include "mozilla/dom/U2FUtil.h"
>  #include "mozilla/dom/WebAuthnCBORUtil.h"
>  #include "mozilla/dom/WebAuthnManager.h"
>  #include "mozilla/dom/WebAuthnTransactionChild.h"
>  #include "mozilla/dom/WebAuthnUtil.h"
>  #include "mozilla/dom/WebCryptoCommon.h"

We can probably remove this now.

::: dom/webauthn/WebAuthnManager.cpp:344
(Diff revision 1)
>    if (NS_WARN_IF(NS_FAILED(srv))) {
>      promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);
>      return promise.forget();
>    }
>  
>    // Process each element of cryptoParameters using the following steps, to

Is `cryptoParameters` an old reference to `mPubKeyCredParams`?

::: dom/webauthn/WebAuthnManager.cpp:358
(Diff revision 1)
>      // Let normalizedAlgorithm be the result of normalizing an algorithm using
>      // the procedure defined in [WebCryptoAPI], with alg set to
>      // current.algorithm and op set to 'generateKey'. If an error occurs during
>      // this procedure, then stop processing current and move on to the next
>      // element in cryptoParameters.

That paragraph can probably go?

::: dom/webauthn/WebAuthnManager.cpp:365
(Diff revision 1)
> -    if (NS_FAILED(GetAlgorithmName(aOptions.mPubKeyCredParams[a].mAlg,
> +    if (NS_FAILED(CoseAlgorithmToWebCryptoId(aOptions.mPubKeyCredParams[a].mAlg,
> -                                   algName))) {
> +                                             algName))) {

So we assign to `algName` here but then don't use that value anywhere, right?

::: dom/webauthn/WebAuthnManager.cpp:370
(Diff revision 1)
> -    // Add a new object of type PublicKeyCredentialParameters to
> -    // normalizedParameters, with type set to current.type and algorithm set to
> +    if (!acceptableParams.AppendElement(aOptions.mPubKeyCredParams[a],
> +                                        mozilla::fallible)){

If all we only check `acceptableParams.IsEmpty()`, maybe we should make that a boolean and change the condition? We don't need to maintain a list.

::: dom/webauthn/tests/browser/tab_webauthn_success.html:41
(Diff revision 1)
>  let makeCredentialOptions = {
>    rp: {id: document.domain, name: "none", icon: "none"},
>    user: {id: new Uint8Array(), name: "none", icon: "none", displayName: "none"},
>    challenge: gCredentialChallenge,
>    timeout: 5000, // the minimum timeout is actually 15 seconds
> -  pubKeyCredParams: [{type: "public-key", alg: "ES256"}],
> +  pubKeyCredParams: [{type: "public-key", alg: -7}],

Can we use constants here and elsewhere? Maybe defined in a common JS file?
Attachment #8918031 - Flags: review?(ttaubert) → review-
Comment on attachment 8918031 [details]
Bug 1381190 - Change to COSE Algorithm identifiers for WebAuthn

https://reviewboard.mozilla.org/r/188930/#review194822

> Is `cryptoParameters` an old reference to `mPubKeyCredParams`?

Yup, caught the GetAssertion uses, but not these.. oops

> So we assign to `algName` here but then don't use that value anywhere, right?

Yes, but this code needs to move to U2F where it really belongs. I'm going to let this remain unused for now and add a comment pointing to a newly filed Bug 1409220.

> If all we only check `acceptableParams.IsEmpty()`, maybe we should make that a boolean and change the condition? We don't need to maintain a list.

Dropping this in favor of Bug 1409220.

> Can we use constants here and elsewhere? Maybe defined in a common JS file?

Good idea. Updated.
Comment on attachment 8918031 [details]
Bug 1381190 - Change to COSE Algorithm identifiers for WebAuthn

https://reviewboard.mozilla.org/r/188930/#review195370
Attachment #8918031 - Flags: review?(ttaubert) → review+
Keywords: checkin-needed
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 10 changes to 10 files
remote: 
remote: WebIDL file dom/webidl/WebAuthentication.webidl altered in changeset ed61bc39a21c without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Attachment #8918031 - Flags: review?(kyle)
(In reply to Mozilla Autoland from comment #9)
> remote: WebIDL file dom/webidl/WebAuthentication.webidl altered in changeset
> ed61bc39a21c without DOM peer review

Agh. Change-blinded to the one line WebIDL update. r?qdot, thanks.
Comment on attachment 8918031 [details]
Bug 1381190 - Change to COSE Algorithm identifiers for WebAuthn

https://reviewboard.mozilla.org/r/188930/#review195548
Attachment #8918031 - Flags: review?(kyle) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c09ea1671fc3
Remove WebAuthnRequest dead code r=ttaubert
https://hg.mozilla.org/integration/autoland/rev/35f1751b91a9
Change to COSE Algorithm identifiers for WebAuthn r=qdot,ttaubert
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.