- Issue created by @cmlara
- πΊπΈUnited States cmlara
Removing β¨ Increase security with single failure for login, password, and tfa Postponed: needs info from the should have list as it appears it would introduce security concerns.
- πΉπ·Turkey orkut murat yΔ±lmaz Istanbul
Hello and thanks for this beautiful module:)
Which items in the "must have" and "should have" lists are considered as beta version blockers?
Best,
Orkut - πΊπΈUnited States cmlara
With π Mark classes as internal/final/api as applicable Needs work we formally moved a large amount of the code to not be API, which could allow us to make sweeping changes across the code base post beta release, limited primarily by our plugin related code which is mostly public API.
That leaves with needing to close any public security concerns and anything that might modify the remaining portion of public API as blockers:
As it stands at the moment I believe we have:
Security Followups:
π SA-CONTRIB-2023-030 and 2.x Active
π Public Followup for 8.x-1.3 security release. PostponedPossible API changes:
π Gracefully handle EncryptionServiceInterface Exceptions Active
π¬ Cleanup accepted codes stored in user data Postponed
β¨ Allow TFA authentication through REST routes Active
π Deprecate the services_tfa module Activeπ Mark removed code as deprecated Postponed may also be necessary to aid developers in upgrading to 2.x.
- π΅πΉPortugal jcnventura
@cmlara, just a note on the above mention of making the plugins be their own module again.. If I understand your objection well, the problem is that it is not really possible to completely remove them, and could interfere with a third-party plugin.
I would rather move to have config entities, so that you could have multiple (or zero) instances of the plugins with user-defined identifiers. Splitting the plugins into their own modules only adds complexity to the release process as ga_login and tfa were installed together in 99% of the cases. And a few users that only installed tfa were confused why nothing seemed to work.
- πΊπΈUnited States cmlara
I would rather move to have config entities, so that you could have multiple (or zero) instances of the plugins with user-defined identifiers.
That sounds like an amazing idea. I can also see that working with the fact that I could see us wanting/needing to add some lifecycle management in the future, and having configuration be on entities would likely help manage that.
Created followup π Convert plugin configuration to Config Entities Active and added as a should-have (we could release 2.x without it using our existing method, however this will likely require a major bump so there is an advantage to making sure it is in 2.x.)
While this doesn't appear to solve my concerns regarding 'dead code' it does appear as a viable fix to some of my UI concerns for UX.
the problem is that it is not really possible to completely remove them, and could interfere with a third-party plugin
I'm not really sure I see how this is possible? The plugins themselves are relativity independent code (and are now marked final so not being extended)
The only code that I think might be relevant to 3rd party plugins would be the TfaBasePlugin, which (and realizing I haven't written this down in the original linked issue) we could consider TFA API and keep in TFA without the impact of extra libraries and unnecessary plugins being loaded.
I don't believe there is any way TFAHotp and TFATotp could break other modules, however if there is I will preemptively mention 2.x already requires a refactor, now would be the perfect time before we lock in the API with a beta release signaling to other maintainers they should start updating their modules.
splitting the plugins into their own modules only adds complexity to the release process as ga_login and tfa were installed together in 99% of the cases. And a few users that only installed tfa were confused why nothing seemed to work.
I can imagine a large majority of sites were using them, and probably will be for the long term. I will addtionaly admit it requires a few more dev cycles to maintain a second release package.
I'm hoping to encourage us to use methods that allow flexibility and keep the security scope of the core TFA module as minimal as possible.
Ideally we never have any vulnerabilities, however when we do it is nice if we can lock them into their own small areas where only those who are impacted need to update and each modules advisory can be more narrowly focused (an issue I faced writing the recent notes regarding 8.x-1.3).
Pulling out the plugins inherently makes TFA 'less opinionated' in its design which is a bonus for all sites. I believe it would also encourage thinking in the abstract, rather than instinctively assuming the plugins included are always how the API will be used, increasing the overall flexibility and security of the API.
Even with removing the plugin from TFA itself, there are a few ways we could still leverage composer to download the plugin for the average deployment while still providing a method for more advanced site owners to prevent the plugin from being downloaded.
- π¦πΊAustralia acbramley
Is π Resolve SA-CONTRIB-2024-003 in 2.x branch Postponed also a must have?
- πΊπΈUnited States cmlara
Updated with some entries that should have been previously added.
Yes π Resolve SA-CONTRIB-2024-003 in 2.x branch Postponed is a must-have.
- π΅πΉPortugal jcnventura
I think ideally, the plan should be to release a 2.0.0-alpha3 that fixes all the postponed security issues, so that the 2.x branch is at least as secure as the 1.x branch.
The only module that I know has a public release and is based on TFA is https://www.drupal.org/project/tfa_email_otp β , and that one is still using 1.x, so I think we should not rush to beta as long as all the work on re-factoring the plugins to be configuration entities and using PHP attributes are still pending. Ideally, we should split the "Should have" list into "beta release-blocker" and "stable release-blocker". And move anything that changes the API into the "beta release-blocker" section.