Implement admin password hashing using SHA-256#311
Implement admin password hashing using SHA-256#311ukane-philemon wants to merge 2 commits intodecred:masterfrom
Conversation
|
Just wondering that this would impact auto recovery systems. If a system crashes it will require admin to manually start the service for a secure setup (Imagine a long Christmas break and the API being down for days). Why not just store the hash to a config file in disk after the first prompt ? This would provide almost the same level of security without the need to prompt the user for password on each startup (Hence boot scripts can be automated) |
|
Hi @ukane-philemon. Definitely appreciate the PR but I have to agree with @degeri here. vspd is designed to be run as a system service which can be started/restarted non-interactively. I notice in this PR you used the database to store the password hash, and that seems like a good idea. Why did you change it? A better solution could be to just store it in a new file in the datadir (eg. Also, in that PR you used bcrypt, and this one is using SHA-256. Why the change? |
|
@degeri @jholdstock thank you for looking into this PR, it's my first time contributing here so I was concerned about what the team would accept hence my switch to sha256 as it is used in dcrdex, except dcrstakepool which use bcrypt. |
|
@jholdstock @degeri I know projects have their preferences when it comes to hashing. |
|
@jholdstock I resolved to using bcrypt in my recent commit. The bcrypt password hashing mechanism is the second choice after Argon2id, for password hashing/storage to achieve FIPS-140 compliance. |
|
Security thoughts on bcrypt vs sha256 @degeri? Personally I have no preference from a security standpoint, but I would rather use SHA because it does not introduce extra dependencies on |
|
Yeah should not be too big of a difference. Lets go with SHA since its less dependencies |
8c6553f to
2b7a9c0
Compare
9bdcee8 to
4079ab0
Compare
|
While I agree with the premise of this change, and the code looks acceptable, I'd like to hold off merging this PR for a while because release 1.1.0 is imminent (to coincide with dcrd/dcrwallet 1.7). Changing vspd from a non-interactive to an interactive process is a very significant change to make so close to a release. |
|
@jholdstock @degeri thanks for your feedback and review of this piece. I'm happy it's slated for v1.1. |
499d19a to
38430c4
Compare
38430c4 to
05a7970
Compare
05a7970 to
eae8cb1
Compare
Per SEI CERT C Coding Standard, it is best practice not to store plain text passwords in memory or on disk. This was achieved by storing the bcrypt hash of the admin pass in a separate file, removing the provided password bytes from memory. Closes #281