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

Merging TLS 1.3 #283

Merged
merged 42 commits into from
Sep 12, 2018
Merged

Merging TLS 1.3 #283

merged 42 commits into from
Sep 12, 2018

Conversation

kazu-yamamoto
Copy link
Collaborator

This is the final difference for TLS 1.3.
I don't intend to merge this PR directly.
Rather, I would like to ask reviewers to give comments.
This is a jumbo patch. I have no idea on how to divide this into smaller pieces.
Comments are welcome.
Meanwhile, I will try to resolve fixmes.

@ocheron
Copy link
Contributor

ocheron commented Aug 22, 2018

Could you clarify what the test suite does?

  • I don't see test cases with TLS13-specific extensions and features like PSK, 0RTT, etc.
  • it's like TLS13 is tested against TLS13 only

@kazu-yamamoto
Copy link
Collaborator Author

The test is for full handshake (1RTT) only at this moment.
I will try to add test cases for other three handshakes.

@ocheron
Copy link
Contributor

ocheron commented Aug 23, 2018

Yes, working on the test suite first would be useful to dive in this large piece.

I think I have progress regarding the version incompatibilities. It looks there is a bug when connecting TLS13-enabled client with a SSL3-only server. This works better and starts to test more version combinations: https://gist.github.com/ocheron/5b8ddfa47f2f5216bcea13213ebee688

Two test cases are still failing:

  • renegotiation: probably not a surprise
  • key usage: maybe this is simply not implemented for 1.3?

Copy link
Contributor

@ocheron ocheron left a comment

Choose a reason for hiding this comment

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

I added some comments and questions in the code, please find also global comments here:

  • hourglass can be a better choice than time to avoid floating-point conversion (hourglass is already used in the test suite and indirectly in x509 packages)
  • new modules should have header and export lists

Client and Server:

  • often there is a sequence of calls encodeHandshake13, updateHandshakeDigest, addHandshakeMessage. This should be captured as a common utility, for example in module Sending13.
  • intersection with availableGroups is not needed as this contain all possible values now. Some of it can use availableECGroups instead, but FFDHE is probably not complex to add.

SimpleClient and SimpleServer:

  • there should be an option --tls13, but not as default until features still missing are added (and TLS13 is default in library too)

core/Network/TLS/Cipher.hs Outdated Show resolved Hide resolved
core/Network/TLS/Struct13.hs Outdated Show resolved Hide resolved
core/Network/TLS/Extra/Cipher.hs Outdated Show resolved Hide resolved
core/Network/TLS/Context/Internal.hs Outdated Show resolved Hide resolved
core/Network/TLS/Context/Internal.hs Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Show resolved Hide resolved
@kazu-yamamoto
Copy link
Collaborator Author

I will try to add test cases for other three handshakes.

Test cases for four handshakes were added.

@kazu-yamamoto
Copy link
Collaborator Author

  • renegotiation: probably not a surprise

Renegotiation does not exist in TLS 1.3 as you may know.

  • key usage: maybe this is simply not implemented for 1.3?

I don't know key usage. How can I learn it?

@ocheron
Copy link
Contributor

ocheron commented Sep 3, 2018

I don't know key usage. How can I learn it?

The requirement is listed in RFC 8446 section 4.4.2.2: The certificate MUST allow the key to be used for signing (i.e., the digitalSignature bit MUST be set if the Key Usage extension is present)

More is in RFC 5280 section 4.2.1.3.

The test case about server key usage can be generalized to TLS13 easily. For client key usage, this is not currently possible because client authentication is not implemented.

@kazu-yamamoto
Copy link
Collaborator Author

  • new modules should have header and export lists

Done.

@kazu-yamamoto
Copy link
Collaborator Author

  • intersection with availableGroups is not needed as this contain all possible values now. Some of it can use availableECGroups instead, but FFDHE is probably not complex to add.

Intersections are deleted.

@kazu-yamamoto
Copy link
Collaborator Author

  • often there is a sequence of calls encodeHandshake13, updateHandshakeDigest, addHandshakeMessage. This should be captured as a common utility, for example in module Sending13.

It appeared that this is an awesome idea. Thanks!

@kazu-yamamoto
Copy link
Collaborator Author

  • there should be an option --tls13, but not as default until features still missing are added (and TLS13 is default in library too)

Done.

@kazu-yamamoto
Copy link
Collaborator Author

hourglass can be a better choice than time to avoid floating-point conversion (hourglass is already used in the test suite and indirectly in x509 packages)

Done.

@kazu-yamamoto
Copy link
Collaborator Author

I think that I did everything I can.

Copy link
Contributor

@ocheron ocheron left a comment

Choose a reason for hiding this comment

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

I added a few more comments about the new changes.
Also did you look at my patch for SSLv3 / RSA encryption?

core/Tests/Tests.hs Outdated Show resolved Hide resolved
core/Tests/Tests.hs Outdated Show resolved Hide resolved
core/Network/TLS/Handshake/State.hs Outdated Show resolved Hide resolved
core/Network/TLS/Context/Internal.hs Outdated Show resolved Hide resolved
core/Network/TLS/IO.hs Show resolved Hide resolved
core/Network/TLS/Handshake/Client.hs Show resolved Hide resolved
core/Network/TLS/Handshake/Server.hs Show resolved Hide resolved
@kazu-yamamoto
Copy link
Collaborator Author

Also did you look at my patch for SSLv3 / RSA encryption?

Do you mean this one?

https://gist.github.com/ocheron/5b8ddfa47f2f5216bcea13213ebee688

If so, I'm now trying to resolve the two test failures.

@kazu-yamamoto
Copy link
Collaborator Author

  • renegotiation: probably not a surprise

Your patch was applied and the bug of renego has been fixed.

@kazu-yamamoto
Copy link
Collaborator Author

I'm now trying to implement client authentication to fix CI.

@ocheron
Copy link
Contributor

ocheron commented Sep 11, 2018

Great, I think this is already a major achievement and we can merge without client authentication.
The test case can run without TLS13 for now.

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this pull request Sep 12, 2018
@kazu-yamamoto kazu-yamamoto merged commit 5424184 into haskell-tls:master Sep 12, 2018
@kazu-yamamoto
Copy link
Collaborator Author

Great, I think this is already a major achievement and we can merge without client authentication.
The test case can run without TLS13 for now.

OK. I modified the test to skip TLS 1.3 and merged this PR to master.
Thank you for your thorough review!

@kazu-yamamoto kazu-yamamoto deleted the work13 branch September 12, 2018 03:07
@kazu-yamamoto kazu-yamamoto mentioned this pull request Sep 13, 2018
7 tasks
@ocheron ocheron mentioned this pull request Nov 18, 2018
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.

2 participants