New issue
Advanced search Search tips

Issue 896404 link

Starred by 2 users
Status: Fixed
Owner:
kpaulhamus@chromium.org
Closed: Oct 23
Cc:
kenrb@chromium.org
martinkr@google.com
engedy@chromium.org
ssoneff@google.com
kpaulhamus@chromium.org
jdoerrie@chromium.org
mknowles@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: ----
Type: ----



Sign in to add a comment

Disable resident credential support in entirety.

Project Member Reported by kpaulhamus@chromium.org, Oct 17 
We should disable resident credentials entirely until properly and fully supported in Chrome, both for privacy and usability concerns.

Chrome lacks an account chooser for selecting which credential should be returned if an empty allow list is passed to getAssertion(). The current behavior for empty lists when credentials for that RP exist is to return a single credential, basically at random. We should disable this.

MakeCredential requests with rk=true should also be disabled. Since we aren't supporting assertions with resident credentials, we shouldn't allow for their creation, either.

Comment 1 by agl@chromium.org, Oct 18 

We should leave this to marinate in Canary for a few days and then request a merge to M71 on the 22nd or 23rd.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 18 

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

commit e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Thu Oct 18 19:10:53 2018

Disable resident key credential creation and usage.

Prevents create() calls where requireResidentKey = true.

Also disables resident credentials in getAssertion calls entirely until
they can be fully and properly supported. (Previously, if an empty list
was passed to getAssertion() and resident credentials for
that RP existed, then a random single credential would be passed back.)


Bug:  896404 
Change-Id: Ic6efcb99891825f929efba616ff5cf5788236a7d
Reviewed-on: https://chromium-review.googlesource.com/c/1287089
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600858}
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/components/password_manager/content/common/credential_manager_mojom_traits.cc
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-with-virtual-authenticator.html
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-from-nested-frame.html
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/blink/public/platform/modules/credentialmanager/credential_manager.mojom
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/blink/public/platform/modules/webauthn/authenticator.mojom
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc
[modify] https://crrev.com/e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc

Comment 3 by kpaulhamus@chromium.org, Oct 18 

Ack, will verify in Canary and request merge on the 22nd.

Comment 4 by kpaulhamus@chromium.org, Oct 22

Labels: Merge-Request-71
Have verified this on Canary and requesting merge to m-71.

Comment 5 by gov...@chromium.org, Oct 22 

Pls apply appropriate OSs label.

Comment 6 by kpaulhamus@chromium.org, Oct 22

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows

Comment 7 by kpaulhamus@chromium.org, Oct 22 

Labels applied. Android is not included because resident keys are already not supported there.
Project Member

Comment 8 by sheriffbot@chromium.org, Oct 23

Labels: -Merge-Request-71 Hotlist-Merge-Approved Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by cr-audit...@appspot.gserviceaccount.com, Oct 23

Labels: -Merge-Approved-71 Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/7dfdde456441b909493395874b95831caf3e187e

Commit: 7dfdde456441b909493395874b95831caf3e187e
Author: kpaulhamus@chromium.org
Commiter: kpaulhamus@chromium.org
Date: 2018-10-23 16:00:11 +0000 UTC

Disable resident key credential creation and usage.

Prevents create() calls where requireResidentKey = true.

Also disables resident credentials in getAssertion calls entirely until
they can be fully and properly supported. (Previously, if an empty list
was passed to getAssertion() and resident credentials for
that RP existed, then a random single credential would be passed back.)


Bug:  896404 
Change-Id: Ic6efcb99891825f929efba616ff5cf5788236a7d
Reviewed-on: https://chromium-review.googlesource.com/c/1287089
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600858}(cherry picked from commit e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49)
Reviewed-on: https://chromium-review.googlesource.com/c/1296544
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#264}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 23

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7dfdde456441b909493395874b95831caf3e187e

commit 7dfdde456441b909493395874b95831caf3e187e
Author: Kim Paulhamus <kpaulhamus@chromium.org>
Date: Tue Oct 23 16:00:11 2018

Disable resident key credential creation and usage.

Prevents create() calls where requireResidentKey = true.

Also disables resident credentials in getAssertion calls entirely until
they can be fully and properly supported. (Previously, if an empty list
was passed to getAssertion() and resident credentials for
that RP existed, then a random single credential would be passed back.)


Bug:  896404 
Change-Id: Ic6efcb99891825f929efba616ff5cf5788236a7d
Reviewed-on: https://chromium-review.googlesource.com/c/1287089
Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#600858}(cherry picked from commit e2c6c8b623a895e42ab4c5cd8fc9329d31ed9f49)
Reviewed-on: https://chromium-review.googlesource.com/c/1296544
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#264}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/components/password_manager/content/common/credential_manager_mojom_traits.cc
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/content/browser/webauth/authenticator_impl.cc
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/content/browser/webauth/authenticator_impl_unittest.cc
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/content/browser/webauth/webauth_browsertest.cc
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-with-virtual-authenticator.html
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-from-nested-frame.html
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/blink/public/platform/modules/credentialmanager/credential_manager.mojom
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/blink/public/platform/modules/webauthn/authenticator.mojom
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc
[modify] https://crrev.com/7dfdde456441b909493395874b95831caf3e187e/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc

Comment 11 by kpaulhamus@google.com, Oct 23

Status: Fixed (was: Assigned)

Comment 12 by kpaulhamus@chromium.org, Oct 31 

 Issue 900559  has been merged into this issue.
►

Sign in to add a comment