Allow Drush uli login command to bypass TFA

I like the idea of this but I'm not in favor of using the tfa_user_settings table via tfa_basic_setup_save_data() for storing these login tokens.

While likely infrequent, an additional effect of an account using this will be their TFA Security page listing TFA settings updated at last log in time, because the saved column is updated via tfa_basic_setup_save_data().

@scor what are some other ways of accomplishing this without storing in tfa_user_settings? Is it possible without data storage?

Additionally, but not a blocker, I'd prefer for TFA plugins to not rely on GET/POST parameters and instead have most Drupal/larger context dependencies injected. You could do that by implementing tfa_basic_cli_login_create() and passing URL parameter in to constructor for use as a object property. See tfa_basic_sms_create() for example.

I like to use tfa_basic for many customer websites but I also organizing my support via SSH/Drush and drush uli without sharing passwords between support peoples. Because of this I can block password access for user 1 with my gate module. For this use case it would helpful to have an option for disabling tfa_basic for user 1. This can be done with a single system variable. But for other use cases like testing situations ("login as user 1234") drush uli is a cleaner and safer way to login as another user as some other solutions out there like masquerade.

Currently I can't imagine a solution without token/get to communicate between drush and webserver process. My module gate can also be used for a two- or multifactor authentication. For this I programmed a similar solution to bypass the onetime login block function provided by the module itself. And on my new chmod-Module I also used the existing cron token for a direct communication between drush and the webserver process through curl.

The #1 patch by @scor won't apply clean and I corrected the code manually to test. So my attached patch contains nothing new. The code is working but I understand with the problem tfa_user_settings table. What about a separate table like the "trusted browser" plugin is using? For example we can use a table "tfa_cli_login" to store token. Maybe we should also store a timestamp there and set a small time window to use this token. Additionally we should delete all old unused tokens on cron. I'd like to help with this because I need it and didn't like to create a complete bypass solution.
I also like to get TFA and my module gate working together in other places. But for this I will open other issues if there is someting needed in this module.

These 3 issues share some similarities, so I'm trying to find a way which will not end in code duplication:

• this
• #2497389: Support self-service TFA reset when locked out
• #2327973: Create a flood limit for a user which blocks that account from logging in with a new browser

This and #2497389: Support self-service TFA reset when locked out will definitely have some overlap (I was trying to make the bypass links universal).
tfa_basic_onetime_token, tfa_basic_onetime_link, tfa_basic_bypass_access are the functions that I imagined to be common.

About this patch:

It creates a new menu callback and uses that to log the user in using tfa_login().

The tokens in the links don't require storage as they're generated similarly to user_pass_rehash() (has the account's last login time in it).

I'm also creating a login plugin, but it doesn't behave like one in code, but does in UX:
The plugin's only purpose is to make this bypass route optional. The menu callback's access controller checks whether the plugin is loaded and only proceeds if it is and the token is valid. This could also be done with a form element and a new variable instead of a plugin, but I could not find a position on the form where it "looked right". I'm not perfectly happy with this solution, so ideas are welcome as always.

The Drush function mimics Drush 7's uli, but without its dependencies (UserList and UserListException), so it should work with Drush 6 (haven't tried it yet). The one in the first patch didn't work with Drush 7 because of the _drush_user_get_users_from_options_and_arguments function that only exists in Drush 6.

Implemented expiration of one-time links, and error messages.

This time without the dummy login plugin. I feel better about this approach.

Can you write a test for this please?

1. +++ b/tfa_basic.module
   @@ -135,6 +143,99 @@ function tfa_basic_setup_access($account, $permission = '') {
   +      return call_user_func($custom_access, $account);
   

   I don't understand the purpose of injecting a callable function, is it so the variable can be wrapped in a function for use in the access callback and in drush?

2. +++ b/tfa_basic.module
   @@ -135,6 +143,99 @@ function tfa_basic_setup_access($account, $permission = '') {
   +  tfa_login($account);
   

   Please add a watchdog here to say "User %name used one-time login link at time %timestamp" of watchdog type 'tfa', similar to what core does https://api.drupal.org/api/drupal/modules!user!user.pages.inc/function/u...

3. +++ b/tfa_basic.module
   @@ -660,6 +761,13 @@ function tfa_basic_form_tfa_admin_settings_alter(&$form, &$form_state, $form_id)
   +    '#description' => t('Required for logging in with links generated by the "drush tbuli" command.'),
   

   How about "Allow the generation of links for one-time-use login without going through two factor authentication. Create with 'drush tbuli'."

Hi @coltrane, thank you for the review!

Tests are coming soon.

I don't understand the purpose of injecting a callable function, is it so the variable can be wrapped in a function for use in the access callback and in drush?

That, and because I didn't want to copy paste 90% the access callback for #2497389: Support self-service TFA reset when locked out. This way it's reusable for every kind of one-time link.

Please add a watchdog here to say "User %name used one-time login link at time %timestamp" of watchdog type 'tfa', similar to what core does

Will do!

How about "Allow the generation of links for one-time-use login without going through two factor authentication. Create with 'drush tbuli'."

Sounds a lot better.

Patch will be coming in the next few days. Or even today. We'll find out.

Thanx @banviktor for your work on this. You are obviously writing a bypass solution with an own menu path. I have an idea for an additional security feature in this solution. But before I explain too much I will "say it" with a patch maybe tomorrow.

Hi @C_Logemann,

Cool! Can't wait to see what you have in mind :)

For the record I chose this path (bypass solution with own menu path) because my attempts at continuing patch #1 resulted in working code, but UX-wise (the links looked ridiculous) and DX-wise (hard to follow what redirects where in code) they didn't feel right.
There is a downside with this though: no flood control inherited from TFA. I could add a separate flood control or one that uses TFA's flood events. The former means code duplication, the latter means depending on code that we should not depend on.

@banviktor:
I applied your #7 patch but didn't get tbuli working currently.

I had a busy day and I am too tired for coding. But I just want to explain my idea:
I see there is already a timebased check inside your code. My idea is also about time:
I think we need tbuli only in special service situations. So we can keep the menu path closed until we need it.
What about a variable like "tfa_basic_bypass_link_expiration_time" (or something shorter) which is set to "Current time + maybe 30 seconds" when drush is requesting token and generating the tbuli link?
In the access callback function ("tfa_basic_bypass_access") the first if check should be a time check based on this variable. If current timestamp is too high it should return FALSE.
So there is no other code executed with user loading an token generation an so on. Maybe we can skip using "%user" in menu-path to avoid the user-load until it's needed. And I think we don't need variable "tfa_basic_bypass_login_access" anymore with this timebased low level access "gate". What do you think about?

Oh that's a pretty cool idea! So anything older than one day and anything newer than tfa_basic_bypass_link_expiration_time gets rejected.
I agree that the tfa_basic_login_links variable (I think you were referring to this instead of tfa_basic_bypass_login_access) makes much less sense with this mechanism. Anyone sees a security issue in removing it? If someone has Drush access that variable wouldn't stop them anyway.
And we won't confuse site administrators with an added UI element.

Thank you @C_Logemann! I will implement this today or tomorrow :)

By the way which Drush version are you using? The patch should work with Drush 7 but haven't tested it with other versions yet.

Sorry for being late with this, I was pretty busy.
So the time limited mechanism is implemented, tests and watchdog message added.

I still couldn't test it with older versions of Drush.

Renamed the menu callback's title from TFA bypass link to TFA login link.

This could be in TFA.

Ported from TFA Basic to TFA.
Drush command still needs testing on older Drush versions.

I think this solves #2535714: drush uli link fails without UI feedback in a way.

Usually I use "drush uli" without any args. So I didn't recognized that there was always a "0" for uid in the onetime link.
I figured out that the current #18 patch tries to load a user based on name and email. Both functions returns user/0 (guest) if $user is empty.
In function drush_tfa_user_login (file tfa.drush.inc) I improved the related if-conditions to check on empty $user-string:

 elseif (($user != "") && ($account = user_load_by_name($user))) {
 }
 elseif (($user != "") && ($account = user_load_by_mail($user))) {
 }

Also I cleaned up elseif-conditions with newlines.

You are right @C_Logemann! Thanks
But you forgot to include tfa.drush.inc in the patch :D

But you forgot to include tfa.drush.inc in the patch

Yes I forgot that new files needs more attention.

Edit: And now I forgot to add my changes because they were not part of the patch and I already made a reset of my git repository.

@C_Logemann
Something's wrong. This is the same as my patch in #18.
In #12 you mentioned you couldn't get the drush command to work properly. Does it work for you now?
Edit: I see :)

And now with my changes in file tfa.drush.inc.

Before I try to figure out why I can't create a proper patch currently I answer the important questions.
I didn't remember but I believe my problems reported in #12 were the same: Get onetime login links for user/0 and didn't try with argument.
But with argument "drush tuli 1" with patch #18 and "drush tuli" with additional changes of #20 I get working onetime login links with actual drush version.

I believe we don't need to test on many drush versions. If it's working with latest stable and current dev I think everything is fine.
In my opinion drush is still for experienced admins which are able to update their drush or work with more than one drush version.

Ah okay I get it now. So it wasn't completely dead because of some drush incompatibility. That's what I thought at first and why I wanted it to be tested.

By the way your patch does not apply because it thinks there is an empty tfa.drush.inc already.

Left is #18 right is #23.

I tried the strategy of this suggestion "Simple way to add a file for a patch" but the patch won't apply in my local test, too. In future I will check this first. But maybe we get some pull request workflow on d.o soon and don't need to operate with patches any more.

