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

Expiring email 2FA codes #191

Closed
Splinter0 opened this issue Jun 19, 2023 · 4 comments
Closed

Expiring email 2FA codes #191

Splinter0 opened this issue Jun 19, 2023 · 4 comments

Comments

@Splinter0
Copy link

Description

Hello, we are using this bundle to provide both google and email based 2FA and it's been working quite well.
One thing that we think could be good to add is the ability to have expiring email codes. Right now we secure our 2FA endpoints through rate-limiting and account-lockout features. However we do feel that is quite essential for these codes to not remain forever valid (until the next login flow) to limit the possible attack surface.

We could implement this internally with a custom code generator that schedules a timed delete of the value in the user column. However I do see a possibility to extend the bundle by offering this as a configuration that can be added.

As of now the storage of the codes is quite simple and the ideal implementation for this would require probably a separate entity with some type of Datetime expiration column. However I'm not sure and maybe it's possible to implement such a feature with the already existing components of the bundle. I also want to make sure that this is not an already planned feature.

I would be more than happy to contribute with a PR, both trying by creating a more advanced code storage entity but also by adding support to the default code generator. Which implementation would you be leaning more towards?

Thanks again for providing such a good bundle to the community.

@scheb
Copy link
Owner

scheb commented Jun 28, 2023

Generally not a bad idea to expire the codes. But to make this work, the TwoFactorInterface needs to become more complex and requires a BC break. So not going to happen within version 6.

I'm not sure if it's even worth it, as the amount of code that you'd "safe" is not much, because you'd still need to write the whole data persisting and code expiring logic yourself. The standard email provider is really just good for people who need a quick out-of-the-box solution and find the feature set sufficient. That's why it's a separate package, so you'd just pull it into your codebase when you really want to use it.

For everything else, I'd recommend: You're likely better off to quickly implement your own 2fa provider. It's not much code and then you can have exactly the behavior that you want.

@Splinter0
Copy link
Author

Thanks for the response, I understand why you might not want to include such behavior in this bundle. I will go ahead and do this with a custom 2FA provider and post it here in the issue, so other people looking to implement something similar have somewhere to start.

@danielburger1337
Copy link
Contributor

Wouldn't the following approach solve BC?

Why should we store createdAt instead of expiresAt? This allows the developer to have custom logic of when to allow resending the email to the user.

namespace Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Email\Generator;

use Scheb\TwoFactorBundle\Model\Email\TwoFactorInterface;

/**
 * @method bool isCodeExpired(TwoFactorInterface $user)
 */
interface CodeGeneratorInterface
{ ... }
namespace Scheb\TwoFactorBundle\Model\Email;

/**
 * @method \DateTimeImmutable|null getEmailAuthCodeCreatedAt()
 * @method void                    setEmailAuthCodeCreatedAt(\DateTimeImmutable|null $createdAt)
 */
interface TwoFactorInterface
{ ... }
namespace Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Email\Generator;

use Psr\Clock\ClockInterface;
use Scheb\TwoFactorBundle\Mailer\AuthCodeMailerInterface;
use Scheb\TwoFactorBundle\Model\Email\TwoFactorInterface;
use Scheb\TwoFactorBundle\Model\PersisterInterface;

/**
 * @final
 */
class CodeGenerator implements CodeGeneratorInterface
{
    public function __construct(
        private readonly PersisterInterface $persister,
        private readonly AuthCodeMailerInterface $mailer,
        private readonly int $digits,
        private readonly string|null $expiresAfter = null,
        private readonly ClockInterface|null $clock = null
    ) {
    }

    public function generateAndSend(TwoFactorInterface $user): void
    {
        $user->setEmailAuthCode((string) $this->doGenerateCode());

        if (\method_exists($user, 'setEmailAuthCodeCreatedAt')) {
            $user->setEmailAuthCodeCreatedAt($this->now());
        }

        $this->persister->persist($user);
        $this->mailer->sendAuthCode($user);
    }

    public function reSend(TwoFactorInterface $user): void
    {
        if ($this->isCodeExpired($user) && \method_exists($user, 'getEmailAuthCodeCreatedAt') && \method_exists($user, 'setEmailAuthCodeCreatedAt')) {
            $user->setEmailAuthCode((string) $this->doGenerateCode());
            $user->setEmailAuthCodeCreatedAt($this->now());

            $this->persister->persist($user);
        }

        $this->mailer->sendAuthCode($user);
    }

    public function isCodeExpired(TwoFactorInterface $user): bool
    {
        if (null === $this->expiresAfter || !\method_exists($user, 'getEmailAuthCodeCreatedAt') || !\method_exists($user, 'setEmailAuthCodeCreatedAt')) {
            return false;
        }

        $now = $this->now();
        $expiresAt = $user->getEmailAuthCodeCreatedAt()?->add(new \DateInterval($this->expiresAfter));

        return null !== $expiresAt && $now->getTimestamp() >= $expiresAt->getTimestamp();
    }

    protected function generateCode(int $min, int $max): int
    {
        return \random_int($min, $max);
    }

    private function doGenerateCode(): int
    {
        $min = 10 ** ($this->digits - 1);
        $max = 10 ** $this->digits - 1;

        return $this->generateCode($min, $max);
    }

    private function now(): \DateTimeImmutable
    {
        return $this->clock?->now() ?? new \DateTimeImmutable();
    }
}
namespace Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Email;


namespace Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Email;

use Scheb\TwoFactorBundle\Model\Email\TwoFactorInterface;
use Scheb\TwoFactorBundle\Security\TwoFactor\AuthenticationContextInterface;
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Email\Generator\CodeGeneratorInterface;
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorFormRendererInterface;
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface;

/**
 * @final
 */
class EmailTwoFactorProvider implements TwoFactorProviderInterface
{
    ...

    public function validateAuthenticationCode(object $user, string $authenticationCode): bool
    {
        if (!($user instanceof TwoFactorInterface)) {
            return false;
        }

        // maybe add a config option to disable automatic resending?
        if (\method_exists($this->codeGenerator, 'isCodeExpired') && $this->codeGenerator->isCodeExpired($user)) {
            $this->codeGenerator->generateAndSend($user);

            return false;
        }

        // Strip any user added spaces
        $authenticationCode = \str_replace(' ', '', $authenticationCode);

        return $user->getEmailAuthCode() === $authenticationCode;
    }
    
    ...
}

The developer can then optionally enable this by setting expires_after:

scheb_two_factor:
    email:
        enabled: true
        expires_after: PT30M # defaults to NULL

@scheb
Copy link
Owner

scheb commented Feb 6, 2024

3rd party package for code-via-email https://github.com/danielburger1337/scheb-2fa-email is covering that now.

@scheb scheb closed this as completed Feb 6, 2024
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

3 participants