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

[Validator] Add a HaveIBeenPwned password validator #27738

Merged
merged 1 commit into from Apr 1, 2019

Conversation

@dunglas
Copy link
Member

commented Jun 27, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR todo

This PR adds a new Pwned validation constraint to prevent users to choose passwords that have been leaked in public data breaches.
The validator uses the https://haveibeenpwned.com/ API. The implementation is similar to the one used by Firefox Monitor. It allows to not expose the password hash using a k-anonymity model. The specific implementation for HaveIBeenPwned has been described in depth by Cloudflare.

Usage:

// Rejects the password if is present in any number of times in any data breach
class User
{
    /** @Pwned */
    public $plainPassword;
}

// Rejects the password if is present more than 5 times in data breaches
class User
{
    /** @Pwned(maxCount=5) */
    public $plainPassword;
}

// Customize the error message
class User
{
    /** @Pwned(message='Please select another password, this one has already been hacked.') */
    public $plainPassword;
}
@stof

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

This constraint already exist as a third-party one here: https://github.com/rollerworks/PasswordStrengthValidator/blob/v1.1.3/src/Validator/Constraints/P0wnedPassword.php

I'm wondering whether we need to have it in core.

@dunglas dunglas force-pushed the dunglas:haveibeenpwned branch from f252fdd to d2eaa56 Jun 27, 2018

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

I wasn't aware of this bundle! Thanks @stof for pointing it out.
IMO it's a good reason to move this kind of very important features in core :)

@stof

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

the threshold is not yet implemented there (it is always the equivalent of your maxCount=1), but there is a issue about it already: rollerworks/PasswordStrengthValidator#22

@stloyd

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

I was thinking about similar thing to add to Symfony, but after re-thinking the idea to have it core I decided to abandon it & probably propose it as documentation tutorial or something like that. Mostly cause it depends on external implementation.

WDYT?

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Great idea! I'm pretty sure that despite depending on a vendor, this would be a nice addition to the core.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

I was thinking about similar thing to add to Symfony, but after re-thinking the idea to have it core I decided to abandon it & probably propose it as documentation tutorial or something like that. Mostly cause it depends on external implementation.

I had the same feelings, but if it's good enough to be included in Firefox, with its huge user base. I guess it's good for us too. It's also used by 1password.

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 27, 2018

@Majkl578
Copy link
Contributor

left a comment

I believe this violates the license / ToS of the service:

In other words, you're welcome to use the public API to build other services, but you must identify Have I Been Pwned as the source of the data . Clear and visible attribution with a link to haveibeenpwned.com should be present anywhere data from the service is used including when searching breaches or pastes and when representing breach descriptions. It doesn't have to be overt, but the interface in which Have I Been Pwned data is represented should clearly attribute the source per the Creative Commons Attribution 4.0 International License.

By design, Symfony, as a server-side framework (and Validator, as server-side component), can't fulfill any of those requirements.

Also hardcoding HTTP requests to some URL sounds like a really bad idea (should the service be compromised, all users are doomed and you are in serious security trouble).

@ChangePlaces

This comment has been minimized.

Copy link

commented Jun 27, 2018

Please don't turn symfony into laravel. The owners of this site could easily 'omit' certain passwords and will know what sites have such passwords.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

Also hardcoding HTTP requests to some URL sounds like a really bad idea (should the service be compromised, all users are doomed and you are in serious security trouble).

I don't understand what you mean here. If you don't call the URL, you cannot use the service at all.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@Majkl578 the paragraph you pasted is titled License — breach & paste APIs. This validator is not using any of these 2 apis, but the "Pwned Passwords" one

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Please don't turn symfony into laravel.

This kind of comments aren't constructive and create a bad atmosphere. That being said, and even if I personally think that we've a lot to learn from Laravel, I don't get what the relation between this PR and Laravel is. AFAIK, they don't provide an integration with HaveIBeenPwned (yet).
If "turning Symfony into Laravel" helps making the web a safer place, I'm 100% for.

The owners of this site could easily 'omit' certain passwords and will know what sites have such passwords.

The password hash isn't sent to the API (only the first 5 chars are). So the API cannot know the password used. You can refer to the Cloudflare paper about k-anonymity I linked in the PR description.

