SA-CONTRIB-2023-030 and 2.x

My opinion:

I don't believe we should run under a belief that we can convince core to be more flexible toward MFA solutions, if we can great, we can always utilize those new features when they become available. At the moment I'm not aware of any issues that directly relate to introducing generic MFA support into Core's API's.

If we run under the assumption we have to take responsibility it raises the first question, is there another way we could do the fixes that doesn't involve coupling as tightly to core? Inbound Path Processors? Kernel Event responders? The options are open, we don't have to keep the fixes we used in 8.x-1.x, we have more time to discuss as long as we don't publish new releases, though it would be nice to get this out sooner than later, we could always iterate on it with a 'better' method in a future release.

If we can't find alternative methods that can be decoupled from core internal API the next question is how much should we extend @internal classes, and how much should we just duplicate into our code base?

Strictly speaking at the moment since were extending internal classes on 8.x-1.x we actually should be marking our core compatibility as anything newer than the current patch release of core, though that would make it harder for sites to deploy security upgrades and would increase our workload.

Personally I'm not against duplicating even large portions of Core into our codebase, we can fairly easily keep track of that code and possibly we could even start using GitLab automation to help us generate MR's of core changes to reduce that workload and increase our adoption rate. By duplicating we are not tied as much to the core implementations, we can have cleaner code by directly changing what we need where we need it instead of trying to work around the core provided code.

This was indeed messy in https://git.drupalcode.org/project/tfa/-/commit/8a29dbbb8671c3ea8055f3ae..., we have selected classes based on version of Drupal core, we have to handle mixed API for different versions of Core. if we were to own all the code ourselves for the controllers it could be much cleaner in that regard at the expense of more code in our module.

I'll admit its not entirely risk free to duplicate core's controller code into our module, if core makes changes to some of the services that are called we have to code solutions to handle that, however the odds are much more likely that will occur only in a minor release, with plenty of lead time to test. Addtionaly those changes are more likely to be to code that is also on API so that it will have formal BC layers, though there are times core makes significant changes w/o a reasonable BC layer.

This also somewhat works with #3374260: Allow TFA authentication through REST routes which may provide some new alternatives for us to consider as well for the REST side of the equation.

As noted the 8.x-1.x D10 support is being discussed in #3370841: Add D10 support to 8.x-1.x, so far I haven't seen anything that would prevent the 8.x-1.x branch from supporting D8,D9 and D10 at once.

While there is a push for a drop of D8 support I will note that personally I adhere to a stricter interpretation of semver than many others here, where dropping a 'supported operating environment' (version of Drupal Core) is a major only release change, this is to promote stability, interoperability, and to encourage planning. I go into a bit more depth in #3357742-9: Guidelines for semantic versioning for contributed projects in relation to Drupal core compatibility. Technically under the strictest of semver rules yes you could drop a supported environment if you included it at the time you made you X.0.0 release which I haven't seen anywhere in the documentation of TFA. A lot of modules will drop support at any time, I've seen even in-support versions of Drupal Core be dropped so that a module could use the 'latest changes', I prefer that a more stable platform is promoted, even if at the same time I really hope no one is using TFA on an 8.9 Drupal Core install.

I have a request for the maintainers: please open a Plan issue outlining the path to 2.0.0... Once this issue is resolved, can we expect a full release soon? Or are there other issues that have to be fixed first?

That is a fair request. I can create one that has what I know at the moment, however for a while it will be a work work in progress as I try and determine just what was the goal for 2.x were along with what additional changes may need to be made or should be made to enhance TFA on both security and functionality.

What I can say is I personally don't expect a full 2.x release prior to D9 going EOL, and even if we were to release a stable 2.x today 4 months feels to be a pretty short timeline to expect the ecosystem to adopt a new API for plugins. I could be wrong, and there may be less issues than I suspect there are, and that more of the work of what was discussed in other issues for 2.x is already done, or a large community support more pour in once I have a solid roadmap published, so I won't say it can't happen sooner, I just personally don't believe it will.

