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

Badge support for AuthenticationManager and API 2fa-auth #67

Open
Seldaek opened this issue Mar 31, 2021 · 4 comments
Open

Badge support for AuthenticationManager and API 2fa-auth #67

Seldaek opened this issue Mar 31, 2021 · 4 comments
Milestone

Comments

@Seldaek
Copy link
Contributor

Seldaek commented Mar 31, 2021

Context: I am currently migrating a project to the experimental AuthenticationManager as well as using this bundle instead of a homegrown 2fa solution.

We do have an API with an existing endpoint for logging in users which returns a special code to the client if 2fa is required, and then expects the login API call again with username, password, AND the 2fa code all present in one to do authentication in a single request.

This does not really play well with your API guidelines which require a two-request flow, which IMO is more complex as the client needs to keep even more state.

Anyway I ended up implementing this myself as it isn't that hard, but I also got pretty familiar with the new auth code as I had to do various authenticator implementations. So I am mostly posting this here for others who may need the help to figure this out.. and I am thinking if this bundle offered better support for it in the future it would maybe be good (could be a v6 thing for sure).

I now create this badge in my API authenticator, which gets added on the passport:

new TwoFactorAuthBadge($credentials['2fa_code'] ?? null)

The badge class is a very simple value object:

<?php

class TwoFactorAuthBadge implements BadgeInterface
{
    private $resolved = false;
    private ?string $twoFaCode;

    public function __construct(?string $twoFaCode)
    {
        $this->twoFaCode = $twoFaCode;
    }

    public function getTwoFaCode(): ?string
    {
        return $this->twoFaCode;
    }

    /**
     * @internal
     */
    public function markResolved(): void
    {
        $this->resolved = true;
    }

    public function isResolved(): bool
    {
        return $this->resolved;
    }
}

And the auth listener which checks the badge is also not that complex, which is why I think it might be worth including here:

<?php

use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Totp\TotpAuthenticatorInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
use Symfony\Component\Security\Http\Authenticator\Passport\Passport;

class TwoFactorAuthListener implements EventSubscriberInterface
{
    private TotpAuthenticatorInterface $totpAuthenticator;

    public function __construct(TotpAuthenticatorInterface $totpAuthenticator)
    {
        $this->totpAuthenticator = $totpAuthenticator;
    }

    public function checkPassport(CheckPassportEvent $event): void
    {
        /** @var Passport $passport */
        $passport = $event->getPassport();
        if (!$passport->hasBadge(TwoFactorAuthBadge::class)) {
            return;
        }

        /** @var TwoFactorAuthBadge $badge */
        $badge = $passport->getBadge(TwoFactorAuthBadge::class);
        if ($badge->isResolved()) {
            return;
        }

        /** @var User $user */
        $user = $passport->getUser();

        if (!$user->getTwoFaActive()) {
            $badge->markResolved();

            return;
        }

        if (null === $badge->getTwoFaCode()) {
            throw new MissingTwoFaCodeException();
        }

        if (false === $this->totpAuthenticator->checkCode($user, $badge->getTwoFaCode())) {
            throw new InvalidTwoFaCodeException();
        }

        $badge->markResolved();
    }

    public static function getSubscribedEvents(): array
    {
        return [CheckPassportEvent::class => ['checkPassport', 512]];
    }
}

There are also two special exceptions MissingTwoFaCodeException and InvalidTwoFaCodeException both extending AuthenticationException that I added to be able to render these correctly in onAuthenticationFailure.

Note: Doing this way, I did not configure 2fa on the API firewall at all, as it's handled purely within the Authenticator via the badge.

@scheb
Copy link
Owner

scheb commented Apr 2, 2021

To me it looks like, the only thing that you're using from the bundle is the TOTP authenticator. Actually, you don't even need that one, you could use the underlaying library directly and skip the bundle's glue code.

Managing the intermediate state between identifying yourself (login) and completing authentication (2fa complete, fully authenticated) is the one main problem that the bundle is solving. In your use-case you don't have that state, since you're doing the whole authentication within a single request. So you have a single-step authentication process that is checking for multiple factors. With the new authenticator system this can actually be done in a very elegant way, adding badges to add additonal requirements to the authentication process. You demonstrated that very well in the example.

So is there any other bundle features that you're using, that would actually make it worth using the bundle? Because to me it looks, with for a single-step authentication use-case you actually don't need the bundle ;)

@scheb
Copy link
Owner

scheb commented Apr 2, 2021

🤔 The more I think about this, the more it makes sense to use the bundle for such a use case - if you want extra features i.e. trusted IPs, trusted devices, backup codes.

I remember that people have asked me before if they could pass the 2fa code together with username+password, but with the old security system it wasn't easy to implement, because its architecture allows only one authentication provider per request (except you're pulling off some really bad hacks). With the new authenticator system it should be possible and relatively easy to support "single step authentication with multiple factors". I think that would be a nice addition for bundle version 6, which would leverage the new possibilities of authenticator-based security.

@scheb scheb added this to the 6.x milestone Apr 2, 2021
@Seldaek
Copy link
Contributor Author

Seldaek commented Apr 2, 2021

The reason we use the bundle is simply that we fully use it for the web clients which have session based auth, so there the full integration makes sense and works great.

The API however is designed differently so I had to find a solution there.

I assumed that backup codes would just work as an input when using totpAuhenticator->checkCode but I haven't set them up yet nor have I looked how it's implemented. If that isn't the case then it sounds like an additional feature request :) IMO it'd make sense that a backup code is just seen as a valid code but admittedly there might be extra considerations preventing this.

Anyway i got this working so no rush for me. If you add something in v6 or whenever that lets me delete some code great but no pressure. I just felt this might benefit others.

@scheb
Copy link
Owner

scheb commented Apr 3, 2021

Ah, I see. Yea makes sense when you're also using it on the web application.

I assumed that backup codes would just work as an input when using totpAuhenticator->checkCode but I haven't set them up yet nor have I looked how it's implemented. [...] IMO it'd make sense that a backup code is just seen as a valid code but admittedly there might be extra considerations preventing this.

No, unfurtunately backup codes wouldn't work. The TotpAuthenticator is really just checking the TOTP code, backup codes are handled by a separate listener, see CheckBackupCodeListener. If you'd use the TwoFactorCodeCredentials badge from the bundle you'd get two-factor code checking and backup codes for free, but that requires a TwoFactorPassport to be used.

Trusted IPs and trusted devices are handled on a whole different layer. These are conditions if 2fa should even be started - in your implementation those could be additional conditions if the TwoFactorAuthBadge is attached or not. They are built in a very specific way ("chain of responsibility" pattern), that doesn't very well go with your implementation. If you need those features as well, have a look at IpWhitelistHandler/TrustedDeviceHandler.

If that isn't the case then it sounds like an additional feature request :)

Jep, accepted, I can see the value.

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

2 participants