Skip to content
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

expand MTI Extensions and add more strict requirements #232

Merged
merged 1 commit into from Aug 28, 2015

Conversation

davegarrett
Copy link
Contributor

Modified extension requirements based on recent list discussion. It's a little verbose with repeated boilerplate, but it's clear no matter which section you read now. I've also moved the relevant requirements up top to a consistent place for each extension's section.

The level of strictness here is now:

  • If attempting to negotiate and the mandatory extension isn't available: MUST error
  • If just detecting an invalid combination, but not actively negotiating: MAY error
  • SNI is clearly listed as MUST support, servers MAY require, and SHOULD error if required
  • The "unsupported_extension" alert is reworded more generally and put in as the consequence for violating the pre-existing "MUST NOT" send the client-only extensions from a server requirements. Also added some more, namely the fact that "client_key_share" is clearly an error to be sent from a server.

@davegarrett
Copy link
Contributor Author

@ekr & @martinthomson: Please take a look when you get a chance. Are we all on the same page with this language? Anything that needs tweaking to focus on the above better?

Edited to add:
Also, do I need to cut out SNI again, or is this wording ok? It's not requiring it to be sent, just noting it must be supported, which was agreed to on list a while ago, and noting the existing case that servers MAY require it with a way to indicate it using the new error alert.

an extension that they did not put in the corresponding ClientHello.
This alert is always fatal.
: Sent by endpoints receiving any hello message containing an extension
known to be invalid when included in the given hello message, including
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to have "invalid" be clearer. E.g., "forbidden".

@davegarrett
Copy link
Contributor Author

@ekr: New commit pushed here with changes for your comments:

  • "invalid" -> "prohibited"
  • Text is now: "Servers MUST NOT negotiate use of ___ cipher suites unless the client supplies a supported ___. If the extension is not provided and no alternative cipher suite is available, the server MUST close the connection with a fatal "missing_extension" alert." So, trying to find a viable cipher suite is preferred and the error is only required when there's nothing else to do.

Edited to note:
The latter doesn't contradict the mandatory error on negotiation without the needed extension, as the server simply has the (unlikely) chance of negotiating something completely different instead. The MAY error is still included later in the changeset, as discussed, so clients are still expected to always do this correctly or risk the server throwing an error.

@@ -2221,6 +2226,25 @@ Note: In versions of TLS prior to TLS 1.3, this extension was named
"elliptic_curves" and only contained elliptic curve groups. See
{{RFC4492}} and {{I-D.ietf-tls-negotiated-ff-dhe}}.

All clients MUST send a valid "supported_groups" extension containing
at least one group for each key exchange algorithm (currently
DHE and ECDHE) for which it offers a cipher suite.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it needs to be reworder for PSK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? It's replacing this, from down below in this same section:

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.

It's almost entirely a cut/paste from your original text.

I don't see anything that PSK needs special note for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PSK is a key exchange algorithm. I realize it was wrongish before, but since we're here, we should fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I sort of consider however the PSK got shared out-of-band as the key exchange. PSK suites just use it.

Just adding "ephemeral" would make it stand completely apart rather easily, I guess.

@davegarrett
Copy link
Contributor Author

@ekr: Ok, repushed with one-word change to clarify to "at least one group for each ephemeral key exchange algorithm". This excludes PSK, but is still fine for (EC)DHE when used with it. Seems like the simplest fix.

ekr added a commit that referenced this pull request Aug 28, 2015
expand MTI Extensions and add more strict requirements
@ekr ekr merged commit 3cb8264 into tlswg:master Aug 28, 2015
@davegarrett davegarrett deleted the strictermti branch August 28, 2015 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants