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

Key update #290

Closed
wants to merge 11 commits into from
Closed

Conversation

kazu-yamamoto
Copy link
Collaborator

This implements key update for TLS 1.3.
A new API updateKey is added.

@kazu-yamamoto
Copy link
Collaborator Author

Ping @ocheron

@kazu-yamamoto
Copy link
Collaborator Author

@ocheron Any show-stoppers for this PR?

@ocheron
Copy link
Contributor

ocheron commented Nov 6, 2018

Only lack of time. I hope to complete review before end of week.

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.

A conforming implementation needs to handle incoming KeyUpdate message like this.
And the new API enables a test case, which is good.

Please find some suggestions.

core/Network/TLS/Struct13.hs Show resolved Hide resolved
core/Network/TLS/Core.hs Outdated Show resolved Hide resolved
debug/src/SimpleClient.hs Show resolved Hide resolved
core/Tests/Tests.hs Show resolved Hide resolved
core/Tests/Tests.hs Show resolved Hide resolved
core/Tests/Tests.hs Show resolved Hide resolved
tls13 <- tls13orLater ctx
when tls13 $ do
sendPacket13 ctx $ Handshake13 [KeyUpdate13 UpdateRequested]
keyUpdate ctx getTxState setTxState
Copy link
Contributor

Choose a reason for hiding this comment

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

About this API, should we allow the caller to update direction Tx only?

Also it could return indication to the caller whether the call was ignored or not (flag tls13).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In TLS 1.3 specification, both Tx and Rx are updated. There is no way to do update one direction only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. updateKey :: Context -> IO Bool

Copy link
Contributor

Choose a reason for hiding this comment

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

About Tx/Rx, this is not my understanding. What happens if Alice sends "update_not_requested" to Bob? Update of traffic key Alice -> Bob only.

One use case for key update is to avoid reaching traffic limits of nonce-based cipher modes. The amount of traffic in both directions is not necessarily balanced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I added the check.

@kazu-yamamoto
Copy link
Collaborator Author

@ocheron Ready for merging?

@kazu-yamamoto
Copy link
Collaborator Author

To make my job easier, I merge this PR now.
@ocheron If you will find any problems, please register it as a new issue.

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this pull request Nov 13, 2018
@kazu-yamamoto
Copy link
Collaborator Author

Rebased and merged. Thank you for reviewing!

@kazu-yamamoto kazu-yamamoto deleted the key-update branch November 13, 2018 02:36
@ocheron
Copy link
Contributor

ocheron commented Nov 17, 2018

If you will find any problems, please register it as a new issue.

Opened #311.

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