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

nharper requested changes on this pull request.



> +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 for=connection>token-binding protocol version</dfn>
+and supported cryptographic algorithms and parameters (the
+<dfn export id=concept-token-binding-key-parameters for=connection>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=connection>token-binding protocol version</a> and
+<a for=connection>token-binding key parameters</a> that were negotiated for
+that TLS connection.  The user agent also computes and saves the value of
+<a for=connection>token-binding Exported Keying Material</a> for that connection.

This is basically a repeat of what was said in the Obtaining a Connection section. Reference that section instead of repeating it.

> + containing a signature (using <var>providedTokenBindingKeyPair</var>) over the
+ <var>providedTokenBindingId</var> as well as <var>tlsConnection</var>'s
+ <a for=connection>token-binding Exported Keying Material</a>, as described in
+ <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.3">section 3.3</a>
+ of [[!TOKBIND-PROTOCOL]].
+
+ <li><p>Set <var>tokenBindingMessage</var> to a list containing <var>providedTokenBinding</var>
+ as the only <a for=/>token binding</a>.
+
+ <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
+   <a lt="obtain a connection" for=connection>obtaining a connection</a> for
+   <var>httpRequest</var>'s <a for=request>referred-token-binding origin</a>
+   with credentials <code>true</code> from the <a>connection pool</a>.

I don't like that `referredTlsConnection` is the result of obtaining a connection - there's no need to open a new connection for referred Token Bindings. (In the HTTP redirect `Include-Referred-Token-Binding-ID` header case, there _should_ already be a connection in the pool since that header was just received over that connection. However, in the `fetch` JS API case, the connection could be gone at this point, so there's no guarantee that this doesn't require opening a new connection.) All that `referredTlsConnection` is needed for is the key parameter.

Instead, I'd structure this so that "getting the token-binding key" only takes an origin as input, and returns a key and key parameters (it could return a tuple, or it could return just a key and the key parameters is a property of the key).

> @@ -1512,18 +1563,38 @@ for each associated <a for="fetch group">fetch record</a> whose
 
 <h3 id=connections>Connections</h3>
 
+<p>A <dfn export id=concept-connection>connection</dfn> is a communication channel between the user agent and an origin.
+
+<p>A <a>connection</a> has an associated <dfn for=connection>origin</dfn>, which is an <a for=/>origin</a>.
+
+<p>A <a>connection</a> has an associated <dfn for=connection>credential</dfn>, which is a boolean. Its value is `false` unless otherwise specified.
+
+<p>A <a>connection</a> has an associated <a for=connection>token-binding protocol version</a>,
+which is a byte sequence.
+Its value is null unless otherwise specified.

As far as I can tell, the only time `token-binding protocol version` gets set is after Token Binding gets negotiated. But Token Binding only gets negotiated if `token-binding protocol version` is set, meaning the effect is that this PR does nothing.

> @@ -1538,6 +1609,35 @@ 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>Send the highest supported
+    <a for=connection>token-binding protocol version</a> and
+    all supported <a for=connection>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]].

I'd leave out explicitly stating to set the `token-binding protocol version` and `token-binding key parameters` in the ClientHello (nit: one word) Extension. Those parameters default to null in this spec and are only set after negotiation. Right now there is nothing that says how to initially set them (e.g. to [0,13] and 2, and I don't think that this spec should say what to set them, leaving that up to the UA.

I think leaving it vague for what exactly to use as the version and key params is in line with how this spec does not specify TLS versions or cipher suites to use.

> +<a for=/>header</a> on each HTTP request sent using that connection.
+The header contains the user agent's proof-of-possession for a given
+origin's <a for=/>token-binding key</a>.
+(The user agent proves possession of the private key by putting a cryptographic
+signature in that header.)
+
+<p>The server associates ('binds') credentials that it issues to that user agent
+with a <a for=/>token binding ID</a> in that <a for=/>header</a>.
+The server also verifies if bound credentials presented to it by a user agent
+match a <a for=/>token binding ID</a> in that <a for=/>header</a>.
+
+<p>The <a for=header>value</a> of the `<a http-header><code>Sec-Token-Binding</code></a>`
+<a for=/>header</a> is a base64url-encoded string [[!RFC4648]]:
+
+<pre>
+Sec-Token-Binding       = 1*( ALPHA / DIGIT / "-" / "_" ) *2( "=" )

This is missing a reference that this is ABNF (or that it is some other format). Also, the Sec-Token-Binding header has no padding, so the bit with the `=` is wrong. Why does this definition need to be here at all instead of referencing the one in draft-ietf-tokbind-https?

