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

securecookie: v2 #43

Open
1 of 4 tasks
elithrar opened this issue Jan 29, 2017 · 25 comments · May be fixed by #88
Open
1 of 4 tasks

securecookie: v2 #43

elithrar opened this issue Jan 29, 2017 · 25 comments · May be fixed by #88

Comments

@elithrar
Copy link
Contributor

elithrar commented Jan 29, 2017

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
Copy link
Contributor Author

elithrar commented Feb 22, 2017

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
Copy link

mholt commented Feb 23, 2017

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

@elithrar
Copy link
Contributor Author

elithrar commented Jun 1, 2017

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

@bbigras
Copy link

bbigras commented Oct 26, 2017

Any progress?

@elithrar
Copy link
Contributor Author

elithrar commented Oct 26, 2017 via email

@bbigras
Copy link

bbigras commented Oct 26, 2017

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
Copy link
Contributor Author

elithrar commented Oct 26, 2017 via email

@srikrsna
Copy link

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

@srikrsna
Copy link

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

@elithrar
Copy link
Contributor Author

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

@srikrsna
Copy link

This would lead to sessions v2 @elithrar

@elithrar
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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

@srikrsna
Copy link

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

@balasanjay
Copy link

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
Copy link
Contributor Author

elithrar commented Jun 3, 2018

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
Copy link
Contributor Author

elithrar commented Jun 3, 2018

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
Copy link
Contributor Author

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
Copy link

desimone commented Jan 17, 2019

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
Copy link

How's sameSite coming along?

@ryanerwin
Copy link

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.

@elithrar elithrar removed their assignment Jul 7, 2023
@devhaozi devhaozi linked a pull request Aug 24, 2024 that will close this issue
12 tasks
@devhaozi
Copy link

Hello maintainers, I'm trying to work on v2 and I'd like to know what key rotation interface you envision?

@devhaozi
Copy link

Since I haven't received any response, I created a new package, please go to: https://github.com/go-rat/securecookie and https://github.com/go-rat/sessions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

9 participants