Problem/Motivation
Currently our composer.json calls out PHP7.0 as supported.
I tend to align that in semver a projecting dropping PHP support should only be done in a major release. Even though 8.x-1.x is not technically semver I prefer to align it to the extent possible.
PHP8.4 for implicit nullable can only be fixed in PHP7.1 and above. These are not critical for PHP8.4 as they will not be required until PHP9 however they do create warnings when error_reporting is enabled. These warnings currently break PHPUnit tests although #3496517: Improve phpunit default configuration and make it customisable may solve this, or we can add our own phpunit.xml.dist to fix.
Currently some commits have added changes that exist in PHP7.1 or newer breaking PHP7.0 in the code base.
Steps to reproduce
See attached
Proposed resolution
TBD
Remaining tasks
Decide on minimum supported PHP version
Implement fixes to restore compatibility if necessary or to adjust composer.json.
Implement Minimum PHP Verison Linting to enforce support for that version of PHP to prevent future accidental breaches.
User interface changes
None
API changes
TBD
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| tfa-php7.0-phplint.txt | 3.61 KB | cmlara |
Comments
Comment #2
cmlaraResponding to comment #3496146-13: Implicitly marking parameter as nullable is deprecated in PHP8.4:
Zend is still providing Generally Available LTS support until the end of 2026 for I believe as far back as 7.2. This doesn't consider possibilities that private contracts may exist for even older versions. This doesn't even consider any other vendor that may have taken on the challenge.
Personally I follow a strict interpretation of SEMVER, you don't drop operating environments without a new major. Do I recommend this be used on a PHP 7.0 install, hell no, however as seen above other vendors do run support for PHP longer than its EOL. This is part of the reason why I as a developer prefer a strict interpretation, we just don't know how our project is being used in other environments, even the most insane of deployments may be still in operation and be secure.
Do I believe that a sizeable number (or even any) sites are runnign TFA on PHP7.0, no, however In my mind I don't see that as a reason to do a 'breaking' change outside of a major.
We can certainly argue that the 8.x-1.x branch isn't bound to SEMVER given its version scheme, though I generally prefer to avoid that as I see it being a bad habit to get into. There is also room to argue that Composers 'operational controls' also come into play in blocking upgrades (avoiding creating a a WSOD) making it non-disruptive, though again I also tend to avoid that as I view 'supported environments' as the standard to key off of.
These warnings become a problem if the module ever needs to support PHP9, however I don't see that as a likely occurrence for the 8.x-1.x branch. The 8.x-1.x branch already has known gross deficiencies, and with no PHP9 release on the roadmap the odds are 2.x will be out well before PHP9, if for some reason it is not, 2.x could be pushed into service well in advance to provide PHP9 support.
That said, this isn't entirely up to me, as 'just a co-maintainer' any of the other developers could override me at any time which is why the this issue was opened to allow them to provide their input.
Comment #3
acbramley commentedI understand where you're coming from, but even Drupal 10 requires at least PHP 8.1. Postponing 8.4 fixes on the off chance someone is using an ancient version of PHP feels backwards to me, especially since we're soft-blocked on a new major given the architectural changes in 2.x that is not production ready.
Comment #4
cmlaraGoing even further, D9.0.0 required PHP 7.3.
I could say there really are no D8.9 installs out there however https://www.drupal.org/project/usage/drupal would show there are some (and is known to under report), very very few however still some. I won't claim any of those are running TFA (and if they were the odds they are updating is even lower). (Side note: last year I upgraded a site that was running Drupal 8.0.0, interestingly, they were running the latest version of the contrib modules they could even ones released several core minors later.
On top of that, I don't see us as developing for a release of core, I see us as developing for an API spec originally defined by Core. API specs never really go end of life they just eventually get replaced. It just so happens that majority of the installs will be Drupal Core releases matching those versions (or at least very minimally patched forks).
I would contend these are not fixes, they are feature enhancements, and this create a subtle yet important contextual difference to the discussions.
I can respect that feeling, I find it annoying at times too, it would be very easy for me as a developer to say "lets just drop that so we can adopt this new shiny feature" however I came up the ranks on the system admin and network engineering sides, I've dealt with crazy deployments in the past where very old software is used in production. SemVer wasn't utilized during those times, however it would of helped.
SemVer is fundamentally about requiring developers to think and plan in advance while retaining all of their past,to move away from the 'wild west' without regard to the history (no matter how old that history is) and make a conscious, visible (and in some circles painful) choice to drop that history.
As frustrating on the developer side as it is to adhere to the the strict principals I see it as adhering to the fundamental promises from developers to the engineers who have to implement our code to make life more predictable.
This isn't wrong, and I'm responsible for that as I haven't set a hard cutoff on the specs for 2.x. It also doesn't help that I took a detour from the major issues recently to look at smaller easier 'wins' to clear my head after a year and a half of security only design changes.
Comment #5
acbramley commentedWhat's your timeline on getting 2.x stable? Why don't we do a 3.x branch that is 8.x-1.x that drops unsupported versions of core/php. Once 2.x is stable that could either continue on a 2.x line or become 4.x.
Comment #6
cmlaraResponding after ping in #3496146: Implicitly marking parameter as nullable is deprecated in PHP8.4 (Thank you for reminding me I had not come back to give you an update).
Unfortunately comment #5 came in around the same time the Drupal Association was sending notice that they were going to terminate further access to D.O. which would prevent me from continuing the 2.x branch. The DA has not yet given a final response to their plans which leaves my ability to contribute on D.O in significant limbo and has significantly reduced the amount of effort I am willing to donate to the Drupal Association.
At this point I am doubtful I will regain the ability to trust the DA going forward. At the moment the most likely outcome for me is that I continue development on another code hosting platform (though I have considered developing solely for internal usage), however that requires me to have the time to dedicate towards migrating to a new public platform. I don't anticipate having this ability until Christmas week.
The other maintainers (who have been fairly inactive for 3 years and minimally responsive on security issues) may continue development here, I don't know. I will however caution site owners that SA-CONTRIB-2023-030 was discovered and publicly disclosed prior to 8.x-1.0 and the the maintainers chose to release 8.x-1.0 anyways, I recommend caution trusting their security related decisions if they continue development.
We potentially could have had a solid 2.0 out the door by now if it were not for this incident.
Re converting 8.x-1.x to 3.x and 2.x to 4.x to allow API changes:
8.x-1.x does not provide assurance that a user has authenticated through a multi-factor login process.
The 8.x-1.x branch is impossible to fully secure without significant architecture changes that are the primary difference between 8.x-1x and 2.x.
I wouldn't suggest any work to try and extend the 8..x-1.x branch lifetime is a poor use of resources.
Comment #7
gregglesFor what it's worth - it seems quite reasonable to me to increase the supported version of php to 7.1 in the 8.x-1.x branch.
If that's not going to happen based on semver and BC principles, then I support the idea of releasing 8.x-1.x as 3.x and 2.x being released as 4.x.
Comment #8
acbramley commentedIMO, we're creating maintenance overhead to support a version of PHP that is well past EOL, and that is not supported by any supported version of Drupal core and therefore we should just update or even drop the PHP requirement from our own composer.json (I don't know many modules that have this in the first place?)
Comment #9
cmlarahttps://search.tresbien.tech/search?q=%5C%22php%5C%22%3A%20f%3A%5Ecompos...
It is the composer.json equvilient of the *.info.yml PHP line.
I will add on, even if a module supports the same PHP versions as Drupal Core, it still has its uses. Two that I'm aware of off hand are:
Its certainly been helpful when I've picked up a random Drupal module and my IDE self re-configures.
Based on experience, I will admit the majority Drupal.org developer may not use it, especially since the majority of Drupal developers may not even maintain a composer.json at all.
Comment #10
acbramley commentedOk let's say we do keep the composer.json entry, can we agree on a PHP version to update to? 8.1 is the minimum required for Drupal 10.6
Comment #11
cmlaraI still stand with the same opinion I had over a year ago.
8.x-1.x would remain as is, there should be no split of 8.x-1.x to try and continue it on. All dev effort would be focused on 2.x. Just reading or responding to messages in this thread is a waste of time a full maintainer or co-maintainer could put in to working on a release series of the project that has a chance at not being security theater.
Ultimately not my choice as just a co-maintainer. All i can do is comment and hope the leaders listen.
Comment #12
acbramley commentedThe time it will take to get 2.x into a production ready state is vastly different to committing small and easy fixes like the PHP 8.4 compatibility. I haven't had a chance yet to review what it will take to get 2.x into a ready state, so I don't think we can really just ignore 8.x-1.x entirely...
Comment #13
cmlaraThe module is already PHP 8.4 compatible.
I believe you mean to say "easy fixes like PHP 8.4 depredations" which are a warning that the code will not be compatible with PHP 9.
While the two are often confused by site owners there is a distinct and significant difference between the two. One prevents the module from running today, one may never be relevant if the branch doesn't live to PHP 9.0's release date.
Its best I not repeat myself more than I already have. Its clear we have differing opinions the value of the 8.x-1.x branch. In order to not be the maintainer who is always disagreeing with the majority I've already submitted a request for removed from the list of active maintainers, that should allow you to push forward with less/no objections internally.
When 8.x-1.1 came out I thought that was the solution, I thought we were safe, I thought we closed the bypasses and anything else would be pure TFA internal mistakes. Being new to TFA's code and not intimately knowing the Drupal Core Authentication and Authorization stack I vastly underestimated the flaws in the 8.x-1.x architecture. Perhaps in time you will share the opinion that 8.x-1.x isn't production ready.
Comment #14
acbramley commentedWell that is not going to help things, it sounds like you are the one with the most knowledge of 2.x. It's ok to disagree, I'm just asking simple questions at the moment to try and see what small steps we can take.
Comment #15
cmlaraMy issue really isn't with the questions, one needs to learn somewhere. I call out my reasons in #3620113: Remove @cmlara from co-maintainer. The D.A. being a very large part of it, and a new hand who who shares more the views of the current maintainers than myself being the final "its time for me to step aside and not be the one who is objecting to every change"
The posts by @greggles in comment #7 and your own push for 8.x-1.x tell me that there is a strong belief that 8.x-1.x is more useful than I see it being. I've tried to dissuade that numerous times, I can only repeat myself so many times and yet each time I'm still pushed with
I'm not sure if I said it before, I had one of these bypasses in my lab for weeks, I didn't notice it. IIRC I stumbled upon it when I was auditing code for some unrelated reason and it struck me that TFA was missing, no intent, no desire, just an accidental realization that if it had been a production server I would of been in significant trouble. The flaw of user_login_finalize() or any system that doesn't use the TFA login form bypassing TFA's protections is well discussed, it can happen again and we from time to time see reports of it occurring. It isn't the reports I see that scare me, its the ones that never get made because the site owner has no clue their TFA is being bypassed that keep me up at night.
~12.5k sites running 8.x-1.x, every day we leave it up is lie. Were pretending its safe, that its stable, when we know it has confirmed publicly seen flaws that lead to silent bypass. You can't get much more egregious than that. And the worse part is my name is on those releases, I hold the ethical responsibility for allowing them out the door.
I recall the Drupal Association going to Europe to argue against possible laws that would make it illegal to ship software with known vulnerabilities. Flaws like TFA's design in 8.x-1.x are exactly where such legislation is useful, to stop us from shipping code we know has vulnerabilities. (No clue how that law turned out, I actually need to go check.)
Comment #16
greggles@acbramley I think the question which version of PHP to support (and how) is entirely reasonable for you to make a decision on. You can ask for feedback here or in #contrib-tfa and you can also make a commit and see how folks respond if/when they update their dev releases.
Comment #17
acbramley commentedI will open a new issue to track this.