Add authenticator attachment used during authentication to assertion payload #1668

z11h · 2021-09-08T16:38:23Z

This spec change adds the authenticator attachments (platform/cross-platform) used during authentication to the assertion payload returned from the browser to the RP.

Fixes: #1666

Also discussed in #1637

Preview | Diff

* add transport docs * Update index.bs Co-authored-by: Nina Satragno <[email protected]> * Update index.bs Co-authored-by: Nina Satragno <[email protected]> * Update index.bs Co-authored-by: Nina Satragno <[email protected]> * Update index.bs Co-authored-by: Nina Satragno <[email protected]> Co-authored-by: zakaria ridouh <[email protected]> Co-authored-by: Nina Satragno <[email protected]>

nsatragno

lgtm

Firstyear · 2021-09-09T00:09:49Z

This field isn't part of the signed collected client data, so can't this be tampered with during the response? This may lead to failse assumptions of security/validity of this data. There is already a signed extension for this purpose, so how is this an improvement?

agl · 2021-09-09T00:17:48Z

This field isn't part of the signed collected client data, so can't this be tampered with during the response?

The user-agent could "tamper" with it, yes. As with several other fields (including the transport list in the registration).

There is already a signed extension for this purpose, so how is this an improvement?

There's no extension defined for this but, even if we defined one, it wouldn't have any support from authenticators. An extension isn't needed for the motivating use here, which is allow a website to figure out whether to offer to register a platform authenticator.

Firstyear · 2021-09-09T00:20:17Z

There's no extension defined for this but, even if we defined one, it wouldn't have any support from authenticators. An extension isn't needed for the motivating use here, which is allow a website to figure out whether to offer to register a platform authenticator.

If this is the case, it should be called a transportSelectionHint to make it extremely clear that it is not an enforceable security property. There have already been multiple instances of RP's implementing webauthn that have incorrectly assumed that criteria are security properties rather than UX hints, and I think we should improve this in the language we use for these values.

agl · 2021-09-09T00:24:05Z

If this is the case, it should be called a transportSelectionHint to make it extremely clear that it is not an enforceable security property. There have already been multiple instances of RP's implementing webauthn that have incorrectly assumed that criteria are security properties rather than UX hints, and I think we should improve this in the language we use for these values.

I don't think we can save such RPs. We can't rename everything that exists and, if we start calling things transportAsReportedByTheBrowser then it suggests that the other values are coming from the authenticator!

agl · 2021-09-09T00:50:13Z

Another point from the above: might RPs do silly things with this like decided that assertions over USB are ok but NFC aren't? If we're saying that the point is to determine platform vs not, might be spell this attachment=platform or attachment=cross-platform?

index.bs

This patch adds the transport used during authentication to the assertion payload returned from the browser to the relying party following a successful authentication. Spec change: w3c/webauthn#1668 This feature is gated behind #enable-web-authentication-assertion-transport Bug: 1243721 Change-Id: I90378d5a4b454f4180d020ed1e5c8fc63f2bf2d2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3124587 Auto-Submit: zakaria ridouh <[email protected]> Reviewed-by: Yutaka Hirano <[email protected]> Reviewed-by: Ken Buchanan <[email protected]> Reviewed-by: Rouslan Solomakhin <[email protected]> Reviewed-by: Nina Satragno <[email protected]> Reviewed-by: Liquan (Max) Gu <[email protected]> Commit-Queue: Ken Buchanan <[email protected]> Commit-Queue: zakaria ridouh <[email protected]> Cr-Commit-Position: refs/heads/main@{#921727}

* transport to platform attachment * pt2 * add section * move authenticator attachment to public key credential * slight changes * slight changes Co-authored-by: zakaria ridouh <[email protected]>

index.bs

Co-authored-by: Nina Satragno <[email protected]> Co-authored-by: Emil Lundberg <[email protected]>

nsatragno

lgtm

index.bs

Co-authored-by: Nina Satragno <[email protected]>

equalsJeffH · 2021-09-22T19:15:27Z

on 2021-09-22 call:
various folks to re-review

equalsJeffH

I've reviewed and caught one link mistake I made (apologies). Also, these changes link to [[#sctn-attachments-from-transports]] and assume the suggestions I made in PR #1670 are accepted , but it seems @emlun hasn't had a chance to review that as yet.

So we can fix the link below, and then either (a) wait for @emlun to review #1670 and proceed per however that works out, or (b) merge this PR and then be prepared to revise things as necessary depending on how #1670 shakes out.

What do folks wish to do?

index.bs

Co-authored-by: =JeffH <[email protected]>

z11h · 2021-09-27T20:06:32Z

@equalsJeffH: Since @emlun is in a different time zone, I'm fine waiting until he has a look at your comments on #1670 and we can sit on this PR until that PR shakes out and we'll go ahead and merge this, to prevent any dead links.

index.bs

emlun · 2021-10-01T11:31:58Z

Thanks for your patience everyone! #1670 has now evolved to be mutually dependent on this PR, so I think it makes more sense to merge it into here instead of into #1621. I couldn't re-target the PR base branch though, because this branch is in https://github.com/z11h/webauthn, so I instead replaced #1670 with z11h#3.

@equalsJeffH

* Describe how to determine authenticator attachment from transports * Accept @equalsJeffH's rewrite of #sctn-attachment-from-transports Co-authored-by: =JeffH <[email protected]> Co-authored-by: =JeffH <[email protected]>

z11h · 2021-10-01T16:18:59Z

@emlun, merged your branch onto my fork.

all comments have been resolved, PR is ready for another look :)

equalsJeffH

This is looking good @z11h & @emlun, thanks for merging PR #1670 into here. Upon review it seems having the new #sctn-attachments-from-transports section embedded in section 5.2, which is about authenticator responses, is odd because it describes client platform behavior that sets an attribute on the PublicKeyCredential interface. I suggest we move the new #sctn-attachments-from-transports section up into section 5.1, say as a peer to and after sctn-isUserVerifyingPlatformAuthenticatorAvailable (it'd now have only three hashs in its title).
WDYT?

Also, I suggest we use the existing term "authenticator transport" rather than introducing a new term "credential transport" (suggestions and a nit fix below).

Plus, we ought to have at least @nsatragno review the normative language we're adding here.

index.bs

nsatragno

Still LGTM with a minor nit.

index.bs

Co-authored-by: =JeffH <[email protected]>

z11h · 2021-10-01T19:08:00Z

@nsatragno resolved.

@equalsJeffH great comments, I've reordered the sections, and agree that authenticator transport is a more fitting term.

equalsJeffH · 2021-10-01T19:58:05Z

thanks @z11h, LGTM. If @emlun approves upon re-review we should be ready to merge :)

emlun

Sorry for the back-and-forth, but seeing it in its new context I don't think the new section (#sctn-attachments-from-transports) makes much sense anymore - the client (unlike an RP) doesn't need to "determine the attachment from the transports", it already knows the attachment. So rewriting the section for this new purpose, the "algorithm" would become something like:

If the authenticator

is using platform attachment

set the value of authenticatorAttachment to platform.

is using cross-platform attachment,

set the value of authenticatorAttachment to cross-platform.

which seems pretty redundant since the §5.4.5. Authenticator Attachment Enumeration (enum AuthenticatorAttachment) section already defines the same mapping. So I think we can get rid of the new section entirely and just refer back to AuthenticatorAttachment. Here are some suggestions for that.

index.bs

Co-authored-by: Emil Lundberg <[email protected]>

z11h

@emlun, proposal updated. :)

index.bs

z11h · 2021-10-06T19:22:26Z

PR updated. :)

equalsJeffH · 2021-10-07T00:59:13Z

Actually, I think this is good enough for now (thanks for your patience @z11h & @nsatragno!) as of commit 6951078 and we ought to land this as-is.

If we wish to further clarify detail-level things that client devs need to be aware of (e.g., underlying platform not supplying transport info, so it's reported as null (as discussed briefly on today's webauthn call)), we ought to submit an issue and deal with it separately.

emlun

Looks good to me, thanks for your patience everyone!

@nsatragno

