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 ✨ Allow TFA authentication through REST routes Active which may provide some new alternatives for us to consider as well for the REST side of the equation.

As it stands, 2.x is the only avenue to Drupal 10. Not releasing 2.x than should probably also include a plan to make 1.x D10 compatible. I have no idea of the impact of that, but I think it should be factored in to the decision. If that's not really an option, I think there is not much to be done and push the mythical fancy core integration to a future major (3.x/4.x/...).

Another thought completely: if we do end up adopting the same solution as 1.x, I think it would be good to point out in comments the core versions that are involved with all the dependency juggling, so it is clear what to remove/change when a decision is made to drop support for certain core versions. Not sure if I'm just repeating a discussion that already happened in the security issue...

I think if the Drupal 10 support in ✨ Add D10 support to 8.x-1.x Fixed works out and is merged, that takes the pressure off of 2.x, so I would prefer that as the short term solution. 2.x can then be worked on as time permits, with releases hidden until it at least has the security fixes implemented from yesterday's security advisory → .

Aha, thanks. I was unaware of that issue.

Personally, I wouldn't have a problem with dropping Drupal 8 support altogether since it's been unsupported with no security updates for a long while, but I understand that it's technically a semantic versioning issue. The alternative is to re-number the branches:

1. 8.x-1.x remains as-is with the last release being 8.x-1.1 which technically supports Drupal.
2. The current 2.x is renumbered as 3.x; this is probably a hassle and annoying.
3. A new 2.x branch is created from the last 8.x-1.x, with Drupal core ^9 being required. Maybe ^9.5 if that's the currently remaining supported version.

Or just do something zany and create a 9.x-1.x branch from 8.x-1.x as a temporary workaround which I guess would also work but feels wrong. 😂

There's no need to create a new branch just to make the minimal changes necessary to support D10, it'd be better to just do a new point release.

@DamienMcKenna You're not wrong. I think the discussion was more about dropping Drupal 8 support in a minor release.

Again, that would be completely a-ok imho, most modules have done that.

I think the discussion was more about dropping Drupal 8 support in a minor release.

Not for me. D8 entered the discussion in the other issue, but I'm not sure why, exactly. I do seem to recall that one of the maintainers expressed a strong desire to retain D8 support, so that might be the reason. For some great background on versioning and core support, see https://mglaman.dev/blog/drupal-module-semantic-versioning-drupal-core-s... (which doesn't say anything to the contrary, does gives some nice background on why adding or dropping core support is usually no reason to change major version numbers).

Then there is the factor of semantic version just being really nice. I've personally done 2.0 releases for modules just to be able to use it (but making it clear in release notes that it is a drop-in replacement; taking the major version number up does mean a little bump in the road for many folks, as composer will avoid automatic major updates with its default version constraints).

So I'm OK with either making 1.x D10 compatible (whether at the expense of D8 support or not, I don't really care), or using 2.0 for that (actually, I have a slight preference for doing it in 2.0, but that is purely personal, since I - somewhat misguided, it seems now - assumed 2.0 was the way toward D10).

+1 for what @DamienMcKenna said. It is OK to release 8.x-1.2 with support for D9 and D10, dropping support for D8.

The only disadvantage is if you want a later release that also supports D8. But D8 is EOL, so I do not see that as a concern. Even D9 will not be supported much longer.

I have a request for the maintainers: please open a Plan issue outlining the path to 2.0.0. This issue is tagged as a Release blocker and a beta blocker, and is the only open issue with either of those tags. Once this issue is resolved, can we expect a full release soon? Or are there other issues that have to be fixed first?

As noted the 8.x-1.x D10 support is being discussed in ✨ Add D10 support to 8.x-1.x Fixed , 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 and Drupal core support → . 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 ✨ Adopt a 'security first' focus Fixed 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.

Thanks for the openness, sounds all perfectly reasonable. If it is possible to have the 1.x branch support D8, 9 and 10 at the same time, I think there is no issue, really, and that should be the push for now (i.e. get ✨ Add D10 support to 8.x-1.x Fixed in a release). That would go a long way to prevent people from stepping in the same trap I did, with concluding 2.x is the path to D10 compatiblity. +1 on the roadmap/plan issue.

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

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.

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

📌 Decorate the user.auth service Fixed 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.

📌 Use an EventSubscriber to process one time login links Needs work to protect One Time Login(password reset) links.

One of the new configurations added to 1.x in SA-CONTRIB-2023-030 was reset_pass_skip_enabled. I'd rather deprecate and delete that setting in 2.x, and have this version of the TfaLoginContextTrait::canResetPassSkip method:

  public function canResetPassSkip() {
    return (PHP_SAPI == 'cli') && class_exists('Drush\Drush');
  }

The objective of that reset_pass_skip_enabled configuration is to allow site admins to recover or to simply not have a password for UID 1, but still be able to use the site as super-admin (or any other admin). From the point of view of security, this is not introducing any vulnerability, as any attacker would at this point be in full control of the server, and can simply disable the TFA module or whatever else they want.

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 📌 Use an EventSubscriber to process one time login links Needs work . 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 ✨ Provide drush command to reset a user's TFA data Fixed 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.

🐛 Installing contrib modules can lead to TFA accidently being bypassed Fixed 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.

📌 Use an EventSubscriber to process one time login links Needs work 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.

Automatically closed - issue fixed for 2 weeks with no activity.