In my GUI program "SourceTree" I have a possibility to build my patch also with new files but this is not the best suggestion for every user. So I tried another way I found: "Add new file to staging and make a git diff against a special branch:
git diff 7.x-2.x > Allow_Drush_uli_to_bypass_TFA-2481253-28.patch

This one I can apply locally.

Seems to be working now :) Thanks

To be sure I tested the "drush tuli" with drush 7.1.0 and 8.0.1 and it works fine. RTBC +1

Have to add a flood event. I will do it shortly.

Flood event added with a test, also edited a few messages.
I don't know whether we want to invoke hook_tfa_flood_hit in this case. That expects a $context parameter.

Thanks a lot @banviktor for your work.
In near future I need this functionality and so I did a test with #33 patch on actual versions of tfa and tfa_basic with a drush 8.0.5 and it's working well. After applying the patch the menu catche needs to be cleared because of the new "tuli"-path.
Everything is working fine. Any chances to get this committed ASAP?

This issue is tagged as RTBC since 8 months. In case of a security workflow as described in comment #3 this feature is very important. I hope we get this committed soon in D7. And sind D8 development is in progress I think we need uprade of this patch. But before I invest my time I like to get a feedback of the maintainers.

I've done a code review of the patch in #33 and it looks sound to me.

I'm interested in getting this into the 7.x branch so we can evaluate an 8.x port.

@maintainers: Any chances for a commit this year?

Please commit.

It would be great if this would be committed, in order to be able to commit #2497389: Support self-service TFA reset when locked out as well and have a good user self service solution.

I have tested this against the rc1. The patch works and the drush command works. So I think it can be included into the rc1.

If you add a user name which does not exist, the URL returned is that of the admin. I would suggest returning an error, to avoid that.

As per #43

@jcnventura You are right. I try to find some time to fix #43 but won't assign myself until I can start. Maybe someone else will find some time before I can do.

Thanks @C_Logemann. Be warned however, that I only maintain the Drupal 8/9 version of the module.. I don't use Drupal 7 anymore, so I'm not touching the D7 version unless it's a security release.

We don't have D7 project with this module anymore so I also have also no interest in a D7 version anymore. Meanwhile bypassing is possible for user-1 via config in D10 version. This was one of main reasons for me to work on this issue. Another reason is testing with different users. And before someone argues about test accounts to manage. We have a customer project with this module where the content is very focused on specific users. There it's very helpful to login as a specific user on a test system with bypassing the TFA to work on customer issues. So it would be still nice to have a "bypass flag" for Drush. When I find some time I will try to uprade the concept above to D10.

Considering #3381701: Provide drush command to reset a user's TFA data is going to provide Drush command to reset TFA data which lets admin to access the user account. Should we close this as won't fix as this issue has security implications.

Admin can bypass user password requirement and login to the site without user knowing someone accessed their account. The whole point of TFA is to prevent someone accessing user account without second factor authentication.

In case someone needs this feature we can add it as sub module.

In local environments tfa module could be disabled temporally:

drush pmu tfa -y
drush uli
# Use the link.
drush cim -y

Deactivating the module complete is not helpful in our testing situations. But because this is possible it's an argument against #49.
But anyway I like to have this feature and see no problem to realize as submodule. But so it's still an issue of the main module and should stay open.

In case someone needs this feature we can add it as sub module.

If this can be done as a login plugin or sub module than it may not actually need to go into TFA and can instead be an Ecosystem module allowing sites the option to download the code or not. This adds a layer of security in that a admin user may have ability to enable modules without having filesystem permissions to write code to the site.

cumbersome and unnecessary for those users that already have drush access to the site, providing them with God rights on the site:

I want to note that we should be very careful assuming that if Drush installed that everyone has 'god' access. Drush may be behind other security measure such as SELinux preventing file writes, settings.php hard coding config (such as TFA enabled) with no write access to override. Drush itself may be behind a sudo config that only allows certain commands, etc.

But because this is possible it's an argument against #49.

I'm not sure I agree with this. There is a significant difference between TFA being bypassed because its been removed/disabled, and TFA being bypassed because we built a feature intended to allow bypassing it.

TFA should be thought of as 'independent' of the site. If we were an external SSO system for example the idea of disabling TFA for a single login wouldn't even be considered an option because it couldn't be done.

There will always be some actions that we can't prevent occurring, however we don't have to support them either.

Deactivating the module complete is not helpful in our testing situations.

#3318456: Allow TFA requirement to be configured per user is closer to handling the testing subject imho. Though even that only holds validity when we use an argument that a counter based token isn't used on the site as any counter token would allow for repeated logins in short succession.

where the TFA screen on an app like Google Authenticator becomes unmanageable with a PIN for each site.

I do want to point out that I believe OTP apps have become much better over the years since this issue was opened with some having very robust organization structures.

It seems that this patch does not work with the latest TFA version.