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

Adding context length configuration for 2FA to ensure better security standards #567

Closed
MattLoyeD opened this issue Aug 30, 2024 · 1 comment

Comments

@MattLoyeD
Copy link
Contributor

Summary

We need to ensure a minimum length for 2FA secret, current secret length by default is 80-bit (16 characters), but 128-bit (26 characters) is becoming minimum in some cases and the best default is 160-bit.

It's recommended to use 128-bit or 160-bit because some Authenticator apps may have problems with non-RFC-recommended lengths (Namely https://play.google.com/store/apps/details?id=org.fedorahosted.freeotp).

Proposal

Just add some contextual config('fortify-options.two-factor-authentication.secret-length', 16), it will be retro compatible and secured as well.
In

public function generateSecretKey()

<?php
    /**
     * Generate a new secret key.
     *
     * @param  int  $secret_length = 16
     * @return string
     */
    public function generateSecretKey(int  $secret_length = 16 )
    {
        return $this->engine->generateSecretKey($secret_length);
    }

In

public function __invoke($user, $force = false)

<?php
   /**
     * Enable two factor authentication for the user.
     *
     * @param  mixed  $user
     * @param  bool  $force
     * @return void
     */
    public function __invoke($user, $force = false)
    {
        if (empty($user->two_factor_secret) || $force === true) {
            $secret_length = (int) config('fortify-options.two-factor-authentication.secret-length', 16);
            $user->forceFill([
                'two_factor_secret' => encrypt($this->provider->generateSecretKey($secret_length)),
                'two_factor_recovery_codes' => encrypt(json_encode(Collection::times(8, function () {
                    return RecoveryCode::generate();
                })->all())),
            ])->save();

            TwoFactorAuthenticationEnabled::dispatch($user);
        }
    }

There is also some adaptation to do on

public function generateSecretKey();

@driesvints
Copy link
Member

Heya, thanks for submitting this.

This seems like a feature request or an improvement. For these, we'd appreciate a pull request instead so we can look at actual code. If you need feedback about an idea, we suggest to post an idea discussion here first. Please only use the issue tracker to report bugs and issues with this library.

Thanks!

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

No branches or pull requests

2 participants