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

add getFinished and getPeerFinished functions for Context, closes #442 #445

Merged
merged 2 commits into from
Jan 19, 2022

Conversation

epoberezkin
Copy link
Contributor

@epoberezkin epoberezkin commented Dec 18, 2021

@kazu-yamamoto this is partially done.

I tried putting Finished into HandshakeState but it has Show instance (although I didn't check if it's actually needed, but it would be a breaking change to remove it), and IORef does not.

Currently, I think, it stores Finished of the current peer for both Client and Server, and stores client's Finished in the server only.

I could not find the place in the client where server's Finished is validated - does client do it at all? If not, should it be added?

Also, re FinishedData vs wire-bytes, FinishedData is the same as ByteString - is it not wire-bytes?

@epoberezkin
Copy link
Contributor Author

epoberezkin commented Dec 18, 2021

No, it's not working yet (ctxFinished in the client and ctxPeerFinished in the server remain Nothing after the connection established), not with TLS1.3 at least... I am missing something

@epoberezkin
Copy link
Contributor Author

@kazu-yamamoto Ok, so the change I made is working with TLS 1.2 and not working with TLS 1.3 - I actually do not see any code in TLS 1.3 that is validating Finished.

@kazu-yamamoto
Copy link
Collaborator

Network.TLS.Handshake.Common13.checkFinished validates Finished. In the server side of TLS 1.3, the server code pushes this action then it is executed in Core.hs.

@epoberezkin
Copy link
Contributor Author

@kazu-yamamoto it now works with 1.3 and @efim-poberezkin added it to some tests - could we merge and publish it please?

@kazu-yamamoto
Copy link
Collaborator

@epoberezkin This PR looks good to me now. Did you make use of this feature to implement tls-unique channel binding already? (I'm just asking whether or not getFinished returns what you want.)

@epoberezkin
Copy link
Contributor Author

Did you make use of this feature to implement tls-unique channel binding already?

yes, it's been deployed in our SMP servers (https://github/simplex-chat/simplexmq) with TLS 1.2 for quite some time (from fork), and we now added TLS 1.3 too (not deployed yet, but passes all the tests)

@epoberezkin
Copy link
Contributor Author

whether or not getFinished returns what you want.

well, whether it returns the correct data from RFC point of view I don't know strictly speaking :) To confirm that we'd have to test against some other TLS implementation :)

But, it definitely populates both finished and peerFinished in both client and server, in both 1.2 and 1.3, and finished of the client is equal to peerFinished of the server (and vice versa), and it's different every time, so it does look alright. And we use clients finished (which is peer-finished in the server) as tls-unique unique channel binding (it's included in every transmission as a session ID, with a signature over the whole transmission including it).

So I think it's "acceptable" :)

@kazu-yamamoto kazu-yamamoto merged commit 4319ca1 into haskell-tls:master Jan 19, 2022
@kazu-yamamoto
Copy link
Collaborator

Thanks! Merged.

I will make a new release tomorrow.

@epoberezkin
Copy link
Contributor Author

ah - I've just realised, should we add comments so they appear in the docs? Or the names of properties are self-explanatory enough for whoever needs it?

@kazu-yamamoto
Copy link
Collaborator

getFinished has its doc already. Which doc are you talking about?

@epoberezkin
Copy link
Contributor Author

ah - ok. All good then :)

@kazu-yamamoto
Copy link
Collaborator

A new version has been released.

@Neustradamus
Copy link

@epoberezkin: @kazu-yamamoto has some questions, can you look on #462 ticket?

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.

4 participants