Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Dec 28, 2021

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

@degeri
Copy link
Member

degeri commented Dec 29, 2021

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)

@jholdstock
Copy link
Member

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. ~/.vspd/password.hash). That would make forgotten password recovery very easy - just delete the file and set a new password through the terminal. It would also allow admins to easily install the file using third party tools like ansible.

Also, in that PR you used bcrypt, and this one is using SHA-256. Why the change?

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Dec 29, 2021

@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.
My initial PR used bcrypt and saved admin password hash to db. Operators can easily change the password (overwriting hash in db) without the need to delete a file in their datadir, by providing the new password via secure terminal or config.
If it's ok, I can close this PR and submit the other?

@ukane-philemon
Copy link
Contributor Author

@jholdstock @degeri I know projects have their preferences when it comes to hashing.
SHA-256 or Bcrypt?
Bcrypt was especially made for passwords but SHA-256 use less code to do what it does.

@ukane-philemon
Copy link
Contributor Author

@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.

@jholdstock
Copy link
Member

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 golang.org/x libraries.

@ukane-philemon ukane-philemon changed the title Implement Admin Password hashing using sha256 Implement Admin Password hashing using bcrypt Jan 13, 2022
@degeri
Copy link
Member

degeri commented Jan 14, 2022

Yeah should not be too big of a difference. Lets go with SHA since its less dependencies

@ukane-philemon ukane-philemon force-pushed the HashAdminPassword branch 2 times, most recently from 8c6553f to 2b7a9c0 Compare January 14, 2022 13:51
@ukane-philemon ukane-philemon changed the title Implement Admin Password hashing using bcrypt Implement admin password hashing using SHA-256 Jan 14, 2022
@ukane-philemon ukane-philemon force-pushed the HashAdminPassword branch 2 times, most recently from 9bdcee8 to 4079ab0 Compare January 14, 2022 14:13
@jholdstock
Copy link
Member

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.

@ukane-philemon
Copy link
Contributor Author

@jholdstock @degeri thanks for your feedback and review of this piece. I'm happy it's slated for v1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config: Consider not storing admin pass in plain text.
3 participants