Update Fetch to support Token Binding #715

vanupam · 2018-04-19T16:51:12Z

This PR obsoletes a prior one:

#325

[This fixes #30.]

Preview | Diff

(Catching up with a rebase)

mikewest · 2018-04-23T07:46:53Z

Thanks for reuploading this, @vanupam! I'll take another pass this week!

nharper

There are two high-level issues I see right now:

I think more thought needs to be given to how a client and server might go about changing key types, and what subtle behavior changes are needed here to make that work.
I don't think the changes to the fetch() API should be in this PR. Those changes add additional complexity and questions when reviewing the PR that could delay the PR (such as handling referred Token Bindings and dealing with changing key types). We don't have plans for implementing the fetch() API changes in Chrome, so I don't see a need for the language yet. Removing those changes from this PR also makes it smaller and easier to review.

nharper · 2018-04-25T22:06:04Z

fetch.bs

+ <li><p>If <var>httpRequest</var>'s <a for=request>referred-token-binding origin</a> is not null,
+ then run these substeps:
+  <ol>
+   <li><p>Let <var>referredTlsConnection</var> be the result of


Continuing a comment thread from the previous PR:

I still don't like that referredTlsConnection is created, when all it's needed for is the key type. For the case of a referred Token Binding via an HTTP header, I think the most sensible thing to do is use the same key type/key as was used for the request that received the HTTP response header. This is behavior that doesn't require opening a new connection.

Supporting the fetch() API's mechanism for referred Token Bindings might have some rule that's this simple, like "use the last used key/key type for that origin", but I don't know if that's the rule we'd actually want to write down for this spec. I think we can write a much simpler "Computing a Token Binding Header Value" that doesn't rely on creating this referredTlsConnection if we limit this PR to exclude changes to the fetch() API.

(On the previous PR, the rationale for why the referredTlsConnection is needed was that the server might change the key type (key parameters) it wants to use. Servers changing the Token Binding key parameters they support is fraught with peril and most likely broken if the server wants to have any chance of keeping tokens bound to some Token Binding key.)

I removed fetch() API related changes.

For referred Token Binding using redirects, the referredTlsConnection will already exist - it will just be looked up.

nharper · 2018-04-25T22:09:15Z

fetch.bs

+<dfn export id=concept-token-binding-protocol-version for=connection>token-binding protocol version</dfn>
+and all supported cryptographic algorithms and parameters (the
+<dfn export id=concept-token-binding-key-parameters for=connection>token-binding key parameters</dfn>),
+in order of preference,


in order of preference,

If we want to make it possible for a server to try to migrate users from one key type to another, this probably needs to be explained in a lot more detail what "order of preference" means for a client. It probably depends on what keys the client already has in its key store for the domain it's trying to connect to. (There are more complications than just that here.)

Current language is intended to make sure that the key-type actually negotiated by the UA with a server is used to build the header.

I think migrating UAs from one key type to another is out of scope for this PR - if we want it to happen seamlessly, we likely need a revised underlying spec.

nharper · 2018-04-25T22:17:45Z

fetch.bs

+<a for=/>request</a> is being sent.
+
+Script code from an <a for=/>origin</a> can set a <a for=/>request</a>'s
+<var>useReferredTokenBinding</var> attribute to ask the user agent to send


Either "request" or "useReferredTokenBinding" is wrong here - a request doesn't have a useReferredTokenBinding attribute, but it does have a "referred-token-binding origin". A Request class/object does have a useReferredTokenBinding.

Updated to use Request attribute.

(Moot now: API changes have been removed.)

nharper · 2018-04-25T22:31:41Z

fetch.bs

@@ -5227,6 +5731,10 @@ constructor must run these steps:
 <a for=request>credentials mode</a> to
 <var>credentials</var>.

+ <li><p>If <var>useReferredTokenBinding</var> is true, set <var>request</var>'s
+ <a for=request>referred-token-binding origin</a> to
+ <var>origin</var>.


This makes it possible for a request's referred-token-binding origin to be set even if no Token Binding key exists for that origin. This seems a bit weird to me.

The intent here is to allow scripts to easily indicate that they want to use referred token binding, without letting them set the referred-token-binding origin. This origin is relevant only when TB is being used and there is a TB key.

(Moot now: API changes have been removed.)

nharper · 2018-05-08T00:03:05Z

The only thing left on my list of comments is what to do with referredTlsConnection. I don't like the aesthetics of needing to go through the step of "obtaining a connection" (even if there most likely is already connection is in the connection pool, and a new connection isn't made), especially since it's the only other place (besides HTTP-network fetch) that calls that step. However, I'll defer judgement of this to those who know the Fetch spec better than I.

On the previous PR, there was a conversation about whether Token Binding should be mentioned in section 2 (Infrastructure) with the definition of Credentials, which I don't think reached a conclusion. I am of the opinion that a Sec-Token-Binding header should be treated as a credential (both in the CORS withCredentials context and in a third-party cookie blocking context): A site could choose to set up and operate its infrastructure so that a Token Binding ID is the only information stored on the client for the client's identity (and the Token Binding ID serves as a key into a server-side session state table).

equalsJeffH · 2018-07-25T23:51:03Z

I merged-from-master and fixed conflicts. I think I did it correctly, but someone should probably review, especially in S 4.5. HTTP-network-or-cache fetch, substep 5.17.2.5, thx.

equalsJeffH · 2018-07-26T00:08:55Z

@nharper

I am of the opinion that a Sec-Token-Binding header should be treated as a credential...

submit that as an issue once this PR lands? or do you consider it a blocker for landing this PR?

equalsJeffH

Overall this looks good. thanks @vanupam! Editorial comments below. Although I did not find any substantive technical issues, AFAICT these changes do not satisfy webauthn's need to have access to token binding information (or perhaps I'm missing it?). See w3c/webauthn#360 and e.g. step 13 of https://w3c.github.io/webauthn/#createCredential as well as step 10 of https://w3c.github.io/webauthn/#discover-from-external-source.

Perhaps the way forward is to land this PR and then open another that addresses w3c/webauthn#360 ?

An overall comment: the phrase "token binding" is hyphenated when used as a component of various names such as "token-binding key", "token-binding type", etc. while it is not hyphenated in the IETF tokbind specs (except as header field names). Suggest to not hyphenate it here.

equalsJeffH · 2018-07-26T17:17:47Z

fetch.bs

+        "authors": [
+            "Emily Stark"
+        ],
+        "href": "https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-02",


should reference Internet Drafts without the revision number. that way the latest rev is always returned. Like so:
"https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct"

equalsJeffH · 2018-07-26T17:48:07Z

fetch.bs

+
+<p class="note no-backref"><a for=/>Request</a>'s <a for=request>use-token-binding flag</a>
+controls whether the user agent will send the <a for=/>token binding ID</a> for the
+<a for=request>origin</a> of the <a for=/>request</a>'s url when it transmits the


nit: "url" should be <a for=request>url</a> here and elsewhere it's used

equalsJeffH · 2018-07-26T17:52:23Z

fetch.bs

+Unless stated otherwise, it is unset.
+
+<p class="note no-backref"><a for=/>Request</a>'s <a for=request>use-token-binding flag</a>
+controls whether the user agent will send the <a for=/>token binding ID</a> for the


suggest:
OLD: will send the <a for=/>token binding ID</a>

NEW: will include a <a for=/>token binding message</a> containing the <a for=/>token binding ID</a>

equalsJeffH · 2018-07-26T18:13:08Z

fetch.bs

+<a for=request>origin</a> of the <a for=/>request</a>'s url, when it transmits the
+<a for=/>request</a> to the server. This is used, e.g., by a relying party to indicate
+(via the user agent) to the server receiving the <a for=/>request</a> that it wants the
+credential issued by the server to be bound to the <a for=/>token binding ID</a> for the


