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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
FileSize
24.43 KB
Damien Tournoud’s picture

Totally in support. Mixed-mode support never worked to begin with, and it is a terrible idea from a security point of view anyway.

pounard’s picture

I definitely agree that this removal must happen.

anavarre’s picture

Issue tags: +Security
andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1 to remove

grendzy’s picture

+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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Knocking back to "needs review," per grendzy's request.

grendzy’s picture

Status: Needs review » Needs work

I 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:

  1. Refactor HTTP logic and session storage into two separate, loosely coupled interfaces. This is probably a chunk of work, and ill-timed so close to beta, and also maybe time not well spent since a big refactor might as well adopt the Symfony session handler.
  2. Or, use a settings.php variable to optionally force session.cookie_secure to FALSE in DrupalKernel::initializeCookieGlobals.
  3. Or, just remove session.cookie_secure from Drupal entirely, and allow it be managed by php.ini (which is what Symfony appears to do, after a quick skim of Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage)
znerol’s picture

Status: Needs work » Needs review

Re #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.

grendzy’s picture

Okay, to get more specific what I was suggesting in comment #8, option 2:

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index 8514448..acbf72f 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -914,10 +914,13 @@ protected function initializeCookieGlobals(Request $request) {
     // separate session cookies for the HTTPS and HTTP versions of the site. So
     // we must use different session identifiers for HTTPS and HTTP to prevent a
     // cookie collision.
-    if ($request->isSecure()) {
+    if ($request->isSecure() && Settings::get('session_cookie_secure', TRUE)) {
       ini_set('session.cookie_secure', TRUE);
+      $prefix = 'SSESS';
+    }
+    else {
+      $prefix = 'SESS';
     }
-    $prefix = ini_get('session.cookie_secure') ? 'SSESS' : 'SESS';
 
     session_name($prefix . substr(hash('sha256', $session_name), 0, 32));

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.

znerol’s picture

Opened #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service. @grendzy: do you think that this will resolve the session-cookie ini-value issue?

znerol’s picture

Can we move forward here?

znerol’s picture

znerol’s picture

We also want to remove the mixed-mode special-casing from UrlGenerator, FormBuilder and friends. When specifying the https 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.

Status: Needs review » Needs work

The last submitted patch, 14: 2342593-remove-mixed-mode-ssl-14.diff, failed testing.

znerol’s picture

Should modify the proper service definition.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2342593-remove-mixed-mode-ssl-16.diff, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
41.25 KB
552 bytes

Fix session test by removing session_test_form_user_login_form_alter, this was only used for the mixed-mode tests.

znerol’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
znerol’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
41.31 KB
Crell’s picture

I 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.

znerol’s picture

Please 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.

catch’s picture

Title: Move mixed mode SSL support to contrib » Remove mixed SSL support from core
Priority: Major » Critical
Related issues: +#2378699: Port session hijacking fixes from SA-CORE-2014-006 to Drupal 8

Supporting 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.

alexpott’s picture

Category: Task » Bug report

As 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.

catch’s picture

Issue tags: +D8 upgrade path
mpdonadio’s picture

This 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.

znerol’s picture

Reroll.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Then I guess this is ready, no?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record for this.

David_Rothstein’s picture

When specifying the https 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.

This 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:

   *   - 'https': Whether this URL should point to a secure location. If not
   *     defined, the current scheme is used, so the user stays on HTTP or HTTPS
   *     respectively. if mixed mode sessions are permitted, TRUE enforces HTTPS
   *     and FALSE enforces HTTP.
   *
   * @return static
   */
  public static function createFromRoute($text, $route_name, $route_parameters = array(), $options = array()) {

Nor does it change any of the code in core that uses it and expects it to behave that way, for example:

function system_authorized_get_url(array $options = array()) {
  global $base_url;
  // Force HTTPS if available, regardless of what the caller specifies.
  $options['https'] = TRUE;
  // Prefix with $base_url so url() treats it as an external link.
  $url = Url::fromUri('base://core/authorize.php');

(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.

it is now possible to completely swap out the session implementation from contrib.

For what it's worth, isn't that also possible as far back as Drupal 6? :)

David_Rothstein’s picture

Category: Bug report » Task

I'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).

znerol’s picture

Status: Needs work » Needs review
FileSize
6.37 KB
48.59 KB
6.37 KB

Rerolled 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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Going out on a limb...?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Still needs a change record.

znerol’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

CR is good (made one tiny edit). Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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!

  • webchick committed 8d601c7 on 8.0.x
    Issue #2342593 by znerol, grendzy, David_Rothstein: Remove mixed SSL...
David_Rothstein’s picture

I 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...

webchick’s picture

Oops, thanks David. Sorry for being a bit too trigger-happy on this one. :)

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

This 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.