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

mikewest commented on this pull request.

Talked in person, here are some comments.

> @@ -1034,6 +1057,35 @@ for other values. If <cite>HTML</cite> changes here, this standard will need cor
 <dfn export for=request id=concept-request-use-url-credentials-flag>use-URL-credentials flag</dfn>.
 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 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
+<a for=/>request</a> to the server. The <a for=/>token binding ID</a> can be used by the server to,
+e.g., bind HTTP cookies or OAuth tokens that it issues to the user agent.
+
+<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=/>origin</a>.

Nit: "which is an origin or null,"

> @@ -1519,6 +1572,16 @@ for each associated <a for="fetch group">fetch record</a> whose
 <a>connection</a> is identified by an <b>origin</b> (an
 <a for=/>origin</a>) and <b>credentials</b> (a boolean).
 
+<p>A <a>connection</a> has associated
+<dfn export for=connection id=concept-tls-connection-metadata>TLS connection metadata</dfn> that consists of:
+<ul class=brief>
+ <li><a for=/>token-binding protocol version</a>, initially null.
+ <li><a for=/>token-binding key parameters</a>, initially null.
+ <li><a for=/>token-binding Exported Keying Material</a>, initially null.

I think this is clearer if you make these properties of "connection", rather than defining a metadata struct thing. That is, I'd suggest rewriting the connection pool paragraph along the lines of:

```
A <dfn>connection</dfn> is a communication channel between the user agent and an origin.

A <a>connection</a> has an associated <dfn for=connection>origin</dfn>, which is an <a for=/>origin</a>.

A <a>connection</a> has an associated <dfn for=connection>credential</dfn>, which is a boolean. Its value is `false` unless otherwise specified.

A <a>connection</a> has an associated <dfn for=connection>token-binding protocol version</dfn>, which is a string(?) or null. Its value is null unless otherwise specified.

...

A user agent has an associated <dfn>connection pool</dfn>, which is a set of <a>connections</a>.
```

Each of the "token-binding XXX" bits can link to the more detailed description in the "At a very high level" section below to describe their values.

> @@ -1538,6 +1601,27 @@ for each associated <a for="fetch group">fetch record</a> whose
     <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
     [[!TLS]]
 
+    <p>If <var>credentials</var> is true and the user agent supports <a for=/>Token Binding</a>,
+    send <a for=/>token-binding protocol version</a> and

Perhaps this is a little clearer if it reads more like "If whatever and whatever, then send X and Y while establishing the HTTP connection (as described in section 2 of whatever). In this case, if token binding negotiation succeeds, set A and B to Q and C to R."

> @@ -1538,6 +1601,27 @@ for each associated <a for="fetch group">fetch record</a> whose
     <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
     [[!TLS]]
 
+    <p>If <var>credentials</var> is true and the user agent supports <a for=/>Token Binding</a>,
+    send <a for=/>token-binding protocol version</a> and
+    <a for=/>token-binding key parameters</a> in a
+    <code>token_binding</code> Client Hello Extension, 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, set
+    <a for=/>token-binding protocol version</a> and
+    <a for=/>token-binding key parameters</a>
+    in <var>connection</var>'s <a for=connection>TLS connection metadata</a>
+    to the negotiated values,
+    and <a for=/>token-binding Exported Keying Material</a> to the computed value.

This bit is still pretty hand-wavey (What are "the negotiated values"? What is "the computed value"?). Does the negotiation spec expose more explicit algorithms we could link to to clarify the expectations?