nit: "credential" should be <a for=/>credentials</a> here (and elsewhere it's used?)

equalsJeffH · 2018-07-26T19:03:08Z

fetch.bs

@@ -2582,6 +2663,350 @@ Cross-Origin-Resource-Policy     = %x73.61.6D.65.2D.6F.72.69.67.69.6E / %x73.61.
 </ol>


+<h3 id=token-binding>Token Binding</h3>
+
+<p>In order to protect security tokens like HTTP cookies and OAuth tokens, user agents and servers


the term used in this spec for "security tokens" is "credentials". Also, the term "token" is already used in this spec in terms of ABNF productions. since the token binding specs do use the "security token" term, i'd explicitly equate "security tokens" and "credentials" at least in this opening parag. and "credentials" should link to https://fetch.spec.whatwg.org/#credentials

also oauth tokens are not otherwise mentioned in this spec. if they are going to be mentioned, perhaps they be added to the list at https://fetch.spec.whatwg.org/#credentials ?

equalsJeffH · 2018-07-26T22:07:51Z

fetch.bs

+where it saves different <a for=/ lt="token-binding key">token-binding keys</a> to be used with different servers.
+Whenever a <a for=/>token-binding key</a> is needed, the user agent looks it up
+in the <a for=/>token-binding key store</a>,
+creating and saving a new <a for=/>token-binding key</a> if one does not exist.


suggest:
OLD: does not exist

NEW: does not exist, per the <a abstract-op lt="Get the token-binding key">get the token-binding key</a> algorithm, below

equalsJeffH · 2018-07-26T22:58:58Z

fetch.bs

+ <code>TokenBinding.tokenbinding_id</code> field be set to
+ <var>tokenBindingId</var>.
+ <li>Let <var>tokenBinding</var>'s <code>TokenBinding.signature</code> be set
+ to the result of computing a signature over <var>tokenBindingType</var>,


suggest:
OLD: over

NEW: over the concatenation of the values of

equalsJeffH · 2018-07-26T23:21:15Z

fetch.bs

@@ -4058,6 +4511,22 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
   <li><p>If <var>connection</var> is failure, return a
   <a>network error</a>.

+   <li><p>If <var>request</var>'s <a for=request>use-token-binding flag</a>
+   is set, build and add a `<a http-header><code>Sec-Token-Binding</code></a>`


nit: this spec seems to use the term "construct" rather than "build"

equalsJeffH · 2018-07-26T23:22:23Z

fetch.bs

+     <li><p>Let <var>tokenBindingMessage</var> be the result of
+     <a abstract-op lt="Compute a Token Binding header value">computing a Token Binding header value</a>
+     for <var>request</var> and <var>connection</var>,
+     using the user agent's <a for=/>token-binding key store</a> and


I do not think we need the phrase beginning with "using" because that info is baked into the definition of the "Compute a Token Binding header value" alg

equalsJeffH · 2018-07-26T23:29:35Z

fetch.bs

+     <a>connection pool</a>.
+
+     <li><p>If <var>tokenBindingMessage</var> is not null, append
+     `<a http-header><code>Sec-Token-Binding</code></a>`/<var>tokenBindingMessage</var>


nit: put a space in /<var> eg: / <var>

zandbelt · 2018-09-14T12:41:51Z

For implementer's interest see the thread on the intent for Chrome to remove token binding support...:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OkdLUyYmY1E

Perhaps a chance to differentiate.

annevk · 2019-09-13T14:03:32Z

I'm going to close this given that there's not sufficient implementer interest, no activity, and a significant number of outstanding issues.

vanupam and others added 12 commits May 29, 2017 11:10

Rebased.

5ccc551

Addressed review comments.

89fa503

rebasing

4ca3b4b

More updates in response to PR comments.

8ba5c98

merge from master and fix conflicts

5be6ece

Edits to address comments.

0a58054

Fix 'conncetion pool' links.

bfee04e

Minor edits.

e938a7e

merge from master and fix conflict

b3585cf

Minor edits.

f966b27

Merge branch 'tokbind' of https://github.com/vanupam/fetch into tokbind

2a61dcd

(Catching up with a rebase)

Edits to address comments.

a0c5c1f

vanupam mentioned this pull request Apr 19, 2018

Update Fetch to support Token Binding. #325

Closed

annevk added addition/proposal New features or enhancements needs tests Moving the issue forward requires someone to write tests labels Apr 20, 2018

mikewest self-assigned this Apr 23, 2018

nharper suggested changes Apr 25, 2018

View reviewed changes

Removed API-related changes.

20ac23d

JeffH added 2 commits July 25, 2018 09:30

commit merge conflicts - will resolve in subsequent commit

b1275d0

resolve merge conflicts

a823d27

equalsJeffH reviewed Jul 26, 2018

View reviewed changes

equalsJeffH mentioned this pull request Jul 27, 2018

Explain how Token Binding IDs get associated with an HTML context. w3c/webauthn#360

Closed

annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Aug 23, 2018

annevk unassigned mikewest Aug 23, 2018

annevk closed this Sep 13, 2019

annevk mentioned this pull request Nov 19, 2019

Fetch: Token Binding mozilla/standards-positions#82

Closed

annevk mentioned this pull request Jan 30, 2021

Add support for federated token binding to fetch #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Fetch to support Token Binding #715

Update Fetch to support Token Binding #715

vanupam commented Apr 19, 2018 •

edited by pr-preview bot

Loading

mikewest commented Apr 23, 2018

nharper left a comment

nharper Apr 25, 2018

vanupam May 2, 2018

nharper Apr 25, 2018

vanupam May 2, 2018

nharper Apr 25, 2018

vanupam May 2, 2018

nharper Apr 25, 2018

vanupam May 2, 2018

nharper commented May 8, 2018

equalsJeffH commented Jul 25, 2018

equalsJeffH commented Jul 26, 2018

equalsJeffH left a comment •

edited

Loading

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

equalsJeffH Jul 26, 2018

zandbelt commented Sep 14, 2018 •

edited

Loading

annevk commented Sep 13, 2019

Update Fetch to support Token Binding #715

Update Fetch to support Token Binding #715

Conversation

vanupam commented Apr 19, 2018 • edited by pr-preview bot Loading

mikewest commented Apr 23, 2018

nharper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nharper commented May 8, 2018

equalsJeffH commented Jul 25, 2018

equalsJeffH commented Jul 26, 2018

equalsJeffH left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandbelt commented Sep 14, 2018 • edited Loading

annevk commented Sep 13, 2019

vanupam commented Apr 19, 2018 •

edited by pr-preview bot

Loading

equalsJeffH left a comment •

edited

Loading

zandbelt commented Sep 14, 2018 •

edited

Loading