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] Make API endpoint for NotCompromisedPasswordValidator configurable #31054

Closed
xelan opened this issue Apr 10, 2019 · 8 comments
Closed

Comments

@xelan
Copy link
Contributor

xelan commented Apr 10, 2019

Description
The new NotCompromisedPasswordValidator is a very nice feature which greatly improves the security of user accounts.

Having a look at the source file for pwnedpasswords.com (pwned-passwords-sha1-ordered-by-count-v4.txt), providing the same basic API for range check endpoints is quite trivial.

Making the API endpoint configurable instead of a hardcoded constant (NotCompromisedPasswordValidator::RANGE_API) would allow to use the password compromise check in intranets without requiring internet access for the validation. Only the range API server would need to periodically refresh its data source.

In addition, integration testing and easier development using a minimal API server just providing a few known hashes would be possible, see #30871.

If you also think the feature might be useful, I can provide a PR.

Example

// src/Entity/User.php
namespace App\Entity;

use Symfony\Component\Validator\Constraints as Assert;

class User
{
    // ...

    /**
     * @Assert\NotCompromisedPassword (
     *     rangeApi = "https://password-check.internal.example.com"
     * )
     */
    protected $rawPassword;
}
@stof
Copy link
Member

stof commented Apr 10, 2019

I don't think it should be part of the annotation. This looks better as a constructor argument of the validator, defined in the project config.

@xelan
Copy link
Contributor Author

xelan commented Apr 10, 2019

Ah, yes you're right, thanks. So similar to the validation.disable_not_compromised_password config value, and therefore the configuration is part of the FrameworkBundle?

@stof
Copy link
Member

stof commented Apr 10, 2019

yes

@stof
Copy link
Member

stof commented Apr 10, 2019

btw, if we add that, we should rework the validation to look like

framework:
    validation:
        not_compromised_password:
            enable: true
            endpoint: 'https://password-check.internal.example.com'

That way, settings of the not_compromised_password validator are grouped together. As disable_not_compromised_password is only available in master and not in a release, we can still rename it without having to do a BC layer, if we do it now.

@xelan
Copy link
Contributor Author

xelan commented Apr 10, 2019

This proposal sounds great 👍

@nicolas-grekas
Copy link
Member

PR welcome :)

@Simperfit
Copy link
Contributor

@xelan Do you want to try to contribute this ?

@xelan
Copy link
Contributor Author

xelan commented Apr 10, 2019

Yes, I'm already working on it 😄

@fabpot fabpot closed this as completed May 6, 2019
fabpot added a commit that referenced this issue May 6, 2019
…rdValidator configurable (xelan)

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

Discussion
----------

[Validator] Make API endpoint for NotCompromisedPasswordValidator configurable

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes, but acceptable [1]
| Deprecations? | no [1]
| Tests pass?   | yes
| Fixed tickets | #30871, #31054
| License       | MIT
| Doc PR        | symfony/symfony-docs#... (TODO)

Makes the API endpoint for the `NotCompromisedPasswordValidator` configurable. The endpoint includes the placeholder which will be replaced with the first digits of the password hash for k-anonymity.

The endpoint can either be set via constructor injection of the validator if the component is used standalone, or via the framework configuration of symfony/framework-bundle.

[1] As discussed in #31054, the validator is not in a stable release yet, therefore the BC break is considered acceptable. No deprecation / BC layer is necessary.

Commits
-------

f6a80c2 [Validator] Make API endpoint for NotCompromisedPasswordValidator configurable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants