Re: [whatwg/fetch] Update Fetch to support Token Binding. (#325)

mikewest requested changes on this pull request.

Thanks! Sorry for the delayed response. Now that I've paged this in again, I should be quicker on the draw.

Unfortunately, I'm OOO again most of tomorrow because kindergarten's still closed. I'll try to get through your response in any downtime, and I'll certainly pick this up again Monday.

> @@ -966,6 +984,40 @@ for other values. If <cite>HTML</cite> changes here, this standard will need cor
 Unless stated otherwise, it is unset.
 
 <p>A <a for=/>request</a> has an associated
+<dfn export for=request id=concept-request-use-token-binding>use-token-binding flag</dfn>.
+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 its <a for=/>token binding ID</a>
+for the <a for=request>origin</a> along with the
+<a for=/>request</a> when it transmits the
+<a for=/>request</a> to the server.

Style nit: The wrapping here seems a little tight.

> @@ -966,6 +984,40 @@ for other values. If <cite>HTML</cite> changes here, this standard will need cor
 Unless stated otherwise, it is unset.
 
 <p>A <a for=/>request</a> has an associated
+<dfn export for=request id=concept-request-use-token-binding>use-token-binding flag</dfn>.
+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 its <a for=/>token binding ID</a>
+for the <a for=request>origin</a> along with the

"its token binding ID for the origin" is a little ambiguous (as the request has an [origin](https://fetch.spec.whatwg.org/#concept-request-origin), which I don't think is what you mean). Is "the token binding ID for the origin of the request's url" accurate?

> +<a for=request>use-token-binding flag</a>
+controls whether the
+user agent will send its <a for=/>token binding ID</a>
+for the <a for=request>origin</a> along with the
+<a for=/>request</a> when it transmits the
+<a for=/>request</a> to the server.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=concept-request-use-referred-token-binding>use-referred-token-binding flag</dfn>.
+Unless stated otherwise, it is unset.
+
+<p class="note no-backref"><a for=request>Request</a>'s
+<a for=request>use-referred-token-binding flag</a>
+controls whether the user agent will send the
+<a for=/>token binding ID</a>
+used by the user agent with the referring <a for=request>origin</a>,

Nit: s/with/for/?

Also, "referring origin" is ambiguous for the same reason as above: requests have a [referrer](https://fetch.spec.whatwg.org/#concept-request-referrer), and it's not clear that that's what you mean here. Perhaps it would be simpler to describe the use case rather than the detail, leaving the latter for the algorithms below?

> +<a for=request>use-referred-token-binding flag</a>
+controls whether the user agent will send the
+<a for=/>token binding ID</a>
+used by the user agent with the referring <a for=request>origin</a>,
+in addition to the <a for=/>token binding ID</a> used by the
+user agent with the <a for=request>origin</a>, when it transmits the
+<a for=/>request</a> to the server.
+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=concept-request-referred-token-binding-origin>referred-token-binding-origin</dfn>,
+which is an <a for=request>origin</a>. Unless stated otherwise, it is null.
+
+<p class="note no-backref"><a for=/>Request</a>'s
+<a for=request>referred-token-binding-origin</a>
+specifies the <a for=request>origin</a> used to compute the <a for=/>referred token Binding ID</a>,
+when its <a for=request>use-referred-token-binding flag</a> is set.

This will probably be clarified below, but I wonder how this works through a redirect chain. Does the user agent relay referred token binding IDs through multiple redirects?

> @@ -1421,6 +1473,18 @@ for each associated <a for="fetch group">fetch record</a> whose
   <p>Let <var>connection</var> be the result of establishing an HTTP connection to
   <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
 
+  <p>If <var>credentials</var> is true and if the user agent supports
+  <a for=/>Token Binding</a>, propose the use of <a for=/>Token Binding</a>

Perhaps a little more detail here, like "indicate the highest supported Token Binding protocol version and key parameters by sending a `token_binding` TLS extension in the ClientHello message, as described in..."?

> @@ -1421,6 +1473,18 @@ for each associated <a for="fetch group">fetch record</a> whose
   <p>Let <var>connection</var> be the result of establishing an HTTP connection to
   <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
 
+  <p>If <var>credentials</var> is true and if the user agent supports

"... and the HTTP connection uses TLS"? Or maybe just change "the TLS connection" to "a TLS connection" in the next clause?

> @@ -1421,6 +1473,18 @@ for each associated <a for="fetch group">fetch record</a> whose
   <p>Let <var>connection</var> be the result of establishing an HTTP connection to
   <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
 
+  <p>If <var>credentials</var> is true and if the user agent supports
+  <a for=/>Token Binding</a>, propose the use of <a for=/>Token Binding</a>
+  while setting up the TLS connection, as described in
+  <a href="https://tools.ietf.org/html/draft-ietf-tokbind-negotiation#section-2">section 2</a>
+  of the Token Binding Negotiation spec [[!TOKBIND-NEGOTIATION]].
+  If Token Binding Negotiation succeeds, update
+  TLS connection metadata with the parameters of the result of the negotiation.

The negotiation results in a protocol version and set of key parameters, right?

@annevk: Should this metadata be defined somewhere, or are you OK with it being somewhat magical?

> @@ -2172,6 +2236,132 @@ exploits pertain to those <a for=request>types</a>. Also, considering "<code>ima
 compatible with deployed content.
 
 
+<h3 id=token-binding>Token Binding</h3>

Style nit: Newline after the heading.

> @@ -2172,6 +2236,132 @@ exploits pertain to those <a for=request>types</a>. Also, considering "<code>ima
 compatible with deployed content.
 
 
+<h3 id=token-binding>Token Binding</h3>
+<p>In order to protect security tokens like <a>credentials</a> and OAuth tokens, user agents

Nit: I'd suggest "cookies" rather than "credentials" to be explicit about the use case. I don't think you'd be able to make token binding with with basic/digest auth, for instance.

> @@ -2172,6 +2236,132 @@ exploits pertain to those <a for=request>types</a>. Also, considering "<code>ima
 compatible with deployed content.
 
 
+<h3 id=token-binding>Token Binding</h3>
+<p>In order to protect security tokens like <a>credentials</a> and OAuth tokens, user agents
+and servers can use Token Binding to cryptographically associate a given
+security token with the user agent, thereby making it more difficult for
+attackers to abuse the capabilities the security token grants.

How about: "`In order to protect security tokens like cookies and OAuth tokens, user agents and servers can use a technique known as <dfn export id=concept-token-binding>Token Binding</dfn> to cryptographically associate a given token with a secret known only to a specific user agent. This association mitigates the risk that attackers can steal the token and use it themselves, as they won't be able to easily replicate the user agent's secret, and therefore can't replicate the cryptographic binding.`". Is that accurate?

> +security token with the user agent, thereby making it more difficult for
+attackers to abuse the capabilities the security token grants.
+This method of associating credentials with a user agent is referred to as
+<dfn export id=concept-token-binding>token binding</dfn>.
+
+<p>The technique is described in
+detail in [[TOKBIND-NEGOTIATION]], [[TOKBIND-PROTOCOL]], and [[TOKBIND-HTTPS]].
+
+<p>A <dfn export id=token-binding-id>token binding ID</dfn> is the representation
+of a <a for=/>token binding</a>, as described in
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.2">section 3.2</a>
+of [[TOKBIND-PROTOCOL]].
+
+<p>At a very high level, a user agent negotiates the use of Token Binding when it
+sets up a TLS connection to a server.
+The user agent maintains a Token Binding key store, where it saves different

Style nit: Wrapping throughout this section.

> +attackers to abuse the capabilities the security token grants.
+This method of associating credentials with a user agent is referred to as
+<dfn export id=concept-token-binding>token binding</dfn>.
+
+<p>The technique is described in
+detail in [[TOKBIND-NEGOTIATION]], [[TOKBIND-PROTOCOL]], and [[TOKBIND-HTTPS]].
+
+<p>A <dfn export id=token-binding-id>token binding ID</dfn> is the representation
+of a <a for=/>token binding</a>, as described in
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.2">section 3.2</a>
+of [[TOKBIND-PROTOCOL]].
+
+<p>At a very high level, a user agent negotiates the use of Token Binding when it
+sets up a TLS connection to a server.
+The user agent maintains a Token Binding key store, where it saves different
+<a for=/>token binding ID</a>s to be used with different servers.

s/servers/origins/ throughout? Or is it the case that a set of coalesced connections use the same binding?

> +<dfn export id=concept-token-binding>token binding</dfn>.
+
+<p>The technique is described in
+detail in [[TOKBIND-NEGOTIATION]], [[TOKBIND-PROTOCOL]], and [[TOKBIND-HTTPS]].
+
+<p>A <dfn export id=token-binding-id>token binding ID</dfn> is the representation
+of a <a for=/>token binding</a>, as described in
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.2">section 3.2</a>
+of [[TOKBIND-PROTOCOL]].
+
+<p>At a very high level, a user agent negotiates the use of Token Binding when it
+sets up a TLS connection to a server.
+The user agent maintains a Token Binding key store, where it saves different
+<a for=/>token binding ID</a>s to be used with different servers.
+It maintains <a for=/>token binding ID</a>s at the granularity of
+effective top-level domain (public suffix) + 1 (eTLD+1).

Hrm. Why eTLD+1? Why not origin-specific?

> +<a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.2">section 3.2</a>
+of [[TOKBIND-PROTOCOL]].
+
+<p>At a very high level, a user agent negotiates the use of Token Binding when it
+sets up a TLS connection to a server.
+The user agent maintains a Token Binding key store, where it saves different
+<a for=/>token binding ID</a>s to be used with different servers.
+It maintains <a for=/>token binding ID</a>s at the granularity of
+effective top-level domain (public suffix) + 1 (eTLD+1).
+If the user agent and server agree to use Token Binding
+and a <a for=/>token binding ID</a> does not already exist for that server,
+the user agent generates a <a for=/>token binding ID</a> (essentially, a public-private key-pair)
+for use with the server,
+using TLS connection metadata saved at the end of the Token Binding Negotiation,
+and saves it in the Token Binding key store.
+Later, the user agent simply looks up the a <a for=/>token binding ID</a> whenever needed.

Nit: "the a"

> +It maintains <a for=/>token binding ID</a>s at the granularity of
+effective top-level domain (public suffix) + 1 (eTLD+1).
+If the user agent and server agree to use Token Binding
+and a <a for=/>token binding ID</a> does not already exist for that server,
+the user agent generates a <a for=/>token binding ID</a> (essentially, a public-private key-pair)
+for use with the server,
+using TLS connection metadata saved at the end of the Token Binding Negotiation,
+and saves it in the Token Binding key store.
+Later, the user agent simply looks up the a <a for=/>token binding ID</a> whenever needed.
+When the user agent sends a <a for=/>request</a> to the server over the TLS connection,
+it proves possession of its <a for=/>token binding ID</a> to the server.
+The server associates credentials that it issues to that user agent with that
+<a for=/>token binding ID</a>,
+and verifies if credentials presented to it by a user agent match the
+<a for=/>token binding ID</a> used when the credentials were issued to that user agent.
+<p class="note no-backref">

Style nit: Newline before paragraph.

> +the user agent generates a <a for=/>token binding ID</a> (essentially, a public-private key-pair)
+for use with the server,
+using TLS connection metadata saved at the end of the Token Binding Negotiation,
+and saves it in the Token Binding key store.
+Later, the user agent simply looks up the a <a for=/>token binding ID</a> whenever needed.
+When the user agent sends a <a for=/>request</a> to the server over the TLS connection,
+it proves possession of its <a for=/>token binding ID</a> to the server.
+The server associates credentials that it issues to that user agent with that
+<a for=/>token binding ID</a>,
+and verifies if credentials presented to it by a user agent match the
+<a for=/>token binding ID</a> used when the credentials were issued to that user agent.
+<p class="note no-backref">
+If the <a for=/>token binding ID</a> for a server is
+deleted by a user agent (e.g., as a result of some user action), the user agent will
+need to re-establish a TLS connection to negotiate Token Binding and create a new
+<a for=/>token binding ID</a>.

Not sure this note is necessary here; seems like something to note in the token binding specs themselves.

> +<code>Include-Referred-Token-Binding-ID</code> <a for=response>header</a>.
+
+<pre>
+Include-Referred-Token-Binding-ID = %x74.72.75.65 ; "true", case-sensitive
+</pre>
+
+<p>The value of this <a for=response>header</a> is specified in
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-https#section-5.3">section 5.3</a>
+of the Token Binding over HTTP spec [[TOKBIND-HTTPS]].
+
+By setting the <code>Include-Referred-Token-Binding-ID</code>
+<a for=response>header</a> to "<code>true</code>",
+the <a for=request>origin</a> that sends the
+redirect <a for=/>response</a> tells the user agent to disclose the
+<a for=/>token binding ID</a> used by the user agent with that
+<a for=request>origin</a> to the target<a for=request>origin</a> as a

Nit: Need a space between "target" and "origin".

It's not clear to me which origin "that origin" is referring to here. The request's origin? The origin of the server generating the redirect response?

> @@ -3061,6 +3251,31 @@ in addition to <a>HTTP fetch</a> above.
   nullity has already been checked. The <a lt=extract for=BodyInit>extracting</a> operation cannot
   throw as it was called for the same <a for=body>source</a> before.
 
+ <li><p>Let <var>useReferredTokenBinding</var> be the result of
+   <a for=header>parsing</a> `<code>Include-Referred-Token-Binding-ID</code>`
+   in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Unset the <var>request</var>'s <a for=request>use-referred-token-binding flag</a>.
+
+ <li><p>If <var>useReferredTokenBinding</var> is a <span>byte-case-insensitive</span>
+     match for `<code>true</code>`, and the <var>request</var>'s
+     <a for=request>use-token-binding flag</a> is set, then perform the following substeps:

Style nit: Inconsistent wrapping here and above. Please follow the example of the rest of the steps in these algorithms.

> @@ -3061,6 +3251,31 @@ in addition to <a>HTTP fetch</a> above.
   nullity has already been checked. The <a lt=extract for=BodyInit>extracting</a> operation cannot
   throw as it was called for the same <a for=body>source</a> before.
 
+ <li><p>Let <var>useReferredTokenBinding</var> be the result of
+   <a for=header>parsing</a> `<code>Include-Referred-Token-Binding-ID</code>`
+   in <var>actualResponse</var>'s <a for=response>header list</a>.

Do you care whether this response comes from the network or from a Service Worker?

> +of the Token Binding over HTTP spec [[TOKBIND-HTTPS]].
+
+By setting the <code>Include-Referred-Token-Binding-ID</code>
+<a for=response>header</a> to "<code>true</code>",
+the <a for=request>origin</a> that sends the
+redirect <a for=/>response</a> tells the user agent to disclose the
+<a for=/>token binding ID</a> used by the user agent with that
+<a for=request>origin</a> to the target<a for=request>origin</a> as a
+<a for=/>referred token binding ID</a> in the <a for=/>Token Binding Message</a> sent to the target server.
+
+
+<h4 id=computing-token-binding-header-value>Computing a Token Binding Header Value</h4>
+<p>If the user agent supports <a for=/>Token Binding</a>,
+build a <code>Sec-Token-Binding</code> <a for=/>header</a>
+for a <a for=/>request</a> <var>httpRequest</var> using the Token Binding key store by running
+these substeps:

What does this algorithm return? A header? A header's value?

> +
+<h4 id=computing-token-binding-header-value>Computing a Token Binding Header Value</h4>
+<p>If the user agent supports <a for=/>Token Binding</a>,
+build a <code>Sec-Token-Binding</code> <a for=/>header</a>
+for a <a for=/>request</a> <var>httpRequest</var> using the Token Binding key store by running
+these substeps:
+
+<ol>
+ <li><p>Initialize <var>tokenBindingMessage</var> to null.
+
+ <li><p>Let <var>providedTokenBindingId</var> be the result of looking up the
+ <a for=/>token binding ID</a> for the <var>httpRequest</var>'s
+ <a for=/>current url</a>.
+
+ <li><p>If <var>providedTokenBindingId</var> is not null, then add a
+ <code>provided_token_binding</code> to <var>tokenBindingMessage</var>.

`|tokenBindingMessage|` is `null` here. It's not clear what you mean by adding `|provided_token_binding|` to it.

> +redirect <a for=/>response</a> tells the user agent to disclose the
+<a for=/>token binding ID</a> used by the user agent with that
+<a for=request>origin</a> to the target<a for=request>origin</a> as a
+<a for=/>referred token binding ID</a> in the <a for=/>Token Binding Message</a> sent to the target server.
+
+
+<h4 id=computing-token-binding-header-value>Computing a Token Binding Header Value</h4>
+<p>If the user agent supports <a for=/>Token Binding</a>,
+build a <code>Sec-Token-Binding</code> <a for=/>header</a>
+for a <a for=/>request</a> <var>httpRequest</var> using the Token Binding key store by running
+these substeps:
+
+<ol>
+ <li><p>Initialize <var>tokenBindingMessage</var> to null.
+
+ <li><p>Let <var>providedTokenBindingId</var> be the result of looking up the

Nit: I think `providedTokenBindingId` is meant to be the same as `provided_token_binding` below?

> +redirect <a for=/>response</a> tells the user agent to disclose the
+<a for=/>token binding ID</a> used by the user agent with that
+<a for=request>origin</a> to the target<a for=request>origin</a> as a
+<a for=/>referred token binding ID</a> in the <a for=/>Token Binding Message</a> sent to the target server.
+
+
+<h4 id=computing-token-binding-header-value>Computing a Token Binding Header Value</h4>
+<p>If the user agent supports <a for=/>Token Binding</a>,
+build a <code>Sec-Token-Binding</code> <a for=/>header</a>
+for a <a for=/>request</a> <var>httpRequest</var> using the Token Binding key store by running
+these substeps:
+
+<ol>
+ <li><p>Initialize <var>tokenBindingMessage</var> to null.
+
+ <li><p>Let <var>providedTokenBindingId</var> be the result of looking up the

Or, no. `provided_token_binding` is an enum value of `TokenBindingType`. This is confusing.

Can you be a little more detailed in this section about how you're building the message?

> @@ -3061,6 +3251,31 @@ in addition to <a>HTTP fetch</a> above.
   nullity has already been checked. The <a lt=extract for=BodyInit>extracting</a> operation cannot
   throw as it was called for the same <a for=body>source</a> before.
 
+ <li><p>Let <var>useReferredTokenBinding</var> be the result of
+   <a for=header>parsing</a> `<code>Include-Referred-Token-Binding-ID</code>`
+   in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Unset the <var>request</var>'s <a for=request>use-referred-token-binding flag</a>.
+
+ <li><p>If <var>useReferredTokenBinding</var> is a <span>byte-case-insensitive</span>
+     match for `<code>true</code>`, and the <var>request</var>'s

It might be simpler if we dropped case-insensitivity here, and just required `true`.

> @@ -3061,6 +3251,31 @@ in addition to <a>HTTP fetch</a> above.
   nullity has already been checked. The <a lt=extract for=BodyInit>extracting</a> operation cannot
   throw as it was called for the same <a for=body>source</a> before.
 
+ <li><p>Let <var>useReferredTokenBinding</var> be the result of
+   <a for=header>parsing</a> `<code>Include-Referred-Token-Binding-ID</code>`
+   in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Unset the <var>request</var>'s <a for=request>use-referred-token-binding flag</a>.
+
+ <li><p>If <var>useReferredTokenBinding</var> is a <span>byte-case-insensitive</span>
+     match for `<code>true</code>`, and the <var>request</var>'s
+     <a for=request>use-token-binding flag</a> is set, then perform the following substeps:
+
+ <ol>
+   <li><p>Set the <var>request</var>'s <a for=request>use-referred-token-binding</a> flag.
+   <li><p>Set the <var>request</var>'s <a for=request>referred-token-binding-origin</span> to
+   <var>request</var>'s <a for=request>origin</a>.

This is the origin of the original page that generated the request (e.g. A in the redirect chain `A -> B -> C -> D`). It's not clear to me that it's a good idea to allow a third-party to determine whether that ID should be exposed.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/325#pullrequestreview-24709508