Skip to content

[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

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

Spomky
Copy link
Contributor

@Spomky Spomky commented Sep 25, 2023

Q A
Branch? 7.3
Bug fix? yes
New feature? yes
Deprecations? no
Tickets Fix #49269
License MIT
Doc PR TODO

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:

  • user identifier stored in the future shall be lowered and normalized as well
  • the queries for existing user 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

@Spomky Spomky requested a review from chalasr as a code owner September 25, 2023 12:40
@carsonbot carsonbot added this to the 6.4 milestone Sep 25, 2023
@Spomky Spomky changed the title First try for user identifier normalization [Security] First try for user identifier normalization Sep 25, 2023
@Spomky Spomky force-pushed the features/user-identifier-normalization branch from aec0d91 to 180fd7a Compare September 25, 2023 13:20
@Spomky Spomky changed the title [Security] First try for user identifier normalization [Security] Add a normalization step for the user-identifier in firewalls Sep 26, 2023
@Spomky Spomky force-pushed the features/user-identifier-normalization branch 3 times, most recently from 25e91d6 to 782ef6a Compare October 5, 2023 19:23
@Spomky Spomky force-pushed the features/user-identifier-normalization branch from 782ef6a to 7bda5b3 Compare November 2, 2023 16:26
@Spomky Spomky changed the base branch from 6.4 to 7.0 November 2, 2023 16:26
@Spomky
Copy link
Contributor Author

Spomky commented Nov 2, 2023

Rebased to target Symfony 7.1

@OskarStark OskarStark modified the milestones: 6.4, 7.1 Nov 2, 2023
@OskarStark OskarStark removed the Bug label Nov 2, 2023
@carsonbot carsonbot changed the title [Security] Add a normalization step for the user-identifier in firewalls Add a normalization step for the user-identifier in firewalls Nov 2, 2023
@carsonbot carsonbot changed the title Add a normalization step for the user-identifier in firewalls [Security] Add a normalization step for the user-identifier in firewalls Nov 20, 2023
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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 :)

@Spomky Spomky force-pushed the features/user-identifier-normalization branch from ed19a61 to adb4098 Compare December 10, 2023 09:25
@Spomky
Copy link
Contributor Author

Spomky commented Dec 10, 2023

do we have any advice on the topic? Or any things we can anticipate that could be part of the same PR?

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.
How the user ID is stored or retrieved is highly dependent on the developer's implementation.
To me, the most important now seems to propagate this into the documentation.

@Spomky Spomky marked this pull request as draft February 1, 2024 20:02
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@nicolas-grekas
Copy link
Member

What's the next step here? How do we make it easy to opt-in for the OWASP-recommended normalization?

@Spomky Spomky force-pushed the features/user-identifier-normalization branch from adb4098 to 4f87434 Compare January 5, 2025 13:53
@Spomky Spomky marked this pull request as ready for review January 5, 2025 13:59
@carsonbot carsonbot added the Bug label Jan 5, 2025
@Spomky
Copy link
Contributor Author

Spomky commented Jan 5, 2025

What's the next step here? How do we make it easy to opt-in for the OWASP-recommended normalization?

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., JOHN.DOE, john.doe, John.DOe) are treated as equivalent.

BR.

@Spomky Spomky force-pushed the features/user-identifier-normalization branch 2 times, most recently from 254b8cd to f81db2a Compare January 5, 2025 14:20
@nicolas-grekas
Copy link
Member

Can you give us a hint about how this can be used? : )

@Spomky
Copy link
Contributor Author

Spomky commented Jan 6, 2025

This feature will most likely be used by the firewall Authenticator service that creates the UserBadge, but it could also be used by the TokenHandler::getUserBadge when handling access tokens.

Below is a simple example of how to implement a NormalizedUserBadge.
In this example, it ensures that any variations in the username—such as uppercase, lowercase, or accented characters—are normalized to a consistent format.

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.”
Keep in mind that any existing usernames in the database should also be normalized accordingly.

Another potential use case involves scenarios where users can log in with various forms of the same identifier: john.doe@acme.com, acme.com\jdoe, https://amce.com/+jdoe or acct:jdoe@acme.com.
By using this feature, you can unify all these variants into a single, standardized format (e.g., “domain\nickname”) without having to create custom queries on the repository side.
In this case, the callback could be as follows (not tested):

        $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;
        };

@Spomky Spomky force-pushed the features/user-identifier-normalization branch from f81db2a to 149754a Compare January 18, 2025 07:20
Copy link
Member

@welcoMattic welcoMattic left a 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

@fabpot fabpot force-pushed the features/user-identifier-normalization branch from 149754a to db01811 Compare February 7, 2025 08:38
@fabpot
Copy link
Member

fabpot commented Feb 7, 2025

Thank you @Spomky.

@fabpot fabpot merged commit e04b670 into symfony:7.3 Feb 7, 2025
2 checks passed
@fabpot
Copy link
Member

fabpot commented Feb 7, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a normalization step for the user-identifier in firewalls
8 participants