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

Stateless handshaker #430

Merged
merged 20 commits into from
May 21, 2020
Merged

Conversation

kazu-yamamoto
Copy link
Collaborator

The old implementation of QUIC early data is just specifying wire data (ByteString) to the configuration. This is good for HTTP/1.1 whose protocol is based on text. But for HTTP/3, it's hard to generate early data in advance since multiple threads generate frames.

The current implementation of QUIC early data is just specifying Bool (use0RTT) to the configuration. If True, the client controller MUST return after sending Client Hello so that QUIC applications can send streams and internal code encrypted them with client early secret.

If False, the client controller MUST return after sending Client Finished. Unfortunately, the old two-phase approach cannot implements this two modes.

This PR removes the controls and the exported states. The handshakers purely synchronize with QUIC threands through callbacks. The result of interoperability tests is very good.

@kazu-yamamoto kazu-yamamoto requested a review from ocheron May 6, 2020 21:05
@ocheron
Copy link
Contributor

ocheron commented May 7, 2020

It's ok to simplify the API even more, but here it seems you remove too much.

If the server does not call recvData I don't see how the pending action verifying the client Finished message is executed. The API must have a way to return handshake errors to QUIC.

@kazu-yamamoto
Copy link
Collaborator Author

You are right. I forgot the pending action.

Could you suggest how to stop recvData in the server side? Is timeout good enough?

@ocheron
Copy link
Contributor

ocheron commented May 7, 2020

I think the cleanest solution would be not to kill the thread but inject locally InpTransportError NoError after the server handshake is complete. quicRecv then returns Error_EOF, which will make recvData exit normally.

Maybe the same can be done on the client side after HandshakeDone.

@kazu-yamamoto
Copy link
Collaborator Author

@ocheron The situation is much simpler than I expected. The pending actions are used only for 0RTT. That's why 0RTT test passed in my QUIC test cases. The server processes Client Finished and returns NewSessionTicket in handshake for the first connection.

To behave the same for the second 0RTT connection, we can use the same logic. See 9e4ff97.

@ocheron
Copy link
Contributor

ocheron commented May 10, 2020

I would prefer to wait more to be sure this is stable and you have all you need.
For example is there enough error handling to test handshake failures like I did here?
ocheron/quic@c26f25cf426875ae1200ef393cf2c4d325fefded
Especially the possibility to send a new connection after a failure, without hanging in between.

quicDone is always called at the end of newQUICServer so I don't think this callback is necessary.

@kazu-yamamoto
Copy link
Collaborator Author

I would prefer to wait more to be sure this is stable and you have all you need.

Good point. I noticed that Context should be passed to a certain callback to call getClientCertificateChain for WAI.

quicDone is always called at the end of newQUICServer so I don't think this callback is necessary.

I don't understand this. newQUICServer should be executed with forkIO. How we can tell when this thread is fisnished?

@ocheron
Copy link
Contributor

ocheron commented May 11, 2020

Instead of newQUICServer sparams callbacks { quicDone = doSomething } one can write:
newQUICServer sparams callbacks >> doSomething.

@kazu-yamamoto
Copy link
Collaborator Author

Thanks. I understand. But I'm planning to pass Context to quicDone for getClientCertificateChain.

@ocheron
Copy link
Contributor

ocheron commented May 18, 2020

I agree there's a need to use the TLS context to query the handshake status. It's even possible to use the context to find handshake mode and ALPN instead of adding this information to ApplicationSecretInfo.

I continued this approach here:

Changes:

  • Handshake mode and negotiated protocol are now taken from the TLS context and
    removed from ApplicationSecretInfo. When 0-RTT is not possible, handshake
    mode is RTT0 but no early traffic secret is set. QUIC should use this as
    signal that 0-RTT has been rejected and that early data should be skipped.

  • Parameter sharedExtensions is renamed sharedHelloExtensions and added to
    ServerHello message before TLS 1.3.

  • Alert protocol between client and server is restored so that a failure during
    TLS handshake interrupts the connection attempt on both sides.

  • SendClientHello is not emitted a second time after HRR so that early traffic
    secret is set just once.

You can include some of this if you find it useful.

@kazu-yamamoto
Copy link
Collaborator Author

Nice idea. I did not notice this.

  • The commits for tls were added in this PR
  • The commits for quic were merged into my repo

Thank you for this improvement!

@kazu-yamamoto
Copy link
Collaborator Author

Are we ready for merging?

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.

OK, no need to wait any longer.
I merge with one more commit disabling PostHandshakeAuth extension in QUIC mode.
The client doesn't need to send it because it's not legal for the server to use PHA.

@ocheron ocheron merged commit 4594272 into haskell-tls:master May 21, 2020
@kazu-yamamoto kazu-yamamoto deleted the stateless-handshaker branch May 21, 2020 11:28
@kazu-yamamoto
Copy link
Collaborator Author

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

2 participants