Subject: Re: [Unbearable] AD Review: draft-ietf-tokbind-negotiation-09.txt

From: Eric Rescorla <[email-masked]>
Date: Mon, 13 Nov 2017 20:44:45 +0800

The new version is looking good. I have two minor comments:

- I still think it would be good to sharpen the discussion of version
negotiation a bit.
Perhaps: "Note that there is no way to advertise a minimum version, so a
client
advertising version N must be prepared to have the server select any older
version; if it has a minimum acceptable version, it MUST check the response
against that and generate an error if the version is too low"

Second:
"Please note that the Token Binding protocol version and key
 parameters are negotiated for each TLS connection, which means that"

You should remove "Please note" because this isn't an aside, it's a new
requirement.

I have pushed the IETF LC button, so feel free to address these later.

-Ekr

On Sun, Oct 8, 2017 at 3:59 AM, Eric Rescorla <[email-masked]> wrote:

> A rich version of this review can be found at:
>
> https://mozphab-ietf.devsvcdev.mozaws.net/D48
>
>    1. If you make an account and login, you can respond to the comments
>
> and we can try to resolve them before you produce a new draft.
>
>    1. When you're ready to produce a new draft, you can either upload
>
> it to the draft repo or send me the pre-draft and either way I'll
> take care of getting it uploaded here, so we can see diffs, etc.
>
>
> *INLINE COMMENTS*
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-326>
> draft-ietf-tokbind-negotiation.txt:102
> uint8 minor;
> } ProtocolVersion;
>
> you should note that this is taken from RFC 5246 or rename it.
>
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-327>
> draft-ietf-tokbind-negotiation.txt:110
> ProtocolVersion token_binding_version;
> TokenBindingKeyParameters key_parameters_list<1..2^8-1>
> } TokenBindingParameters;
>
> This is kind of hard to read because you are just defining the size of the
> enum here and then you have the definitions in the other draft. I think you
> should instead copy the definition from the other draft and then have a
> pointer, because as is it looks like no values are defined.
>
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-328>
> draft-ietf-tokbind-negotiation.txt:117
> client. [I-D.ietf-tokbind-protocol] describes version {1, 0} of the
> protocol.
>
> I see you are using the TLS 1.2 negotiation structures. You should
> probably add some text to make clear that this implies you are supporting
> all lower values.
>
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-329>
> draft-ietf-tokbind-negotiation.txt:158
> protocol version offered by the client in the "token_binding"
> extension and the highest version supported by the server.
>
> Given our experience with TLS negotiation, you probably need to state very
> clearly that you need to do min(client, server) version even if the client
> version is higher than you know about.
>
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-330>
> draft-ietf-tokbind-negotiation.txt:193
> extensions are not negotiated (see security considerations
> section below for more details).
>
> I would tend to think that some of these would be illegal_parameter.
>
> View Inline <https://mozphab-ietf.devsvcdev.mozaws.net/D47#inline-331>
> draft-ietf-tokbind-negotiation.txt:210
> Please note that the Token Binding protocol version and key
> parameters are negotiated for each TLS connection, which means that
>
> I would remove "please note" here, because you aren't reminding people,
> this is a separate requirement.
>
> *REPOSITORY*
> rIETFREVIEW ietf-review
>
> *REVISION DETAIL*
> https://mozphab-ietf.devsvcdev.mozaws.net/D47
>
> *EMAIL PREFERENCES*
> https://mozphab-ietf.devsvcdev.mozaws.net/settings/panel/emailpreferences/
>
> *To: *ekr-moz, ekr
>
>

---

From: Andrei Popov <[email-masked]>
Date: Mon, 13 Nov 2017 14:37:24 +0000

Sounds good; I’ll make the suggested changes in TBPROTO and TBNEGO along with any other updates that may result from LC.

Cheers,

Andrei

