the "Force https for login links" checkbox is on the admin configuration page, and i can see that it stores it in the same variable as in the 6.x version, but it doesn't seem to do anything with it. is it up to us to change the login link to use https now?

Comments

brad.bulger’s picture

Category: Support request » Bug report
Issue summary: View changes

As long as there is a configuration option to force HTTPS that connects to nothing in the module code, this is a bug.

widukind’s picture

StatusFileSize
new3.5 KB

Please see attached patch for an attempt to solve this problem.
It redirects unencrypted requests for /saml_login to HTTPS if the "force https" option is turned on.
Additionally, it attempts to set the HTTPS protocol on all login/logout links generated by this module.

X-ref: The hook_url_inbound_alter() implementation provided in this patch has been derived from a subject-related patch over here: https://www.drupal.org/node/2381943#comment-9429473

widukind’s picture

StatusFileSize
new3.51 KB
new742 bytes

Whoops, I goobered this up. Needed to generate the URL first before attempting to rewriting its protocol to HTTPS.

Fixed now, please see attached patch and interdiff.

snufkin’s picture

I don't think this should be included in the module. HTTPS should be enforced site-wide, should never be SSO specific.

erykmynn’s picture

I don't think I follow the logic. Why does the module have a "use https" config setting? Why not have parity with the HTTPS-only cookie option in the SAML Library config?

There is a long list of potential reasons you don't want your editors and admins being logged in over HTTP, especially if you're using Varnish. But also because of allowing unencrypted access to the back end.

The existing Drupal solution (secure login) is bypassed by this Simplesaml module, so it's kind of hard to imagine where else you could sensibly enforce this.

snufkin’s picture

@erykmynn I am not sure why that feature was set up initially, I'm more in favour of what you propose, using the configuration the SAML library implements.

Noone should be logging in and using the site over plain HTTP, what I'm saying is that HTTPS access to the site should be enforced via hosting configuration, securepages and other generic solutions. The fact that this module bypasses all that is a big issue, but a separate bug.

In my opinion we should follow the approach:
- remove all custom https related configuration from the module
- ensure that the library settings are respected
- document and reference how https can be enforced via other contributed modules

erykmynn’s picture

Hi @Snufkin,

@Widukind and I were talking this morning and we agree that the force HTTPS option could potentially be abandoned, and that the cleanest way to control the login links is with a re-write, or by implementing the hook_url_inbound_alter() seen here as a standalone helper module (or potentally a submodule) for those with less access to server config.

We do have a overlapping but quite discreet issue that also has to do with how the module behaves when HTTPS is mandated in the library config... https://www.drupal.org/node/2381943

We really like your idea about respecting the library config, so I think @Widukind is looking at re-writing the patch there to not have any Drupal config--instead it will check the SAML Config!

widukind’s picture

StatusFileSize
new5.58 KB
new2.02 KB

Alright, I yanked any notion of enforcing HTTPS from the module.

Turns out that a simple rewrite rule added to .htaccess does the trick for us.

# Force redirect to HTTPS for SimpleSAMLphp Auth module's login path
RewriteCond %{HTTPS} !=on
RewriteCond %{HTTP:X-Forwarded-Proto} !https
RewriteRule ^saml_login https://%{HTTP_HOST}%{REQUEST_URI} [R=301,QSA,L]

@Snufkin - the need to enforce HTTPS and how to do so should probably documented somewhere. What would be the proper place - a sub-page on d.o and/or the module's README file?

snufkin’s picture

Probably both. For now I've started a documentation page (that is loooooong overdue) here: https://www.drupal.org/node/2402399. Feel free to update it, it needs lots of love.

snufkin’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Active » Fixed

Committed the latest patch, removing the admin interface and the (unused) function that checks if this setting was turned on. The documentation should be sufficient for htaccess changes.

  • snufkin committed 2c67b1e on 7.x-3.x authored by widukind
    Issue #2022029 by widukind: how to force https in 7.x
    

Status: Fixed » Closed (fixed)

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