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

Specify if clients are expected to follow redirects for icon URLs #1285

Closed
lgarron opened this issue Aug 23, 2019 · 11 comments
Closed

Specify if clients are expected to follow redirects for icon URLs #1285

lgarron opened this issue Aug 23, 2019 · 11 comments
Assignees
Milestone

Comments

@lgarron
Copy link
Contributor

@lgarron lgarron commented Aug 23, 2019

At GitHub, we have "nice" profile icon URLs like https://github.com/lgarron.png

That URL redirects to https://avatars2.githubusercontent.com/u/248078?v=4 , which is moderately stable but could change some day. Could I ask for the spec to specify whether RPs are expected to follow the redirect from the more permanent URL?

I can't find information on this; #1139 is the closest I've found

@emlun

This comment has been minimized.

Copy link
Contributor

@emlun emlun commented Aug 25, 2019

Both user.icon and rp.icon are specified by the RP*, so RPs aren't expected to process them at all after passing them into a WebAuthn function call. #1139 has some discussion on what authenticators and/or clients are expected to do with them.

* Or by the user under the RP's supervision, unless the user modifies the client-side script. In either case, the RP controls the values for the purposes of this discussion.

@lgarron

This comment has been minimized.

Copy link
Contributor Author

@lgarron lgarron commented Aug 25, 2019

Ah, I'm sorry, I meant to ask about clients!

#1139 is moderately relevant, but it would be useful to know if we (as an RP) can count on clients to do at least one redirect!

@lgarron lgarron changed the title Are RPs expected to follow redirects for icon URLs? Are clients expected to follow redirects for icon URLs? Aug 25, 2019
@lgarron lgarron changed the title Are clients expected to follow redirects for icon URLs? Specify if clients are expected to follow redirects for icon URLs Aug 26, 2019
@nadalin nadalin modified the milestones: L3-WD-01, L2-WD-02 Aug 28, 2019
@jcjones

This comment has been minimized.

Copy link
Contributor

@jcjones jcjones commented Aug 28, 2019

For practical purposes, no user agent is going to actively fetch these icons, as they would be potent correlation mechanisms for resident credentials -- after so much effort is taken elsewhere in the specification to avoid correlation.

Perhaps the right thing to do here is to amend the definition for these icons to specify that these are only valid if they are data: URLs of a valid image type.

@lgarron

This comment has been minimized.

Copy link
Contributor Author

@lgarron lgarron commented Aug 28, 2019

Perhaps the right thing to do here is to amend the definition for these icons to specify that these are only valid if they are data: URLs of a valid image type.

In that case, it would be nice to specify a more reasonable expectation than 128 bytes of storage for the icon.

128 bytes is reasonable for a URL, but squeezing icons into 128 data: bytes is basically futile. A recommended target of 4K or 8K is somewhat doable. (Perhaps it could be specified like srcset, offering a large image for computer/phone-resident authenticator storage with lots of space vs.small one for space-constrained physical key storage.)

@dwaite

This comment has been minimized.

Copy link

@dwaite dwaite commented Aug 28, 2019

My understanding of the correlation issue is that a client may cache the icon at the conclusion of authentication and registration, but a client will not resolve the icon before authentication using the network due to correlation concerns.

I agree with @lgarron that very few relying parties will manage to fit a usable icon within the 85 bytes you can hypothetically squeeze out of a 128 character data URL. It is more likely they will use a regular URL and let clients deal with the UX of not having an icon.

@emlun

This comment has been minimized.

Copy link
Contributor

@emlun emlun commented Sep 2, 2019

A recommended target of 4K or 8K is somewhat doable.

Sorry, this is not going to happen for hardware authenticators at least. Resident credentials already require a significant amount of storage space.

@ve7jtb

This comment has been minimized.

Copy link
Contributor

@ve7jtb ve7jtb commented Sep 3, 2019

The browser could cache it during make credential, and after authenticating. I am guessing that browsers may not be able to easily display only cached icons.
Larger storage requirements is not going to fly for roaming authenticators, unless you want them to store only one or two credentials.
On a practical note do we have the size etc of the icons sufficiently defined so that a platform could display them? This may be a bit like the authenticator icon in meta-data, something that seems like a good idea, but is never used in practice.
If browsers are not going to display them I would drop them going forward to make more room.

@dwaite

This comment has been minimized.

Copy link

@dwaite dwaite commented Sep 3, 2019

On a practical note do we have the size etc of the icons sufficiently defined so that a platform could display them? This may be a bit like the authenticator icon in meta-data, something that seems like a good idea, but is never used in practice

Thats my concern. We also have two icons:

  1. The RP entity icon, which likely is the same for all RP registrations. It isn't useful for choosing a credential to use, but may be useful if you are managing the credentials on an authenticator.

  2. The user entity icon, which is likely different for each registration. It might be useful for choosing a credential to use, but will likely only contain a usable value if the RP maintains profile information including a photo or avatar. It also must be user-differentiated, so fundamentally must leak information on loading.

If desired, the RP icon would have a good shot of being changed to be widely usable. For example, we could reference an image source set via .well-known of the RP domain, and have clients fetch that without session state. Of course, a well-known lookup also technically no longer needs the URL field.

