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

crypto/tls: support NSS-formatted key log file #13057

Closed
shanemhansen opened this issue Oct 26, 2015 · 30 comments
Closed

crypto/tls: support NSS-formatted key log file #13057

shanemhansen opened this issue Oct 26, 2015 · 30 comments
Assignees
Milestone

Comments

@shanemhansen
Copy link
Contributor

@shanemhansen shanemhansen commented Oct 26, 2015

In order to aid in debugging protocols which utilize tls (like http2), wireshark and other tools support reading a "key log" file as described here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

crypto/tls.Config struct should support logging keys to a io.Writer.

@bradfitz bradfitz changed the title crypto/tls support nss formatted key log file crypto/tls: support NSS-formatted key log file Oct 27, 2015
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 27, 2015

/cc @agl

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Nov 5, 2015
joneskoo added a commit to joneskoo/http2-keylog that referenced this issue Jul 10, 2016
@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Jul 10, 2016

I wish for this to be added, it would be so helpful debugging especially HTTP2. It is a trivial code change as demonstrated by my proof of concept. Obviously needs refining to bring this into standard library, but you can see that it's hardly any code to implement this.

I think the biggest question is how to enable this in a Go idiomatic fashion, especially through net/http. NSS uses environment variable SSLKEYLOGFILE for the file where to dump secrets.

As I understand the CLIENT_RANDOM based format is sufficient to cover everything, and I don't know if pre-1.8.0 Wireshark would be relevant for HTTP2 anyway.

I only demonstrated server handshake but client should be identical.

@bradfitz bradfitz modified the milestones: Go1.8, Unplanned Jul 11, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 11, 2016

@joneskoo, I'd love to see this too. Please send a change once the Go 1.8 dev cycle opens after Go 1.7 is out in early August.

@shanemhansen

This comment has been minimized.

Copy link
Contributor Author

@shanemhansen shanemhansen commented Jul 11, 2016

I've been playing around trying to get this to work locally. No luck yet.
There seem to be a couple supported formats.

Wet to API I believe AGL suggested a io.Writer in the tls.Context.

I'd love to play with a branch of you have one.
On Jul 10, 2016 6:03 PM, "Brad Fitzpatrick" notifications@github.com
wrote:

@joneskoo https://github.com/joneskoo, I'd love to see this too. Please
send a change once the Go 1.8 dev cycle opens after Go 1.7 is out in early
August.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#13057 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAWY0ZGU3dxkbuMP0GJHVbYhDYcgGXp3ks5qUYhBgaJpZM4GWEKV
.

@agl agl self-assigned this Jul 12, 2016
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Jul 12, 2016

This change isn't suitable until we're in the 1.8 cycle, but we'll get something working for 1.8.

Would adding an io.Writer member to tls.Config work for everyone?

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Jul 12, 2016

@agl io.Writer works for me. I just finished trying out what it might look like; feel free to use or discard this implementation. Some trivial changes were needed in net/http as it does a shallow copy of the TLS config. Wonder what else. https://gist.github.com/joneskoo/254cd57e20acc7035d2c06658297b203

This is what I think it would look like for client and server:

Screenshot

I realize we're still waiting for the 1.8 to open, but this being open source, you can't stop me 🙈! But on all seriousness, I'll hold until 1.8 is open.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 12, 2016

@joneskoo, please submit a CLA and send your change via Gerrit (which won't let you upload until you've done a CLA). We can't look at your code on Github until we know you've submitted a CLA.

@agl, I think we might want something a bit more generic than an io.Writer, which means we're locking ourselves into a format.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jul 16, 2016

Actually, an io.Writer sounds fine. If anybody wanted to do something fancier, they can supply an io.Writer which parses the NSS keylog format back out, which is trivial enough.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Jul 27, 2016

@bradfitz If you wish to check out the change on https://github.com/joneskoo/http2-keylog I have signed the CLA and this repository is explicitly intended for submission to Go standard library when 1.8 opens. I need to find a slot to rebase and clean up for the Gerrit submission.

@jboelter

This comment has been minimized.

Copy link

@jboelter jboelter commented Jul 27, 2016

Need tls clone method #15771

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Aug 20, 2016

@agl, Submitted https://go-review.googlesource.com/27434 crypto/tls: add KeyLogWriter for debugging

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 20, 2016

CL https://golang.org/cl/27434 mentions this issue.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Aug 27, 2016

Can someone please explain what's the timeline going forward from this? When can I expect a review to happen and after that's all clear, how does the code actually end up in 1.8 development tree? The contributing doc doesn't really explain how review and merge works from a contributor's perspective, it only talks about the tools.

I've addressed Brad's suggestions already.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 27, 2016

LGTM. I'll just submit it for now. @agl can do another pass later.

@gopherbot gopherbot closed this in 320bd56 Aug 27, 2016
@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Aug 28, 2016

This should stay open until @agl gives this a look, right?

I'm expecting review comments to be on things I don't really know about. My change was "KISS", minimal changes.

  • TLS features, e.g. renegotiation; should the key be logged there as well? What's the proper place for the trigger?
  • Tests; I just looked at some existing tests and made the code reach the key logger in a trivial way; is this sufficient for standard library?