…payload (#1668) SHA: 5227874 Reason: push, by @nsatragno Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@nsatragno

…payload (w3c#1668) SHA: 5227874 Reason: push, by @nsatragno Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

nsatragno approved these changes Sep 8, 2021

View reviewed changes

nadalin assigned z11h Sep 8, 2021

nadalin added this to the L3-WD-01 milestone Sep 8, 2021

nadalin added the type:technical label Sep 8, 2021

nadalin requested review from akshayku, emlun and sbweeden September 8, 2021 19:14

sbweeden requested changes Sep 9, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

sbweeden mentioned this pull request Sep 16, 2021

Clarify, simplify and align parameter descriptions #1621

Merged

z11h changed the title ~~Add transport used during authentication to assertion payload~~ Add platform attachment used during authentication to assertion payload Sep 16, 2021

z11h changed the title ~~Add platform attachment used during authentication to assertion payload~~ Add authenticator attachment used during authentication to assertion payload Sep 17, 2021

transport to platform attachment (#2)

388cd8e

* transport to platform attachment * pt2 * add section * move authenticator attachment to public key credential * slight changes * slight changes Co-authored-by: zakaria ridouh <[email protected]>

nsatragno requested changes Sep 20, 2021

View reviewed changes

sbweeden mentioned this pull request Sep 21, 2021

Describe how to determine authenticator attachment from transports #1670

Closed

emlun requested changes Sep 21, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

z11h and others added 2 commits September 21, 2021 16:23

Apply suggestions from code review

4ea12b8

Co-authored-by: Nina Satragno <[email protected]> Co-authored-by: Emil Lundberg <[email protected]>

resolve comments pt 2

989efe3

nsatragno approved these changes Sep 22, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

Apply suggestions from code review 2

ac0cbf9

Co-authored-by: Nina Satragno <[email protected]>

akshayku approved these changes Sep 22, 2021

View reviewed changes

nadalin requested a review from equalsJeffH September 22, 2021 19:15

sbweeden approved these changes Sep 22, 2021

View reviewed changes

equalsJeffH suggested changes Sep 27, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Update index.bs

e751287

Co-authored-by: =JeffH <[email protected]>

ve7jtb reviewed Sep 29, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

emlun mentioned this pull request Oct 1, 2021

Issue 1618 review sbweeden attachment transport z11h/webauthn#3

Merged

emlun and others added 2 commits October 1, 2021 11:11

Issue 1618 review sbweeden attachment transport (#3)

359ba0f

* Describe how to determine authenticator attachment from transports * Accept @equalsJeffH's rewrite of #sctn-attachment-from-transports Co-authored-by: =JeffH <[email protected]> Co-authored-by: =JeffH <[email protected]>

reword for clarity

8851db6

equalsJeffH suggested changes Oct 1, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

nsatragno approved these changes Oct 1, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

z11h and others added 2 commits October 1, 2021 13:55

Apply suggestions from code review

a1d2c98

Co-authored-by: =JeffH <[email protected]>

reorder sections

ea3ebbe

equalsJeffH approved these changes Oct 1, 2021

View reviewed changes

emlun requested changes Oct 4, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

z11h and others added 2 commits October 5, 2021 11:13

Apply suggestions from code review

e64580b

Co-authored-by: Emil Lundberg <[email protected]>

implement changes to proposal

0cd8e11

z11h commented Oct 5, 2021

View reviewed changes

emlun requested changes Oct 5, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

formatting change & add lost portion

6951078

emlun approved these changes Oct 7, 2021

View reviewed changes

nsatragno merged commit 5227874 into w3c:main Oct 7, 2021

equalsJeffH mentioned this pull request Oct 14, 2021

Possible experiences in a future WebAuthn #1637

Closed

MasterKale mentioned this pull request Jan 15, 2022

Provide request deserialization, response serialization #1683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authenticator attachment used during authentication to assertion payload #1668

Add authenticator attachment used during authentication to assertion payload #1668

z11h commented Sep 8, 2021 •

edited by pr-preview bot

Loading

nsatragno left a comment

Firstyear commented Sep 9, 2021

agl commented Sep 9, 2021

Firstyear commented Sep 9, 2021

agl commented Sep 9, 2021

agl commented Sep 9, 2021

nsatragno left a comment

equalsJeffH commented Sep 22, 2021

equalsJeffH left a comment

z11h commented Sep 27, 2021

emlun commented Oct 1, 2021

z11h commented Oct 1, 2021

equalsJeffH left a comment

nsatragno left a comment

z11h commented Oct 1, 2021

equalsJeffH commented Oct 1, 2021

emlun left a comment

z11h left a comment •

edited

Loading

z11h commented Oct 6, 2021

equalsJeffH commented Oct 7, 2021 •

edited

Loading

emlun left a comment

Add authenticator attachment used during authentication to assertion payload #1668

Add authenticator attachment used during authentication to assertion payload #1668

Conversation

z11h commented Sep 8, 2021 • edited by pr-preview bot Loading

nsatragno left a comment

Choose a reason for hiding this comment

Firstyear commented Sep 9, 2021

agl commented Sep 9, 2021

Firstyear commented Sep 9, 2021

agl commented Sep 9, 2021

agl commented Sep 9, 2021

nsatragno left a comment

Choose a reason for hiding this comment

equalsJeffH commented Sep 22, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

z11h commented Sep 27, 2021

emlun commented Oct 1, 2021

z11h commented Oct 1, 2021

equalsJeffH left a comment

Choose a reason for hiding this comment

nsatragno left a comment

Choose a reason for hiding this comment

z11h commented Oct 1, 2021

equalsJeffH commented Oct 1, 2021

emlun left a comment

Choose a reason for hiding this comment

z11h left a comment • edited Loading

Choose a reason for hiding this comment

z11h commented Oct 6, 2021

equalsJeffH commented Oct 7, 2021 • edited Loading

emlun left a comment

Choose a reason for hiding this comment

z11h commented Sep 8, 2021 •

edited by pr-preview bot

Loading

z11h left a comment •

edited

Loading

equalsJeffH commented Oct 7, 2021 •

edited

Loading