We don't have infrastructure to use the user icon ad-hoc (such as an intermediate, anonymizing cache). So the user icon is only really displayable if previously cached by a registration or authentication action, loaded by user action, etc. We still would have the problem that not all RPs have a usable picture for this, or guidance on how it will be displayed.

@nadalin

This comment has been minimized.

Copy link
Contributor

@nadalin nadalin commented Sep 20, 2019

@jcjones Create PR

@jcjones

This comment has been minimized.

Copy link
Contributor

@jcjones jcjones commented Sep 20, 2019

I will make a PR to deprecate the image fields and any others that are unlikely to be utilized by platforms or authenticators.

@jcjones

This comment has been minimized.

Copy link
Contributor

@jcjones jcjones commented Oct 23, 2019

Still working on this!

jcjones added a commit to jcjones/webauthn that referenced this issue Oct 29, 2019
As discussed in issue w3c#1285, the image URL fields for PublicKeyCredentialEntity,
while intended for user interface design, are potent correlation mechanisms if
they are downloaded by RPs. RPs would have to take extraordinary care, beyond
reasonable measures, to avoid uses by RPs with mal-intent to cross-correlate
accounts. It is better for User Agents to use existing origin/icon mechanisms for
their UX designs, or to define new such mechanisms as-needed, that are
origin-wide rather than provide the possibility to embed detailed tracking
information into these URLs.
@jcjones jcjones closed this in dbcf596 Oct 30, 2019
jcjones added a commit that referenced this issue Oct 30, 2019
Fix #1285 - Remove icons from PublicKeyCredentialEntity
emlun added a commit that referenced this issue Nov 6, 2019
As discussed in issue #1285, the image URL fields for PublicKeyCredentialEntity,
while intended for user interface design, are potent correlation mechanisms if
they are downloaded by RPs. RPs would have to take extraordinary care, beyond
reasonable measures, to avoid uses by RPs with mal-intent to cross-correlate
accounts. It is better for User Agents to use existing origin/icon mechanisms for
their UX designs, or to define new such mechanisms as-needed, that are
origin-wide rather than provide the possibility to embed detailed tracking
information into these URLs.
emlun added a commit that referenced this issue Nov 6, 2019
Fix #1285 - Remove icons from PublicKeyCredentialEntity
Kenny1366 pushed a commit to Kenny1366/webkit that referenced this issue Jan 11, 2020
https://bugs.webkit.org/show_bug.cgi?id=203346
<rdar://problem/56558488>

Reviewed by Brent Fulgham.

Source/WebCore:

Covered by new tests within existing test files.

* Modules/webauthn/AuthenticatorAssertionResponse.h:
(WebCore::AuthenticatorAssertionResponse::setName):
(WebCore::AuthenticatorAssertionResponse::name const):
(WebCore::AuthenticatorAssertionResponse::setDisplayName):
(WebCore::AuthenticatorAssertionResponse::displayName const):
(WebCore::AuthenticatorAssertionResponse::setNumberOfCredentials):
(WebCore::AuthenticatorAssertionResponse::numberOfCredentials const):
Adds new members to store new fields of the response from the authenticator. Field "icon"
is omitted given it could be used to track users according to w3c/webauthn#1285.
* Modules/webauthn/fido/DeviceResponseConverter.cpp:
(fido::readCTAPGetAssertionResponse):
Adds new logic to parse above fields from an authenticator response.

Source/WebKit:

This patch implements authenticatorGetNextAssertion as suggested by the spec:
https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#authenticatorGetNextAssertion

The work flow is as follow:
1) When a valid assertion response is received, check its numberOfCredentials member;
2) When it is larger then 1, use authenticatorGetNextAssertion to get all remaining responses;
3) Once all responses are gathered, ask UI clients to pick one to return.

* UIProcess/API/APIWebAuthenticationPanelClient.h:
(API::WebAuthenticationPanelClient::selectAssertionResponses const):
* UIProcess/WebAuthentication/Authenticator.h:
* UIProcess/WebAuthentication/AuthenticatorManager.cpp:
(WebKit::AuthenticatorManager::selectAssertionResponses):
* UIProcess/WebAuthentication/AuthenticatorManager.h:
* UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:
(WebKit::MockHidConnection::parseRequest):
* UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:
(WebKit::CtapAuthenticator::continueGetAssertionAfterResponseReceived):
(WebKit::CtapAuthenticator::continueGetNextAssertionAfterResponseReceived):
* UIProcess/WebAuthentication/fido/CtapAuthenticator.h:

Tools:

* TestWebKitAPI/Tests/WebCore/CtapResponseTest.cpp:
(TestWebKitAPI::TEST):
* TestWebKitAPI/Tests/WebCore/FidoTestData.h:
Adds new test case for new logic in DeviceResponseConverter.

LayoutTests:

* http/wpt/webauthn/public-key-credential-get-failure-hid.https-expected.txt:
* http/wpt/webauthn/public-key-credential-get-failure-hid.https.html:
* http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt:
* http/wpt/webauthn/public-key-credential-get-success-hid.https.html:
* http/wpt/webauthn/resources/util.js:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@254356 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.