Add support for encrypting cookies via node crypto's key pairs

[pluma]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Currently the plugin takes a signing secret as an option to allow cryptographically signing cookies. It Would Be Cool™ if the plugin could also take a node crypto key (for symmetric encryption) or key pair (for assymmetric encryption) to allow encrypting and decrypting the cookie payload. I'm not sure how useful it would be to be able to swap out the encryption/decryption logic (e.g. using symmetric encryption would require sending both the encrypted payload and the initialization vector, so this would require picking an arbitrary format to represent the two) as with the signing functionality so I'd consider that out of scope.

Motivation

Previously discussed in #214. Implementing this would eliminate the need for fastify-secure-session. Having this functionality directly in the cookie library would also make it more obvious it can be used for other things than sessions.

Example

// symmetric encryption, faster
const generateKeyAsync = util.promisify(crypto.generateKey);
const key = await generateKeyAsync("aes", { length: 256 });
app.register(require("@fastify/cookie"), { keys: key });

// asymmetric encryption, slower
const generateKeyPairAsync = util.promisify(crypto.generateKeyPair);
const { privateKey, publicKey } = await generateKeyPairAsync("rsa", { modulusLength: 2048 });
app.register(require("@fastify/cookie"), { keys: { privateKey, publicKey } });

// loading a key for symmetric encryption from a file
const buf = fs.readFileSync("./secret.key");
const key = crypto.createSecretKey(buf);
app.register(require("@fastify/cookie"), { keys: key });

// loading a key pair from files
const privateBuf = fs.readFileSync("../.ssh/id_rsa");
const publicBuf = fs.readFileSync("../.ssh/id_rsa.pub");
const privateKey = crypto.createPrivateKey(privateBuf);
const publicKey = crypto.createPublicKey(publicBuf);
app.register(require("@fastify/cookie"), { keys: { privateKey, publicKey } });

// reading a cookie
const session = req.decryptCookie(req.cookies.sess);

// setting a cookie
reply.setCookie("sess", reply.encryptCookie("very secret session state data"));

// alternative
reply.setCookie("sess", "very secret session state data", { encrypted: true });

[mcollina]

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[pluma]

/cc @Uzlopak FYI, since you suggested this in #214.

[gurgunday]

Hey! I was actually working on a PR and hope to open a draft PR soon (tonight), but I'm pretty sure Uzlopak would do a better job lol

Edit: #217

[Uzlopak]

I will write tonight (german time) an answer.

Stay tuned.

[Uzlopak]

I personally dont like to add the symmetric encryption. It suggests people, that it is a good idea to store bigger chunks of data as a cookie. See #208 So we need atleast to add a note, to be careful and not to think that we can have multiple kb of data stored as a cookie. Also the proposal of #217 of course base64-encodes the encrypted data... yeah that adds again 30% overhead. There is no alternative, but still. The memory size is limitted.

I dont like that we by default set aes in your PR #217. We should either be unopinionated and set nothing as default or we should use the most secure implementation of them all as default.

In fastify-secure-session we use libsodiums secret key box. secret key box uses xsalsa20-poly1905. node supports chacha20-poly1305. To have a comparable security level we would need to add the extended nonce support to get xchacha20-poly1305. I did not dig deeper on how to do it. But I would personally use xchacha20-poly1305 if I would have the choice.

Thats the "beauty" of fastify-secure-session: It uses libsodium and the devs from libsodium decided to use xsalsa20-poly1305. We trust their decision that it is cryptographically hardened. But at the moment we suggest something as default in fastify-cookie people trust our choice and think the default value wont be wrong/weak.

I am totally ok, to give examples in the readme on how to use aes or chacha20-poly1305. But here should be a big fat Note that they should investigate and think why they use aes or chacha20-poly1305 or whatever in their product. Something similar to https://github.com/fastify/csrf-protection#security-disclaimer

See also this nice page https://soatok.blog/2020/07/12/comparison-of-symmetric-encryption-methods by @soatok

[gurgunday]

Hey, thanks for the detailed response! I understand your point of view and you are right that it makes more sense to prevent people from shooting themselves in the foot. In fact, this is why I love Fastify: it has the perfect balance of being unopinionated and making small-but-important architectural decisions.

I prefer encrypted cookies because they make it so much faster to set up basic stuff like sessions and they are easy to reason about.

My point of view is that lowering the number of dependencies and plugins is good for the long-term health of the ecosystem. Today, fastify-cookie already offers crypto functionality in the form of signing, and while I love using fastify-secure-session with libsodium for encrypted cookies, I do think it would be more intuitive if this plugin also provided the means to apply encryption using sane defaults from Node's crypto module — like chacha20-poly1305.

The defaults can be changed with major bumps from time to time, and in the end, we would have similar functionality to secure-session, but with less dependencies and plugins. The difference can be explained in the README.

I'll check out the article and play around with my PR, swapping AES with chacha20-poly1305, adding more customization, and key rotation.

[pluma]

FWIW my main use case for encryption would be storing internals like session IDs. Signing them works fine but there's really no reason for users to see the value. Signing is fine to prevent tampering, but encryption would provide actual secrecy.

I don't know whether fastify should specify any defaults but if it does, those defaults should be picked by someone who actually understands cryptography (i.e. someone specialized in security) rather than a regular developer like me. I don't see any problem with providing an example that uses values articles on the Internet suggest are reasonable.

If node's crypto supports the necessary algorithms, I'm not sure why libsodium would be necessary. If it doesn't, that sounds like something that should be addressed in node. After all, it should be possible to swap out the encrypter according to the PR and substitute your own libsodium-based one.

EDIT: Also I don't see any point in supporting asymmetric encryption as it's a bad fit for the use case and is also slower.

[gurgunday]

Edit 2: Agree with everything Uzlopak said since fastify-secure-session already exists and is fully functional.

[jorgevrgs]

Is this discussion covering the option to add async sign/unsign to the Signer interface?

[simoneb]

Is this still relevant?