-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Add a normalization step for the user-identifier in firewalls #51744
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
[Security] Add a normalization step for the user-identifier in firewalls #51744
Conversation
aec0d91
to
180fd7a
Compare
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php
Outdated
Show resolved
Hide resolved
25e91d6
to
782ef6a
Compare
782ef6a
to
7bda5b3
Compare
Rebased to target Symfony 7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a changelog entry :)
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Tests/Authenticator/Passport/Badge/UserBadgeTest.php
Outdated
Show resolved
Hide resolved
ed19a61
to
adb4098
Compare
I checked how the built-in user providers could handle this, but I'm not sure if any of them can be changed as well. |
What's the next step here? How do we make it easy to opt-in for the OWASP-recommended normalization? |
adb4098
to
4f87434
Compare
Hello @nicolas-grekas, This PR is now up to date, rebased against the 7.3 branch, and squashed. It’s ready for review. The next step is to finalize the documentation. It should demonstrate how to inject a normalizer to ensure that different variations (e.g., BR. |
254b8cd
to
f81db2a
Compare
Can you give us a hint about how this can be used? : ) |
This feature will most likely be used by the firewall Authenticator service that creates the UserBadge, but it could also be used by the Below is a simple example of how to implement a namespace App\Security;
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
use Symfony\Component\String\UnicodeString;
use function Symfony\Component\String\u;
final class NormalizedUserBadge extends UserBadge
{
public function __construct(string $identifier)
{
$callback = static fn (string $identifier) => u($identifier)->normalize(UnicodeString::NFKC)->ascii()->lower()->toString();
parent::__construct($identifier, identifierNormalizer: $callback);
}
} From the Authenticator: <?php
namespace App\Security;
final class PasswordAuthenticator extends AbstractLoginFormAuthenticator
{
// Simplified for brievety
public function authenticate(Request $request): Passport
{
$username = (string) $request->request->get('username', '');
$password = (string) $request->request->get('password', '');
$request->getSession()
->set(SecurityRequestAttributes::LAST_USERNAME, $username);
return new Passport(
new NormalizedUserBadge($username),
new PasswordCredentials($password),
[
//All other useful badges
]
);
} When a user attempts to log in with “John.Doe” or “JOhn.doe,” both will be treated as “john.doe.” Another potential use case involves scenarios where users can log in with various forms of the same identifier: $callback = static function (string $identifier) {
$normalized = u($identifier)->normalize(UnicodeString::NFKC)->ascii()->lower()->toString();
$username = '';
$domain = '';
match (true) {
str_contains($normalized, '@') => [
$localPart, $domain = explode('@', $normalized, 2),
$username = str_contains($localPart, '.')
? substr(explode('.', $localPart, 2)[0], 0, 1) . explode('.', $localPart, 2)[1]
: $localPart,
],
str_contains($normalized, '\\') => [
$domain, $username = explode('\\', $normalized, 2)
],
str_contains($normalized, '://') => [
$parts = parse_url($normalized),
isset($parts['host'], $parts['path']) && [
$domain = $parts['host'],
$username = ltrim($parts['path'], '/')
]
],
default => $username = $normalized
};
$username = preg_replace('/[^a-z0-9]/', '', $username);
return $domain . '\\' . $username;
}; |
f81db2a
to
149754a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @Spomky!
I think the documentation of this feature must provide real life example to help users to use it properly (I think mostly about username policy of Google where foo.bar@gmail.com
, foobar@gmail.com
, and f.o.o.b.a.r@gmail.com
refers to the same user).
For the code, it's a 👍 for me
149754a
to
db01811
Compare
Thank you @Spomky. |
As a next step, I'm wondering if we should somehow define some reusable normalizers that developers can use without reinventing the wheel (like the Google strategy mentioned above). |
With this PR, the user identifier is lowered and normalized to make sure they are case-insensitive.
When enable, please note that the user provider or the user repository should be adapt:
This is a very first attempt to resolve issue #49269. Any comments and reactions are welcome.
See also https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#user-ids