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

Password validation using Unpwn #2082

Merged
merged 10 commits into from Oct 31, 2019

Conversation

@matiaskorhonen
Copy link
Contributor

matiaskorhonen commented Aug 6, 2019

Check if the password chosen by the user has already been compromised in a data breach.

This uses the Unpwn gem by Andre Arko as suggested in #2052. Unpwn 1.0 hasn't been published to RubyGems.org yet, so this shouldn't be merged before that happens…

Unpwn checks passwords locally against the top one million passwords, as provided by the nbp project. Then, it uses the haveibeenpwned API to check proposed passwords against the largest corpus of publicly dumped passwords in the world.

The Pwned Passwords API “implements a k-Anonymity model that allows a password to be searched for by partial hash”.

You can read more about the k-Anonymity model in Troy Hunt's blog post about it, but the gist of it is that it allows passwords to be checked for breaches without revealing the password to a third party.

If the API cannot be reached or it errors out, the pwned password check is bypassed and the password is presumed to be valid.

Unpwn implements a bloom filter for the million most common passwords, thus preventing extraneous API calls.

The latest version hasn’t been pushed to RubyGems yet.
@matiaskorhonen matiaskorhonen referenced this pull request Aug 6, 2019
@indirect

This comment has been minimized.

Copy link
Member

indirect commented Aug 15, 2019

I should probably release this, shouldn't I. Ok, going over to do that...

@matiaskorhonen matiaskorhonen marked this pull request as ready for review Aug 15, 2019
Copy link
Member

dwradcliffe left a comment

Looks pretty good! Thanks for doing this!
What is the timeout for the remote http call? Can we configure it?
It would be neat to see a toxiproxy test to prove that a failed or slow api wouldn't impact our service, but I'm not sure how much work that would be.

@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Aug 16, 2019

Unpwned doesn't currently pass options down to Pwned.pwned?, so currently the time is the default for read_timeout in Ruby, 60 seconds.

I agree that 60 seconds is way too long, so I think that should be fixed before this is merged. I'd argue that something around 3 to 5 seconds would be acceptable.

I've never used toxiproxy, so I don't really know how to go about adding a test using it…

@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Aug 19, 2019

I created a PR on Unpwn to allow the timeout to be passed through: indirect/unpwn#2

@sonalkr132

This comment has been minimized.

Copy link
Member

sonalkr132 commented Aug 22, 2019

I've never used toxiproxy, so I don't really know how to go about adding a test using it…

It is not too much work, just add the service and call down on it in tests

@sonalkr132

This comment has been minimized.

Copy link
Member

sonalkr132 commented Aug 22, 2019

It is not too much work

Okay, it is not too much work if the server was on localhost. Not sure how would one go about it for external service, perhaps override it to resolve to localhost using /etc/hosts?

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Aug 23, 2019

@matiaskorhonen ping me if you need any help with Toxiproxy, I can help you if needed or I can contribute that requested test if needed.

@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Aug 23, 2019

@simi If you could write that test, that'd be great

Also updates Unpwn to get support for request options
@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Sep 16, 2019

Aside from the Toxiproxy test, PR is pretty much ready to merge.

I tried adding the Toxiproxy endpoint using the code below, but even after disabling certificate verification I only get 403's from the upstream. I suspect that the HTTP Host header or something is missing…

if (Rails.env.test? || Rails.env.development?) && Toxiproxy.running?
  port = 22_222
  Toxiproxy.populate(
    [
      {
        name: 'haveibeenpwned',
        listen: "127.0.0.1:#{port}",
        upstream: 'api.pwnedpasswords.com:443'
      }
    ]
  )

  Pwned::Password::API_URL = "https://127.0.0.1:#{port}/range/"
end
test/factories.rb Outdated Show resolved Hide resolved
@matiaskorhonen matiaskorhonen force-pushed the matiaskorhonen:unpwn branch 2 times, most recently from fbcf161 to 443257f Oct 23, 2019
test/test_helper.rb Show resolved Hide resolved
@matiaskorhonen matiaskorhonen force-pushed the matiaskorhonen:unpwn branch from 443257f to ffceb41 Oct 23, 2019
@matiaskorhonen matiaskorhonen force-pushed the matiaskorhonen:unpwn branch from ffceb41 to c2d68e7 Oct 23, 2019
@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Oct 23, 2019

Something seems to be wrong at Travis so the builds aren't working (operations are timing out while trying to setup the test environment).

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Oct 23, 2019

I have restarted the main build it looks ok now.

@simi
simi approved these changes Oct 23, 2019
@matiaskorhonen

This comment has been minimized.

Copy link
Contributor Author

matiaskorhonen commented Oct 30, 2019

Could this be merged?

@indirect

This comment has been minimized.

Copy link
Member

indirect commented Oct 31, 2019

Thanks so much for all your work on this!

@indirect indirect merged commit 8ce4bda into rubygems:master Oct 31, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matiaskorhonen matiaskorhonen deleted the matiaskorhonen:unpwn branch Oct 31, 2019
@sonalkr132 sonalkr132 deployed to production Nov 6, 2019 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.