provide an option to skip sql sanitization for tfa

Thanks for the proposal and patch.

Can you share some of the motivation here? Why would a site want this feature?

Hello Greg,

Please find the below motivation for the patch.

I usually do production database sanitization before importing it to lower environments (like development and test).

I have TFA setup on a production website and whenever I sanitize the production database, TFA data is getting sanitized as it's not having the option to skip the sanitization. Due to this in lower environments, while testing users need to set up TFA again and they can't use the same TFA which they have set up on production.

That is an interesting problem. I wonder what other solutions we could provide. Maybe a drush command could overwrite all the fallback code values with a known set of values that developers could use?

When I used tfa, I've previously just disabled it in development environments in addition to sanitizing the database.

Let's keep thinking of solutions.

Hello Greg,

Thanks for quickly having look.
Yes disabling can be an option but I do still need users with admin roles to have TFA on non-prod env for safety and with the current TFA module there is no option to skip the TFA sanitization

This patch just gives an option for users if they want to skip sanitization.

The idea of a set of sanitizations to skip seems like an interesting option to have in drush itself. Have you looked about adding that feature there?

Hello Greg,

Apologies for coming late on this thread. May I please know what do you mean by adding that option in drush itself?

with this patch, I'm adding one parameter, and its used in the sql-sanitize command.

./vendor/bin/drush sql:sanitize --sanitize-tfa=no

This will skip the sanitization.

An alternative could be that the drush sql:sanitize could have an argument like --sanitization-to-skip=tfa and it would take a comma separated list of hook names to do the same thing at a more global level. That seems more appropriate to me.

And to me. If done right, there would be nothing to do from the tfa side, as this would all be handled by drush. Maybe we should create an issue in https://github.com/drush-ops/drush/issues ?

--sanitization-to-skip=tfa and it would take a comma separated list of hook names to do the same thing at a more global level

A downside of such a method is it implies that a sanitation could be skipped, however there may be some sanitation that may be so critical they should never be allowed to be skipped.

Drush current method of allowing each plugin/hook to supply its own options allows a module to say "ok you can skip that, no you may not skip that"

Theoretically Drush could be modified to accept a hook name and a task name to ignore, however at that point you still need to make changes to modules like TFA to say "These are each of my tasks, and these tasks may be skipped" in which case why not just have TFA provide an option directly and act upon it as Drush currently does?

The above becomes more true when you consider that the current method allows more fine grained control, such as 'skip sanitizing admin users' instead of skipping all sanitation.

That last point point actually brings up a question for this, instead of making this an 'all or nothing' option for TFA perhaps this should be modified to be a more focused exemption and only skip sanitation for a smaller subset of accounts that way the plain text seeds for the majority of accounts are not at risk in a development environment.

Adding on to my comment in #12:

Since the included plugins encrypt the seed (and all other plugins should be protecting the secret data as well) skipping sanitation to allow Dev environments to log in would either require the Production Secret being shared with the Dev environment or a key rotation.

It is best practice to not share encryption keys between Production and Dev so that leaves key rotation.

Rotation would in generally require a custom process to rotate the encryption key, at which point one could just inject arbitrary tokens post sanitation that if necessary could be standardized as part of the Dev environment setup process.

Perhaps this really should be won't-fix because of cryptographic best practices?

Setting back to active until we decide if we want to re-focus this issue to add a Drush command to allow setting specific keys as suggested in #6 or if we want to just won't-fix this on the reasons given in #13.

Re-roll patch for 8.x-1.x

Having the same use case as @shivamitakari. Our clients want to keep their TFA settings when we copy the database from production to the other environments.
A new patch was created for version 2.x.

Back to Active per #14

Re-roll patch #16 for 8.x-1.8

Rerolled the patch #20 so that if there is a database prefix configured then it is used.

As this issue keeps attracting re-rolls of a patch that should not be committed and no feedback has been provided on alternative options to redirect the issue in a way that aligns with secure software design I’m going to close this as won’t-fix.

If anyone has a suggestion that can be reconciled with #6 or #13 a new targeted issue can be opened.

You should not close this without providing a solution.
We still need to skip the sanitation of the TFA for most of our clients.
No admins want to set a TFA per environment and re-set it every time we pull the production database down to the other environments.
Having this in drush doesn't make sense because not all the projects use the TFA module.

You should not close this without providing a solution.

The won't fix is that the proposal is non-viable and will not be committed. Any other suggestion as an alternative would involve a significantly different solution, the existing discussions would be of little direct value.

Alternatives have also been provided such as not sanitizing or disabling TFA.

A drush command that allows provisioning tokens has been suggested in other issues, I would support such a feature and it would allow for automated setting tokens for developers after sanitation. A similar solution was suggested in #6 for adding a command that provisions tokens however it too has not received traction due to the non-viable patches being re-rolled.

No admins want to set a TFA per environment and re-set it every time we pull the production database down to the other environments.

It was requested in #13 how the developer avoid the compliance breach of copying a private encryption key between environments. Adding on to that, how would the developers deal with the replay attack risk of a key being submitted in dev and yet still being usable on the production environment?

Judging by the comments it sounds like your development firm may not be dealing with these other security concerns. If the dataset is desired to be sanitized that should also includes encryption keys(which when sanitized will render the TFA user data invalid), if the dataset does not desire to be sanitized there would be no need to call the drush sanitize command.

Users may not understand these risks, this is why they contract development firms and why we TFA as a security solution should not add features that do not have a reasonable use case.

Having this in drush doesn't make sense

Agree, see comment #12.