> +using the user agent's <a for=/>token-binding key</a> store, by running these substeps:
+
+<ol>
+ <li><p>Let <var>keyDomainName</var> be null.
+
+ <li><p>If <var>tokenBindingOrigin</var>'s host is an IP address, set <var>keyDomainName</var> to
+ <var>tokenBindingOrigin</var>'s host.
+
+ <p>Otherwise, set <var>keyDomainName</var> to the registrable domain of
+ <var>tokenBindingOrigin</var>'s host, determined by running the steps in the Algorithm section of
+ <a href=https://publicsuffix.org/list/>Public Suffix List</a>
+ [[PUBLIC-SUFFIX]]
+
+ <li><p>Let <var>tokenBindingKeyPair</var> be null.
+
+ <li><p>Set <var>tokenBindingKeyPair</var> to the result of looking up the

Can you add a note to that effect?

> + <li><p>If <var>tokenBindingKeyPair</var> is null, create a new key pair for
+ <var>keyDomainName</var> using <var>tokenBindingKeyParameters</var>, save it in the
+ <a for=/>token-binding key store</a>, and set <var>tokenBindingKeyPair</var> to that new key pair.
+
+ <li><p>Return <var>tokenBindingKeyPair</var>.
+</ol>
+
+
+<h4 id=computing-token-binding-header-value>Computing a Token Binding Header Value</h4>
+
+<p><dfn abstract-op export id=concept-compute-token-binding-header-value>Compute a Token Binding header value</dfn>
+that can be passed in a `<a http-header><code>Sec-Token-Binding</code></a>`
+<a for=/>header</a> for a <a for=/>request</a>
+<var>httpRequest</var> and a <a>connection</a> <var>tlsConnection</var>,
+using the <a for=/>token-binding key store</a> and the
+<a for=connection>connection pool</a>, by running these substeps:

Can you update this to `for=/`, here and elsewhere? The pool isn't a property of the connection.

> @@ -1246,6 +1298,7 @@ or "<code>worker</code>".
     </ol>
 
    <li><p>If the ongoing fetch is <a for=fetch>terminated</a>, then abort these in-parallel steps.
+

Nit: Stray `\n`?

> @@ -1538,6 +1601,27 @@ for each associated <a for="fetch group">fetch record</a> whose
     <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
     [[!TLS]]
 
+    <p>If <var>credentials</var> is true and the user agent supports <a for=/>Token Binding</a>,
+    send <a for=/>token-binding protocol version</a> and

(Also, I think we decided to drop the "supports Token Binding" bit in an earlier iteration of this review?)

> @@ -2325,6 +2409,251 @@ X-Content-Type-Options           = "nosniff" ; case-insensitive</pre>
 <a for=request/destination>script-like</a> or "<code>style</code>" are considered as any exploits
 pertain to them. Also, considering "<code>image</code>" was not compatible with deployed content.
 
+<h3 id=token-binding>Token Binding</h3>
+
+<p>In order to protect security tokens like HTTP 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
+(a <dfn export id=concept-token-binding-key>token-binding key</dfn>) known only to a specific

Nit: I'd move the parenthetical after "specific user agent".

> @@ -2325,6 +2409,251 @@ X-Content-Type-Options           = "nosniff" ; case-insensitive</pre>
 <a for=request/destination>script-like</a> or "<code>style</code>" are considered as any exploits
 pertain to them. Also, considering "<code>image</code>" was not compatible with deployed content.
 
+<h3 id=token-binding>Token Binding</h3>
+
+<p>In order to protect security tokens like HTTP 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
+(a <dfn export id=concept-token-binding-key>token-binding key</dfn>) known only to a specific
+user agent. This association mitigates the risk that attackers can steal the token and use it
+themselves, as they will not be able to easily replicate the user agent's secret,
+and therefore will be unable to replicate the cryptographic binding of the token.
+
+<p>Details are described in TOKBIND-NEGOTIATION, TOKBIND-PROTOCOL and
+TOKBIND-HTTPS and integration is defined here.
+[[TOKBIND-NEGOTIATION]], [[TOKBIND-PROTOCOL]], and [[TOKBIND-HTTPS]].

I'd move the `[[TOKBIND-NEGOTIATION]]`, etc. into the sentence, rather than repeating them.

