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

Allow hashed passwords #1581

Closed
wants to merge 3 commits into from
Closed

Allow hashed passwords #1581

wants to merge 3 commits into from

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Mar 20, 2016

Description:
At the moment passwords and API keys are stored in plain-text in the configuration.yaml file. This PR is a RFC for the feature of encrypted/hashed passwords as opt-in.

Related issue (if applicable): Story

Example entry for configuration.yaml (if applicable):

http:
  api_password: $5$rounds=535000$PEN/g8jYEZT.81GW$SISbV67EfbJOITelNNeZUKZ/s6KY2rT3CjW5vW2diwB

The script script/gen_hash.py is a helper to created the needed hash. Only the http component is supported at the moment.

Checklist:

  • Local tests with tox run successfully.
  • TravisCI does not fail. Your PR cannot be merged unless CI is green!
  • Fork is up to date and was rebased on the dev branch before creating the PR.
  • Commits have been squashed.
  • If code communicates with devices:
    • New dependencies have been added to the REQUIREMENTS variable (example).
    • New dependencies are only imported inside functions that use them (example).
    • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
    • New files were added to .coveragerc.
  • If the code does not interact with devices:
    • Tests have been added to verify that the new code works.

@@ -32,6 +32,8 @@

DOMAIN = "http"

REQUIREMENTS = ["passlib==1.6.5"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use Python built-in ? https://docs.python.org/3.4/library/hashlib.html

@balloob
Copy link
Member

balloob commented Mar 23, 2016

I think the idea of storing hashed passwords for http to be a good idea.

Some problems that I foresee is that both the remote instances API and the MQTT server need to be able to access the password. The problem here is that if we can decrypt a password we're pretty much giving a false sense of security. So that makes it pretty much impossible at this point.

This all should probably be part of a bigger overhaul to add a proper authentication layer. But I don't know how that would look right now.

@MartinHjelmare
Copy link
Member

For future reference, I think bcrypt is currently a better (more secure) way of hashing passwords than sha256, after reading a couple of blogs about the subject.

@SEJeff
Copy link
Contributor

SEJeff commented Apr 2, 2016

bcrypt is excellent for this, but so is sha256, albeit being more crackable by GPUs and custom ASIC attacks. FWIW, NIST suggests PBKDF2). All of these are quite good and considered very resistant to cracking.

For @fabaff it does seem like hashlib would be preferable over passlib however. @balloob is right there.

@fabaff fabaff changed the title Allow encrypted passwords Allow hashed passwords Apr 10, 2016
@fabaff
Copy link
Member Author

fabaff commented Apr 10, 2016

I switched to hashlib for the hashing. Sorry the title was misleading.

While an approach like Ansible Vault would add extra protection for sensitive data like password and API key it would make it hard to auto-restart Home Assistant without user interaction.

@balloob
Copy link
Member

balloob commented Apr 12, 2016

@fabaff I will be closing this PR in favor of the result of this discussion. It should make everything better https://community.home-assistant.io/t/migrate-http-stack-to-wsgi-based-framework/576

@balloob balloob closed this Apr 12, 2016
@fabaff fabaff deleted the encrypted-passwords branch June 2, 2016 19:49
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants