Allow TFA bypass during password reset by default for super admin (uid1)

To be honest, the whole reason why reset_pass_skip_enabled was added is to allow drush uli to still work.

I don't really like the way it is implemented, since as you point out, it is set by default to a value that prevents drush uli from working.. However, if you're able to use drush uli, you can also run drush cset to set the value to whatever you want.

I'd much rather that the code would detect if the Drupal core has been bootstrapped by Drush, and just allow drush uli to work. Such a code would have no impact on the security vulnerability it was designed to prevent, since the attacker is now running Drush, so he already owns the server. I like the extra check on this comment: https://www.drupal.org/project/drush/issues/548798#comment-5710388 → , since it not only checks that PHP is being run on the command line, but also checks for the existence of Drush functions. Note that that code is for Drupal 6, and for Drupal 10, the check should probably be if the Drush class exists.

> since the attacker is now running Drush, so he already owns the server

Thats my exact point. Attacker can even override TOTP Seed, when they have Drush access.

I don't see the config reset_pass_skip_enabled available in 2.x-dev branch ?

So it seems it is logical to enable reset_pass_skip_enabled by default, to avoid locking out of admin from the site.

you overlooked and not enabled it , admin wont be able to reset password even with drush cli.

In hindsight perhaps there should have been an update hook even though this went out in a security update, to enable it for sites that were already deployed, as a compromise between secure by default but not disrupting existing site operations. At this point we are far enough into it I don't see there being value to try and push one now

For new deployments it is however very much intentional, if the admin user is a member of a role that Requires TFA than the admin should be protected as well and it should take an explicit action to disable that protection. If the admin role is not a member of a role that requires TFA and the account doesn't have TFA setup than the option is moot.

A few alternatives are in #3374221-18: SA-CONTRIB-2023-030 and 2.x → however if even none of those would work (such as users don't have access to Drush) it is in my opinion acceptable for the final alternative fix to not be 'point and click' since this should be an extremely rare occurance for an admin to loose their token.

However, if you're able to use drush uli, you can also run drush cset

Thats my exact point. Attacker can even override TOTP Seed, when they have Drush access.

I noted the following in #3374221-18: SA-CONTRIB-2023-030 and 2.x → , however I will note it here too for others that read only this issue:
This is not necessarily a true statement, its possible based on server configs (such as sudo) a user may only be able to run drush uli, and be prohibited access to drush cset. They may not necessarily have ability to even read any of the code in the deployed site let alone modified the code based on the directory permissions the account posses.

#2481253: Allow Drush uli login command to bypass TFA → makes an interesting argument for an additional command separate from drush uli, which would add separation and additional checks above the existing ULI links.

I disagree with the issue summery that having large lists of TFA tokens is significant issue in modern times. However when the issue was opened in 2015 authentication OTP apps may have been less streamlined and it may have indeed been an issue. These days I find apps can be much more organized and support searching, making this less of an issue.

It may or may not be as relevant today as a solution, I would need more time to review the concept to opine.

In my opinion, this feature is a "won't fix".

If a user needs to bypass tfa it should not be based on uid 1. It should be based on some other mechanism like access to a special flow or feature in drush.

I have just observed today that setting reset_pass_skip_enabled doesnt let you to login to the site with drush uli it keeps on asking for TFA code.

Use Case :

I have a production site which I intended to set up on local, and pulled database from production and restored to local. I do not have credentials and TFA for Super Admin. So I tried to use drush uli but it asks for TFA but I dont have access to TFA tokens. So I disabled reset_pass_skip_enabled through drush command drush cset tfa.settings reset_pass_skip_enabled 0 and cleared cache and ran cron as well but still same issue.

drupal10 $ddev drush @book.local st
Drupal version   : 10.1.5
Site URI         : https://book.local
DB driver        : mysql
DB hostname      : db
DB port          : 3306
DB username      : db
DB name          : book
Database         : Connected
Drupal bootstrap : Successful
Default theme    : olivero
Admin theme      : claro
PHP binary       : /usr/bin/php8.1
PHP config       : /etc/php/8.1/cli/php.ini
PHP OS           : Linux
PHP version      : 8.1.23
Drush script     : /var/www/html/vendor/bin/drush
Drush version    : 12.2.0.0
Drush temp       : /tmp
Drush configs    : /var/www/html/vendor/drush/drush/drush.yml
Install profile  : standard
Drupal root      : /var/www/html/web
Site path        : sites/book
Files, Public    : sites/book/files/public
Files, Private   : ../files-private/book
Files, Temp      : /tmp

considering this flag lockouts admin setting it as major.

drush cset tfa.settings reset_pass_skip_enabled 0
This disables the bypass, this makes TFA required for password resets of the super admin.

As the flag is not available in 2.x it is causing the issue. May be we need to add update hook to remove config from database ?

It is at most stray config that will need cleanup when we deal with the final parts of 📌 SA-CONTRIB-2023-030 and 2.x Active . It causes no harm to 2.x development as it is not referenced anywhere.

I also locks out all other users as well. When using drush uli with uid argument.

That is by design. The bypass was only intended for the UID 1 account, and only to limit the damage to existing workflows on that account as part of a security release.

I intended to set up on local, and pulled database from production and restored to local.

I believe a drush sql sanitization would also work in that scenario.

Do we have any drush command or sql query to disable TFA for uid 1 alone?

As mentioned by @greggles in #10 we really should not develop or maintain any feature that is bound to UID 1 only.

One of the mentioned alternatives from #8 is ✨ Provide drush command to reset a user's TFA data Fixed . This is likely the closest issue matching your request.

@cmlara in #12 you mentioned

I believe a drush sql sanitization would also work in that scenario.

I believe that wont work as mentioned in #3351021 📌 provide an option to skip sql sanitization for tfa Active it sanitizes TFA data as well , which in turn we need to reset TFA, but we cant as we don't have TFA code to successful login.

I didn't checked 2.x code but on 1.x I think everything is fine. Beside drush cset ther eis also the possibility to set config via settings.local.php. So I just tried the following and everything works fine:
$config['tfa.settings']['reset_pass_skip_enabled'] = TRUE;

Because Admins with drush access can change everything including configuration in database or just override as described above there is no need to open this security setting by default. Or to be more clear: The suggested config default "reset_pass_skip_enabled: true" would be a reason to open a security issue in my opinion.
So +1 for close. But it should be "works as designed" status.

I had left this open to just allow a little more time for discussion in case it was needed. At this time however at least one of the alternative solutions (Drush command to disable TFA for a user) is making significant progress and other options (using settings.php) are indeed also valid solutions.

I'm going to agree this should be Closed-WAD since it being disabled by default was indeed ntentional under a 'security first' standard.

cmlara → closed merge request !56