> +    <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, set
+    <a for=/>token-binding protocol version</a> and
+    <a for=/>token-binding key parameters</a>
+    in <var>connection</var>'s <a for=connection>TLS connection metadata</a>
+    to the negotiated values,
+    and <a for=/>token-binding Exported Keying Material</a> to the computed value.
+
+    <p class="note no-backref">
+    The user agent will use <a for=/>Token Binding</a> for any
+    <a for=/>request</a> sent over a TLS connection for which Token Binding
+    negotiation was successful.
+    <a for=/>Token Binding</a> is only proposed when <var>credentials</var>
+    is true, so connections which successfully negotiate a binding will never be
+    pooled with connections that do not include credentials.

I think we also need to note something about the coalescing behavior for H/2 connections to distinct origins. Do we use a single binding for `google.com` and `youtube.com`, for example? I think we do, given the request-specific nature of the actual key, and the connection-level nature of the version metadata, but it's worth spelling out somewhere.

> +of [[TOKBIND-PROTOCOL]].
+
+<p>At a very high level, a user agent negotiates the use of Token Binding with
+a server when it sets up a TLS connection to the server.
+It sends its highest supported
+<dfn export id=concept-token-binding-protocol-version>token-binding protocol version</dfn>
+and supported cryptographic algorithms and parameters (the
+<dfn export id=concept-token-binding-key-parameters>token-binding key parameters</dfn>)
+in a <code>token_binding</code> Client Hello Extension, 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]].
+The user agent saves the values of the <a for=/>token-binding protocol version</a> and
+<a for=/>token-binding key parameters</a> that were negotiated for the
+TLS connection in metadata for that connection.
+
+<p>The user agent uses a `<dfn http-header><code>Sec-Token-Binding</code></dfn>`

Perhaps this could be rephrased to draw out the distinction between the connection-level version/parameters, and the request-level metadata? "This connection-level metadata is used to populate a `Sec-Token-Binding` header on each HTTP request using a given connection, which contains a request-specific proof-of-possession for a given origin's token-binding key." Or something?



> +<p>In order to protect security tokens like HTTP 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
+(a <dfn export id=concept-token-binding-key>token-binding key</dfn>) known only to a specific
+user agent. This association mitigates the risk that attackers can steal the token and use it
+themselves, as they will not be able to easily replicate the user agent's secret,
+and therefore will be unable to replicate the cryptographic binding of the token.
+
+<p>Details are described in TOKBIND-NEGOTIATION, TOKBIND-PROTOCOL and
+TOKBIND-HTTPS and integration is defined here.
+[[TOKBIND-NEGOTIATION]], [[TOKBIND-PROTOCOL]], and [[TOKBIND-HTTPS]].
+
+<p>A <dfn export id=token-binding-id>token binding ID</dfn> is the non-secret representation
+of a <a for=/>token-binding key</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]].

Is it worth noting that the key is site-specific? (The key is site-specific, right?)

> @@ -3237,6 +3566,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> `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`

Nit: "extracting header list values given `Header-Name` and response’s header list."

> @@ -3237,6 +3566,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> `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`
+ in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Set the <var>request</var>'s <a for=request>referred-token-binding origin</a> to null.
+
+ <li><p>If <var>useReferredTokenBinding</var> is `<code>true</code>`

"extracting header list values" returns a list, which allows you to define the behavior if multiple values are returned. Do we reject the response? Use the first/last value?

> @@ -3237,6 +3566,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> `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`
+ in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Set the <var>request</var>'s <a for=request>referred-token-binding origin</a> to null.
+
+ <li><p>If <var>useReferredTokenBinding</var> is `<code>true</code>`
+ and the <var>request</var>'s <a for=request>use-token-binding flag</a> is set,
+ then set the <var>request</var>'s <a for=request>referred-token-binding origin</a> to
+  <var>request</var>'s <a for=request>origin</a>.

