Skip to content

Conversation

@ocheron
Copy link
Contributor

@ocheron ocheron commented Apr 27, 2017

Adds new information into SessionData to fully implements resumption checks. SNI is especially important because it may have an influence on credentials and therefore ciphers.

In the new code I left SNI test case-sensitive. It makes the code simpler and should not cause any issue. I don't think a client will write the hostname differently the second time and still expect resumption.

After this PR it becomes possible to reorganize the ServerHello code so that the resumption case skips most of it. But this likely conflicts with any other server work.

import Data.ByteString (ByteString)
import Data.Word

type HostName = String
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found the same code in multiple modules. I would like to remove the other definitions and let the modules import Types.

I guess that this redundancy is the reason why HostName is not highlighted in the HTML doc produced by haddock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I've seen it too and just replicated what already exists. But I can try to do better.

IIRC it's potentially more complex that it looks because x509 packages have this too.
If a type alias is to be exported, it could be from there and not tls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the goal of this alias was to remember to do something sensible about this, not really expose it to the user. A real Hostname type is coming soon (couple of months away), and when that happens, I'm planning to move those types in tls and x509 to use 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.

Then in the PR I'd rather keep this 5th local definition of a type alias for now and remove them all when a remplacement is ready.

Does the Hostname plan include something for IDN support?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, we're still not quite there but the plan is to have punycode and full IDN

@kazu-yamamoto
Copy link
Collaborator

Other parts look good to me.

@kazu-yamamoto kazu-yamamoto self-requested a review May 1, 2017 00:20
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

OK. Let's merge this PR as is.

@kazu-yamamoto kazu-yamamoto mentioned this pull request May 1, 2017
ocheron added a commit that referenced this pull request May 1, 2017
@ocheron
Copy link
Contributor Author

ocheron commented May 1, 2017

Rebased and merged.
Thank you for reviewing.

@ocheron ocheron closed this May 1, 2017
@ocheron ocheron deleted the resume-new-session-data branch May 1, 2017 13:04
ocheron added a commit that referenced this pull request May 3, 2017
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