patternjavascriptMinor
Simple Password-Manager in Node.js
Viewed 0 times
managernodesimplepassword
Problem
I have programmed a simple command line password-manager in Node.js during my learning process, and since I am done with it, I would highly appreciate a review.
Comments could be based on any topic. I am looking for a review which covers:
Also, the assumption in this program was that any
```
console.log('Starting password manager...');
var storage = require('node-persist');
storage.initSync();
var crypto = require('crypto-js');
var argv = require('yargs')
.command('create', 'Create an entry to store some service credentials.', function (yargs) {
yargs.options({
name: {
demand: true,
alias: 'n',
description: 'Service name.',
type: 'string'
},
username: {
demand: true,
alias: 'u',
description: 'The username or email for the account.',
type: 'string'
},
password: {
demand: true,
alias: 'p',
description: 'The password for the account.',
type: 'string'
},
masterPassword: {
demand: true,
alias: 'm',
description: 'The master password to access the system.',
type: 'string'
}
}
Comments could be based on any topic. I am looking for a review which covers:
- Security flaws.
- Performance and Efficiency.
- Better ways to achieve this.
- Violations of anything conceptual, best practices or conventions.
Also, the assumption in this program was that any
name field entry in this program entered by a user is unique.package.json:{
"name": "password-manager",
"version": "1.0.0",
"description": "Manage your passwords locally",
"main": "app.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1",
"start": "node app.js"
},
"author": "Hassan Althaf",
"license": "MIT",
"dependencies": {
"crypto-js": "^3.1.5",
"node-persist": "0.0.6",
"yargs": "^3.15.0"
}
}app.js:```
console.log('Starting password manager...');
var storage = require('node-persist');
storage.initSync();
var crypto = require('crypto-js');
var argv = require('yargs')
.command('create', 'Create an entry to store some service credentials.', function (yargs) {
yargs.options({
name: {
demand: true,
alias: 'n',
description: 'Service name.',
type: 'string'
},
username: {
demand: true,
alias: 'u',
description: 'The username or email for the account.',
type: 'string'
},
password: {
demand: true,
alias: 'p',
description: 'The password for the account.',
type: 'string'
},
masterPassword: {
demand: true,
alias: 'm',
description: 'The master password to access the system.',
type: 'string'
}
}
Solution
You list 4 things you'd like feedback on, but I only have something specific to the first (security). My other notes are spread out over the other 3 categories.
There's no code in this review, since your code looks fine: The issues are elsewhere.
Security
You have to enter the password(s) (both the master password and, in the case of the
Yes, you can clear your history or just remove the sensitive lines, but you shouldn't have to remember that or have to set it up to be automatic. So basically, you're storing all the password securely - but you're also copying them, in the clear, to
It'd be better to prompt for the password(s) from within the script, in the secure style of
If one is really paranoid, using Node.js is inherently problematic, since you don't have low-level control over things like memory allocation and (perhaps especially) deallocation. But that's a little beyond the scope of things here (besides, someone in a position to exploit something in this vein already owns your computer, so all bets are off anyway).
Other stuff
I'd make
This also solves another problem: You can't have two services with the same name. Or rather, you can have multiple services with the same name, but you'll always find the first, and only the first. Yes, you yourself said that you assume names to be unique, but that's a pretty poor assumption to make, and it's easily avoided.
Again, if you use an object with service names as the keys, you get a couple of benefits right off the bat:
The 2nd point also makes it heck of lot easier to:
In terms of user interface, the
The only way to display the help is to (paradoxically) already know the commands, and intentionally leave out some required args - then you get the help text. But that's obviously backwards.
In terms of features, you'll want a way to list (or at least search for) services. Currently, you have to remember the exact name of a service you've previously stored. Which is just as bad as having to remember passwords to begin with!
For instance, would a username/password for this site be stored as "codereview", "CodeReview", "codereview.stackexchange.com", "codereview.SE", "stackexchange", or just "cr" or something else? Sure, you can decide a pattern you always use for the service name (e.g. it's always the URL), but then you're back to the update problem: If you change the password, you can't update the service. So you'll have to break your pattern to make up a new service name, just so you can find it again later. If you remember what you called it, now that you're not using a pattern...
Also, for this to be really, truly useful, it'd be nice have it stay running for a little while and accept commands directly without requiring and re-requiring your master password. Even if all service names are unique, you still might have to try several names before you find the right one. And currently, you'd have to re-enter your master password each and every time.
The two ways a password manager can fail is if it's less secure than other methods, or if it's so cumbersome that you just don't use it.
There's no code in this review, since your code looks fine: The issues are elsewhere.
Security
You have to enter the password(s) (both the master password and, in the case of the
create command, the service password) on the command line. So unless you clean up your command history, they'll be plainly visible to anyone with access to your computer. Typically, all you'd have to do is press the up-arrow in a shell until you find a previous command.Yes, you can clear your history or just remove the sensitive lines, but you shouldn't have to remember that or have to set it up to be automatic. So basically, you're storing all the password securely - but you're also copying them, in the clear, to
~/.bash_history or equivalent.It'd be better to prompt for the password(s) from within the script, in the secure style of
sudo or ssh. I don't know how to best do secure password entry with Node.js, but some quick googling turned up some possible solutions.If one is really paranoid, using Node.js is inherently problematic, since you don't have low-level control over things like memory allocation and (perhaps especially) deallocation. But that's a little beyond the scope of things here (besides, someone in a position to exploit something in this vein already owns your computer, so all bets are off anyway).
Other stuff
I'd make
accounts an object instead of an array. That way you won't have to loop through every account to find the right one, but can just say accounts[name].This also solves another problem: You can't have two services with the same name. Or rather, you can have multiple services with the same name, but you'll always find the first, and only the first. Yes, you yourself said that you assume names to be unique, but that's a pretty poor assumption to make, and it's easily avoided.
Again, if you use an object with service names as the keys, you get a couple of benefits right off the bat:
- you'll have exactly one set of credentials per service name,
- you can fetch the right one immediately without looping
The 2nd point also makes it heck of lot easier to:
- know if a service exists,
- remove services; right now, you can only add new ones,
- update services; right now, you can only add another one with the same name (which you can't find later), or make up a new name (which poses its own problems; see below).
In terms of user interface, the
help command does nothing. And if you just run the script with no arguments/commands, it just says "Starting" and then exits. If you give some random thing as arguments/commands, it also just exits.The only way to display the help is to (paradoxically) already know the commands, and intentionally leave out some required args - then you get the help text. But that's obviously backwards.
In terms of features, you'll want a way to list (or at least search for) services. Currently, you have to remember the exact name of a service you've previously stored. Which is just as bad as having to remember passwords to begin with!
For instance, would a username/password for this site be stored as "codereview", "CodeReview", "codereview.stackexchange.com", "codereview.SE", "stackexchange", or just "cr" or something else? Sure, you can decide a pattern you always use for the service name (e.g. it's always the URL), but then you're back to the update problem: If you change the password, you can't update the service. So you'll have to break your pattern to make up a new service name, just so you can find it again later. If you remember what you called it, now that you're not using a pattern...
Also, for this to be really, truly useful, it'd be nice have it stay running for a little while and accept commands directly without requiring and re-requiring your master password. Even if all service names are unique, you still might have to try several names before you find the right one. And currently, you'd have to re-enter your master password each and every time.
The two ways a password manager can fail is if it's less secure than other methods, or if it's so cumbersome that you just don't use it.
Context
StackExchange Code Review Q#112212, answer score: 4
Revisions (0)
No revisions yet.