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

[Eric Rescorla <[e-mail]>]

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 <[e-mail]> 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
>
>