node-spdyproxy
node-spdyproxy copied to clipboard
igrigorik
expand the auth process to support multiple users
This patch allows the authentication of multiple users via a plain text database storing the username, salt, and PBKDF2 password hash of each user, which can be generated by a newly added tool called spdyproxy-pwd.js. Another newly added module called auth.js parses the database and performs authentication for the SPDY Proxy server, with caching of already authenticated users for optimal performance. The log output of the server is also tweaked to include the username of the initiator of each request.
Regarding assertNodeVersion(), I did it like that so the version requirement only comes up if auth.js is actually in use, but not if someone opts for no authentication at all (AFAIK, package.json doesn't provide means of modular version requirement). It's also why I put require(../lib/auth) behind if (opts.auth) instead of up top.
Since I generally consider "as little dependency as possible" to be an important virtue, I would also think of bcrypt as unnecessary. I'm curious: Is there any reason against using the standard crypto lib? Or do you simply want a less proprietary format for the auth database? If we really want substantially higher security, we may want to consider PKC-based authentication instead -- but frankly, I don't see any compelling rationale for it.
The latest stable is 0.8.18. Asking for 0.8.7 is not a big deal (and you should be on it to begin with). If for no other reason than simplicity, I'd skip the modularity argument and just rev to latest version.
Re, bcrypt: fair enough, but it goes both ways. I'm not intimately familiar with the core crypto library in node, but I know enough to be wary of any "roll your own" solutions when it comes to security. When in doubt, I'd rather rely on a more popular package, which gets more eyeballs. Hence my question about bcrypt.
After some more reflection, I'm willing to concede on the version requirement issue. My initial reluctance had to do with (among others) the fact that the official Node.js distro packages for Debian / Ubuntu / etc. are all still 0.6.x. But since you're requiring 0.8.x already, that doesn't really make a difference.
I absolutely understand and support abundant caution against "roll your own" solutions in area of security. However, PBKDF2 isn't a "roll your own" solution -- it's as standard as it comes. If anything, I'd consider relying on an external bcrypt lib to be more "roll your own" than using the integrated crypto lib. As for "popularity", is Node.js' own crypto lib really less popular than some other bcrypt lib? How do you know that?
@henrypijames the bcrypt comment wasn't about doing the switch, rather asking about the merits of it. If you're confident in PBKDF2, then I'm ok with it - just wanted to flag it.
With that out of the way. I guess just a quick cleanup on the dependencies and we can merge?
P.S. Thanks for your patience!
From what I understand about PBKDF2, it's generally (though not unanimously) considered superior over bcrypt, in term of the design of the algorithm. But frankly, I'm not sure how confident I can be about its implementation in the Node.js standard crypto lib -- it's truly worrisome that that particular bug could not only exist, but remain undetected for such a long time. Then again, I consider the risk potential of either existing or future bugs to be significant lower than the dependency cost (and also risk potential) of 3rd-party crypto libs. (I normally use CrytoJS for in-browser PBKDF2 -- it's not bad, but as a pure-JS implementation, significantly slower than Node.js' standard crypto lib.)
I will do the version dependency cleanup (and possibly some other minor cleanups) and make a new commit shortly.
Our agreement on raising the version requirement in package.json to 0.8.7 notwithstanding, I would still like to keep the assertVersion() code, and here's why:
I came to realize that this requirement was necessary in the first place because originally I didn't install node-spdyproxy via NPM, but simply downloaded the files (only the two .js files -- I didn't even look into package.json). And as I started to write the auth extension I (very coincidentally) encountered the anomaly that the code was generating different hashes on my local system and on a server where I was test-deploying. Googling about it lead me to that bug report, and only then the realization that the Node.js 0.6.x on my local system was inadequate.
I imagine that this could happen to other people as well: Those not using NPM wouldn't realize this problem unless the code itself throws an error. And while Debian / Ubuntu / etc. users with their Node.js 0.6.x distro package would be stopped by NPM because of the version requirement, some of them might think "what's the big deal" and go ahead either editing the package.json to lower the requirement or downloading the files manually. And when they encounter no error, they might think everything is fine.
I know this is a somewhat unlikely (though not unrealistic, as it did happen to myself) scenario. However, the severity of the problem -- a quasi undetectable error in the authentication process -- warrants abundant precaution, IMHO. The extra code is extremely light, and it explains the reason for the version requirement in clear terms. I think it's worth keeping.
PS: This aside, I still want to do a bit of cleanup before merging.
@henrypijames apologies about the radio silence. Any chance you can rebase against latest master? There have been a few updates to the user/pass logic, which may need a small refactor.
@igrigorik: Apologies right back about the radio silence on my side. But now I've finally found time to rebase my changes and would like to renew the pull request.
@igrigorik: Would you please tell me what's still holding this one up?