@bradfitz bradfitz reopened this Aug 28, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 28, 2016

Sure, we can reopen.

The other thing I thought of after it was submitted was concurrency against the configured io.Writer in the presence of multiple connections. We probably just want some package-level mutex to guard all writes.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Aug 28, 2016

Good point about concurrent use. There's already a private mutex for session ticket keys; that shouldn't be reused? https://golang.org/src/crypto/tls/common.go#L392

TODO

  • Does renegotiation needs special consideration? Now only handshakes are logged
  • Is test coverage sufficient? How to test better?
  • Protect against concurrent connections with same config
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 28, 2016

There's already a private mutex for session ticket keys; that shouldn't be reused?

I think I'd make a new, explicit one. Can you send that change first?

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 10, 2016

@bradfitz https://go-review.googlesource.com/29016 adds the mutex you suggested - should I also rename the existing "mutex" to "sessionTicketMutex"?

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 10, 2016

I did a "find references" search for masterFromPreMasterSecret and both of the two usages (client and server handshake) have the keylog call. Renegotiation appears to be part of the same handshake.

Can someone confirm that those are indeed the only two places where we get a new master secret for a connection?

And finally my question whether the test coverage is in fact sufficient?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 10, 2016

CL https://golang.org/cl/29016 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 10, 2016
Concurrent use of tls.Config is allowed, and may lead to
KeyLogWriter being written to concurrently. Without a mutex
to protect it, corrupted output may occur. A mutex is added
for correctness.

The mutex is made global to save size of the config struct as
KeyLogWriter is rarely enabled.

Related to #13057.

Change-Id: I5ee55b6d8b43a191ec21f06e2aaae5002a71daef
Reviewed-on: https://go-review.googlesource.com/29016
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 11, 2016

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Sep 11, 2016

It'll be in the release notes. Adding an Example_keyLogWriter would be nice, though. Want to send one? I don't care about it looking out of place. We need more examples.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 11, 2016

@bradfitz Done https://go-review.googlesource.com/29050

However since example is also a test, it fails because the master secrets are obviously unique for every connection and the one in the Output is not valid for any other execution. I expect this is addressed in code review.

I guess it's better to put this in crypto/tls anyway because the feature is really for any TLS, not just HTTP although that's probably what it's most useful with.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 11, 2016

CL https://golang.org/cl/29050 mentions this issue.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 11, 2016

@bradfitz ping? I used httptest in the example now, I guess you're the expert?

There's two variants, PS2 and PS4. I wish it was a bit cleaner; https://github.com/joneskoo/http2-keylog/blob/master/h2keylog-client/client.go#L41-L51 captures it better in my opinion.

I'd prefer an example with a real-world net/http with http2 which the current PS4 doesn't really do, which I raised in the other issue. I would gladly welcome someone else giving it a try and taking over, continuing on my work. I don't know when I'll be able to work on this next, could be some time.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Sep 29, 2016

@joneskoo, waiting for you to reply to feedback.

@joneskoo

This comment has been minimized.

Copy link
Contributor

@joneskoo joneskoo commented Sep 30, 2016

@quentinmit quentinmit added the NeedsFix label Oct 5, 2016
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 25, 2016

@bradfitz, can you clarify: what is remaining on this issue?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 25, 2016

I think it's done. I think @joneskoo had questions about renegotiations, but I don't think we care. We can address that later if it comes up.

@bradfitz bradfitz closed this Oct 25, 2016
gopherbot pushed a commit that referenced this issue Nov 17, 2016
For #13057.

Change-Id: Idbc50d5b08e055a23ab7cc9eb62dbc47b65b1815
Reviewed-on: https://go-review.googlesource.com/29050
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 25, 2017
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Add support for writing TLS client random and master secret
in NSS key log format.

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

Normally this is enabled by a developer debugging TLS based
applications, especially HTTP/2, by setting the KeyLogWriter
to an open file. The keys negotiated in handshake are then
logged and can be used to decrypt TLS sessions e.g. in Wireshark.

Applications may choose to add support similar to NSS where this
is enabled by environment variable, but no such mechanism is
built in to Go. Instead each application must explicitly enable.

Fixes golang#13057.

Change-Id: If6edd2d58999903e8390b1674ba4257ecc747ae1
Reviewed-on: https://go-review.googlesource.com/27434
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Add support for writing TLS client random and master secret
in NSS key log format.

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

Normally this is enabled by a developer debugging TLS based
applications, especially HTTP/2, by setting the KeyLogWriter
to an open file. The keys negotiated in handshake are then
logged and can be used to decrypt TLS sessions e.g. in Wireshark.

Applications may choose to add support similar to NSS where this
is enabled by environment variable, but no such mechanism is
built in to Go. Instead each application must explicitly enable.

Fixes golang#13057.

Change-Id: If6edd2d58999903e8390b1674ba4257ecc747ae1
Reviewed-on: https://go-review.googlesource.com/27434
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.