Anyway, this site is run by a well known security expert from Microsoft, and trusted by internet backbones such as Firefox and CloudFlare. This feature is 100% optin, if you don't trust this service, don't use it.

@Majkl578 In addition to what @stof said, I sent a mail to Troy Hunt to be sure we're in the path from a legal PoV. I think that it's the website that will use this validator that need to comply with ToS, not Symfony. We'll make that bold in the documentation.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@goetas

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Not really happy to see third party api landing into the symfony core (that's becoming bigger and bigger recently)

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

maxCount should be minCount, i.e. it's considered as pwned if it's present for a minimum number of times.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

IMO we could add an optional dependency on a third-party HTTP client. Since there's no PSR, the closest thing is https://github.com/php-http/httplug

Edit: There's a draft PSR-18: https://github.com/php-fig/fig-standards/blob/master/proposed/http-client/http-client.md

protected function createValidator()
{
$httpClient = function (string $url) {
if ('https://api.pwnedpasswords.com/range/3EF27' === $url) {

This comment has been minimized.

Copy link
@DocFX

DocFX Jun 28, 2018

String instead of constant? Just asking. :)
private const API_ERROR_VALIDATOR_STRING or sth like that, maybe?

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@teohhanhui I think it's safer to wait for PSR-18 and use something lightweight like the current signature for now. It will be easy to switch when PSR-18 will be stable.

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

@teohhanhui I planned to switch to threshold as suggested by @stof. wdyt?

@ChangePlaces

This comment has been minimized.

Copy link

commented Jun 28, 2018

Also wanted to point out, the twitter stream of the guy responsible for this service has many messages of him talking about how much it's costing to host the service. Never a good sign.

@dunglas dunglas force-pushed the dunglas:haveibeenpwned branch 4 times, most recently from 49f2d4f to 51ac9c3 Mar 11, 2019

return;
}
$httpClient = $this->httpClient;

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 11, 2019

Member

Seems unecessary

$hashPrefix = substr($hash, 0, 5);
$url = sprintf(self::RANGE_API, $hashPrefix);
$result = $httpClient->request('GET', $url)->getContent();

This comment has been minimized.

Copy link
@fabpot

fabpot Mar 11, 2019

Member

If the endpoint is unavailable (500/503/... for instance), what should we do?

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 11, 2019

Author Member

Currently, we throw. But this has been discussed at some point, I'll add an option to ignore this constraint if the API is down (disabled by default).

Show resolved Hide resolved src/Symfony/Component/Validator/Tests/Constraints/NotPwnedValidatorTest.php Outdated
public function testThresholdNotReached()
{
$constraint = new NotPwned(['threshold' => 10]);
$this->validator->validate(self::PASSWORD_LEAKED, $constraint);

This comment has been minimized.

Copy link
@GregoireHebert

GregoireHebert Mar 15, 2019

Suggested change
$this->validator->validate(self::PASSWORD_LEAKED, $constraint);
$this->validator->validate(self::PASSWORD_LEAKED, new NotPwned(['threshold' => 10]));
Show resolved Hide resolved src/Symfony/Component/Validator/Constraints/NotPwned.php

@dunglas dunglas force-pushed the dunglas:haveibeenpwned branch from d78966f to 7fe4b25 Mar 21, 2019

@dunglas

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

  • Added a new option to not throw in case of error
  • Fixed issues raised in comments

Should be ready to be merged

@dunglas dunglas force-pushed the dunglas:haveibeenpwned branch from 7fe4b25 to a7c18ac Mar 21, 2019

return;
}
throw $e;

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 21, 2019

Member

I still don't think that throwing here by default makes sense. What should be configurable is whether a HTTP failure will make the validator create a constraint violation or just skip.

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 21, 2019

Author Member

I still don't think that throwing here by default makes sense.

The system must be as secure as possible by default. If there is an outage for the service, I prefer to retry latter to create the account of my company user than letting using something like "mum", or one that already has leaked. Now this behavior can be change using a simple attribute.

What should be configurable is whether a HTTP failure will make the validator create a constraint violation or just skip.

It's exactly what the new attribute does, or am I missing something?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 21, 2019

Member

Throwing an exception will not create a constraint violation, but will lead in a server error. From the user's point of view that's the worst that could happen as they won't get any feedback of what went wrong and if there is anything they could do.

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 21, 2019

Author Member

But if de don’t throw, how the monitoring system will detect the ongoing issue? It should be very exceptional and should probably trigger an alert.

Alternatively I can change the attribute to accept three value: throw (default), skip or fail. wdyt?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Mar 22, 2019

Member

I think we should log by default and add a scream option (defaulting to false) to allow to opt-in.

This comment has been minimized.

Copy link
@sroze

sroze Mar 31, 2019

Member

If there is an exception it basically means that 3rd party is down, which means that there is an unrecoverable error. If you (as a system) decided that whatever password entered should NOT have been "pwned" then at this point we should throw an exception here, not log something. Now, if you "prefer" it to not have been "pwned" just set skipOnError to true and you're covered.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 1, 2019

Member

I'm on the same side as @sroze

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

This one should be in 4.3 :) Let's talk on Slack about the best way to finish it.

@sroze

sroze approved these changes Mar 31, 2019

Copy link
Member

left a comment

Looks good to me 👍

return;
}
throw $e;

