-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement admin password hashing using SHA-256 #311
base: master
Are you sure you want to change the base?
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