Update Token Binding code to the latest drafts

Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version 10.

BUG=467312
Committed: https://crrev.com/78e6d2b3325b16d8b637b9457a775557e1ec7e0e
Cr-Commit-Position: refs/heads/master@{#419981}

[nharper, 2016-09-13 20:54:00]

The CQ bit was checked by nharper@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 20:54:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nharper, 2016-09-13 20:59:51]

Description was changed from

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.

BUG=467312
==========

to

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

BUG=467312
==========

[nharper, 2016-09-13 20:59:51]

nharper@chromium.org changed reviewers:
+ davidben@chromium.org

[commit-bot: I haz the power, 2016-09-13 21:14:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 21:14:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2016-09-19 21:24:14]

lgtm with comments

https://codereview.chromium.org/2337253004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:811:
crypto_config_.tb_key_params.push_back(kTB10);
Mention the change in the commit message.

https://codereview.chromium.org/2337253004/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:561: TokenBindingSignatureMap::iterator it
=
Optional: auto?

https://codereview.chromium.org/2337253004/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2337253004/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:419: // puts the result in |*out|. Returns OK or
ERR_FAILED.
Did you mean to remove the old comment?

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.cc
File net/ssl/token_binding.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:38: CBB_add_u8_length_prefixed(&public_key, &ec_point)
&&
I... okay. They could also have said:

  struct {
      TokenBindingKeyParameters key_parameters;
      opaque key<1..2^16-1>;
  } TokenBindingID;

Then elsewhere you say:

This specification defines the following key types and their serializations.
rsa2048_pkcs1.5 and rsa2048_pss use an RSAPublicKey. ecdsap256 uses such and
such encoding.

That's how we avoid the extra length prefix in TLS 1.3.

PS: If we're stuck with TB thinking it's the center of the universe and defining
custom serializations, they should at least rename RSAPublicKey since
RSAPublicKey is the name of an ASN.1 type. But I also think we should very
strongly prefer that TB not get in the key serialization game. ECPoint should
also be renamed because ECPoint is the name of a TLS structure that's different
(it includes the leading byte).

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:163: !CBS_get_u8_length_prefixed(&public_key,
&ec_point) ||
CBS_len(&public_key) != 0 ||

[nharper, 2016-09-19 22:17:02]

nharper@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[nharper, 2016-09-19 22:18:33]

Description was changed from

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

BUG=467312
==========

to

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version
10.

BUG=467312
==========

[nharper, 2016-09-19 22:32:23]

ptal

https://codereview.chromium.org/2337253004/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:811:
crypto_config_.tb_key_params.push_back(kTB10);
On 2016/09/19 21:24:14, davidben wrote:
> Mention the change in the commit message.

Done.

https://codereview.chromium.org/2337253004/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:561: TokenBindingSignatureMap::iterator it
=
On 2016/09/19 21:24:14, davidben wrote:
> Optional: auto?

Done.

https://codereview.chromium.org/2337253004/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2337253004/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:419: // puts the result in |*out|. Returns OK or
ERR_FAILED.
On 2016/09/19 21:24:14, davidben wrote:
> Did you mean to remove the old comment?

Yes. Removed.

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.cc
File net/ssl/token_binding.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:38: CBB_add_u8_length_prefixed(&public_key, &ec_point)
&&
On 2016/09/19 21:24:14, davidben wrote:
> I... okay. They could also have said:
> 
>   struct {
>       TokenBindingKeyParameters key_parameters;
>       opaque key<1..2^16-1>;
>   } TokenBindingID;
> 
> Then elsewhere you say:
> 
> This specification defines the following key types and their serializations.
> rsa2048_pkcs1.5 and rsa2048_pss use an RSAPublicKey. ecdsap256 uses such and
> such encoding.
> 
> That's how we avoid the extra length prefix in TLS 1.3.

Maybe that can be changed in a future draft.
> 
> PS: If we're stuck with TB thinking it's the center of the universe and
defining
> custom serializations, they should at least rename RSAPublicKey since
> RSAPublicKey is the name of an ASN.1 type. But I also think we should very
> strongly prefer that TB not get in the key serialization game. ECPoint should
> also be renamed because ECPoint is the name of a TLS structure that's
different
> (it includes the leading byte).

You can open an issue at https://github.com/TokenBinding/Internet-Drafts/issues
about the naming of RSAPublicKey if you want.

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:163: !CBS_get_u8_length_prefixed(&public_key,
&ec_point) ||
On 2016/09/19 21:24:14, davidben wrote:
> CBS_len(&public_key) != 0 ||

CBS_get_u8_length_prefixed will return 0 (false) if public_key is too short, and
the whole thing will short circuit if the line above fails (which would result
in an uninitialized public_key, and anyway calling CBS_len() on a null cbs will
still de-ref it), so I don't see any use for this check.

[davidben, 2016-09-19 22:35:40]

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.cc
File net/ssl/token_binding.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:163: !CBS_get_u8_length_prefixed(&public_key,
&ec_point) ||
On 2016/09/19 22:32:23, nharper wrote:
> On 2016/09/19 21:24:14, davidben wrote:
> > CBS_len(&public_key) != 0 ||
> 
> CBS_get_u8_length_prefixed will return 0 (false) if public_key is too short,
and
> the whole thing will short circuit if the line above fails (which would result
> in an uninitialized public_key, and anyway calling CBS_len() on a null cbs
will
> still de-ref it), so I don't see any use for this check.

It's to ensure there isn't garbage trailing the ECPoint in public_key.

[nharper, 2016-09-19 22:45:06]

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.cc
File net/ssl/token_binding.cc (right):

https://codereview.chromium.org/2337253004/diff/20001/net/ssl/token_binding.c...
net/ssl/token_binding.cc:163: !CBS_get_u8_length_prefixed(&public_key,
&ec_point) ||
On 2016/09/19 22:35:40, davidben wrote:
> On 2016/09/19 22:32:23, nharper wrote:
> > On 2016/09/19 21:24:14, davidben wrote:
> > > CBS_len(&public_key) != 0 ||
> > 
> > CBS_get_u8_length_prefixed will return 0 (false) if public_key is too short,
> and
> > the whole thing will short circuit if the line above fails (which would
result
> > in an uninitialized public_key, and anyway calling CBS_len() on a null cbs
> will
> > still de-ref it), so I don't see any use for this check.
> 
> It's to ensure there isn't garbage trailing the ECPoint in public_key.

Oh! that makes sense. Done.

[Devlin, 2016-09-20 00:30:19]

extensions lgtm

[davidben, 2016-09-20 18:39:16]

(still lgtm)

[nharper, 2016-09-20 22:34:15]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2016-09-20 22:35:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 02:12:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 02:12:12]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nharper, 2016-09-21 04:03:19]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2016-09-21 04:03:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 05:42:54]

Description was changed from

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version
10.

BUG=467312
==========

to

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version
10.

BUG=467312
==========

[commit-bot: I haz the power, 2016-09-21 05:42:55]

Committed patchset #5 (id:70041)

[commit-bot: I haz the power, 2016-09-21 05:45:34]

Description was changed from

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version
10.

BUG=467312
==========

to

==========
Update Token Binding code to the latest drafts

draft-ietf-tokbind-protocol-10:
- The TokenBinding.signature now closes over the token binding type.
- Public key serialization is length prefixed to support parsing unknown key
types.
draft-ietf-tokbind-negotiation-05:
- Require Renegotiation Indication
draft-ietf-tokbind-https-06:
- Sec-Token-Binding header base64 shouldn't include pad characters.
- Include-Referred-Token-Binding-ID header's value is case insensitive.

This also updates the tls and quic code to indicate support for draft version
10.

BUG=467312

Committed: https://crrev.com/78e6d2b3325b16d8b637b9457a775557e1ec7e0e
Cr-Commit-Position: refs/heads/master@{#419981}
==========

[commit-bot: I haz the power, 2016-09-21 05:45:35]

Patchset 5 (id:??) landed as
https://crrev.com/78e6d2b3325b16d8b637b9457a775557e1ec7e0e
Cr-Commit-Position: refs/heads/master@{#419981}