For those who are not aware I was added to the dev team for TFA as a co-maintainer just over 2 weeks ago in #3370863: Adopt a 'security first' focus when trying to discuss having the module choose more 'security first' design before I would start raising issues on code I feel we we could move to be more security first focused to limit the risk of previous or future decisions turning into security vulnerabilities, and if they do, limit the scope of impacted sites.

As for this issue being tagged as the only release blocker, currently that is intended to indicate that we should not repeat the same faults of the 8.x-1.0 release publishing a stable (or really even a non-stable) release with a known security vulnerability present.

I will note I'm personally not against us pulling the 8.x-1.1 patch forward to 2.x and publishing another alpha with that code so that 2.x has the fix sooner. In my opinion we can always iterate upon it for a longer term fix, it was just the request of a 'higher ranking' maintainer during the security issue review that they would prefer the 8.x-1.1 fix not make it into the 2.x repo as is. FWIW I agree the coupling to core is less than ideal and I would prefer a cleaner way myself. I however would prefer 'secure and a pain to maintain' over 'insecure and easy to maintain'.

As I'm not the 'project owner' I have to dance the fine line between pushing issues forward, and taking an action that could end with being removed by any of the other maintainers at any time.

Realizing I didn’t comment here, the WIP roadmap has been opened in #3374845: Roadmap for 2.0.0 release.

I've been looking at this issue from various viewpoints over the past several months. Most solutions I have tested either were too tightly coupled to core internal API, or required us to maintain duplicate copies of core code.

At the moment the following 3 issues combined appear to me to be the best combination for minimal coupling while still providing TFA protection where it would be expected.

#3392068: Move TfaLoginContextTrait back to a dedicated Class and add factory. Not mandatory however it allows for easier unit testing.

#3378072: Decorate the user.auth service would allow us to protect REST routes (that depend upon user/password authentication) and protect other unknown login paths that may be called in 3rd party modules.

#3392427: Use an EventSubscriber to process one time login links to protect One Time Login(password reset) links.

I'd rather deprecate it already and never add that setting in 2.x,

I’m in agreement. So far I have no plans in any of the child issues to bring the setting into 2.x.

The issue most likely to do so would be #3392427: Use an EventSubscriber to process one time login links. I’m noticing I haven’t pushed up my WIP branch on that issue however from memory I’m sure I did not import canResetPassSkip(). I’ll look at my WIP branch today and see if there is any work left that prevents upload or if I was just waiting on other issues to be merged in.

return (PHP_SAPI == 'cli') && class_exists('Drush\Drush');
While the link generation may run inside the CLI the actual link verification does not. I don’t believe this would actually ever allow access for the password reset page which is the only location we are currently using this method.

Personally I’m much more supportive of #3381701: Provide drush command to reset a user's TFA data and a related comment in #2931150-16: Confirmation forms should not require passwords where it was suggested a Drush command be added to allow admins to provision tokens. Combined these two would provide a method to reset/disable TFA for a user from the CLI.

as any attacker would at this point be in full control of the server,

While we often think that a user on the CLI has full control I will note it is entirely possible one might have access to some commands without having full access to others. I have seen my fair share of bad sudo configs that allow access to more than they intended yet did not allow full access.

#3410444: Installing contrib modules can lead to TFA accidently being bypassed was committed in February. The commit added an event listener that significantly locks down the ability to login unless the TFA module code has been invoked at some point.

#3392427: Use an EventSubscriber to process one time login links still needs to be committed to allow password resets to function, however unlike the flaw that opened this issue password resets currently fail secure denying access.

https://project.pages.drupalcode.org/tfa/technical/set-user-protection/ contains the technical details of the new protection.

While we still do not have password resets allowed I am going to close this issue out as the original vulnerability has been resolved.