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

generate new ClientHello.random for HelloRetryRequest (for issue #185) #189

Closed
wants to merge 1 commit into from

Conversation

davegarrett
Copy link
Contributor

PR to add a simple requirement that retried ClientHello messages must not reuse random values and servers must check for and reject them if they do. (suggested by Karthik)

@ekr
Copy link
Contributor

ekr commented Jun 20, 2015

I think it's an open issue whether we should require a new ClientRandom, but requiring that the server check for it is harmful because it makes it impossible to do a stateless HelloRetryRequest.

@davegarrett
Copy link
Contributor Author

Ok, then should I downgrade it to a SHOULD (or MAY) or drop the check requirement completely? I don't have a strong objection to dropping it.

On Friday, June 19, 2015 11:52:07 pm ekr wrote:

I think it's an open issue whether we should require a new ClientRandom, but requiring that the server check for it is harmful because it makes it impossible to do a stateless HelloRetryRequest.

@davegarrett
Copy link
Contributor Author

Also, would that also require the session hash to completely restart on the new ClientHello and discard the previous messages? (see also issue #104) I added in clarification for that before noticing your comment, so that may need changing as well.

@martinthomson
Copy link
Contributor

Weeeelll, if we include a cookie, it is possible to check statelessly. But the cookie might get a little big :)

@davegarrett davegarrett force-pushed the patch-3 branch 2 times, most recently from d4430c8 to fdaac68 Compare August 21, 2015 23:52
@davegarrett
Copy link
Contributor Author

I've revised this to drop the required check. (also now based on current master, as it was rather bitrotted)

@ekr
Copy link
Contributor

ekr commented Nov 20, 2015

I don't think we want this.

@ekr ekr closed this Nov 20, 2015
@davegarrett davegarrett deleted the patch-3 branch November 21, 2015 17:39
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.

3 participants