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

adds Related Origin Requests #2040

Merged
merged 32 commits into from
Jul 17, 2024
Merged

adds Related Origin Requests #2040

merged 32 commits into from
Jul 17, 2024

Conversation

timcappalli
Copy link
Member

@timcappalli timcappalli commented Mar 12, 2024

Adds the Related Origin Requests feature (explainer)

  • Updates "Create a New Credential"
  • Updates "Use an Existing Credential"
  • Adds a new subsection: "Using Web Authentication across related origins"
  • Adds a new definition for "Origin Label"
  • Updates "Validating the origin of a credential"
  • Updates "enum ClientCapability"

Resolves #1372


Preview | Diff

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
@abergs
Copy link
Contributor

abergs commented Mar 13, 2024

Is there any considerations to "non https origins", like a browser extension?

Our use case may be niche, but I'll share it:

As a Bitwarden user, you can self-host your server. For example, i can host my vault and data on "vault.andersaberg.com", but still use the bitwarden clients (browser, ios, android, native desktop) to access my data. You simply configure your client to go to your self-hosted domain for the auth and api calls.

Would the bitwarden extension be able to use a passkey, bound to "vault.andersaberg.com", if there is a "vault.andersaberg.com/.well-known/webauthn"-file proclaiming "bitwarden.com", or a browser extension ID to be an associated domain? I'm guessing yes, if the extension has host permission on that domain (vault.andersaberg.com)?

I have the same question for mobile apps - but I don't think anyone here can respond to that.

index.bs 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 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 Outdated Show resolved Hide resolved
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extracted procedure is definitely an improvement! 😄 Some more things to clean up, though...

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 Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@nadalin nadalin added this to the L3-WD-02 milestone Apr 3, 2024
timcappalli and others added 5 commits April 29, 2024 18:57
Co-authored-by: Emil Lundberg <emil@yubico.com>
allows for exiting the loop early on match

Co-authored-by: Emil Lundberg <emil@yubico.com>
@timcappalli
Copy link
Member Author

timcappalli commented Jun 26, 2024

e4f24d9 should address the issue where a new origin is added to the well-known that matches an existing label, but the max label count was already hit (e.g. adding an additional ccTLD origin).

Some code to prove out the updated algorithm: https://gist.github.com/timcappalli/58d3d345a4173effea63f2c5d31fbf3e

Copy link
Contributor

@MasterKale MasterKale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm looks to be in a good place - I especially like how we're now lifting up origin sorting by label to the client level to take care of. It's one less footgun for RP's that need to implement this.

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fact we've been dancing back and forth between issues caused by the max label count and origin ordering indicates we may need a different approach. I'm thinking a better design might be like this:

  • Instead of a flat list of origins, origins is an object where keys are second level domains (SLDs).
  • Each value in origins is a list of origins permitted for that SLD.
  • If origins has too many (> ~5?) keys, or origins[inputSld] is too long (> ~100?), then the origin validation procedure returns false. These limits are determined by client policy.

So the example from the test code (I added foo.bar.otherdomain4.sg so there is a subdomain example):

{
  origins: [
    "https://shopping.sg",
    "https://shopping.co.uk",
    "https://otherdomain1.com",
    "https://otherdomain2.com",
    "https://otherdomain3.com",
    "https://shopping.ie",
    "https://otherdomain4.com",
    "https://otherdomain5.com",
    "https://otherdomain4.sg",
    "https://foo.bar.otherdomain4.sg",
    "https://shopping.ms"
  ]
}

would instead become (and be invalid since origins has more than 5 keys):

{
  origins: {
    "shopping": [
      "https://shopping.co.uk",
      "https://shopping.ie",
      "https://shopping.ms",
      "https://shopping.sg"
    ],
    "otherdomain1": ["https://otherdomain1.com"],
    "otherdomain2": ["https://otherdomain2.com"],
    "otherdomain3": ["https://otherdomain3.com"],
    "otherdomain4": [
      "https://otherdomain4.com",
      "https://otherdomain4.sg",
      "https://foo.bar.otherdomain4.sg"
    ],
    "otherdomain5": ["https://otherdomain5.com"]
  }
}

Would that work? This would at least make the origin label limit much less sensitive to ordering issues and much easier to understand.

I'm not sure whether there should be the limit on list lengths in that case, but I reckon there probably should be some kind of bound on the workload dumped on the client? Alternatively, we could require the RP to sort the origin lists so that the client can binary search for a match instead of having to linear search an unbounded list (and if the RP doesn't, the validation may result in a false negative).

Thoughts on that?

@emlun
Copy link
Member

emlun commented Jul 1, 2024

(On second thought, maybe the linked definition of SLD only accounts for a single top domain segment, not multiple ones like .co.uk. Whatever the appropriate term for this may be, what I mean is the "eTLD+1 - eTLD" segment described in the draft as "Split domain into labels and let label be the right-most one.". This is the interpretation of SLD used by the psl NPM package referenced in the example code.)

@timcappalli
Copy link
Member Author

timcappalli commented Jul 1, 2024

@emlun this was discussed on the last call. The preference was to put all the work on the client and keep the well-known simple for RPs.

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 Outdated

### Validating Related Origins ### {#sctn-validating-relation-origin}

To validate the calling origin is an authorized related origin for a given ceremony:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should explicitly define callerOrigin and rpIdRequested as parameters of the procedure, and the references in the client algorithms should explicitly specify the arguments.

