brave / vault

Brave personal data store vault.
https://brave.com
Mozilla Public License 2.0
19 stars 18 forks source link

server.auth.strategy 'session' has hard-coded password? #41

Closed diracdeltas closed 8 years ago

diracdeltas commented 8 years ago

in src/index.js:

  server.auth.strategy('session', 'cookie', {
    password: 'cookie-encryption-password',
    cookie: 'sid',
    isSecure: runtime.login.isSecure
  })

what is this auth strategy used for?

please address before open sourcing, thanks

mrose17 commented 8 years ago

i could use your counsel on this:

  1. authentication is via github ... being able to authenticate as a member of the organization identifed by login.organization configuration variable.
  2. once authenticated, the "sid" cookie is created that contains the encrypted authorization tokens for the user.
  3. the core reference for creating the "sid" cookie is is https://www.npmjs.com/package/iron in which we have

    var password = 'some_not_random_password';

what is going on here is that the "password" is one of the inputs to an algorithm to encrypt information in the "sid" cookie which can be used to verify that the user has previously authenticated.

  1. by my reading of the iron documentation, any static string will do. we could generate a random string on startup, which means that sessions would be "reset" whenever the server restarts. that's fine with me!
diracdeltas commented 8 years ago

If we regenerate the string on startup, all user cookies are invalidated every time the server is restarted. Could we just put a randomly-generated string in config/config.development.js for this?

therealklanni commented 8 years ago

You're using MongdoDB, right? Why not store a randomly generated secure key there?

bridiver commented 8 years ago

What is this authentication for? I thought we weren't maintaining any session/cookie based logins for the vault anymore? We actually can't according to our current privacy policy.

mrose17 commented 8 years ago

@therealklanni - hi! i like @diracdeltas' suggestion better because it fits in with the existing configuration file. there's nothing wrong with using a DB instead except that we currently don't keep configuration in a database, it's in config/config.*.js (depending on whether the code is running on a server or under development or test)

mrose17 commented 8 years ago

@bdriver - it is used by administrative users only, e.g., to populate the ad-manifest table...

bridiver commented 8 years ago

gotcha, thanks!

mrose17 commented 8 years ago

@diracdeltas - what's your thinking on the PR?