I'm not sure this works the way you want it to; perhaps the origin of the request's current URL? Need to trace this down, as the redirect behavior for the requests' origin is complicated.

> @@ -3237,6 +3566,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> `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`
+ in <var>actualResponse</var>'s <a for=response>header list</a>.
+
+ <li><p>Set the <var>request</var>'s <a for=request>referred-token-binding origin</a> to null.
+
+ <li><p>If <var>useReferredTokenBinding</var> is `<code>true</code>`
+ and the <var>request</var>'s <a for=request>use-token-binding flag</a> is set,
+ then set the <var>request</var>'s <a for=request>referred-token-binding origin</a> to
+  <var>request</var>'s <a for=request>origin</a>.

Also: how does this work with `sandbox`? Or, worse, `Content-Security-Policy: sandbox`? We don't yet have a document when processing this header so the current spec text won't modify the relevant origin. Is that reasonable?

> +The <a for=/>token-binding key store</a> is typically maintained alongside
+the user agent's cookie store.
+
+<h4 id=getting-token-binding-key>Getting a Token Binding Key</h4>
+
+<p><dfn abstract-op export id=concept-get-token-binding-key>Get the token-binding key</dfn>
+for an <a for=/>origin</a> <var>tokenBindingOrigin</var> and
+<a for=/>token-binding key parameters</a> <var>tokenBindingKeyParameters</var>,
+using the user agent's <a for=/>token-binding key store</a>,
+by running these substeps:
+
+<ol>
+ <li><p>Let <var>keyDomainName</var> be null.
+
+ <li><p>If <var>tokenBindingOrigin</var>'s <a for=origin>host</a> is an <a>IPv4 address</a> or an <a>IPv6 address</a>,
+ set <var>keyDomainName</var> to <var>tokenBindingOrigin</var>'s <a for=origin>host</a>.

Do we actually need to support IP addresses? Or could we punt on them, preferring hostnames instead?

> +
+<h4 id=federated-token-binding>Federated Token Binding</h4>
+
+<p>In the scenario described above, servers bind tokens to be used by user agents
+with those servers.
+To enable existing web authorization protocols to use bound credentials,
+there is a need to support scenarios where bound credentials issued by one server
+(e.g., an identity provider) are presented by the user agent to a different server
+(e.g., a relying party).
+[[TOKBIND-PROTOCOL]] and [[TOKBIND-HTTPS]] describe support for Federated Token Binding.
+
+<p>A <dfn export id=referred-token-binding-id>referred-token-binding ID</dfn> is the
+<a for=/>token binding ID</a> for a server other than the server to which the
+<a for=/>request</a> is being sent.
+
+Script code from an <a for=/>origin</a> can set a <a for=/>request</a>'s

It would be helpful to point to the script you're talking about here. Either via an example, or at least a pointer to the requestinit property.

> +
+  <li><p><a for=/>token-binding protocol version</a> in <var>tlsConnection</var>'s
+ <a for=connection>TLS connection metadata</a> is null.
+ </ol>
+
+ <li><p>Let <var>providedTokenBindingKeyPair</var> be the result of
+ <a abstract-op lt="Get the token-binding key">getting the token-binding key</a>
+ for the <a for=/>origin</a> of <var>httpRequest</var>'s <a for=request>current url</a>
+ and the <a for=/>token-binding key parameters</a> in <var>tlsConnection</var>'s
+ <a for=connection>TLS connection metadata</a>.
+
+ <li><p>Let <var>providedTokenBindingId</var> be the result of computing a
+ <a for=/>token binding ID</a> from <var>providedTokenBindingKeyPair</var>,
+ as described in
+ <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.2">section 3.2</a>
+ of [[!TOKBIND-PROTOCOL]].

As discussed in person, this reference doesn't clearly describe to me what I need to be doing to compute the token binding ID from the key pair I have access to here.

-- 
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-98842249