This comment has been minimized.

Copy link
@sroze

sroze Mar 31, 2019

Member

If there is an exception it basically means that 3rd party is down, which means that there is an unrecoverable error. If you (as a system) decided that whatever password entered should NOT have been "pwned" then at this point we should throw an exception here, not log something. Now, if you "prefer" it to not have been "pwned" just set skipOnError to true and you're covered.

@fabpot

fabpot approved these changes Apr 1, 2019

@fabpot fabpot force-pushed the dunglas:haveibeenpwned branch from a7c18ac to ec1ded8 Apr 1, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Thank you @dunglas.

@fabpot fabpot merged commit ec1ded8 into symfony:master Apr 1, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 1, 2019

feature #27738 [Validator] Add a HaveIBeenPwned password validator (d…
…unglas)

This PR was squashed before being merged into the 4.3-dev branch (closes #27738).

Discussion
----------

[Validator] Add a HaveIBeenPwned password validator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

This PR adds a new `Pwned` validation constraint to prevent users to choose passwords that have been leaked in public data breaches.
The validator uses the https://haveibeenpwned.com/ API. The implementation is similar to the one used by [Firefox Monitor](https://blog.mozilla.org/futurereleases/2018/06/25/testing-firefox-monitor-a-new-security-tool/). It allows to not expose the password hash using a k-anonymity model. The specific implementation for HaveIBeenPwned has been [described in depth by Cloudflare](https://blog.cloudflare.com/validating-leaked-passwords-with-k-anonymity/).

Usage:

```php
// Rejects the password if is present in any number of times in any data breach
class User
{
    /** @pwned */
    public $plainPassword;
}

// Rejects the password if is present more than 5 times in data breaches
class User
{
    /** @pwned(maxCount=5) */
    public $plainPassword;
}

// Customize the error message
class User
{
    /** @pwned(message='Please select another password, this one has already been hacked.') */
    public $plainPassword;
}
```

Commits
-------

ec1ded8 [Validator] Add a HaveIBeenPwned password validator
protected static $errorNames = [self::PWNED_ERROR => 'PWNED_ERROR'];
public $message = 'This password has been leaked in a data breach, it must not be used. Please use another password.';

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 1, 2019

Contributor

should be added in validators.en.xlf + any language you know :)

not sure we'll do another round of good first issues for the remaining locales :}

use Symfony\Component\Validator\Constraint;
/**
* Checks if a password has been leaked in a data breach.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 1, 2019

Contributor

perhaps clarify the password should NOT be leaked :/

@sstok sstok referenced this pull request Apr 3, 2019

Open

Search P0wned password DB? #92

wouterj added a commit to symfony/symfony-docs that referenced this pull request Apr 6, 2019

feature #11300 Added docs for the NotCompromisedPassword constraint (…
…javiereguiluz)

This PR was squashed before being merged into the master branch (closes #11300).

Discussion
----------

Added docs for the NotCompromisedPassword constraint

Documents symfony/symfony#27738.

Commits
-------

78a9387 Added docs for the NotCompromisedPassword constraint

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.