Problem/Motivation
Mixed mode SSL capability was introduced into Drupal Core in order to support the use-case implemented by Secure Pages module. However, since #2205295: Objectify session handler and #2228341: Objectify session management functions + remove session.inc it is now possible to completely swap out the session implementation from contrib.
The current implementation slows down the conversion of the Drupal session components to a Symfony based implementation. Also given that Mixed mode SSL as implemented by the Secure Pages module is only one of several possible solutions, keeping that implementation in core does not seem to be desirable.
Proposed resolution
Remove mixed mode SSL from core and move it to contrib.
Remaining tasks
Debate.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff.txt | 6.37 KB | znerol |
#33 | 2342593-remove-mixed-mode-ssl-33.diff | 48.59 KB | znerol |
#33 | interdiff.txt | 6.37 KB | znerol |
#28 | 2342593-remove-mixed-mode-ssl-27.diff | 42.19 KB | znerol |
#21 | 2342593-remove-mixed-mode-ssl-20.diff | 41.31 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedTotally in support. Mixed-mode support never worked to begin with, and it is a terrible idea from a security point of view anyway.
Comment #3
pounardI definitely agree that this removal must happen.
Comment #4
anavarreComment #5
andypost+1 to remove
Comment #6
grendzy CreditAttribution: grendzy commented+1 to remove, in principle. As a securepages module maintainer, I would like some time (perhaps a week) to review the patch, before committing.
One difficulty I foresee is that session storage is currently entangled with session HTTP logic. This presents a problem when you want to combine a storage plugin (e.g. memcached) with a logic plugin (e.g. secure pages). Also DrupalKernel.php still has some session code.
IMO the easiest way to support a wide range of contrib with the current code is to allow contrib to force
session.cookie_secure
to FALSE, without necessarily swapping out the session handler.Comment #7
webchickKnocking back to "needs review," per grendzy's request.
Comment #8
grendzy CreditAttribution: grendzy commentedI read and tested the patch, and the concerns I raised in comment #6 seem validated. In addition, some important session set-up happens in DrupalKernel::initializeCookieGlobals which AFAIK is not pluggable (at least it's not in core.services.yml, corrections welcomed).
I can see three ways to address those concerns:
Comment #9
znerol CreditAttribution: znerol commentedRe #8 #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator (http middlewares can be swapped out by contrib) other session related issues are linked from #1858196: [meta] Leverage Symfony Session components.
Regrettably 3 is not an option because we need to set the
session.cookie_secure
ini value depending on whether the request came in via HTTP or HTTPS.Comment #10
grendzy CreditAttribution: grendzy commentedOkay, to get more specific what I was suggesting in comment #8, option 2:
But I can see how #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator would provide the same flexibility. As long as there's a way for contrib to control
session.cookie_secure
, and ideally without having to replace a lot of unrelated storage code, I think contrib will be fine.Comment #11
znerol CreditAttribution: znerol commentedOpened #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service. @grendzy: do you think that this will resolve the session-cookie ini-value issue?
Comment #12
znerol CreditAttribution: znerol commentedCan we move forward here?
Comment #13
znerol CreditAttribution: znerol commentedReroll after #2343607: Cleanup UrlGenerator and #2313883: Minor code flow improvements to SessionHandler::write().
Comment #14
znerol CreditAttribution: znerol commentedWe also want to remove the mixed-mode special-casing from
UrlGenerator
,FormBuilder
and friends. When specifying thehttps
option when generating an URL or enforcing a form-action, I expect that this will be respected. Not only when mixed-mode sessions are enabled, but always.If anyone needs another reason for removing mixed-mode support from core: this tweet gives one.
Comment #16
znerol CreditAttribution: znerol commentedShould modify the proper service definition.
Comment #17
znerol CreditAttribution: znerol commentedComment #19
znerol CreditAttribution: znerol commentedFix session test by removing
session_test_form_user_login_form_alter
, this was only used for the mixed-mode tests.Comment #20
znerol CreditAttribution: znerol commentedComment #21
znerol CreditAttribution: znerol commentedComment #22
Crell CreditAttribution: Crell commentedI will leave this to grendzy to RTBC, but it looks OK to me.
Technically this issue is "move to contrib", not "remove", so do we need something in contrib first before we commit it? It's not a module per se so I don't know if that usual guideline applies.
Comment #23
znerol CreditAttribution: znerol commentedPlease understand that we need a decision here soon. Either RTBC and commit this or postpone it to 8.1.x/9.x.
If it gets committed it will speed up several other session related issues because of reduced code complexity. Otherwise I will need to find other ways to extract mixed mode sessions from the session manager, such that alternative storage backends can implement it savely withouth having to replicate it.
Comment #24
catchSupporting this stalled the porting of memcache 7.x session support, which still isn't stable.
Additionally #2378699: Port session hijacking fixes from SA-CORE-2014-006 to Drupal 8 shows the implementation is hard to get right, given core didn't.
Also 'all SSL' for authenticated users, and all SSL in general, is much more common than it was when this was added, and one way anon->auth session data preservation is much less complex to deal with.
So yes I think we should remove it from core, bumping to critical.
Comment #25
alexpottAs znerol has pointed out this was added for secure pages and the recent trend has been to move to only SSL - for example on monday https://www.tes.co.uk/ moved to only SSL.
+1 to moving to contrib. Especially since it'll solve #2378699: Port session hijacking fixes from SA-CORE-2014-006 to Drupal 8 - making critical bug fix due to that.
Comment #26
catchComment #27
mpdonadioThis also appears to be one of the complications with #2042447: Install a module user interface does not install modules (or themes), so there is the possibility that progress can be made on that, too, which is important for a set of users. The EFF's Let's Encrypt initiative will also (hopefully) raise the number of sites doing HTTPS everywhere.
Comment #28
znerol CreditAttribution: znerol commentedReroll.
Comment #29
Crell CreditAttribution: Crell commentedThen I guess this is ready, no?
Comment #30
alexpottWe need a change record for this.
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch does not change any of the documentation that describes it as happening the old way (i.e. only forcing HTTPS when mixed mode sessions are enabled), for example:
Nor does it change any of the code in core that uses it and expects it to behave that way, for example:
(From #2042447: Install a module user interface does not install modules (or themes) that particular case is apparently broken already and forcing everyone to HTTPS regardless of whether their server supports it or not, but it's not supposed to.)
Basically if this patch changes the 'https' option as described above then core can never use that option, since core can never assume that a site has the ability to be served over HTTPS.
The problem with not using it, though, is that the current code is there for improved security. If the site does have mixed-mode enabled, we absolutely want sensitive URLs like the above to be over HTTPS rather than HTTP.
Would a contrib version of mixed-mode SSL support be able to alter the URL in system_authorized_get_url() and similar places to provide that? If so, it seems fine to just remove.
For what it's worth, isn't that also possible as far back as Drupal 6? :)
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedI'm also moving this back to a task, since this patch does not actually solve #2378699: Port session hijacking fixes from SA-CORE-2014-006 to Drupal 8 (or at least not all of it).
Comment #33
znerol CreditAttribution: znerol commentedRerolled after #2378699: Port session hijacking fixes from SA-CORE-2014-006 to Drupal 8, updating the documentation.
We still give contrib/sites owners the possibility to set that flag on any form / URL they desire (via form alter, path processor). Neither core nor contrib uses the flag consistently (e.g., why is it missing on the login form?). The maintainers of Secure Pages have to keep track of sensible forms / paths anyway because of that, therefore not using the flag in the authorize system does not make much of a difference.
Comment #34
Crell CreditAttribution: Crell commentedGoing out on a limb...?
Comment #35
alexpottStill needs a change record.
Comment #36
znerol CreditAttribution: znerol commentedCR: https://www.drupal.org/node/2384903
Comment #37
geerlingguy CreditAttribution: geerlingguy commentedCR is good (made one tiny edit). Back to RTBC.
Comment #38
webchickOk, looks like we're good to go here, since the missing CR was the last complaint. Sounds like #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service will take care of contrib's use case around this functionality.
Committed and pushed to 8.0.x. Thanks!
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedI didn't review the whole thing carefully, but the changes look good to me.
I made several edits to the change record though (https://www.drupal.org/node/2384903/revisions/view/7889381/7890469). Mainly to clarify that the entire meaning of the #https and 'https' options have changed (so that they are now almost exclusively intended for custom code, not contrib modules).
I also published the change record since it was previously in draft state...
Comment #41
webchickOops, thanks David. Sorry for being a bit too trigger-happy on this one. :)
Comment #43
Wim LeersThis lacked test coverage (probably a pre-existing problem, but still), which allowed the regression #2649404: Form API's #https is broken for user login block (no longer opts in form to use HTTPS action URL) to be introduced unnoticed.