michaelryanmcneill / shibboleth

Shibboleth plugin for WordPress
https://wordpress.org/plugins/shibboleth/
19 stars 11 forks source link

Reconsider checking for 'HTTP_' version of variables #8

Closed jrchamp closed 6 years ago

jrchamp commented 6 years ago

https://github.com/michaelryanmcneill/shibboleth/blob/b95eac843be2b872afb0fd0073977f95de914abf/shibboleth.php#L36

Typically, the HTTP_ prefixed keys within the $_SERVER superglobal come from the headers sent by the client. Unless support for this prefix is required, I would propose removing it to limit the potential security risk.

michaelryanmcneill commented 6 years ago

This has been implemented to support proxied Shibboleth service providers. If traffic is being routed using a ProxyPass from an upstream server, it will be passed with the HTTP_ prefix. Any ideas on better ways to support this use case while increasing security?

jrchamp commented 6 years ago

Not any great solutions, but:

The first one is just an ordering thing, similar to a cipher list, leaving the HTTP_ ones at the end.

The second one we might try and find the 'Shib-Session-ID' first (given that it's required) and use the "format" that was used for that one for all the rest? Hopefully all of the attributes have the same transformation? (This might help with #7 if the problem is that UID is being overloaded.)

michaelryanmcneill commented 6 years ago

I believe the solution of going through on a priority basis seems like a good idea. It would also be nice to support spoofKey as well. I think that determining what "type" we're using would be great, but I'm not exactly sure how to accomplish that. A patch to support the priority variables would be super helpful, but I'll try and spare some cycles to implement a fix.

michaelryanmcneill commented 6 years ago

I'm going to try overloading UID to see if that reproduces #7.

michaelryanmcneill commented 6 years ago

This is being handled in the 1.9-alpha branch by option. Users will be able to select what type of attribute checking method they'd like to use, defaulting to standard environment variables. It also will support spoofKey checking.

michaelryanmcneill commented 6 years ago

This will be a big change, and could potentially temporarily break users until they update their settings. An upgrade notice will be critical to notify users of the potential breaking changes. I've gotten everything functional here, just need to add some informational text.

jrchamp commented 6 years ago

The changes in the alpha branch look great. I do like how specific choosing the authentication style is. This does mean that attempting to serve the same website through two different authentication styles simultaneously would require a code change on the second system. Not something I expect to support, but worth noting in this discussion.

michaelryanmcneill commented 6 years ago

This has been completed and will be included in the next release. I'm going to change this release from 1.9 to 2.0 to start with a new fresh release count.