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?
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2022029-8.patch | 2.02 KB | widukind |
| #8 | interdiff.txt | 5.58 KB | widukind |
| #3 | interdiff.txt | 742 bytes | widukind |
| #3 | 2022029-3.patch | 3.51 KB | widukind |
| #2 | 2022029-2.patch | 3.5 KB | widukind |
Comments
Comment #1
brad.bulger commentedAs long as there is a configuration option to force HTTPS that connects to nothing in the module code, this is a bug.
Comment #2
widukind commentedPlease 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-9429473Comment #3
widukind commentedWhoops, 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.
Comment #4
snufkin commentedI don't think this should be included in the module. HTTPS should be enforced site-wide, should never be SSO specific.
Comment #5
erykmynn commentedI 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.
Comment #6
snufkin commented@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
Comment #7
erykmynn commentedHi @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!
Comment #8
widukind commentedAlright, 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.
@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?
Comment #9
snufkin commentedProbably 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.
Comment #10
snufkin commentedCommitted 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.