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 logging #317

Merged
merged 7 commits into from
Dec 12, 2018
Merged

Key logging #317

merged 7 commits into from
Dec 12, 2018

Conversation

kazu-yamamoto
Copy link
Collaborator

This PR implements master key logging.

@ocheron
Copy link
Contributor

ocheron commented Dec 3, 2018

Better than commented code, but will it be really useful?
The master secret is already available in Information.

@kazu-yamamoto
Copy link
Collaborator Author

Oh. I did not notice Information. I will seek the approach to display master keys in Information.

@kazu-yamamoto
Copy link
Collaborator Author

For TLS 1.3, we need to add fields for early secret and handshake secret in Information. If we do so, they must be stored in somewhere since applications can get Information anytime. But I think that the implementation should forget early secret and handshake secret as soon as possible for security reasons. Any thoughts?

@ocheron
Copy link
Contributor

ocheron commented Dec 4, 2018

That's why I try to understand if this is useful (other than temporarily / for an implementor).

@kazu-yamamoto
Copy link
Collaborator Author

When hs-tls is used in a server, we can get keys from Firefox/Chrome. But if it is used in a client, there is no way to get keys.

If there is no strong objection, I would like to merge this PR as is.

@ocheron
Copy link
Contributor

ocheron commented Dec 5, 2018

Ok, however if this is long-term need then it would probably deserve an API similar to what already exists for logging.

Notably:

  • switch to a generic IO action instead of forcing to use hPutStrLn stderr with the hexadecimal conversion built in
  • use an ADT instead of String for the type of key

And it's still not clear why you log the client random systematically, and not the server random for instance. Also why the master secret is shown as CLIENT_RANDOM.

@kazu-yamamoto
Copy link
Collaborator Author

Notably:

  • switch to a generic IO action instead of forcing to use hPutStrLn stderr with the hexadecimal conversion built in
  • use an ADT instead of String for the type of key

Thank you for good suggestions.

And it's still not clear why you log the client random systematically, and not the server random for instance. Also why the master secret is shown as CLIENT_RANDOM.

I don't understand this. Do you mean that you don't know the format of key logging? If so, please refer to:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

@ocheron
Copy link
Contributor

ocheron commented Dec 10, 2018

Indeed I didn't know the format, thanks for the link.

So that means logging to something other than stderr will be probably useful, much less for configuring the format.

(although using an ADT internally for the key type may still be a good idea)

@kazu-yamamoto
Copy link
Collaborator Author

Rebase and pushed -f.

@kazu-yamamoto
Copy link
Collaborator Author

Is bea812b what you want?

@ocheron
Copy link
Contributor

ocheron commented Dec 11, 2018

Yes, clear and clean.
I see you chose String and not ByteString, just like packet logging.
This is quite fine for debug data.

Did you reinstate dumpKey by mistake?

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

Did you reinstate dumpKey by mistake?

Sorry. This is a mistake on rebasing. Deleted.

@kazu-yamamoto kazu-yamamoto deleted the keylog branch December 12, 2018 01:24
@kazu-yamamoto
Copy link
Collaborator Author

Merged. Thank you for your review!

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