Implement "Extended Master Secret" extension (RFC7626)

[RyanGordon]

Description

This change implements RFC7627 "Extended Master Secret" extension with
optional enforcement by default

Reference issue

Fixes #86

[codecov-io]

Codecov Report

Merging #93 into master will increase coverage by 0.8%.
The diff coverage is 76.66%.

@@            Coverage Diff            @@
##           master      #93     +/-   ##
=========================================
+ Coverage   71.51%   72.31%   +0.8%     
=========================================
  Files          52       53      +1     
  Lines        2952     3056    +104     
=========================================
+ Hits         2111     2210     +99     
+ Misses        597      594      -3     
- Partials      244      252      +8

Impacted Files	Coverage Δ
cipher_suite_tls_psk_with_aes_128_ccm8.go	76.47% <ø> (ø)	⬆️
config.go	100% <ø> (ø)	⬆️
state.go	66.66% <ø> (ø)	⬆️
flight.go	94.73% <100%> (+30.03%)	⬆️
extension.go	66.66% <100%> (+1.66%)	⬆️
prf.go	62.18% <100%> (+0.97%)	⬆️
conn.go	80.06% <100%> (+1.64%)	⬆️
extension_use_master_secret.go	62.5% <62.5%> (ø)
client_handlers.go	82.36% <72.36%> (-0.43%)	⬇️
server_handlers.go	84.91% <73.91%> (-0.31%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56a964a...2f74fec. Read the comment docs.

[Sean-Der]

Test coverage is really great!

No issue from me, just a few minor nits just so we have a little less state if possible

[Sean-Der]

@RyanGordon So not using errors.go isn't a huge deal, but is just nice to see the full range of errors and give us a little type safety.

In the future we may export these, and would be great if we can just upcase them all when we get to that point. I think that could be a huge advantage over OpenSSL, actually let people really understand what happened and why.

[Sean-Der]

Have you ever read anything about what may/may be kept in memory? I thought when writing things originally I read somewhere that certain things should be zero'ed out even (maybe reading OpenSSL)

crypto/tls itself only keeps the masterSecret []byte so I think we should push back on this

[RyanGordon]

So two problems that I ran into:

1. In order to calculate the extendedMasterSecret hash, you have to keep a binary "log" of the handshakes, until you calculate the hash
2. For the client, the last part of the handshake is sending out the clientKeyExchange so we have to wait for the initalizeCipherSuite to run at that point which then calculates the extendedMasterSecret and uses the serverKeyExchange (specifically publicKey, namedCurve, signature and hashAlgorithm from it) to validate the key signature (if not disabled)

I'm happy to see if there are ways to more directly scrub it from memory but I am struggling to see how I (or another library?) could not keep it around

[Sean-Der]

Can we pull the *handshakeMessageServerKeyExchange from the *handshakeCache?

[daenney]

Even the handshake cache would still be an unprotected memory region right?

We could consider introducing a dependency on https://godoc.org/github.com/awnumar/memguard, for creating a secure enclave where we store sensitive information. It's a little more extreme than just zeroing out a buffer but it goes to fairly great lengths to help you keep a memory area protected.

[RyanGordon]

@Sean-Der Yes, we could pull it from the handshake cache but I would have to unmarshal it again which would be a bit extra computation. I don't have a strong preference either way

@daenney Yes, I think it's definitely worthwhile to have a discussion on how we could better protect memory in these cases. I opened up an issue here so we can have a fuller discussion on it: #97

[Sean-Der]

My preference is double computation. Until we measure we don’t know how expensive something is.

But the more state we have the more difficult things become to keep in your head/and we have to guard and more nil checks (that’s just like my opinion though man)

[Sean-Der]

@daenney that’s super interesting I had no idea about memguard.

I will play with it/understand the trade offs! I don’t have a security background/I am pretty unaware of what the community is doing :/

[Sean-Der]

sessionHash, err := c.handshakeCache.sessionHash(c.state.cipherSuite.hashFunc())

[RyanGordon]

Fixed!

[RyanGordon]

Actually lint fails with it this way:

client_handlers.go:19:16: declaration of "err" shadows declaration at client_handlers.go:9 (govet)
		sessionHash, err := c.handshakeCache.sessionHash(c.state.cipherSuite.hashFunc())
		             ^
server_handlers.go:131:18: declaration of "err" shadows declaration at server_handlers.go:106 (govet)
				sessionHash, err := c.handshakeCache.sessionHash(c.state.cipherSuite.hashFunc())

[Sean-Der]

Sorry I realize a lot of issues I have with code... is actually mine -_-

If we can bring down the stored state to the absolute minimum (less bugs and random things changing the world) this is an approve from me!

I will go back and send PRs your way to make things better.

[Sean-Der]

@RyanGordon this LGTM!

free to merge w/e you are happy with it and all the tests pass! After it lands I am going to do do a cleanup pass (I will submit it!).

When reading the code during the review I saw a lot of things that I didn't like. Then I realized it was code I had written and never come back to fix up.