Skip to content

Replace pbkdf2 with bcrypt#16071

Merged
balloob merged 9 commits intohome-assistant:devfrom
Eriner:feature-bcrypt
Aug 26, 2018
Merged

Replace pbkdf2 with bcrypt#16071
balloob merged 9 commits intohome-assistant:devfrom
Eriner:feature-bcrypt

Conversation

@Eriner
Copy link
Copy Markdown
Contributor

@Eriner Eriner commented Aug 20, 2018

Description:

bcrypt isn't inherently better than pbkdf2, but everything "just works"
out of the box.

  • the hash verification routine now only computes one hash per call
  • a per-user salt is built into the hash as opposed to the current
    global salt
  • bcrypt.checkpw() is immune to timing attacks regardless of input
  • hash strength is a function of real time benchmarks and a
    "difficulty" level, meaning we won't have to ever update the iteration
    count

Note that I haven't tested this code myself. Also, this won't be a clean upgrade for current "beta" users, not sure how that should be handled.

Example entry for configuration.yaml (if applicable):

auth_providers:
  - type: homeassistant

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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.

@Eriner Eriner requested a review from a team as a code owner August 20, 2018 07:55
@homeassistant homeassistant added cla-signed core small-pr PRs with less than 30 lines. labels Aug 20, 2018
@ghost ghost added the in progress label Aug 20, 2018
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So one downside of using bcrypt is that it requires an external dependency. They are providing wheels for Windows, Mac and Linux, so that's good:

image

For Python 3.7 there are currently only wheels for Windows. Windows users have the most difficulty building stuff, so that's good.

Because it's part of the PYCA org, I think I would trust it to remain maintained.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll need a migration path. We can only do the migration while a user is logging in. Verify with old pattern -> if ok, migrate to new salt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

@Eriner
Copy link
Copy Markdown
Contributor Author

Eriner commented Aug 20, 2018

Added upgrade path, though tests are failing because I'm a shitty python dev. From what I can tell in the upgrade test, I need to set the generated password in the underlying structure but I'm not sure how to do that from the test; asking someone here is faster than me spending time trying to figure it out. Also, there are probably issues with encoding/types somewhere. I'm doing my best but would appreciate some help with this one for the sake of time.

@ghost ghost assigned balloob Aug 21, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 21, 2018

Fixed the test. Also made a small change to the migration code. It's 👍 from me but let me know if you agree.

@Eriner
Copy link
Copy Markdown
Contributor Author

Eriner commented Aug 21, 2018

Glad I wasn't too far off. Now that this is on the right track, give me another day or so of staring at it to make sure that I haven't made any silly mistakes.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 21, 2018

Added one extra test to make sure that incorrect legacy passwords still blow up.

@Eriner
Copy link
Copy Markdown
Contributor Author

Eriner commented Aug 21, 2018

Can we plan to remove the upgrade path and legacy hashing after some period of time? Any unnecessary complexity in authentication and authorization logic makes me uncomfortable.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 21, 2018

Yeah, I would say in ~2 months

Eriner and others added 9 commits August 26, 2018 22:20
bcrypt isn't inherently better than pbkdf2, but everything "just works"
out of the box.

  * the hash verification routine now only computes one hash per call
  * a per-user salt is built into the hash as opposed to the current
  global salt
  * bcrypt.checkpw() is immune to timing attacks regardless of input
  * hash strength is a function of real time benchmarks and a
  "difficulty" level, meaning we won't have to ever update the iteration
  count
@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 26, 2018

Going to make this part of 0.77. Because this change is forward compatible, but not backwards compatible, I don't want it to launch any later than 77, which will enable the auth system by default.

@balloob balloob added this to the 0.77 milestone Aug 26, 2018
@awarecan
Copy link
Copy Markdown
Contributor

Need add a note somewhere, if user rollback from 0.77 to prior verison, he need clean .storage/auth*.* and .storage/hassio

@balloob balloob merged commit bacecb4 into home-assistant:dev Aug 26, 2018
@ghost ghost removed the in progress label Aug 26, 2018
balloob pushed a commit that referenced this pull request Aug 26, 2018
* Replace pbkdf2 with bcrypt

bcrypt isn't inherently better than pbkdf2, but everything "just works"
out of the box.

  * the hash verification routine now only computes one hash per call
  * a per-user salt is built into the hash as opposed to the current
  global salt
  * bcrypt.checkpw() is immune to timing attacks regardless of input
  * hash strength is a function of real time benchmarks and a
  "difficulty" level, meaning we won't have to ever update the iteration
  count

* WIP: add hash upgrade mechanism

* WIP: clarify decode issue

* remove stale testing code

* Fix test

* Ensure incorrect legacy passwords fail

* Add better invalid legacy password test

* Lint

* Run tests in async scope
@Eriner
Copy link
Copy Markdown
Contributor Author

Eriner commented Aug 27, 2018

Yes, LGTM. Thanks @balloob!

@balloob balloob mentioned this pull request Aug 29, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants