Overhaul `CookieJar`: more efficient, more secure, more docs. #76

SergioBenitez · 2017-02-15T08:13:12Z

This commit completely rewrites CookieJar so that it is more efficient while being simpler and more correct overall. At a high-level, a CookieJar now returns borrows to Cookie structures whenever possible. CookieJar and children jars (now PrivateJar and SignedJar) are distinct types. The "encrypted" child jar has been replaced by PrivateJar which uses authenticated encryption. All crypto is done via ring.

A detailed list of changes is below:

The "permanent" child jar has been removed.
A make_permanent method has been added to Cookie.
A permanent method has been added to CookieBuilder.
A named constructor has been added to Cookie.
CookieJar no longer has a generic lifetime parameter.
A CookieJar no longer contains RefCells.
CookieJar::{add_original, add, remove, clear} take &mut self.
CookieJar::new does not take in a key parameter.
CookieJar::find returns a borrow to a Cookie.
CookieJar::remove takes in a Cookie.
CookieJar::clear has been deprecated.
A CookieJar tracks its modifications more robustly, passing a new suite of unit tests.
Computing the delta is zero-cost operation.
Cookie iteration is significantly simplified.
The signed method has been removed from CookieJar.
A new Signed trait was added with a signed(&key) method.
The Signed trait is implemented for CookieJar.
SignedJar cookies sign via ring's HMAC-SHA256 implementation.
The encrypted method has been removed from CookieJar.
A new Private trait was added with a private(&key) method.
The Private trait is implemented for CookieJar.
PrivateJar cookies use AEAD (AES-256-GCM) from ring.
The openssl dependency was dropped.
A ring dependency was introduced.
All items are extensively documented.

Fixes #35.
Fixes #50.
Fixes #64.
Closes #68.
Fixes #75.

Finally, I'd like to, once again, request that I be given maintainer access to the repository/crate. I'd really like to continue improving and maintaining this crate in the future.

alexcrichton · 2017-02-17T17:49:08Z

src/jar.rs

+    /// jar.add(Cookie::new("name", "value"));
+    /// assert_eq!(jar.find("name").map(|c| c.value()), Some("value"));
+    /// ```
+    pub fn find<'a>(&'a self, name: &str) -> Option<&'a Cookie<'static>> {


I think that the 'a here can be elided, right?

Oh also, for symmetry with HashMap we may want to call this method get

At the same time, perhaps this could implement Index with &str to allow doing:

println!("{}", jar["foo"].value());

This may prefer to avoid IndexMut as changes need to be tracked for delta

Yep, the 'a can be elided. Yeah, get sounds good to me. I'm against implementing Index since the contents of the CookieJar are usually determined by some opaque actor (i.e, a web framework), so you don't know what's in the jar. As a result, the index is likely to panic often, thus being error prone. We could return an Option from the Index impl, but then it'll stray from convention.

Sure yeah, seems ok to not implement Index for now (certainly a conservative approach)

alexcrichton

Looks fantastic, thanks so much! Minor comments here and there but nothing major.

Some other random thoughts:

I wonder if we could drop rustc-serialize as a dep? If it's only used for base64 we don't really need it (could use a perhaps more targeted crate)
Maybe chrono should be investigated instead of time? Or perhaps migrating to std::time if possible?

alexcrichton · 2017-02-17T17:49:34Z

src/jar.rs

+    /// jar.add(Cookie::new("name", "value"));
+    /// assert_eq!(jar.find("name").map(|c| c.value()), Some("value"));
+    /// ```
+    pub fn find<'a>(&'a self, name: &str) -> Option<&'a Cookie<'static>> {


Oh also, for symmetry with HashMap we may want to call this method get

alexcrichton · 2017-02-17T17:51:51Z

src/jar.rs

-        for cookie in root.new_cookies.borrow().iter() {
-            ret.push(map.get(cookie).unwrap().clone());
+    /// Removes all cookies from this cookie jar.
+    #[deprecated(since = "0.7.0", note = "calling this method may not remove \


Oh if the crate is going to 0.7.0 I think it's fine to just delete this outright and mention it in the tag's changelog

I wanted to deprecate it to smooth transition because I feel that this method may actually be used in the wild. I don't know if that's true or not, however.

alexcrichton · 2017-02-17T17:54:52Z

src/secure/private.rs

+const BASE64_NONCE_LEN: usize = 16;
+
+/// Extends `CookieJar` with a `private` method to retrieve a private child jar.
+pub trait Private<'a, 'k> {


To me it seems somewhat odd to have this as a trait because I don't think it's implementable by downstream clients, right? (no constructors for PrivateJar)

Is there a reason to have this not just as an inherent method on CookieJar?

I don't...think so. Let me think about that for a bit and change it if I can't think of any reason.

alexcrichton · 2017-02-17T17:56:39Z

src/secure/private.rs

+    ///
+    /// # Panics
+    ///
+    /// Panics if `key` is not exactly 32 bytes long.


For flexibility could we accept keys of different lengths? I could imagine us switching to different encryption algorithms in the future which may have different key length restrictions.

Although I'm not privvy to the security implications here, I'd naively expect this to be able to take any length key and then hash that to 32-bytes and use that as a key, but does that weaken security?

We could accept keys of length greater than 32 bytes, but accepting and padding anything below that weakens security if the attacker knows the size of the key the user is using.

The problem with taking a variable-length key and hashing it is that many people will use a password instead of a key. And a hash of a password isn't a strong key. Taking a fixed length of bytes as a key encourages people to read the docs and not pass a password instead of a real key.

If you are worried about the implications of changing algorithms in the future, then I recommend that you provide a function that returns the currently-required key length.

And/or, you could design the API such that the user only has to supply one master (256-bit) key, and then different keys for HMAC-signing and sealing are derived (e.g. using ring::hkdf) from that master key. That's what I recommend, because then it is less likely that the user will use related keys for HMAC signing and for sealing.

And/or, you could replace the use of HMAC for sign-only operations with the use of seal_in_place(), where the data to be signed-but-not-encrypted is the ad and the plaintext is empty. This will sign with GHASH (GCM) instead of HMAC in the case of AES-GCM. Then it is OK to use the same key for both signing-only and sign-and-encrypt operation.

alexcrichton · 2017-02-17T17:58:44Z

src/jar.rs

+    /// jar.add(Cookie::new("name", "value"));
+    /// assert_eq!(jar.find("name").map(|c| c.value()), Some("value"));
+    /// ```
+    pub fn find<'a>(&'a self, name: &str) -> Option<&'a Cookie<'static>> {


At the same time, perhaps this could implement Index with &str to allow doing:

println!("{}", jar["foo"].value());

This may prefer to avoid IndexMut as changes need to be tracked for delta

alexcrichton · 2017-02-17T18:06:50Z

src/secure/private.rs

+// Keep these in sync, and keep the key len synced with the `private` docs.
+static ALGO: &'static Algorithm = &AES_256_GCM;
+const KEY_LEN: usize = 32;
+const NONCE_LEN: usize = 12;


Is there perhaps a reference to add for why 12 was chosen?

The nonce size is determined by the AEAD algorithm, here, 12 for a 96-bit nonce.

It's ALGO.nonce_len().

alexcrichton · 2017-02-17T18:07:39Z

src/secure/signed.rs

+    ///
+    /// # Panics
+    ///
+    /// Panics if `key` is not exactly 64 bytes long.


(similar concerns about usability with the 32-byte restriction above, just makign a note)

alexcrichton · 2017-02-17T18:09:09Z

src/secure/signed.rs

+    /// assert_eq!(jar.signed(&key).find("name").unwrap().value(), "value");
+    /// ```
+    pub fn add(&mut self, mut cookie: Cookie<'static>) {
+        let key = SigningKey::new(DIGEST, self.key);


Since this is constant, is there a downside to creating this then the Signed jar was created? That'd help avoid the 'k lifetime parameter I think as well.

Oh I think this'd apply to Private as well

The only downside is that we might have to include the lifetime in the future if SigningKey changes or if we use a different library that doesn't store the key directly. In other words, we lose flexibility to swap out ring easily.

Do you think the tradeoff is worth it?

alexcrichton · 2017-02-17T18:11:10Z

src/secure/signed.rs

+    /// signed_jar.remove(Cookie::named("name"));
+    /// assert!(signed_jar.find("name").is_none());
+    /// ```
+    pub fn remove(&mut self, cookie: Cookie<'static>) {


Could this (and other remove methods) perhaps return Option<Cookie<'static>>? (the value that was actually removed)

Well, we reuse the cookie removed from original_cookies, so we'd have to clone there. If the cookie was previously added via add, we could just return.

I'd rather not clone unnecessary. We could return a bool though.

Oh I thought we already had the cookie on hand to return and we just dropped it somewhere. Reading again though it looks like the returned value is squirreled away somewhere else, so yeah let's just return a bool.

alexcrichton · 2017-02-17T18:11:39Z

src/secure/signed.rs

+
+// Keep these three in sync, and keep the key len synced with the `signed` docs.
+static DIGEST: &'static Algorithm = &SHA256;
+const BASE64_DIGEST_LEN: usize = 44;


Could this be a function of KEY_LEN?

No, this is a function of DIGEST_LEN, which isn't declared but would be 256 / 8 = 32. The function would the same as for BASE64_NONCE_LEN.

It's approximately DIGEST.output_len * 5 / 4`, or whatever the overhead calculation for base64 is.

See my comment on BASE64_NONCE_LEN for the true calculation.

alexcrichton · 2017-02-17T18:15:06Z

Also cc @briansmith (usage of ring here), although to me at least it all looked great! (and ergonomic to use)

briansmith

I didn't review this carefully. Also, I was in a hurry so forgive the terseness or any perceived curtness.

briansmith · 2017-02-17T23:56:14Z

src/secure/private.rs

+/// This type is only available when the `secure` feature is enabled.
+pub struct PrivateJar<'a, 'k> {
+    parent: &'a mut CookieJar,
+    key: &'k [u8]


It is better for key to be replaced with opening_key : OpeningKey, sealing_key: SealingKey. This way, you minimize the chance of misuse of the key bytes.

This ties us to ring pretty heavily. See my other comment about whether this tradeoff is worth it or not.

This ties us to ring pretty heavily. See my other comment about whether this tradeoff is worth it or not.

I don't think it more tightly couples the library to ring in any significant way because these fields are private and so can't be accessed outside the module/crate.

I also like @briansmith's suggestion, but @SergioBenitez to leave the door open to having a slice of bytes in the future perhaps this could add a PhantomData<&'k [u8]>?

slice of bytes in the future perhaps this could add a PhantomData<&'k [u8]>?

Is it really a good idea to reference the key as a slice anyway? In typical use, the user would need to create a wrapper around the PrivateJar and key storage just to accomodate the key being a slice. IMO, even if my suggestion isn't taken, the key should be copied into a known value in the PrivateJar.

@briansmith That's exactly what I was thinking: in any case, we could just copy the key into the structure. I've removed the lifetime.

briansmith · 2017-02-17T23:57:55Z

src/secure/private.rs

+    fn unseal(&self, value: &str) -> Result<String, &'static str> {
+        let (nonce_s, sealed_s) = value.split_at(BASE64_NONCE_LEN);
+        let nonce = nonce_s.from_base64().map_err(|_| "bad nonce base64")?;
+        let mut sealed = sealed_s.from_base64().map_err(|_| "bad sealed base64")?;


All else being equal, IMO it is better to do the base64 decoding once and then split the decoded bytes, e.g. using split_at_mut(ALGO.nonce_len()).

briansmith · 2017-02-18T00:02:22Z

src/secure/private.rs

+    pub fn find(&self, name: &str) -> Option<Cookie<'static>> {
+        if let Some(cookie_ref) = self.parent.find(name) {
+            let mut cookie = cookie_ref.clone();
+            if cookie.value().len() <= BASE64_NONCE_LEN {


Shouldn't this check be in unseal() to avoid a panic?

The docstring for unseal clarifies its intentions, and since it's used a single time, it doesn't really matter where this is.

briansmith · 2017-02-18T00:08:17Z

src/secure/private.rs

+    pub fn add(&mut self, mut cookie: Cookie<'static>) {
+        // Generate the nonce.
+        let mut nonce = [0; NONCE_LEN];
+        SystemRandom::new().fill(&mut nonce).expect("couldn't randomly fill nonce");


At a minimum, please document the fact that using a random nonce this short is not the best thing.

The nonce is only 96 bits. That means there is a 2⁴⁸ chance of collision even assuming the RNG is perfect. That's usually too low for nonce generation, which is why I don't recommend using regular AEAD algorithms for this kind of application, and also why there is no SealingKey::open_in_place_with_own_key(). In briansmith/ring#411 and other issues, we're tracking adding better algorithms to ring to support this kind of use case.

I don't think this is something to be concerned about. AES-256-GCM is the industry standard AEAD, and while 96 bits of nonce isn't the best-possible-thing, I don't think it's reasonable to be concerned about a nonce collision occurring after 281,474,976,710,656 cookies have been encrypted.

In any case, I designed the code such that replacing the AEAD algorithm with something better is simply changing 3 trivial lines. Once there's something better, we should certainly move to that.

AES-256-GCM is the industry standard AEAD, and while 96 bits of nonce isn't the best-possible-thing, I don't think it's reasonable to be concerned about a nonce collision occurring after 281,474,976,710,656 cookies have been encrypted.

Here is what the actual AES-GCM standard, from NIST, says:

Unless an implementation only uses 96-bit IVs that are generated by the deterministic construction: The total number of invocations of the authenticated encryption function shall not exceed 2³², including all IV lengths and all instances of the authenticated encryption function with the given key.

(The emphasis is NIST's, not mine.)

In other words, if you use random nonces with AES-GCM then you must not encrypt more than ~4 billion cookies with the same key before replacing the key, according to the standard.

briansmith · 2017-02-18T00:10:59Z

src/secure/private.rs

+        let key = SealingKey::new(ALGO, self.key).expect("sealing key creation");
+
+        // Setup the input and output for the sealing operation.
+        let overhead = ALGO.max_overhead_len();


Why not make in_out contain nonce+ciphertext+tag (tag == overhead), and then base64-encode the whole thing in one shot? It seems like the resulting value would be slightly more efficiently encoded, and you'd save some mallocs in this function and in unseal(), I think.

Then...

Awesome suggestion. Thank you!

briansmith · 2017-02-18T00:34:43Z

src/secure/signed.rs

+
+// Keep these three in sync, and keep the key len synced with the `signed` docs.
+static DIGEST: &'static Algorithm = &SHA256;
+const BASE64_DIGEST_LEN: usize = 44;


It's approximately DIGEST.output_len * 5 / 4`, or whatever the overhead calculation for base64 is.

briansmith · 2017-02-18T00:35:30Z

src/secure/signed.rs

+use {Cookie, CookieJar};
+
+// Keep these three in sync, and keep the key len synced with the `signed` docs.
+static DIGEST: &'static Algorithm = &SHA256;


IMO, SIGNING_HMAC_DIGEST_ALG or similar is a better name for this. In particular, it is strange that we have DIGEST here and then ALGO for the AEAD algorithm.

Yeah, I toyed with the name a bit here. SIGNING_HMAC_DIGEST_ALG is just too long for no real gain. Maybe just ALGO, or HMAC_DIGEST?

I think HMAC_DIGEST is better than ALGO.

briansmith · 2017-02-18T00:37:04Z

src/secure/private.rs

+// Keep these in sync, and keep the key len synced with the `private` docs.
+static ALGO: &'static Algorithm = &AES_256_GCM;
+const KEY_LEN: usize = 32;
+const NONCE_LEN: usize = 12;


It's ALGO.nonce_len().

briansmith · 2017-02-18T00:37:17Z

src/secure/private.rs

+
+// Keep these in sync, and keep the key len synced with the `private` docs.
+static ALGO: &'static Algorithm = &AES_256_GCM;
+const KEY_LEN: usize = 32;


Maybe document that this is ALGO.key_len().

I think the "keep in sync comment" implies this pretty directly.

briansmith · 2017-02-18T00:38:15Z

src/secure/signed.rs

+// Keep these three in sync, and keep the key len synced with the `signed` docs.
+static DIGEST: &'static Algorithm = &SHA256;
+const BASE64_DIGEST_LEN: usize = 44;
+const KEY_LEN: usize = 64;


Why did you choose the digest algorithm block size instead of the digest algorithm output size?

FWIW, in the upcoming, not-yet-released version of ring, I've added a new function to generate a serializable HMAC key; see briansmith/ring@f0d4760, based on this review.

Based on this review, in the upcoming version of ring, I added a new API to help people generate serializable HMAC keys of a good length. See
briansmith/ring@f0d4760. Using the block size isn't wrong though.

No great reason except that it makes brute-force-key searching harder. So while the security of any leaked, decrypted cookies via some collision doesn't increase, the probability that they've found the real key is greatly reduced. Likely doesn't matter in practice.

No great reason except that it makes brute-force-key searching harder. So while the security of any leaked, decrypted cookies via some collision doesn't increase, the probability that they've found the real key is greatly reduced. Likely doesn't matter in practice.

It's not a big deal. I just want to point out that if the difference between a 256-bit key and a 512-bit key matters here then SHA-256 probably isn't the right thing to use. In general I recommend people use 256-bit keys for SHA-256 and 512-bit keys for SHA-384 (and don't use SHA-512).

You're right. I'll just use 32-byte keys.

SergioBenitez · 2017-02-18T04:13:36Z

I wish we could migrate to std::time, but unless I'm missing something, it doesn't have a concept of absolute calendar time such as Tm. I've tried working with chrono before and unfortunately am not a fan. :(

On the base64 front: I'm totally down for using something more focused, but what? I'd guess the base64 crate would be the thing to use. Do you think it's worth it? rustc-serialize is a fairly lightweight dependency, even though it includes much more functionality than we're using.

briansmith · 2017-02-18T21:15:46Z

src/secure/private.rs

+// Keep these in sync, and keep the key len synced with the `private` docs.
+static ALGO: &'static Algorithm = &AES_256_GCM;
+const KEY_LEN: usize = 32;
+const NONCE_LEN: usize = 12;


I reviewed the whole PR and I think you don't need the KEY_LEN or NONCE_LEN constants at all. You can now always use ALGO.key_len() and ALGO.nonce_len() instead, AFAICT, since there's no place where you need them to be compile-time constants.

briansmith · 2017-02-18T21:17:48Z

src/secure/signed.rs

+            return Err("length of value is <= BASE64_DIGEST_LEN");
+        }
+
+        let (digest_str, value) = cookie_value.split_at(BASE64_DIGEST_LEN);


If you base64-decode first and then split, you don't need to define BASE64_DIGEST_LEN, since you can use HMAC_ALG.output_len for the split index instead.

If you base64-decode first and then split, you don't need to define BASE64_DIGEST_LEN, since you can use HMAC_ALG.output_len for the split index instead.

Sorry, I just realized that it doesn't make sense to base64-encode the entire value since, presumably, the initial value was already header-safe and base64-encoding would be unnecessarily wasteful.

briansmith · 2017-02-18T23:03:53Z

FWIW, I released ring 0.7 with a new API for the HMAC key length recommendation; see https://briansmith.org/rustdoc/ring/hmac/fn.recommended_key_len.html. Your choice is also correct, but if you use recommended_key_len(HMAC_DIGEST_ALG) instead, then you don't need to have a separate KEY_LEN constant.

alexcrichton · 2017-02-21T16:33:30Z

Thanks so much for the review @briansmith!

alexcrichton · 2017-02-21T16:34:28Z

I wish we could migrate to std::time, but unless I'm missing something, it doesn't have a concept of absolute calendar time such as Tm.

Yeah that's true, all we'd get is offsets from SystemTime and such. I'd love to drop time as a dep and leave heavy lifting (if necessary) to other crates, but the time may not yet be ripe to do so.

On the base64 front: I'm totally down for using something more focused, but what? I'd guess the base64 crate would be the thing to use. Do you think it's worth it? rustc-serialize is a fairly lightweight dependency, even though it includes much more functionality than we're using.

Oh just kinda a random thought, I'd love to burn rustc-serialize to the ground but it's a private dep so we can change this at any time :)

alexcrichton · 2017-02-21T16:35:22Z

src/secure/signed.rs

+    /// # Panics
+    ///
+    /// Panics if `key` is not exactly 64 bytes long.
+    #[doc(hidden)]


Could this be a submodule of jar so the constructor doesn't need to be public to the world?

briansmith · 2017-02-21T19:35:27Z

On the base64 front: I'm totally down for using something more focused, but what? I'd guess the base64 crate would be the thing to use. Do you think it's worth it? rustc-serialize is a fairly lightweight dependency, even though it includes much more functionality than we're using.

In #78 I suggest using Z85 encoding instead of Base64 encoding. It's more space-efficient and just as compatible as Base64-encoding, AFAICT. Also there are two crates that implement Z85 already.

alexcrichton · 2017-02-21T20:08:51Z

@briansmith oh oops sorry! I was away all weekend and been digging backwards through email so I ended up seeing that later :). In any case, sounds like a great idea to me!

alexcrichton · 2017-02-22T17:27:45Z

src/jar.rs

-        fn sign(root: &Root, cookie: Cookie<'static>) -> Cookie<'static> {
-            secure::sign(&root._key, cookie)
-        }
+    pub fn private<'a, 'k>(&'a mut self, key: &[u8]) -> PrivateJar<'a> {


This 'k is now unused I think

(and I think the 'a can be elided)

alexcrichton · 2017-02-22T17:30:13Z

src/secure/private.rs

+        let mut data;
+        let output_len = {
+            // Create the `SealingKey` structure.
+            let key = SealingKey::new(ALGO, &self.key).expect("sealing key creation");


Could this be stored in the PrivateJar struct itself?

alexcrichton · 2017-02-22T17:36:56Z

Ok so API-wise I think we're all set here modulo the key length question. I'd like to not have a restriction that the keys passed in are a precise length because that seems (a) difficult to change over time and (b) surprising in terms of trying to match it.

It sounds like hashing the input is a bad idea, but could we perhaps say that you need to pass in at least X bytes of data and we use slices within that for various keys? That way we can also provide a sample command for generating the keys to pass in perhaps.

briansmith · 2017-02-23T01:37:41Z

I'd like to not have a restriction that the keys passed in are a precise length because that seems (a) difficult to change over time and (b) surprising in terms of trying to match it.

Keep in mind that if/when the algorithms change, the keys need to change too. For example, let's say you switch AES-256-GCM to ChaCha20-Poly1305. Even though the key size is the same, it isn't safe to use the same key (or, rather, we assume it is unsafe and nobody has proven otherwise, nor will anybody ever do so).

Thus, if you want to have some flexibility then you need to specify, at least, a minimum key length, e.g. 256 bits. Then you can use HKDF, (see ring::hkdf::extract_and_expand) to derive the keys you need. The info parameter needs to be something that unambiguously identifies the algorithm, usage, and context, e.g. "HTTP-cookie-encrypt-and-auth;AES-256-GCM;v1.0;shared-across-origin:" + origin for the AEAD key and "HTTP-cookie-auth-only;HMAC-SHA2-256;v1.0;shared-across-origin:" + origin for the HMAC key, where origin is the origin of the cookie.

Then whenever you change anything related to the crypto in cookie-rs, you need to change the those labels. Also note that with this scheme, different origins will always have different keys. For this latter reason alone, it may be a good idea to move to the derived-key scheme.

However, still, I recommend that instead of specifying a minimum key length, you instead specify a fixed key length. In general we (crypto people) don't like variables, especially regarding key lengths. (We now generally regard the way HMAC's key was specified as a mistake.)

SergioBenitez · 2017-02-23T02:14:14Z

@alexcrichton I'm not sure I understand what you mean by "sample command for generating the keys to pass in". Do you mean like a shell command? If so, how would that be any different if we had a fixed-key length? And if you mean some example Rust code, then you should consider that most of these keys will be fixed. That is, they won't be generated at runtime. This is because (especially in production) you generally want to accept cookies that were signed the last time you started your application or by other instances of that application. As a result, you need one, fixed key.

In general, I'm in agreement with @briansmith here, as evidenced by my choice of a fixed-key length to begin with.

alexcrichton · 2017-02-23T16:24:45Z

@briansmith that sounds like a great idea! (sorry I'm pretty unfamiliar with hkdf). I'd be totally fine though requiring a fixed key length if we can use that to derive further keys (even if we change underlying algorithms)

@SergioBenitez eh I basically just mean as a new users I see this key restriction or these random constants pass in and I'm not sure how to do so myself. Just some docs for "here's how to create your own key then pass it in".

SergioBenitez · 2017-02-24T02:52:57Z

@alexcrichton I think allowing variable sized keys or extracting keys of different lengths from one key is something that should be built on-top of cookie but not done by cookie itself. I think requiring a fixed-size key is the safest thing we can do. What's more, I don't see the 32-byte key size changing any time soon. The only algorithm I can see us switching to anywhere in the vicinity of the near future is AES-256-GCM-SIV which, if I'm not mistaken, also requires 32-byte keys.

As far as how to get these random constants...the safest way (and the way Rocket will do this) is to encode the key in a text-safe format (base64 in Rocket) and then get the byte representation of the key. It's trivial to generate a safe 32-byte random key in this way: openssl rand -base64 32.

briansmith · 2017-02-25T21:15:13Z

don't see the 32-byte key size changing any time soon. The only algorithm I can see us switching to anywhere in the vicinity of the near future is AES-256-GCM-SIV which, if I'm not mistaken, also requires 32-byte keys.

But, you shouldn't use the same key for different algorithms. So if there's any possibility that the algorithm will change (and I think there is), it is better to derive the keys from the master key.

As far as how to get these random constants...the safest way (and the way Rocket will do this) is to encode the key in a text-safe format (base64 in Rocket) and then get the byte representation of the key. It's trivial to generate a safe 32-byte random key in this way: openssl rand -base64 32.

Yes, and this is why variable-length inputs are bad, because some callers are likely to pass in the base64-encoded key bytes, instead of decoding the base64 first. Using base64-encoded bytes as a key, without decoding them, is bad.

SergioBenitez · 2017-02-25T21:21:35Z

But, you shouldn't use the same key for different algorithms. So if there's any possibility that the algorithm will change (and I think there is), it is better to derive the keys from the master key.

How do we defend against this with hkdf? Use a different salt each time we change the algorithm?

Running hkdf isn't free. Again, I think if a library on-top of cookie wants to use hkdf, then so be it, but it's not something I think cookie should handle.

Yes, and this is why variable-length inputs are bad, because some callers are likely to pass in the base64-encoded key bytes, instead of decoding the base64 first. Using base64-encoded bytes as a key, without decoding them, is bad.

Indeed.

briansmith · 2017-02-25T21:26:00Z

Running hkdf isn't free. Again, I think if a library on-top of cookie wants to use hkdf, then so be it, but it's not something I think cookie should handle.

Yes, I explained in #76 (comment).

I don't want to get too hung up in providing advice here, so I won't debate it further.

alexcrichton · 2017-02-27T19:19:21Z

@SergioBenitez lets use hkdf. "This probably won't change soon" sounds like "when this changes it'll be too difficult to change when we really need to". Let's cover our bases and start out conservatively? If this really has a perf impact or something like that we can reevaluate.

This commit completely rewrites `CookieJar` so that it is more efficient while being simpler and more correct overall. At a high-level, a `CookieJar` now returns borrows to `Cookie` structures whenever possible. `CookieJar` and children jars (now `PrivateJar` and `SignedJar`) are distinct types. The "encrypted" child jar has been replaced by `PrivateJar` which uses authenticated encryption. All crypto is done via `ring`. A detailed list of changes is below: * The "permanent" child jar has been removed. * A `make_permanent` method has been added to `Cookie`. * A `permanent` method has been added to `CookieBuilder`. * A `named` constructor has been added to `Cookie`. * `CookieJar` no longer has a generic lifetime parameter. * A `CookieJar` no longer contains `RefCell`s. * `CookieJar::{add_original, add, remove, clear}` take `&mut self`. * `CookieJar::new` does not take in a `key` parameter. * `CookieJar::find` returns a borrow to a `Cookie`. * `CookieJar::remove` takes in a `Cookie`. * `CookieJar::clear` has been deprecated. * A `CookieJar` tracks its modifications more robustly, passing a new suite of unit tests. * Computing the `delta` is zero-cost operation. * Cookie iteration is significantly simplified. * The `signed` method has been removed from `CookieJar`. * A new `Signed` trait was added with a `signed(&key)` method. * The `Signed` trait is implemented for `CookieJar`. * `SignedJar` cookies sign via ring's HMAC-SHA256 implementation. * The `encrypted` method has been removed from `CookieJar`. * A new `Private` trait was added with a `private(&key)` method. * The `Private` trait is implemented for `CookieJar`. * `PrivateJar` cookies use AEAD (AES-256-GCM) from ring. * The `openssl` dependency was dropped. * A `ring` dependency was introduced. * All items are extensively documented. Fixes #35. Fixes #50. Fixes #64. Closes #68. Fixes #75.

…lity.

SergioBenitez · 2017-03-01T08:07:42Z

Okay. Now using HKDF via a Key struct. I quite like this approach.

This should be all!

alexcrichton · 2017-03-01T17:31:20Z

👍

alexcrichton · 2017-03-01T17:31:42Z

@SergioBenitez do you want to do the release on this one?

SergioBenitez · 2017-03-02T01:00:54Z

Released! :)

This was referenced Feb 15, 2017

cookie jar implementation seanmonstar/reqwest#14

Closed

Encrypt and sign session cookies rwf2/Rocket#20

Closed

Sessions? rwf2/Rocket#196

Closed

alexcrichton reviewed Feb 17, 2017

View reviewed changes

briansmith reviewed Feb 18, 2017

View reviewed changes

briansmith mentioned this pull request Feb 20, 2017

Use an AEAD cipher instead of AES-CBC + HMAC-SHA1 #50

Closed

alexcrichton reviewed Feb 21, 2017

View reviewed changes

alexcrichton mentioned this pull request Feb 21, 2017

Use Z85 encoding HMAC values and encrypted values #78

Closed

alexcrichton reviewed Feb 22, 2017

View reviewed changes

Clean up key = value parsing.

ad1ec04

SergioBenitez added 10 commits March 1, 2017 00:06

Rename CookieJar::find to CookieJar::get.

9357bce

Make unseal and verify not panic on bad value lengths.

6aa7ced

Base64 encode the nonce and sealed value together.

1824633

Make 'private' and 'signed' direct methods of 'CookieJar'.

8b4c726

Store keys directly to afford a generic lifetime.

5d39c58

Use 32-byte keys for HMAC-SHA256.

2cb2916

Update to ring 0.7.

c9e869f

Rename DIGEST to HMAC_DIGEST.

ab2339d

Take crypto keys indirectly via 'Key' for improved security and usabi…

4ba4735

…lity.

alexcrichton merged commit 62ae077 into rwf2:master Mar 1, 2017

Overhaul CookieJar: more efficient, more secure, more docs. #76

Overhaul CookieJar: more efficient, more secure, more docs. #76

Conversation

SergioBenitez commented Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 17, 2017

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez commented Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Feb 18, 2017

alexcrichton commented Feb 21, 2017

alexcrichton commented Feb 21, 2017

Choose a reason for hiding this comment

briansmith commented Feb 21, 2017

alexcrichton commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 22, 2017

briansmith commented Feb 23, 2017 • edited Loading

Overhaul `CookieJar`: more efficient, more secure, more docs. #76

Overhaul `CookieJar`: more efficient, more secure, more docs. #76

SergioBenitez commented Feb 15, 2017 •

edited

Loading

SergioBenitez Feb 18, 2017 •

edited

Loading

SergioBenitez commented Feb 18, 2017 •

edited

Loading

briansmith commented Feb 23, 2017 •

edited

Loading