-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MTI Extensions (incl. issue #116) #201
Conversation
15e387e
to
cea28ca
Compare
implementation. | ||
The use of MD5, SHA-1, and SHA-224 are deprecated. The md5_RESERVED, | ||
sha1_RESERVED, and sha224_RESERVED values MUST NOT be offered by | ||
any implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the implementation is willing to negotiate TLSv1.2 connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right; there's a potential issue here. TLS 1.3 clients with this provision would offer SHA-256 and TLS 1.2 servers would either send a SHA-256 chain if they have one, send a chain with SHA-1 instead, or possibly abort. Based on recent discussion, yes, we would need to check what various implementations would do here. If they abort, I'd have to change this to allow sending SHA-1 support but not negotiating it for TLS 1.3.
@tomato42: I pushed a new commit here to address your comments. I've changed SHA1 to a SHOULD NOT except for backwards compatibility and added explicit error cases for if the other two extensions are missing (better explicit than implicit). |
ed92b73
to
0c4ec04
Compare
alert message and close the connection. | ||
If no suitable identity is present, the server MUST NOT negotiate | ||
a PSK cipher suite and MAY respond with an "unknown_psk_identity" | ||
alert message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't TLS1.2 allow for masking if the given identity is correct or not? I think many people may want that, even if it simply can't be made timing resistant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean this:
https://tools.ietf.org/html/rfc4279#section-2
If the server does not recognize the PSK identity, it MAY respond
with an "unknown_psk_identity" alert message. Alternatively, if the
server wishes to hide the fact that the PSK identity was not known,
it MAY continue the protocol as if the PSK identity existed but the
key was incorrect: that is, respond with a "decrypt_error" alert.
That's still here. This section is why I have "MAY respond with an "unknown_psk_identity"", rather than a "SHOULD" or "MUST".
Yes, I was looking for the explicit description to send alerts in that case. I like those changes. 👍 |
9055bf7
to
4f33fe8
Compare
Rebased and squashed a little bit. |
permitted to send an empty client_key_share extension as this is used | ||
to elicit the server's parameters if the client has no useful | ||
ClientKeyShareOffers for the same parameters. Servers receiving duplicate | ||
ClientKeyShareOffers SHOULD respond with an "illegal_parameter" alert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everywhere else we're saying that the endpoint MUST send an alert and end a connection, I don't see why this should be an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An exact duplicate doesn't hurt; it's just waste. There's really no point in forcing implementations to check past the point where they find what they need. I'm not strongly against a MUST here, though.
I've split out all the purely editorial stuff into PR #208 to reduce the size of this notably. This is based on top of that (the first two commits are that PR). Most of the changesets are relatively narrow in focus, now. Each commit can be reviewed more or less separately before merging the whole PR. Having them all here means not having to handle nearly as many merge conflicts, because lots of these modify similar areas. |
7295f81
to
6ea5d31
Compare
e458fa7
to
26b3945
Compare
Rebased on top of master now that the editorial bits are merged. |
26b3945
to
7b116fb
Compare
@ekr: Ok, this has now been pruned down to the first two commits, which covers just the MTI extensions and updating of the alerts registry (just updating the documentation with the current standards). The other stuff is in another branch which I'll submit as a new PR after this part is dealt with. GitHub forgot @ekr's comments above, as they were on the old commit itself rather than technically on the PR (an annoying habbit of GitHub, apparently). Still all here: I've cut the allowed assumption of the TLS 1.2 SHA1 defaults when the extension isn't sent. The rest of the issues center on whether the TLC BCP is supposed to be the current standard set of expectations or not, as that's pretty much all in line with that RFC. |
There's certainly not WG consensus that the UTA BCP applies to TLS as a whole, so this is not the time to merge those requirements in. Feel free to start a message on the mailing list, but I need to get -08 out so it's going to have to wait |
Additionally, any implementations that allow handshake renegotiation in prior | ||
versions of TLS MUST implement the "renegotiation_info" extension. When | ||
negotiating TLS 1.2 or prior, endpoints MUST support and use the | ||
"renegotiation_info" extension or refuse all renegotiation attempts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted previously, please remove the requirement to do RI. Feel free to start a separate mailing list thread with this topic.
Understood. I'll split them off, then. Shouldn't take long. |
server MUST generate a fatal "handshake_failure" alert. | ||
Servers receiving a TLS 1.3 ClientHello without this extension yet | ||
offering either DHE or ECDHE cipher suites MUST send a | ||
"missing_extension" alert message and close the connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems over-aggressive. I approve of requiring the extension but I don't think there was agreement that servers MUST choke if you don't send it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with that. If the extension is mandatory to send, then a hello without it is essentially malformed. Not requiring it to check if it's not negotiating (EC)DHE at the moment might be ok, if that's what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually quite common to have requirements that something be a certain way and that the other side not check. To give an extreme example, consider the following:
http://rtcweb-wg.github.io/jsep/#sec.profile-names, where you are required
to send UDP/TLS/RTP/SAVP[F] and there is an extensive list of other values you must treat as equivalent to that.
I don't have a problem with a requirement that you MUST NOT negotiate (EC)DHE without a there being a supported_groups extension that contains the group in question and that if there is no valid configuration then you must send "handshake_failure", but there simply wasn't WG consensus for requiring anything stronger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the jsep analogy here is bad. In that case, you have additional compatibility requirements.
Here, I think that the rule you have is perfect. The server should pick a cipher suite, then check named_groups. If there isn't a usable group, barf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Thu, Aug 27, 2015 at 10:07 AM, Martin Thomson notifications@github.com
wrote:
In draft-ietf-tls-tls13.md
#201 (comment):@@ -2242,12 +2267,15 @@ extension type (Supported Group Extension):
The client MUST supply a "supported_groups" extension containing at
least one group for each key exchange algorithm (currently
DHE and ECDHE) for which it offers a cipher suite.
-If the client does not supply a "supported_groups" extension with a
-compatible group, the server MUST NOT negotiate a cipher suite of the
-relevant type. For instance, if a client supplies only ECDHE groups,
-the server MUST NOT negotiate finite field Diffie-Hellman. If no
-acceptable group can be selected across all cipher suites, then the
-server MUST generate a fatal "handshake_failure" alert.
+Servers receiving a TLS 1.3 ClientHello without this extension yet
+offering either DHE or ECDHE cipher suites MUST send a
+"missing_extension" alert message and close the connection.I think that the jsep analogy here is bad. In that case, you have
additional compatibility requirements.Here, I think that the rule you have is perfect. The server should pick a
cipher suite, then check named_groups. If there isn't a usable group, barf.
If you want to make this change, please take it to the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Understood. I'll split them off, then. Shouldn't take long." Thanks. I'm about to shut down for the night but I'll deal with this tomorrow aM. |
7b116fb
to
a579e7b
Compare
a579e7b
to
9cbe9eb
Compare
Ok, commit updated on this PR:
Took a little bit extra time to double-check that I got everything you and Travis wanted changed. (it wanted an informative ref listing for the TLS 1.2 spec, which we apparently didn't have in the current version) GitHub's comment system is beginning to annoy me, btw. :| I haven't added in the MTI bits for the client_key_share & pre_shared_key extensions that are in the larger branch, as you didn't review those yet, so just a note for now. This is also rebased onto current master and I made sure the second commit is documentation only (the bulk of the changed lines are just shifting whitespace so they all fit). |
This is a set of changes covering mailing list discussions about error alerts, certificate acceptance, SHA-1 deprecation, as well as including some recommendations about curves. There's also quite a few other little fixes mixed in there, including some cleanups in RFC2119 language. The alerts list has been fully updated to include all standardized alerts (PSK alert is needed for new PSK extension, in particular). This also adds RFC 5746 ("renegotiation_info" info extension) to the list of newly obsoleted RFCs, as renegotiation has already been removed completely thus making it no longer applicable as of this new TLS version.
PR #169 has the discussed change to certificate ordering requirements, however it is waiting on @seanturner to assess consensus so I'm not including it here.
(Edited to note: final commits from this PR have changed and parts have been broken off into other branches for other PRs)