Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] Add a HaveIBeenPwned password validator#27738

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromdunglas:haveibeenpwned
Apr 1, 2019

Conversation

@dunglas
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRtodo

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

Usage:

// Rejects the password if is present in any number of times in any data breachclass User{/** @Pwned */public$plainPassword;}// Rejects the password if is present more than 5 times in data breachesclass User{/** @Pwned(maxCount=5) */public$plainPassword;}// Customize the error messageclass User{/** @Pwned(message='Please select another password, this one has already been hacked.') */public$plainPassword;}

linaori, Taluu, TomasVotruba, gmponos, azjezz, sh4ka, OskarStark, theofidry, andreybolonin, ndench, and 8 more reacted with thumbs up emojiMajkl578 reacted with thumbs down emojiandreybolonin, Deuchnord, and javiereguiluz reacted with heart emoji
@stof
Copy link
Member

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.

rejinka, jakzal, alister, mickaelandrieu, goetas, dominikzogg, sstok, joelwurtz, colinodell, k0d3r1s, and 6 more reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

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

TomasVotruba, andreybolonin, and sstok reacted with thumbs up emoji

@stof
Copy link
Member

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

@stloyd
Copy link
Contributor

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
Copy link
Contributor

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

@dunglas
Copy link
MemberAuthor

dunglas commentedJun 27, 2018•
edited
Loading

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 alsoused by 1password.

OskarStark reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to thenext milestoneJun 27, 2018
Copy link
Contributor

@Majkl578Majkl578 left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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, butyou 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, butthe 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 tosome URL sounds like a really bad idea (should the service be compromised, all users are doomed and you are in serious security trouble).

@ChangePlaces
Copy link

ChangePlaces commentedJun 27, 2018•
edited
Loading

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.

sstok, kix, and apfelbox reacted with laugh emojichiqui3d reacted with eyes emoji

@stof
Copy link
Member

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.

dunglas reacted with thumbs up emoji

@stof
Copy link
Member

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

dunglas reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

dunglas commentedJun 27, 2018•
edited
Loading

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.

ogizanagi, nicolas-grekas, kbond, Kocal, mablae, colinodell, azjezz, andreybolonin, chiqui3d, tseho, and 3 more reacted with thumbs up emojish4ka, andreybolonin, GwendolenLynch, and ndench reacted with heart emoji

@stof
Copy link
Member

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

dunglas reacted with thumbs up emoji

@goetas
Copy link
Contributor

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

Jean85, ChangePlaces, pizzaminded, Majkl578, mcfedr, umulmrum, gmponos, versgui, DavidBadura, and eugenefvdm reacted with thumbs up emoji

@teohhanhui
Copy link
Contributor

teohhanhui commentedJun 28, 2018•
edited
Loading

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

@teohhanhui
Copy link
Contributor

teohhanhui commentedJun 28, 2018•
edited
Loading

IMO we could add an optional dependency on a third-party HTTP client. Since there's no PSR, the closest thing ishttps://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

joelwurtz reacted with thumbs up emoji

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

Choose a reason for hiding this comment

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

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

dunglas reacted with thumbs up emoji
@dunglas
Copy link
MemberAuthor

@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
Copy link
MemberAuthor

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

teohhanhui reacted with thumbs up emoji

@ChangePlaces
Copy link

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.

return;
}

$httpClient = $this->httpClient;
Copy link
Member

Choose a reason for hiding this comment

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

Seems unecessary

$hashPrefix = substr($hash, 0, 5);
$url = sprintf(self::RANGE_API, $hashPrefix);

$result = $httpClient->request('GET', $url)->getContent();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@dunglas
Copy link
MemberAuthor

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

Should be ready to be merged

AlexandrePavy reacted with thumbs up emoji

return;
}

throw $e;
Copy link
Member

Choose a reason for hiding this comment

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

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.

ogizanagi and Majkl578 reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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?

jvasseur reacted with thumbs up emojiMajkl578 reacted with thumbs down emoji
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should log by default and add ascream option (defaulting tofalse) to allow to opt-in.

ro0NL, ogizanagi, and chalasr reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

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 setskipOnError to true and you're covered.

OskarStark, dunglas, and GwendolenLynch reacted with thumbs up emoji

Choose a reason for hiding this comment

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

I'm on the same side as@sroze

@fabpot
Copy link
Member

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

Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

return;
}

throw $e;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 setskipOnError to true and you're covered.

OskarStark, dunglas, and GwendolenLynch reacted with thumbs up emoji
@fabpot
Copy link
Member

Thank you@dunglas.

@fabpotfabpot merged commitec1ded8 intosymfony:masterApr 1, 2019
fabpot added a commit that referenced this pull requestApr 1, 2019
…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     <!-- seehttps://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        | todoThis PR adds a new `Pwned` validation constraint to prevent users to choose passwords that have been leaked in public data breaches.The validator uses thehttps://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 breachclass User{    /**@pwned */    public $plainPassword;}// Rejects the password if is present more than 5 times in data breachesclass User{    /**@pwned(maxCount=5) */    public $plainPassword;}// Customize the error messageclass 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.';
Copy link
Contributor

Choose a reason for hiding this comment

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

should be added invalidators.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.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps clarify the password should NOT be leaked :/

wouterj added a commit to symfony/symfony-docs that referenced this pull requestApr 6, 2019
…javiereguiluz)This PR was squashed before being merged into the master branch (closes#11300).Discussion----------Added docs for the NotCompromisedPassword constraintDocumentssymfony/symfony#27738.Commits-------78a9387 Added docs for the NotCompromisedPassword constraint
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
fabpot added a commit that referenced this pull requestJul 15, 2019
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     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | -  <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | - <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply   (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->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
symfony-splitter pushed a commit to symfony/validator that referenced this pull requestJul 15, 2019
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     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | -  <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | - <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply   (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->It has 2 messages translated to Hungarian introduced insymfony/symfony#27738 andsymfony/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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof left review comments

@xabbuhxabbuhxabbuh requested changes

@fabpotfabpotfabpot approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

+10 more reviewers

@jvasseurjvasseurjvasseur left review comments

@DocFXDocFXDocFX left review comments

@sebastianblumsebastianblumsebastianblum left review comments

@ogizanagiogizanagiogizanagi left review comments

@Majkl578Majkl578Majkl578 requested changes

@linaorilinaorilinaori left review comments

@ro0NLro0NLro0NL left review comments

@GregoireHebertGregoireHebertGregoireHebert left review comments

@srozesrozesroze approved these changes

@muratdemir0muratdemir0muratdemir0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

23 participants

@dunglas@stof@stloyd@linaori@ChangePlaces@goetas@teohhanhui@sebastianblum@xabbuh@sstok@andreybolonin@fabpot@Majkl578@nicolas-grekas@sroze@jvasseur@GregoireHebert@OskarStark@ro0NL@DocFX@ogizanagi@muratdemir0@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp