-
Notifications
You must be signed in to change notification settings - Fork 36
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
implement stateless resets #141
implement stateless resets #141
Conversation
f51e228
to
e29ba77
Compare
@marten-seemann this seems to fail in CI... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I haven't reviewed the tests yet.
- This seems generally sound structurally, but I had technical comments about the internals.
I can review the tests once they work in CI
@@ -82,6 +83,7 @@ var alertText = map[Alert]string{ | |||
AlertUnknownPSKIdentity: "unknown PSK identity", | |||
AlertNoApplicationProtocol: "no application protocol", | |||
AlertNoRenegotiation: "no renegotiation", | |||
AlertStatelessRetry: "stateless retry", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't your fault, but I wonder if we should give these non-alerts a special name so we don't acccidentally try to send them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #149
conn.go
Outdated
@@ -74,9 +74,16 @@ type Config struct { | |||
AllowEarlyData bool | |||
// Require the client to echo a cookie. | |||
RequireCookie bool | |||
// TODO: fix comment here: no more default cookie handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO seems like it needs fixing.
conn.go
Outdated
// The CookieSource is used to encrypt / decrypt cookies. | ||
// TODO: implement a check for that | ||
// If non-blocking mode is used, and cookies are required, this field has to be set. | ||
// TODO: implement that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These TODOs also seem to need fixing
conn.go
Outdated
var err error | ||
caps.CookieSource, err = newDefaultCookieSource() | ||
if err != nil { | ||
logf(logTypeHandshake, "Error initializing client state: %v", alert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this say server state?
conn.go
Outdated
@@ -687,6 +706,7 @@ func (c *Conn) Handshake() Alert { | |||
|
|||
for !connected { | |||
// Read a handshake message | |||
logf(logTypeHandshake, "readmessage") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make this |logTypeVerbose| and make the text clearer.
server-state-machine.go
Outdated
plainCookie, err := state.Caps.CookieSource.DecodeToken(clientCookie.Cookie) | ||
if err != nil { | ||
logf(logTypeHandshake, fmt.Sprintf("[ServerStateStart] Error decoding token [%v]", err)) | ||
return nil, nil, AlertAccessDenied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you want AccessDenied here, but rather DecryptError or IllegalParameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put a TODO here because if you were to change your key, this wouldn't work.
server-state-machine.go
Outdated
if _, err := syntax.Unmarshal(plainCookie, cookie); err != nil { | ||
logf(logTypeHandshake, fmt.Sprintf("[ServerStateStart] Error unmarshaling cookie [%v]", err)) | ||
return nil, nil, AlertAccessDenied | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be internal_error, because it can't happen.
server-state-machine.go
Outdated
logf(logTypeHandshake, "[ServerStateStart] Error computing truncated ClientHello [%v]", err) | ||
return nil, nil, AlertDecodeError | ||
logf(logTypeHandshake, "[ServerStateStart] Error in PSK negotiation [%v]", err) | ||
return nil, nil, AlertInternalError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised this is internal error. @bifurcation ?
server-state-machine.go
Outdated
binder := computeFinishedData(params, binderKey, ctxHash.Sum(nil)) | ||
if !bytes.Equal(binder, clientPSK.Binders[selectedPSK].Binder) { | ||
logf(logTypeNegotiation, "Binder check failed for identity %x; [%x] != [%x]", psk.Identity, binder, clientPSK.Binders[selectedPSK].Binder) | ||
return nil, nil, AlertInternalError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like decrypt_error.
logf(logTypeHandshake, "[ServerStateStart] Error generating cookie [%v]", err) | ||
return nil, nil, AlertInternalError | ||
} | ||
shouldSendHRR = appCookie != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this idiomatic go? I would have done if....
a1ccfc5
to
533628f
Compare
Hi, @ekr thanks for the review! Seems like I forgot a bunch of TODOs and a few debug statements. I removed those, and added a bunch of documentation about I really like the idea of storing the negotiated parameters in the cookie as well. By doing so, I was able to remove a rather ugly hack that was necessary to make PSK mode work. Is there anything else besides the The code for the cookie source is what we copied from Google's QUIC implementation 1.5 years ago or so. I changed the cookie source to use a nonce size of 32 byte, as you suggested, and also removed the HKDF. I'm not sure though why're suggesting using a per-message key, I thought using the same key will be ok as long as the nonce is not reused. Can you please explain? |
cdef62e
to
c3810f4
Compare
The issue with the nonce here is that AES-GCM fails catastrophically if you have nonce reuse. If you use a random nonce, you get a non-trivial probability of reuse with enough connections (the 50% mark is at 2^{48} but the risk is unacceptably high well below that). Unfortunately, just making the nonce bigger doesn't help because AES-GCM hashes a >96 bit nonce down to 96 bits. If you want to use GCM, you can guarantee a unique key/nonce pair by generating a longer nonce and then using HKDF to turn the master key into a key/nonce. Because the output is longer, the chance of collision is reduced. |
c3810f4
to
6fe79a8
Compare
I just pushed a fixed. We're now creating a new key (which is HKDF derived using the nonce as a salt) for every nonce. |
cookie_source.go
Outdated
} | ||
|
||
const tokenKeySize = 16 | ||
const tokenNonceSize = 16 | ||
var _ CookieSource = &DefaultCookieSource{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am probably not enough of a Golang wizard, but why is this neded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this is not needed. It makes the compiler check that DefaultCookieSource
actually implements the CookieSource
interface. Since it's an assignment to _
, it will be removed during compilation.
cookie_source.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
return aead.Seal(nonce, nonce, data, nil), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is quite what you want. Rather, you want to do:
r := make([]byte, 32)
rand.Read(r)
key = hkdf(secret, R, []byte("mint cookie key")) // key length
nonce = hkdf(secret, R, []byte("mint cookie nonce")) // 12 bytes
And then you can use nonce the way you have it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, if you prefer you can just read the key and the nonce as successive values from the hkdf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is that supposed to work? When decoding the cookie, I don't know r
any more, and I can't create the AEAD to open the cookie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cookie is r || aead.Seal(blah...)
Right now you are just using nonce == r, so the difference I am proposing is you hkdf r to make the nonce.
cookie_source.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
return cipher.NewGCMWithNonceSize(c, cookieNonceSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want NewGCM()
cookie_source_test.go
Outdated
_, err := cs.DecodeToken([]byte("too short")) | ||
assertError(t, err, "it should reject too short tokens") | ||
_, err = cs.DecodeToken(append(bytes.Repeat([]byte{0}, cookieNonceSize), []byte("invalid token")...)) | ||
assertError(t, err, "it should reject invalid tokens") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be good to have one with a bogus nonce, e.g., by stomping the first byte.
6fe79a8
to
8d11ddc
Compare
@ekr, I implemented the changes for the |
@marten-seemann I think you forgot to push |
I don't think so. I rebased and amended the commit. |
Oh, ok. Will take a look
…On Thu, Nov 30, 2017 at 1:44 PM, Marten Seemann ***@***.***> wrote:
I don't think so. I rebased and amended the commit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#141 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABD1oWBf6g5W4Xakvb7Mqyicm_25VN12ks5s7yGhgaJpZM4QVzSU>
.
|
cookie_source.go
Outdated
aeadNonce := make([]byte, 12) | ||
if _, err := io.ReadFull(h, aeadNonce); err != nil { | ||
return nil, nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. Assuming you didn't change anything else, I think this is good to Go. @bifurcation ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change anything else. This should be ready to merge now.
.circleci/config.yml
Outdated
@@ -0,0 +1,10 @@ | |||
version: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this change is necessary for this functionality, please put it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bifurcation: Is there anything else you need from me to merge this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm my understanding, we basically have a three-layer cookie here:
- Application content (from
CookieHandler
) - Mint framing (
cookie{}
) - Protection (
CookieSource
)
Assuming I'm understanding the structure here, this looks fine to me. Couple of nits and one file name change in the comments, then it's ready to merge.
@@ -82,6 +83,7 @@ var alertText = map[Alert]string{ | |||
AlertUnknownPSKIdentity: "unknown PSK identity", | |||
AlertNoApplicationProtocol: "no application protocol", | |||
AlertNoRenegotiation: "no renegotiation", | |||
AlertStatelessRetry: "stateless retry", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #149
cookie_source.go
Outdated
@@ -0,0 +1,86 @@ | |||
package mint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the other files, this should be cookie-source.go
(reserving underscores for _test
).
conn.go
Outdated
// The default cookie handler uses 32 random bytes as a cookie. | ||
CookieHandler CookieHandler | ||
// A CookieHandler can be used to set and validate a cookie. | ||
// The cookie returned by the CookieHandler will be part of the cookie sent on the wire, and endoced using the CookieSource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "encoded"
conn.go
Outdated
// It should make sure that the Cookie cannot be read and tampered with by the client. | ||
// If non-blocking mode is used, and cookies are required, this field has to be set. | ||
// In blocking mode, a default cookie source is used, if this is unused. | ||
CookieSource CookieSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: CookieSource
doesn't seem like the right name for something whose job is to protect / unprotect cookie values. CookieProtection
or something like that?
8d11ddc
to
f9e1617
Compare
@bifurcation: I rebased this PR on top of the current master, fixed the typo, and renamed the |
@marten-seemann Thanks! Merging and moving on to #145 ... |
This PR changes the server state machine such that sending HelloRetryRequests can be performed statelessly.
In non-blocking mode,
Handshake()
will return a new alert,AlertStatelessRetry
. When receiving this return, an app can discard all mint-related state (i.e. deleteConn
garbage collected). When not using non-blocking mode, the handshake is performed as stateless as possible, at no point the initial ClientHello and the HelloRetryRequest will be stored.The cookie that is sent to the client in the HelloRetryRequest is composed of two parts: In the mint-part of the cookie, we store the hash of the initial ClientHello. The second part can be used to store an application-defined cookie (used when
Config.CookieHandler
is set). The application has to provide a way to encrypt this cookie (it should use authenticated encryption for this). This can be done by settingConfig.CookieSource
. In blocking mode, we default to use AES-GCM if noCookieSource
is set.@ekr, @bifurcation: Please tell me what you think about this. The stateless handshake works for me with this PR, but I can't guarantee I didn't miss anything subtle here. My current understanding of the TLS 1.3 draft doesn't go a lot farther than what I need for my mint changes and the QUIC integration.