[Validator] Add a HaveIBeenPwned password validator #27738

dunglas · 2018-06-27T10:07:11Z

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 · 2018-06-27T10:10:25Z

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 · 2018-06-27T10:12:25Z

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 · 2018-06-27T10:14:34Z

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

src/Symfony/Component/Validator/Constraints/PwnedValidator.php

stloyd · 2018-06-27T10:22:50Z

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 · 2018-06-27T13:35:14Z

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

dunglas · 2018-06-27T13:38:27Z

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.

Majkl578

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 · 2018-06-27T15:40:46Z

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 · 2018-06-27T15:58:38Z

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 · 2018-06-27T16:00:43Z

@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 · 2018-06-27T16:34:01Z

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 · 2018-06-27T20:22:59Z

Licensing is not an issue: https://twitter.com/troyhunt/status/1012066309251592192

goetas · 2018-06-27T22:04:22Z

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

teohhanhui · 2018-06-28T06:00:21Z

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

teohhanhui · 2018-06-28T06:04:19Z

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

DocFX · 2018-06-28T08:54:52Z

src/Symfony/Component/Validator/Tests/Constraints/PwnedValidatorTest.php

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


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

dunglas · 2018-06-28T09:11:00Z

@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 · 2018-06-28T09:12:10Z

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

ChangePlaces · 2018-06-28T10:08:38Z

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.

fabpot · 2019-03-11T15:16:19Z

src/Symfony/Component/Validator/Constraints/NotPwnedValidator.php

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


Seems unecessary

fabpot · 2019-03-11T15:16:48Z

src/Symfony/Component/Validator/Constraints/NotPwnedValidator.php

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


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

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).

src/Symfony/Component/Validator/Tests/Constraints/NotPwnedValidatorTest.php

GregoireHebert · 2019-03-15T12:03:28Z

src/Symfony/Component/Validator/Tests/Constraints/NotPwnedValidatorTest.php

+    public function testThresholdNotReached()
+    {
+        $constraint = new NotPwned(['threshold' => 10]);
+        $this->validator->validate(self::PASSWORD_LEAKED, $constraint);


Suggested change

$this->validator->validate(self::PASSWORD_LEAKED, $constraint);

$this->validator->validate(self::PASSWORD_LEAKED, new NotPwned(['threshold' => 10]));

src/Symfony/Component/Validator/Constraints/NotPwned.php

dunglas · 2019-03-21T12:24:15Z

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

Should be ready to be merged

xabbuh · 2019-03-21T18:54:17Z

src/Symfony/Component/Validator/Constraints/NotPwnedValidator.php

+                return;
+            }
+
+            throw $e;


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.

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?

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.

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?

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

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.

I'm on the same side as @sroze

fabpot · 2019-03-31T17:44:35Z

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

sroze

Looks good to me 👍

sroze · 2019-03-31T17:50:34Z

src/Symfony/Component/Validator/Constraints/NotPwnedValidator.php

+                return;
+            }
+
+            throw $e;


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 · 2019-04-01T16:48:18Z

Thank you @dunglas.

@pwned

…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  | 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](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

ro0NL · 2019-04-01T17:03:41Z

src/Symfony/Component/Validator/Constraints/NotPwned.php

+
+    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.';


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 :}

ro0NL · 2019-04-01T17:03:58Z

src/Symfony/Component/Validator/Constraints/NotPwned.php

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


perhaps clarify the password should NOT be leaked :/

…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

