feature: use binary keys instead of passwords #55
Conversation
ghost
assigned 
|
@linuxwolf for purposes of releasing mozilla-lockwise/lockbox-extension#362 would you like this reviewed by @jimporter (knowing a broader review is needed from others soon)? Or do you want to keep this branch and keep the extension pegged to it for now? Or did you have something else entirely in mind? |
|
@jimporter @devinreams This PR should be good to review now.
I don't think it's worth keeping separate; does mean PR should land before mozilla-lockwise/lockbox-extension#362 (and that PR updated to point to the shared repo). |
lib/datastore.js
Outdated
| * deriving the master key | ||
| * @param {jose.JWK.Key} [opts.appKey] The master app key to setup with | ||
| * @param {string} [opts.salt] The salt to use in deriving the master | ||
| * keys. |
There was a problem hiding this comment.
Micro-nit: no punctuation seems to be the consensus here.
lib/itemkeystore.js
Outdated
| * @readonly | ||
| */ | ||
| get group() { return instance.get(this).group || ""; } | ||
| /** | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Looks like a merge conflict didn't get resolved here.
lib/itemkeystore.js
Outdated
| @@ -77,15 +72,20 @@ class ItemKeyStore { | |||
| * a plain Object with the following properties: | |||
| * | |||
| * * `group` {**String**} The name of the keystore's group | |||
| <<<<<<< HEAD | |||
.eslintrc.json
Outdated
| "indent": ["error", 2, {"SwitchCase": 1, "VariableDeclarator": {"var": 2, "let": 2, "const": 3}}], | ||
| "linebreak-style": ["error", "unix"], | ||
| "no-var": "off", |
There was a problem hiding this comment.
What do we need var for? I only see it in a couple places in this diff...
There was a problem hiding this comment.
var is used in the build-utils files, which are also linted.
There was a problem hiding this comment.
Does it need to be var there, and if so, would it be better to just ignore the no-var rule on those lines?
There was a problem hiding this comment.
node.js still has some issues with let at the top-level. I can go back through those files to fix it, but really rather file a chore for now.
lib/datastore.js
Outdated
| @@ -84,7 +114,8 @@ class DataStore { | |||
| * See {@link datastore.create} for the details of what `{cfg}` | |||
| * parameters are supported. | |||
| * | |||
| * @param {Onject} cfg - The configuration parameters | |||
| * @param {Object} cfg - The configuration parameters. | |||
There was a problem hiding this comment.
Nit: The consensus seems to be no hyphen and no punctuation. I don't have a preference for how we do this, but consistency would be nice.
linuxwolf
dismissed
jimporter’s stale review
latest commits should address review comments
Resolves #52
Switches the datastore from using a password-based key to using a binary key (from FxA).
This work is assuming there is no migration from the former model to this new model. It is also providing a path to incorporating an external salt for key derivations; in practice this will be the FxA uid, or a persisted UUID while the system is not bound to an FxA account.