adds sha512 hash functions but does not convert raw password implemen… #2218
Conversation
…tation to salted hash
| @@ -45,6 +51,54 @@ | |||
| }, extra=vol.ALLOW_EXTRA) | |||
|
|
|||
|
|
|||
| def collect_hash(config): | |||
There was a problem hiding this comment.
- D400: First line should end with a period (not 'n')
…tation to salted hash
Conflicts: homeassistant/components/http.py tests/components/test_http.py
|
A single round of SHA512 doesn't buy you much. Python has a kdf in the stdlib; it would be better to use that. https://docs.python.org/3.4/library/hashlib.html#key-derivation |
|
I'm not altogether clear what the goal of this PR is... |
|
I was working with @rmkraus yesterday on www.pivotaltracker.com/story/show/115886789 and I wanted to make the progress (if you can call it that) that I made available to others. |
| or | ||
| None if password or hash are not configured | ||
| """ | ||
| if CONF_API_HASH in config: |
There was a problem hiding this comment.
Generally speaking, you'll want to collect all information out of the config in one location. Ideally, this function would have inputs of config_hash and config_password, or something along those lines.
Especially when config options change, it can be troublesome to have the parsing of the config data spread throughout the code.
|
@JshWright @fmoor |
|
@rmkraus To what end? If the goal is to protect the password in the event the config file in compromised, this is a very tiny improvement... A single round of SHA512 is meaningless in terms of protecting the password. A modern computer could run through billions of hashes per second. Additionally, there are already a ton of secrets stored in the config file (various API keys, passwords, etc). I don't think protecting this single password is much of an advantage. A final consideration... the embedded MQTT server also uses the API password, so it can't go away entirely without migrating that to its own password configuration. |
|
About multiple rounds of hashing, you're definitely correct. The idea here is to help build things up incrementally while assisting in code review. It is not a finished PR yet. There are other secrets and not a whole lot that can be done about that outside of encrypting the entire config file and decrypting it at runtime with a known key from the user. I know other options are being discussed too. Like now the yaml files can source environment variables which allows passwords to be pushed to the init script rather than the config file. The breaking change to the MQTT implementation has been discussed, albeit not on GitHub, and a workaround should be viable. It just hasn't been approached yet in this PR. |
| alpha_numeric = string.ascii_letters + string.digits | ||
| salt = ''.join(random.sample(alpha_numeric, SALT_LENGTH)) | ||
| assert '$' not in salt, 'salt contains "$"' | ||
| hasher = hashlib.sha512() |
There was a problem hiding this comment.
This algorithm can probably be simplified by using the crypt module. Additionally, other hashes would be automatically supported. Check out this article for an example.
There was a problem hiding this comment.
What's driving the crypt requirement?
There was a problem hiding this comment.
I'm sorry, I'm not sure I understand the context of your question.
What is driving the requirement to use the crypt module, what is driving the requirement to encrypt passwords at all, what is driving the requirement to encrypt password in this way, or something else that I haven't considered?
There was a problem hiding this comment.
I'm referring to the apparent implicit requirement for crypt(3) compatible hash functions.
There was a problem hiding this comment.
Cool, gotcha.
Well, no matter how we handle hashing, we will require an external library. I could very well be wrong about this, but I believe all of the hashing in Python is done with an external library. Given that, I think a dependency on Python being compiled with the crypt library is, at least, as reasonable as any other.
Open for discussion though.
There was a problem hiding this comment.
@JshWright has pointed me towards a much better function to use for generating the hash via Gitter. Check it out here. It will handle things like iterating through the hash. Thanks to him for pointing it out, I was not previously familiar with this function.
100,000 iterations of the sha-512 algorithm should suffice for our purposes.
|
Is the idea to hash the password for http in the config, or to hash between the server and browser? If its to protect configuration.yaml, in #2262 I proposed a way to split secrets and store it using one of two methods, hashing could potentially be added as an option the method 1 there, since it can then cover any secret. The catch would just be to save those hash password (same problem as I'd have for the second method), or do you have a way to generate hashed entries for the config? |
|
#2312 adds keyring support (beside outsourcing values in a separate file) which enables one to store passwords, API keys, tokens, and user names in a secure place without worrying about leaking them if your |
|
I'll close this PR. It seems that with the new keyring support, there is no longer a real need for this. |
Description:
adds hash string parsing and password salting and hashing functions, but does not change current raw password string implementation to use hashes. Conversion is left to someone with a much better understanding of the ways that home-assistant uses passwords. (this may be me in the future if no one else gets to it first...)
Related issue (if applicable):
pivotal story: www.pivotaltracker.com/story/show/115886789
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If code communicates with devices:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass…tation to salted hash