> +<code>https://rp.org/</code> to the target <a for=request>origin</a>
+<code>https://idp.org/</code>.
+The user agent discloses the <a for=/>token binding ID</a> by including a
+<a for=/>referred-token-binding ID</a> for <a for=request>origin</a>
+<code>https://rp.org/</code> (in addition to the <a for=/>token binding ID</a>
+for <a for=request>origin</a> <code>https://idp.org/</code>)
+in the <a for=/>Token Binding Message</a> that is created for the
+<a for=/>request</a> to <code>https://idp.org/</code>.
+
+<p>Alternately, servers that redirect a user agent to a different server can use the
+`<dfn http-header><code>Include-Referred-Token-Binding-ID</code></dfn>`
+response <a for=/>header</a>.
+
+<pre>
+Include-Referred-Token-Binding-ID = %x74.72.75.65 ; "true", case-sensitive
+</pre>

I don't think this definition is needed here (it's in TOKBIND-HTTPS mentioned below). If this is left in, define the format (presumably ABNF).

> @@ -1566,6 +1641,35 @@ 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>Send the highest supported
+    <a for=connection>token-binding protocol version</a> and
+    all supported <a for=connection>token-binding key parameters</a> in a

This says all supported `token-binding key parameters`, but in the connection section it says `token-binding key parameters` is only one byte. Is it a list of parameters, or only a single parameter?

> @@ -61,6 +61,19 @@ url: https://tools.ietf.org/html/rfc7234#section-1.2.1;text:delta-seconds;type:d
         "publisher": "IETF",
         "title": "HTTP Client Hints"
     },
+    "DATAURL": {
+        "authors": ["Simon Sapin"],
+        "href": "https://simonsapin.github.io/data-urls/",
+        "title": "The data URL scheme"
+    },

This addition isn't referenced in this PR anywhere.

> + [[PUBLIC-SUFFIX]]
+
+ <li><p>Let <var>tokenBindingKeyPair</var> be null.
+
+ <li><p>Set <var>tokenBindingKeyPair</var> to the result of looking up the
+ <a for=/>token-binding key</a> pair for <var>keyDomainName</var> and
+ <var>tokenBindingKeyParameters</var> in the <a for=/>token-binding key store</a>.
+
+ <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-id>Computing a Token Binding Id</h4>

This section feels like it repeats too much from TOKBIND-PROTOCOL, and at the same time doesn't provide enough detail (e.g. in step 1 it says "let _publicKeyData_ be a `Uint8Array` object containing the public key of token-binding key pair `tokenBindingKeyPair`", yet there's no description of how to get a serialize a key pair to a `Uint8Array` - that's a detail described in TOKBIND-PROTOCOL).

Instead of trying to provide this amount of detail, I'd defer it all to TOKBIND-PROTOCOL, and match up variables in this spec with struct fields in section 3 of TOKBIND-PROTOCOL. For example, you could say something like "return the serialization of the `TokenBindingID` struct from section 3 of TOKBIND-PROTOCOL, where the `TokenBindingID.key_parameters` field is set to `tokenBindingKeyParameters` and `TokenBindingID.TokenBindingPublicKey` set to `tokenBindingKeyPair`." @mikewest does something like that sound reasonable, or do you think the amount of detail provided here is needed in the Fetch spec?

> + for the <a for=/>origin</a> of <var>httpRequest</var>'s <a for=request>current url</a>
+ and <var>providedTokenBindingKeyParameters</var>.
+
+ <li><p>Let <var>providedTokenBindingId</var> be the result of
+ <a abstract-op lt="Compute a Token Binding ID">computing a token binding ID</a>
+from <var>providedTokenBindingKeyPair</var> and
+<var>providedTokenBindingKeyParameters</var>.
+
+ <li><p>Let <var>providedTokenBinding</var> be the result of computing a <a for=/>token binding</a>
+ using <var>providedTokenBindingId</var> and <a for=/>token-binding type</a>
+ <code>provided_token_binding</code>,
+ containing a signature (using <var>providedTokenBindingKeyPair</var>) over the
+ <var>providedTokenBindingId</var> as well as <var>tlsConnection</var>'s
+ <a for=connection>token-binding Exported Keying Material</a>, as described in
+ <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.3">section 3.3</a>
+ of [[!TOKBIND-PROTOCOL]].

This step is poorly defined. It's unclear what type _providedTokenBinding_ is (it says it's the result of computing a token binding, which would make me think "token binding" is the type, yet that's a concept not a type).

I would word this as something like the following:

Let _providedTokenBinding_ be the result of serializing a TokenBinding struct as defined in section 3 of TOKBIND-PROTOCOL, with `TokenBinding.tokenbinding_type` set to (insert appropriate variable here) `TokenBinding.tokenbindingid` set to (insert appropriate variable here), and `TokenBinding.signature` computed according to section 3.3 of TOKBIND-PROTOCOL, using (insert appropriate variables here).

> +
+     <li><p>Let <var>referredTokenBindingId</var> be the result of
+     <a abstract-op lt="Compute a Token Binding ID">computing a token binding ID</a>
+     from <var>referredTokenBindingKeyPair</var> and
+     <var>referredTokenBindingKeyParameters</var>.
+
+     <li><p>Let <var>referredTokenBinding</var> be the result of computing a
+     <a for=/>token binding</a> using <var>referredTokenBindingId</var> and
+     <a for=/>token-binding type</a>
+     <code>referred_token_binding</code>, containing a signature
+     (using <var>referredTokenBindingKeyPair</var>) over the <var>referredTokenBindingId</var>
+     as well as <var>tlsConnection</var>'s
+     <a for=connection>token-binding Exported Keying Material</a>, as described in
+     <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.3">section 3.3</a>
+     of [[!TOKBIND-PROTOCOL]].
+    </ol>

Ditto about this being poorly defined. Instead of effectively repeating the substeps from above, would it make sense to define a routine that takes a key, a type (provided or referred), and an EKM value, and returns a TokenBinding struct that can be used here and above? Perhaps replace the "Computing a token binding ID" routine with one that computes a TokenBinding struct (or maybe its serialization)?

> +     <code>referred_token_binding</code>, containing a signature
+     (using <var>referredTokenBindingKeyPair</var>) over the <var>referredTokenBindingId</var>
+     as well as <var>tlsConnection</var>'s
+     <a for=connection>token-binding Exported Keying Material</a>, as described in
+     <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3.3">section 3.3</a>
+     of [[!TOKBIND-PROTOCOL]].
+    </ol>
+
+   <li><p>If <var>referredTokenBinding</var> is not null, <a for=list>append</a>
+   it to <var>tokenBindingMessage</var>.
+  </ol>
+
+  <li><p>If <var>tokenBindingMessage</var> is not null, compute and return its base64url-encoding
+  [[!RFC4648]] as described in
+  <a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3">section 3</a>
+  of [[!TOKBIND-PROTOCOL]]. Otherwise return null.

What is the definition of a base64url-encoding of a list? Again, I'd defer to TOKBIND-PROTOCOL for this. I don't think that what you've describe here will result in a properly serialized TokenBindingMessage struct.

> +</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>connection pool</a>, by running these substeps:
+
+<ol>
+ <li><p>Return null if any of the following are true.
+
+ <ol>
+  <li><p><var>httpRequest</var>'s <a for=request>use-token-binding flag</a> is not set.

The "Computing a Token Binding Header Value" routine defined here is only called in one place, which already checked the use-token-binding flag. This check could probably be left out here, but I also don't mind a belt and suspenders check.

> @@ -3849,7 +4301,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
   <ol>
    <li><p>Let <var>aborted</var> be the termination's aborted flag.
 
-   <li><p>If <var>connection</var> uses HTTP/2, then transmit an <code>RST_STREAM</code> frame.
+   <li><p>If <var>connection</var> is an HTTP/2 connection, then transmit an
+   "<code>RST_STREAM</code>" to cancel the underlying stream.

Nit: extraneous change. Is a rebase needed?

> @@ -4010,7 +4463,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
    <li><p>Otherwise, if <var>stream</var> is <a for=ReadableStream>readable</a>,
    <a abstract-op>error</a> <var>stream</var> with a <code>TypeError</code>.
 
-   <li><p>If <var>connection</var> uses HTTP/2, then transmit an <code>RST_STREAM</code> frame.
+   <li><p>If <var>connection</var> is an HTTP/2 connection, then transmit an
+   "<code>RST_STREAM</code>" to cancel the underlying stream.

Ditto

> @@ -4799,7 +5253,7 @@ runs the associated steps:
   <p>Otherwise, <a>throw</a> a <code>TypeError</code>.
 
  <dt><i>JSON</i>
- <dd><p>Return the result of running <a>parse JSON from bytes</a> on <var>bytes</var>.
+ <dd><p>Return the result of running <a>parse JSON with bytes</a> on <var>bytes</var>.

Ditto

> @@ -5223,6 +5688,10 @@ constructor must run these steps:
  <code>credentials</code> member if it is present, and
  <var>fallbackCredentials</var> otherwise.
 
+ <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>.
+

Nit: move this below the next list item so that the credentials bits stay together.

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