I think we should also put a <dfn> here and reference that rather than simply linking to the section with a different link text. Right now the link text "related origins validation procedure" appears nowhere else in the document, so readers have to rely solely on the hyperlink in order to discover what is actually referenced (this is not the only case of that throughout the spec, but IMO it's preferable to minimize them).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should explicitly define callerOrigin and rpIdRequested as parameters of the procedure, and the references in the client algorithms should explicitly specify the arguments.

Do you have an example of what that should look like in Bikeshed?

I think we should also put a here and reference that rather than simply linking to the section with a different link text. Right now the link text "related origins validation procedure" appears nowhere else in the document, so readers have to rely solely on the hyperlink in order to discover what is actually referenced (this is not the only case of that throughout the spec, but IMO it's preferable to minimize them).

Not sure I understand. What would a definition look like for a procedure? Isn't that just the section that defines the procedue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a few examples in the spec:

So for example, this section could start with:

### Validating Related Origins ### {#sctn-validating-relation-origin}

The <dfn abstract-op>related origins validation procedure</dfn>, given arguments |callerOrigin| and |rpIdRequested|,
is as follows:

and then the references in create() and get() could look like this:

                            1. Run the [$related origins validation procedure$] with arguments |callerOrigin| and |rpIdRequested|.
                                If the result is [FALSE], throw a "{{SecurityError}}" {{DOMException}}.

This has a few advantages: most importantly it makes the connection clearer and less reliant on hidden metadata, so it's more likely to survive format conversions such as into PDF or plain text (or even actual print, if someone were to actually do that), or quotations. Less importantly it also shows Bikeshed's list of references when you click the definition, which doesn't happen for links to sections.

Alternatively, the link to the section should be a plain section reference without replacing the link text, so that it renders the whole section heading, for the reasons above.

Like I said, we don't always respect these concerns throughout the spec, and I don't think we always should, but I think this is one of the cases where there's little or nothing to gain from not doing so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be fair, there is also at least this counter-example where we do link to a section with a (slightly) different link text:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense. Addressed in 9da2c4b. Thanks for explaining @emlun!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 11, 2024
With iOS 18 beta 3, this feature now interoperates with Mobile Safari.
(I've not tested macOS Safari yet, but I believe it'll work there too.)
Thus, enable this in Chrome.

See w3c/webauthn#2040

Change-Id: Ided47fa72995552cb629be32dc3cde93c5ab1da5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5691372
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1326471}
@LeviSchuck
Copy link

Hello, I have some feedback regarding the minimum supported limit of five origins.

Amazon appears to have at least 17 TLDs that serve customers (rather than redirect customers to .com). Authentication occurs without redirecting to the .com origin. Should Amazon adopt related origins and somehow overcome the issue of prior passkey registrations on a localized TLD, they would be constrained by the five origin limit. Perhaps a limit of 20 would be more appropriate?

Here's the list I processed:

ae,ca,co.jp,co.uk,com,com.au,de,eg,es,fr,in,it,nl,pl,sa,se,sg

There may be more co.{cctld} and similar com.{cctld} that I missed.

I have no affiliation with Amazon. They just seemed to be an interesting use case for this specification change.

@Firehed
Copy link

Firehed commented Jul 16, 2024

@LeviSchuck The label described in the algorithm (if I've followed it correctly through my own local testing) would resolve to amazon on all of their owned TLDs, which counts as a singular entry towards the limit. I assume it was designed specifically with this use-case in mind.

I'd love to see this confirmed (or refuted) by one of the authors, but my read was that you'll only run into the limit if you're authenticating across a lot of wildly different domains (such as how signing in to Google often sends you to youtube.com)

@MasterKale
Copy link
Contributor

The algorithm is indeed focused more on the part after the TLD, a.k.a. the "label". So long as all 17 of those TLDs use amazon before them then the current algorithm defined in this PR will handle those just fine.

@LeviSchuck
Copy link

Thank you for replying @Firehed ! @MasterKale helped me understand what a label means outside of this thread.

I can think of no other case where

  1. Identity pools are shared across many origins, and
  2. A consistent authentication origin is not used for each identity pool.

The limit of 5 labels seems fine.

timcappalli and others added 2 commits July 18, 2024 00:00
@emlun's feedback

Co-authored-by: Emil Lundberg <emil@yubico.com>
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good to merge with the addition of a couple more polish opportunities:

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Co-authored-by: Emil Lundberg <emil@yubico.com>
@timcappalli timcappalli dismissed sbweeden’s stale review July 17, 2024 18:39

Discussed on 2024-07-17 call

@timcappalli timcappalli merged commit 2b8e368 into main Jul 17, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Jul 17, 2024
SHA: 2b8e368
Reason: push, by timcappalli

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@emlun emlun deleted the tc-related-origins branch July 17, 2024 18:56
github-actions bot added a commit to nsatragno/webauthn that referenced this pull request Jul 24, 2024
SHA: 2b8e368
Reason: push, by nsatragno

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@supersensen
Copy link

From the perspective of decentralized network user authentication, the /.well-known/webauthn file is like providing hackers with a complete target list.

Therefore, it is hoped that from the "PublicKeyCredentialCreationOptions" before creating credentials at registration, it can be defined whether origin verification is required. If it is not required, the browser should be informed that any domain can pass the verification.

The right to publicly disclose the origins list should be entrusted to the users who make use of it, and hopes that it will be given due attention.

@agl
Copy link
Contributor

agl commented Aug 23, 2024

Therefore, it is hoped that from the "PublicKeyCredentialCreationOptions" before creating credentials at registration, it can be defined whether origin verification is required. If it is not required, the browser should be informed that any domain can pass the verification.

No domain is required to populate that file. The existing RP ID validation rules still apply and, if satisfied, no .well-known request will be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider allowing cross-domain credential use