New issue
Advanced search Search tips

Issue 803832 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: ----


Participants' hotlists:
Hotlist-1


Sign in to add a comment

Complete AuthenticatorSelectionCriteria handling

Project Member Reported by kpaulhamus@chromium.org, Jan 19 2018

Issue description

Implement the full logic to appropriately filter authenticator types. Particularly relevant now that we are adding support for more than USB U2F devices.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275

commit 65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Sat Mar 10 03:28:29 2018

[Webauthn] Add the authenticator filtering preferences to mojom.

This updates the filtering preferences (AuthenticatorSelectionCriteria,
AttestationConveyancePreference, UserVerificationRequirement) in blink
and plumbs the options through authenticator.mojom,
along with typeconverters.

These preferences are used to determine the types of authenticators
the relying party wants/needs to use.

Bug:  803832 
Change-Id: I070ec4bc04de81ad955e878df45a716e3f9083b9
Reviewed-on: https://chromium-review.googlesource.com/947038
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542335}
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/LayoutTests/external/wpt/webauthn/createcredential-badargs-authnrselection.https-expected.txt
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/LayoutTests/http/tests/credentialmanager/resources/credential-helpers.js
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/Source/modules/credentialmanager/AuthenticatorSelectionCriteria.idl
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/Source/modules/credentialmanager/CredentialManagerTypeConverters.cpp
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/Source/modules/credentialmanager/CredentialManagerTypeConverters.h
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/Source/modules/credentialmanager/PublicKeyCredentialRequestOptions.idl
[modify] https://crrev.com/65b1f85ea81f9bca264b70b2c7aa9b5d37ec3275/third_party/WebKit/public/platform/modules/webauth/authenticator.mojom

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e457399d3d0c318be2ca9ad661a963f461bb05a1

commit e457399d3d0c318be2ca9ad661a963f461bb05a1
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Tue Mar 13 18:46:25 2018

[webauthn] Check userVerification and requireResidentKey

Adds the tighter restrictions detailed in
https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html#using-the-ctap2-authenticatormakecredential-command-with-ctap1-u2f-authenticators

where:
1) a create() call must not require user verification or resident keys
2) a get() call must not require user verification
in order to dispatch to u2f authenticators.

Bug:  803832 
Change-Id: Ib9cf9d3bcf0b71f0d32fec76f1aaf175278a02ef
Reviewed-on: https://chromium-review.googlesource.com/949453
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542870}
[modify] https://crrev.com/e457399d3d0c318be2ca9ad661a963f461bb05a1/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/e457399d3d0c318be2ca9ad661a963f461bb05a1/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/e457399d3d0c318be2ca9ad661a963f461bb05a1/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/e457399d3d0c318be2ca9ad661a963f461bb05a1/device/fido/u2f_sign.cc

Labels: Merge-Request-66
Requesting merge to 66 for both CLs in this bug. They add more specific restrictions on the types of requests we permit, otherwise we might allow requests that can't actually be supported by U2F authenticators.

Both CLs were verified on Windows Canary.
Project Member

Comment 4 by sheriffbot@chromium.org, Mar 15 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
What are the implications if we wait until M67? 
Since M66 has already reached beta, it can probably wait. The current behavior is in M65 behind a flag, and the API will still be behind a flag in M66.

It could cause some annoyance to developers while they're developing against the API. If they try using certain combinations of these options, they won't get the correct error when using U2F authenticators. (Realistically, no one should be using these options except in testing scenarios, since they're meant for CTAP authenticators and such devices are not on the market yet).
Labels: -Merge-Review-66 Merge-Rejected-66
per #6, rejecting merge to M66. Let's target this for 67. 
SG, thanks.

Comment 9 by engedy@chromium.org, Mar 31 2018

Labels: Pri-2
Owner: kpaulhamus@chromium.org
Status: Started (was: Available)
Kim, is there more work on this one, or can we close this now that  Issue 823546  is done?
Status: Untriaged (was: Started)
Also, what's the urgency on this one?
Labels: Hotlist-WebAuthnFixit
Status: Started (was: Untriaged)
Urgency N/A. It's fixed following https://chromium-review.googlesource.com/c/chromium/src/+/989413.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3eef95f193bc924766a26eeb72f1ed20d58ecef2

commit 3eef95f193bc924766a26eeb72f1ed20d58ecef2
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Tue Apr 03 10:37:45 2018

[webauthn] Filter out platform devices for U2F requests.

This requirement is derived from clause 5.1.3.19.1 in the spec:

  "If options.authenticatorSelection.authenticatorAttachment is present
   and its value is not equal to authenticator’s attachment modality,
   continue",

and the fact that there are no U2F platform devices.

This CL also fixes the error message when the all U2F devices are
filtered out by the resident key and user verification criteria. While
the CL (crrev.com/c/949453) introducing these options correctly
documented the intended behavior in a comment in u2f_sign, it did not
actually change the DOMException itself.

Finally, the CL moves related webauthn tests from
credentialmanager_browsertests to webauth_browsertests.

Bug:  803832 
Change-Id: Ie8f124af3783c2aaec85abd3baac050cfbb926b7
Reviewed-on: https://chromium-review.googlesource.com/989413
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547664}
[modify] https://crrev.com/3eef95f193bc924766a26eeb72f1ed20d58ecef2/chrome/browser/password_manager/credential_manager_browsertest.cc
[modify] https://crrev.com/3eef95f193bc924766a26eeb72f1ed20d58ecef2/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/3eef95f193bc924766a26eeb72f1ed20d58ecef2/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/3eef95f193bc924766a26eeb72f1ed20d58ecef2/content/browser/webauth/webauth_browsertest.cc

Status: Fixed (was: Started)

Sign in to add a comment