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

Session compression and SNI #223

Closed
wants to merge 1 commit into from
Closed

Session compression and SNI #223

wants to merge 1 commit into from

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.

@@ -19,6 +19,8 @@ module Network.TLS.Types
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.

None yet

3 participants