Allow for function to be passed in place of static key about fastify-secure-session HOT 10 CLOSED

I figured it was some type of secret management like that. My point I’m making with the above is that from where I’m sitting a function doesn’t make it that extendible because I don’t see how you keep from logging out old sessions that aren’t expired yet.
I get the key change at runtime, it’s just that if you do it how you describe you will log out users with active sessions which is a poor ux.

I could definitely be missing something - especially in regards to how you would plan to support not logging people out when you rotate the key. The hash mutation (rough idea) would be split for your keys so that old session decryption would be supported.

You could also make the key hash immutable on app startup. Typically I've seen key rotation as something that only happens every 90 days or so, which seems like a fine use case to restart the application with the env variable updated to a new comma-separated list of keys. (again this is working off the function implementation approach, I still think the library supporting an array of keys attempting to decode from first to last and pushing/popping when you rotate is a simpler approach, especially since the heroku plugin you mentioned actually works that way already - https://devcenter.heroku.com/articles/securekey#provisioning-the-add-on - see specifically the block where it mentions process.env.SECURE_KEYS=KEY1,KEY2 )

If I'm missing something I would definitely appreciate you taking the time to explain - specifically how a function implementation keeps from logging out valid sessions due to key change. I'm just trying to learn here 😄

from fastify-secure-session.

How is it more work?

I may just be operating on a different wavelength here or something. I get that it's not complicated to support a function to get a key, I think I read your initial issue question as having the main objective of rotating keys.

Reading it again, it seems like from your perspective you're more interested in a key that is simply represented as a function that returns the value for the key. I get that that by itself is very simple, but I actually see it as a completely separate thing from key rotation. I'll remove the reference to this issue with my PR and keep its scope just to key rotation.

I don't have a use case for a key being represented by a function, I care a lot more about being able to rotate keys in a way that doesn't log people out. Sorry for any confusion

from fastify-secure-session.

sure thing!

from fastify-secure-session.

As a point of note, with something like express-session an array of keys is used instead of a function. What advantage does providing a function to get the key you want at runtime provide instead of just specifying an array of keys to the key option like other implementations?
Are you wanting to rotate the key during runtime (therefore disabling all sessions created before that point in time?) or instead something like setting another cookie to track which key that user's session was signed with - ie:

const keys = {
  0: Buffer.from('my-random-key')
}
// /api/login
// set-cookie sessionName=mySessionCookie, keyIndex=0
// ...some time passes, key is rotated, app is redeployed (or key rotation is added dynamically during runtime, either would work i think

const keys = {
  0: Buffer.from('my-random-key'),
  1: Buffer.from('my-new-shiny-key')
}

function getKey(keyIndex) { return keys[keyIndex] }

fastify.register('fastify-secure-session', { key: getKey } );

// older session user comes back
// fastify secure-session uses `getKey(request.cookies['keyIndex']` which returns:
// keys[0] which is the correct key and will therefore decode it

More work on the above is definitely needed and in is likely not even a good idea, I'm just trying to gauge how you're trying to go about using a function for key rotation, partially out of curiosity, partially because I think it would be fun to implement :)

from fastify-secure-session.

@wolfejw86 You could totally not use a function but I've found them to be the most extendable approach for future proofing these types of libraries :)

Basically just support key changes at runtime. Buffer.from() will be run once and not updated. I think your example updates the dictionary over time? Mutates it right? Instead I would have a service that fetches a key at runtime (ENV variables or external service doesn't technically matter).

I am using this service https://devcenter.heroku.com/articles/securekey.

from fastify-secure-session.

@wolfejw86 the primary issue I want to resolve here is simply to be able to use BOTH the old and new keys when checking hashes.

Example: We are signing all new sessions using the newer KEY2. However since there are still users who have session signed against KEY1, we should check both during hash session validation so they can keep their session. SECURE_KEYS=KEY1 fails so lets check against the newer KEY2 as well (or vice versa order). Ah! that works, user is signed in with one of our rotated keys.

I think this library only allows the key to be set during initialization unless I am mistaken?

My initial thinking was to make this region more dynamic:

if (!sodium.crypto_secretbox_open_easy(msg, cipher, nonce, key)) {
  // unable to decrypt
  request.session = new Session({})
  next()
  return
}

I suppose that may prove to be a little awkward though now that I look more at it.

I think the approach you put forward will work great as long as both of these forms can be supported:

const keys = {
  0: Buffer.from('my-random-key'),
  1: Buffer.from('my-new-shiny-key')
}

const keys = {
  0: process.env.SECRET1
  1: () => myservice.getDynamicKey()
  2: myservice.getDynamicKey
}

Supporting function invocation solves the issue of changing values and there is no penalty to add simple check like typeof key === 'function' ? key() : key

from fastify-secure-session.

Hmm, I am not a fan of that approach. That puts a lot more work on the "caller" to implement. When the end goal / result is to support dynamic key rotation, this is a lot to prescribe in my opinion. In scenario 1 of your above example the library will have to pass something back to the application to know which key to retrieve which is undesirable in my opinion. In scenario 2, it's just a lot of overhead period as now we're back to the array style implementation due to the need to iterate over the hash somehow and also check if each one is a function or not.

I really feel like there's just an easier answer that meets the objective (key rotation and support older keys) and puts way less work on the "caller" (your application) in this scenario.

Have key be an array (in your heroku example parsed from a comma-separated value env variable) and just pass that in (Array<string | Buffer>)

I took a stab at implementing it here - #28

Edit: I no longer believe the goal of my PR and this issue are aligned.

from fastify-secure-session.

How is it more work?

type Key = string | () => string

🤷

from fastify-secure-session.

@wolfejw86 gotcha, thanks! Yeah I guess there are two topics here and I casually blended both together. I think your implementation works. I only worry that it doesn't solve the generic case of those values changing up under you at runtime (which may never happen in many cases).

from fastify-secure-session.

Yeah you're right. I want to think about it some more. Even though that's not a situation I'm used to, it's probably worth supporting 🤔

from fastify-secure-session.