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

nharper requested changes on this pull request.

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

1) 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.

2) 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.

> +<var>providedTokenBindingKeyParameters</var>.
+
+ <li><p>Let <var>providedTokenBinding</var> be the result of
+<a abstract-op lt="Compute a Token Binding">computing a token binding</a>
+using <a for=/>token-binding type</a> <code>provided_token_binding</code>,
+<var>providedTokenBindingId</var>, <var>providedTokenBindingKeyPair</var>,
+and <var>tlsConnection</var>'s
+<a for=connection>token-binding Exported Keying Material</a>.
+
+ <li><p>Append <var>providedTokenBinding</var> to
+ <var>tokenBindingMessage</var>'s <code>TokenBindingMessage.tokenbindings</code> field.
+
+ <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.)

> +Separate Token Binding messages (generated using different token-binding
+keys) will be sent as headers in the requests to the different origins.
+This implies that all origins that can be coalesced into the same
+HTTP/2 connection need to support the same Token Binding protocol
+version and key parameters.
+
+<h4 id=negotiating-token-binding>Negotiating Token Binding</h4>
+
+The user agent proposes and agrees to the use of Token Binding 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]].
+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 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.)

> +<h4 id=using-federated-token-binding>Using 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
+<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.

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

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