From: Unbearable [mailto:[email-masked]] On Behalf Of Eric Rescorla
Sent: Monday, November 13, 2017 8:45 PM
To: IETF Tokbind WG <[email-masked]>; [email-masked]
Subject: Re: [Unbearable] AD Review: draft-ietf-tokbind-negotiation-09.txt

- I still think it would be good to sharpen the discussion of version negotiation a bit.
Perhaps: "Note that there is no way to advertise a minimum version, so a client
advertising version N must be prepared to have the server select any older
version; if it has a minimum acceptable version, it MUST check the response
against that and generate an error if the version is too low"

Second:

You should remove "Please note" because this isn't an aside, it's a new requirement.

-Ekr

On Sun, Oct 8, 2017 at 3:59 AM, Eric Rescorla <[email-masked]<mailto:[email-masked]>> wrote:

https://mozphab-ietf.devsvcdev.mozaws.net/D48<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD48&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=D9HCiwSJS93SnRi6rSfxuyEW7ZzVpmZ5GVrJ3t%2Bju94%3D&reserved=0>
1.      If you make an account and login, you can respond to the comments

and we can try to resolve them before you produce a new draft.
2.      When you're ready to produce a new draft, you can either upload

INLINE COMMENTS
View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-326&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=Awfhu%2FfpvjkRxUhxUGeUeBX5M44r%2F6P1pnruF5AZDI8%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:102
uint8 minor;
} ProtocolVersion;

View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-327&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=nmwywgNFMTXKC8iNYXSJIXTgo7c6KeuUQazFmVtYkuM%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:110
ProtocolVersion token_binding_version;
TokenBindingKeyParameters key_parameters_list<1..2^8-1>
} TokenBindingParameters;

This is kind of hard to read because you are just defining the size of the enum here and then you have the definitions in the other draft. I think you should instead copy the definition from the other draft and then have a pointer, because as is it looks like no values are defined.

View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-328&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=%2FWEW3IaoGZ0vhZjxrduM%2F24TCASDxABnGzFM5mgtDVY%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:117
client. [I-D.ietf-tokbind-protocol] describes version {1, 0} of the
protocol.

I see you are using the TLS 1.2 negotiation structures. You should probably add some text to make clear that this implies you are supporting all lower values.

View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-329&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=bagPbcPyI4M%2BjTR7enU3UWIZVmtmnGZ7mDjR55PuSoQ%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:158
protocol version offered by the client in the "token_binding"
extension and the highest version supported by the server.

Given our experience with TLS negotiation, you probably need to state very clearly that you need to do min(client, server) version even if the client version is higher than you know about.

View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-330&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=5fG2KmYqj6F1a8NqK2ZLsJUNpigjXzu1dXmL7m1ql%2Fg%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:193
extensions are not negotiated (see security considerations
section below for more details).

View Inline<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47%23inline-331&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=j7Eu7dXvqsb2TESK92cVCDxp5tlgd2gTDeeH420xDYU%3D&reserved=0>draft-ietf-tokbind-negotiation.txt:210
Please note that the Token Binding protocol version and key
parameters are negotiated for each TLS connection, which means that

I would remove "please note" here, because you aren't reminding people, this is a separate requirement.

REPOSITORY
rIETFREVIEW ietf-review

REVISION DETAIL
https://mozphab-ietf.devsvcdev.mozaws.net/D47<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2FD47&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=MZ1%2BrjbUiKmRF99kzUIOC4qLqTfUpdSrW23bfL0k1uk%3D&reserved=0>

EMAIL PREFERENCES
https://mozphab-ietf.devsvcdev.mozaws.net/settings/panel/emailpreferences/<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmozphab-ietf.devsvcdev.mozaws.net%2Fsettings%2Fpanel%2Femailpreferences%2F&data=02%7C01%7CAndrei.Popov%40microsoft.com%7Cf0a714721f2e4f8d6fce08d52a946d1d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636461739339769143&sdata=%2FThiCVzQFWJ0YXuxkQLBLqenEaFfnNrjaDiO6AE08FM%3D&reserved=0>

To: ekr-moz, ekr