This PR was squashed before being merged into the 3.4 branch (closes #32539). Discussion ---------- [Validator] Add missing Hungarian translations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | -  | License | MIT | Doc PR | -   It has 2 messages translated to Hungarian introduced in #27738 and #32435. AFAIK it should be based on 3.4, but tell me if I should rebase any of the commits. Commits ------- 2fee912 [Validator] Add missing Hungarian translations

This PR was squashed before being merged into the 3.4 branch (closes #32539). Discussion ---------- [Validator] Add missing Hungarian translations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | -  | License | MIT | Doc PR | -   It has 2 messages translated to Hungarian introduced in symfony/symfony#27738 and symfony/symfony#32435. AFAIK it should be based on 3.4, but tell me if I should rebase any of the commits. Commits ------- 2fee9124ba [Validator] Add missing Hungarian translations

carsonbot added Status: Needs Review Validator Feature labels Jun 27, 2018

dunglas force-pushed the haveibeenpwned branch from f252fdd to d2eaa56 Compare June 27, 2018 10:10

stof reviewed Jun 27, 2018

View reviewed changes

src/Symfony/Component/Validator/Constraints/PwnedValidator.php Outdated Show resolved Hide resolved

muratdemir0 approved these changes Jun 27, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 27, 2018

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

Majkl578 suggested changes Jun 27, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Jun 27, 2018

DocFX reviewed Jun 28, 2018

View reviewed changes

dunglas force-pushed the haveibeenpwned branch from 49f2d4f to 51ac9c3 Compare March 11, 2019 14:14

fabpot requested changes Mar 11, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 11, 2019

GregoireHebert reviewed Mar 15, 2019

View reviewed changes

dunglas force-pushed the haveibeenpwned branch from d78966f to 7fe4b25 Compare March 21, 2019 12:23

dunglas force-pushed the haveibeenpwned branch from 7fe4b25 to a7c18ac Compare March 21, 2019 12:24

xabbuh requested changes Mar 21, 2019

View reviewed changes

carsonbot removed the Status: Needs Work label Mar 31, 2019

sroze approved these changes Mar 31, 2019

View reviewed changes

carsonbot added the Status: Reviewed label Mar 31, 2019

fabpot approved these changes Apr 1, 2019

View reviewed changes

[Validator] Add a HaveIBeenPwned password validator

ec1ded8

fabpot force-pushed the haveibeenpwned branch from a7c18ac to ec1ded8 Compare April 1, 2019 16:48

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

ro0NL reviewed Apr 1, 2019

View reviewed changes

sstok mentioned this pull request Apr 3, 2019

Search P0wned password DB? rollerworks/PasswordStrengthBundle#92

Closed

javiereguiluz mentioned this pull request Apr 5, 2019

Added docs for the NotCompromisedPassword constraint symfony/symfony-docs#11300

Merged

pierlon mentioned this pull request Apr 5, 2019

HaveIBeenPwned password validator laravel/ideas#1591

Open

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

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

1ed mentioned this pull request Jul 14, 2019

[Validator] Add missing Hungarian translations #32539

Merged

	$this->validator->validate(self::PASSWORD_LEAKED, $constraint);
	$this->validator->validate(self::PASSWORD_LEAKED, new NotPwned(['threshold' => 10]));


		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.';

[Validator] Add a HaveIBeenPwned password validator #27738

[Validator] Add a HaveIBeenPwned password validator #27738

Conversation

dunglas commented Jun 27, 2018

stof commented Jun 27, 2018

dunglas commented Jun 27, 2018

stof commented Jun 27, 2018

stloyd commented Jun 27, 2018

linaori commented Jun 27, 2018

dunglas commented Jun 27, 2018 • edited Loading

Majkl578 left a comment • edited Loading

Choose a reason for hiding this comment

ChangePlaces commented Jun 27, 2018 • edited Loading

stof commented Jun 27, 2018

stof commented Jun 27, 2018

dunglas commented Jun 27, 2018 • edited Loading

stof commented Jun 27, 2018

goetas commented Jun 27, 2018

teohhanhui commented Jun 28, 2018 • edited Loading

teohhanhui commented Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

dunglas commented Jun 28, 2018

dunglas commented Jun 28, 2018

ChangePlaces commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 31, 2019

sroze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Jun 27, 2018 •

edited

Loading

Majkl578 left a comment •

edited

Loading

ChangePlaces commented Jun 27, 2018 •

edited

Loading

dunglas commented Jun 27, 2018 •

edited

Loading

teohhanhui commented Jun 28, 2018 •

edited

Loading

teohhanhui commented Jun 28, 2018 •

edited

Loading