securecookie: v2

[elithrar]

Preface: we're thinking about what a gorilla/sessions v2 would look like. This naturally extends to securecookie, which provides a lot of the underlying implementation.

Key areas for improvement in v2:

• Simplify the error interfaces: multi-error and the error types are overly complex and lead to a lot of error-handling code downstream. Generalizing to user-error (and making it harder to provide bad keys and input!), authentication error (crypto) and data error (marshalling bugs) should be enough.
• Replace AES-CTR + HMAC-SHA-256 with XSalsa20Poly1305 (via nacl/secretbox). This is an AEAD construct that provides encryption+authentication together, securely.
• Make the key rotation interface better (variadic is confusing: move to an Option struct)
• Keep all of the great fuzzing tests.

[elithrar]

WIP branch here: https://github.com/gorilla/securecookie/tree/elithrar/v2

Next up:

• Consider whether we need to implicitly run user-provided keys through a KDF on init. This complicates the API as we would need to maintain state, which isn't how securecookie is used right now. The alternative is to make all examples use something like scrypt.GenerateFromPassword to guide inexperienced users down the right path to providing strong keys.
• Support multiple keys - replacing the existing Codec piece with Options.RotatedKeys and attempting to use provided keys on each signature verification failure. Needs good docs; cascading through multiple keys is slow. RotatedKeys should only be used for a short period of time.
• Set good defaults for expirations.

Future:

• Support the SameSite cookie attribute
• Update all existing tests

[mholt]

I was just gonna post and ask about SameSite attribute. Thanks for beating me to it! ;)

[elithrar]

Update: still a WIP. I'm waiting for Go's "dep" tool to land so I can version the library more cleanly.

[bbigras]

Any progress?

[elithrar]

dep hasn't landed in the Go toolchain yet, so nothing yet. There is an (old) WIP branch - https://github.com/gorilla/securecookie/tree/elithrar/v2 - but unlikely to commit significant time until dep lands. Is there a specific need that v2 would address for you?

…

On Thu, Oct 26, 2017 at 1:38 PM Bruno Bigras ***@***.***> wrote: Any progress? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcPQQ3Ji3iDWbImBEs6eE1ZQ7xSkzks5swO3bgaJpZM4LwqsY> .

[bbigras]

dep hasn't landed in the Go toolchain yet

Oh sorry. I saw "dep is safe for production use" on https://github.com/golang/dep and I assumed it landed.

Is there a specific need that v2 would address for you?

I have been looking forward to use SameSite since support was added for it in Chrome. I'm not aware of the other changes.

[elithrar]

We can probably look to add SameSite to the current version (I'll also accept a PR!)

…

On Thu, Oct 26, 2017 at 2:38 PM Bruno Bigras ***@***.***> wrote: dep hasn't landed in the Go toolchain yet Oh sorry. I saw "dep is safe for production use" on https://github.com/golang/dep and I assumed it landed. Is there a specific need that v2 would address for you? I have been looking forward to use SameSite since support was added for it in Chrome. I'm not aware of the other changes. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcAsW4WiOxPEBRi74W_0QbW3rrdKzks5swPvkgaJpZM4LwqsY> .

[srikrsna]

I don't see how a cookie attribute like SameSite would apply to this when its only concerned with the value.

[srikrsna]

Any update on this and how can I help in the WIP branch?

[elithrar]

There's no priority on this right now. Is there a particular need that would be fulfilled by v2? @srikrsna

[srikrsna]

This would lead to sessions v2 @elithrar

[elithrar]

@srikrsna Sure, but what do you need out of sessions v2? What about sessions v1 doesn't fit your use-case? (be precise, it helps us understand what users are after!)

[elithrar]

Proposed, simplified API:

type SecureCookie struct {
    // fields
}

// No need to provide two keys: we authenticate by default, or use an AEAD construct for encryption.
func New(key []byte, options *Options) (*SecureCookie, error) { }

// TBD as to whether we make these methods, but I prefer simple struct members where possible
type Options struct {
    // No need to keep *Multi methods: if len(opts.RotatedKeys) > 0 then we can attempt those
    RotatedKeys [][]byte
    // Not enabled by default. Authenticated cookies are, however.
    Encrypt bool
    MaxAge int
    MaxLength int
    Serializer Serializer
}

// Remains as-is
type Serializer interface {
    Serialize(src interface{}) ([]byte, error)
    Deserialize(src []byte, dst interface{}) error
}

// Encode & Decode use the serializer defined by Options.Serializer
func (sc *SecureCookie) Encode(name string, val {}interface) (string, error) { } 
func (sc *SecureCookie) Decode(name string, val string, dest interface{}) error { } 

This gets us to a dramatically simpler API.

• No need to care about generating multiple Codecs , or the Codec type
• We can lazily initialize the authentication or encryption instances for rotated keys (e.g. a map of key => instance, as needed)
• Much smaller API surface: the goal of the library is to give you a cookie value that you can call http.SetCookie(w, &http.Cookie{Name: "name", Value: valueFromSecureCookie, MaxAge: maxAge}, and similarly extract those values back to Decode
• It is still a lower-level library: just the primitives
• Make all the hard crypto choices for the user: NaCl blessed algorithms, and no options for otherwise.

I was considering how we could re-purpose https://golang.org/pkg/encoding/#BinaryMarshaler (and Unmarshaller) into our own interface via composition, but that might be too much of a burden upon the package user (their types need to implement these methods).

[srikrsna]

@elithrar Apologies for not being clear. The requirement is the new Context with sessions v2. Last I checked there is leak due to the shallow copying involved and I am using the new context.

[elithrar]

@srikrsna - if you have a leak, please file an issue in sessions. You don’t need v2 to avoid that.

[srikrsna]

@elithrar there's one already gorilla/sessions#122

[balasanjay]

Some drive-by thoughts:

1. Why default to no encryption? Given your two chosen algorithms, the overhead is 32 bytes for authentication and 40 bytes for authenticated encryption. With such a minor difference in overhead, it seems like you should just make the safer choice and let the discerning client explicitly choose to skip encryption, if they really want to save 8 bytes. (Also, as I understand it, HTTP2 will give headers that are constant between requests a fixed identifier, and compress these away)

2. It looks like you're encrypting the cookie name along with the payload. Why not drop down a tiny bit to the underlying AEAD API and pass the name to "additional data", so its authenticated, but not encrypted (and then leave it out of the payload altogether). Arguably, the AEAD APIs are pretty much as safe as NaCl directly, and I believe there's an implemention in x/crypto of chacha20poly1305 which has 256 bits of security and has a large enough nonce that its safe for this use-case (and has just 28 bytes of total overhead, which is even less than SHA512-256).

3. Rather than doing trial decryption, Google's crypto libraries tend to prepend a 4-byte truncated hash of the key to the encrypted payload. For instance, the late Keyczar did that, Tink (new Keyczar) does that, and actually the current session ticket support in crypto/tls does that too. Thoughts on doing the same here?

4. I'd also suggest starting the encoding with a version byte (essentially, just a constant "0") for now, to let you version the actual format gracefully. (The decryption should verify that there's exactly a constant "0" prefix, or else reject it for being an unknown format).

[elithrar]

I have an updated v2 branch here: https://github.com/gorilla/securecookie/tree/elithrar/v2

Following up on @balasanjay's comments:

1. Yes, agree here on just going for encryption in full: my updated branch uses secretbox/nacl (XSalsa20Poly1305), specifically as it's: a) well tested and b) more secure against nonce re-use vs. AES-GCM, which appeals to the use-case of this library and its user base.

2. I'd prefer to use nacl/secretbox here, although the underlying API doesn't provide an argument for additional data. The higher level API doesn't implement cipher.AEAD unfortunately. I don't think this is a huge pain, as package users aren't going to be interacting with payloads directly. Encrypting name|timestamp|serializedData in full is acceptable.

3. Not a bad idea. That would make key rotation much cleaner. Let me look at the examples in crypto/tls to make sure we don't expose ourselves to any issues in doing so.

4. Maybe. I want to be careful around "versioning" and any idea of "backwards compat." -> I'd rather version the entire library around its underlying crypto construct. If XSalsa20Poly1305 is broken, the world will have some serious issues, and we'd move to something else + bump the major version, which would invalidate all previous sessions.

[elithrar]

Note: I still have work to do around:

• Errors: I think the error type we expose is OK, but want to sit on it for a bit.
• Tests: need to update all tests, inc. fuzz tests
• Versioning: Need to consider how we expose a v2 safely come Go 1.12 (when vgo is formally in the toolchain, as part of go get) so that "new" users get v2, but users who rely on v1 can still use v1.

[elithrar]

Thinking further on how we do key rotation: there's the likelihood that users would want to change Options as they change keys. Thus, the RotatedKeys struct member should possibly be a slice of { key []byte, options *Options }.

[desimone]

Hi @elithrar , just a note -- though you are likely already aware -- XChaCha20-Poly1305 has made it into /x/crypto which does use cipher.AEAD. I've implemented something similar to your V2 branch using XChaCha. I would be happy to help move this effort forward.

[lucille-bellepleure]

How's sameSite coming along?

[ryanerwin]

I'm not seeing the same message under Chrome, but I typically use Firefox and when using securecookie without the SameSite option many error messages are triggered that clogs up the Console...

For every cookie you'll get:

Cookie “_foo_session” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite

So if you're using several cookies, you'll get a lot of these warnings... Would be nice to support the SameSite tag so these warnings can be bypassed. For development, we probably want to use SameSite=Lax as development is often run on localhost without https.