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

equalsJeffH commented on this pull request.

Overall this looks good. Editorial comments below. I did not find any substantive technical issues. 

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.



> @@ -61,6 +61,14 @@ url: https://tools.ietf.org/html/rfc7234#section-1.2.1;text:delta-seconds;type:d
         "publisher": "IETF",
         "title": "HTTP Client Hints"
     },
+    "EXPECT-CT": {
+        "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"

> @@ -1078,6 +1101,34 @@ which is "<code>omit</code>", "<code>same-origin</code>", or
 <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

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

> @@ -1078,6 +1101,34 @@ which is "<code>omit</code>", "<code>same-origin</code>", or
 <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

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

> +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> or null. Unless stated otherwise, it is null.
+
+<p class="note no-backref">When <a>request</a>'s
+<a for=request>referred-token-binding origin</a> is not null,
+it is used to compute the <a for=/>referred-token-binding ID</a>.
+The user agent will send the
+<a for=/>token binding ID</a> used for that <a for=request>origin</a>,
+in addition to the <a for=/>token binding ID</a> used 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. 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?)

> @@ -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 ?

> @@ -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
+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
+(a <dfn export id=concept-token-binding-key>token-binding key</dfn>).
+The user agent uses a different <a for=/>token-binding key</a>
+for every different <a for=/>registrable domain</a>.
+The association of key and token 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.

I'd add a Note or statement to the effect that this guarantee holds only insofar as servers verify such cryptographic bindings and fail if verification fails.

> +<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
+known only to a specific user agent
+(a <dfn export id=concept-token-binding-key>token-binding key</dfn>).
+The user agent uses a different <a for=/>token-binding key</a>
+for every different <a for=/>registrable domain</a>.
+The association of key and token 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.

suggest:
OLD: here

NEW: in the following sections.

> +of the Token Binding Negotiation spec [[!TOKBIND-NEGOTIATION]].
+While setting up a TLS <a>connection</a>, it sends its highest supported
+<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 a <code>token_binding</code> TLS Extension in the ClientHello.
+If the server is willing to use Token Binding for the <a>connection</a>,
+it responds with a <code>token_binding</code> TLS Extension in the ServerHello.
+That extension specifies the
+<a for=connection>token-binding protocol version</a> and
+<a for=connection>token-binding key parameters</a> that will be used
+for Token Binding over that <a>connection</a>.
+
+<p>The user agent uses the negotiated values to update connection-level
+metadata for the <a>connection</a>.  Specifically, it saves the

wrt the term:
> connection-level metadata

I would change the latter term to be "connection metadata" and \<dfn> it up in the https://fetch.spec.whatwg.org/#connections section, and link to it when used.

> +
+<p>The user agent uses the negotiated values to update connection-level
+metadata for the <a>connection</a>.  Specifically, it saves the
+<a for=connection>token-binding protocol version</a> and
+<a for=connection>token-binding key parameters</a>
+from the <code>token_binding</code> TLS Extension in the ServerHello.
+It also computes and saves the value of
+<a for=connection>token-binding Exported Keying Material</a> for that
+<a>connection</a>.
+
+<h4 id=using-token-binding>Using Token Binding</h4>
+
+<p>The connection-level metadata described above is used generate a
+`<dfn http-header><code>Sec-Token-Binding</code></dfn>` request
+<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

suggest:
OLD: a given 

NEW: the request's

> +
+<p>The connection-level metadata described above is used generate a
+`<dfn http-header><code>Sec-Token-Binding</code></dfn>` request
+<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]].

wrt the term:
> base64url-encoded string

there's more technical details to the definition than just "base64url-encoded string". I would not put the details nor ABNF here, I'd just refer to:
https://tools.ietf.org/html/draft-ietf-tokbind-https#section-2

> +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]].
+<p><a>ABNF</a> for the <a for=header>value</a> is:
+
+<pre>
+Sec-Token-Binding       = 1*( ALPHA / DIGIT / "-" / "_" )
+</pre>
+
+<p>The data in a `<a http-header><code>Sec-Token-Binding</code></a>` <a for=/>header</a> is a
+<dfn export id=token-binding-message>Token Binding Message</dfn> as specified in
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-https#section-2">section 2</a>
+of the Token Binding over HTTP spec [[!TOKBIND-HTTPS]].

s/HTTP/HTTPS/

> +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.
+
+<p>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>.
+
+<p><a>ABNF</a> for the <a for=header>value</a> is:

wrt the ABNF, again, I would not include this level of detail in this spec -- it's already specified in https://tools.ietf.org/html/draft-ietf-tokbind-https#section-5.3

> +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.
+
+<p>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>.
+
+<p><a>ABNF</a> for the <a for=header>value</a> is:
+<pre>
+Include-Referred-Token-Binding-ID = %x74.72.75.65 ; "true", case-sensitive

for example, it is actually _case-insensitive_, see https://tools.ietf.org/html/draft-ietf-tokbind-https#section-5.3

> +<code>https://idp.org/</code>, the <a for=request>origin</a> that
+issues the redirect <a for=/>response</a> (say <code>https://rp.org/</code> -
+the "referring <a for=request>origin</a>" in the <a for=/>Token Binding</a>
+sense) tells the user agent to disclose the <a for=/>token binding ID</a>
+used by the user agent for its <a for=request>origin</a>
+<code>https://rp.org/</code> to the target <a for=request>origin</a>
+<code>https://idp.org/</code>.
+The user agent discloses that <a for=/>token binding ID</a> while processing
+the `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`
+<a for=/>header</a> in the redirect
+<a for=/>response</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>.

Even though it seems nominally correct, I find the above parag tough to parse and understand. Perhaps it should be 3 or 4 sentences rather than just 2 ?

> @@ -1668,6 +1738,18 @@ 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>Propose the use of <a href=#token-binding>Token Binding</a>

suggest:
Propose -> Negotiate

> @@ -1668,6 +1738,18 @@ 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>Propose the use of <a href=#token-binding>Token Binding</a>
+    for <var>connection</var> as described in
+    <a href=#negotiating-token-binding>Negotiating Token Binding</a>.

bikeshed will make nice intra-document links if you compose them like: `[[#negotiating-token-binding]]`  (tho fetch.bs does not seem to use that intra-doc linking approach?)

> +<code>https://idp.org/</code>.
+The user agent discloses that <a for=/>token binding ID</a> while processing
+the `<a http-header><code>Include-Referred-Token-Binding-ID</code></a>`
+<a for=/>header</a> in the redirect
+<a for=/>response</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>.
+
+<h4 id=token-binding-key-store>Token Binding Key Store</h4>
+
+<p>The user agent maintains a
+<dfn id=concept-token-binding-key-store/>token-binding key store</dfn>,
+where it saves different <a for=/ lt="token-binding key">token-binding keys</a> to be used with different servers.

suggest:
OLD: `it saves different <a for=/ lt="token-binding key">token-binding keys</a> to be used with different servers` 

NEW: `it stores <a for=/ lt="token-binding key">token-binding keys</a> indexed by <a for=/>connection</a>'s <a for=connection>origin</a>'s <a for=origin>host</a> and <a for=connection>token-binding key parameters</a>`

..since that's how the following  "getting a token binding key" alg is looking them up.

> +<a for=/>response</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>.
+
+<h4 id=token-binding-key-store>Token Binding Key Store</h4>
+
+<p>The user agent maintains a
+<dfn id=concept-token-binding-key-store/>token-binding key store</dfn>,
+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.
+The <a for=/>token-binding key store</a> is typically maintained alongside

is this last sentence needed?  offhand it does not seem pertinent. 

> +<a for=/>header</a> in the redirect
+<a for=/>response</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>.
+
+<h4 id=token-binding-key-store>Token Binding Key Store</h4>
+
+<p>The user agent maintains a
+<dfn id=concept-token-binding-key-store/>token-binding key store</dfn>,
+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`

> +<a for=/>token binding ID</a> <var>tokenBindingId</var>,
+<a for=/>token-binding key</a> pair <var>tokenBindingKeyPair</var>,
+and <a for=connection>token-binding Exported Keying Material</a>
+<var>tokenBindingEKM</var>,
+by running these substeps:
+
+<ol>
+ <li>Let <var>tokenBinding</var> be a <code>TokenBinding</code> struct from
+<a href="https://tools.ietf.org/html/draft-ietf-tokbind-protocol#section-3">section 3</a>
+ of [[!TOKBIND-PROTOCOL]].
+ <li>Let <var>tokenBinding</var>'s <code>TokenBinding.tokenbinding_type</code>
+ field be set to <var>tokenBindingType</var>, and
+ <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

> @@ -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"

> @@ -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>`
+   request <a for=/>header</a> by running the following substeps:
+
+    <ol>
+     <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

> @@ -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>`
+   request <a for=/>header</a> by running the following substeps:
+
+    <ol>
+     <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
+     <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>` 

-- 
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/715#pullrequestreview-140826499