Should this be enh or more a bug?

If it's broken it's a bug. ;)

Created attachment 60800 [details] [review]
Enable Shibboleth for Plack

To test: 

1) Enable plack
2) Alter the apache config block to ensure shibboleth is passing attributes via headers instead of environment. (when running under plack, apache act's merely as a Proxy and so cannot pass environment to the separate plack process).
3) Checkin shibboleth logins are now working using the plack instance.

Do you think we can use http://www.ssocircle.com/en/portfolio/publicidp/ for testing ?

Or is better to use https://www.testshib.org/ ?

Created attachment 60847 [details] [review]
Enable Shibboleth for Plack

So after more local testing, i found that the plack environment could be a little more complex than my initial tests.

This patch obsoletes the original and calls 'get_shib_login' later in the runtime (i.e. outside of the begin block) so we have a valid environment by the time the routine run.

In short, it should all work now so long as you've updated your Apache configs as per the inline perldoc documentation.

I believe the UseHeaders and UseEnvironment variables for the shibboleth service provider software are mutually exclusive (they appeared to be in my brief testing), so I don't believe it is possible to run in a half and half setup (unless you have two entirely separate vhosts.. one for plack and one for non-plack running).

(In reply to Zeno Tajoli from comment #5)
> Do you think we can use http://www.ssocircle.com/en/portfolio/publicidp/ for
> testing ?
> 
> Or is better to use https://www.testshib.org/ ?

Either IdP should work perfectly happily.  I've tested here against teshshib, openfiede and some customer systems using simplesamlphp and ms active directory services.

There are two instances of get_shib_login in Auth.pm that probably should be get_login_shib instead?

I get redirected to a login page when I add

ShibUseEnvironment Off
ShibUseHeaders On

to my Apache config.

There is an additional

Require valid-user

in the config of the test system, just removing that leads to the same result, so I wonder if it is connected and how I can test around that?


  <Location />
                AuthType shibboleth
                ShibRequireSession On
                ShibUseEnvironment Off
                ShibUseHeaders On
                Require valid-user
  </Location>

Created attachment 61426 [details] [review]
Enable Shibboleth for Plack

Fixed the get_login_shib.. thanks for spotting that.. seems I'd already corrected it locally.. Oops.

As for the apache config..

It's the `ShibRequireSession On` line that means you are enforcing a shibboleth login for all users I believe.. I don't think that's required if you want optional login.  I'm not sure where that line came from on your test system?

To help, I've included a copy of my exact config from the demo server where I've been testing:

   # Optional Shibboleth Configuration - Plack Alternative
   <Location />
      #ShibRequestSetting applicationId demo.koha-ptfs.co.uk
      AuthType shibboleth
      ShibUseEnvironment Off
      ShibUseHeaders On
      ShibRequireSession Off
      Require shibboleth
      #Require valid-user
   </Location>

https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPApacheConfig is very helpful for understanding what the different apache directives do ;)

Very helpful link, thanks!

We are enforcing Shib login on purpose, there is not supposed to be any other way to log in. So that does not work with the config needed for Plack?

Hmm, slightly confused by the comment then.. I thought you were finding that it was always redirecting but that wasn't the behaviour you wanted.  I'll quiz you on IRC tomorrow to clarify the question.

Thanks for testing,

Martin

What's missing here to get it moving again? I think the Plack - Shibboleth incompatibility is going to be a real problem for users as Koha without Plack is no fun...

Mirko, if you'd like to enforce Shib login, you should have a look at Bug 18506.

Without enforcing Shib login, we use this patch successfully in production with this configuration:

      AuthType shibboleth
      ShibUseEnvironment Off
      ShibUseHeaders On
      ShibRequireSession Off
      Require shibboleth

I'm signing this off.

Created attachment 68216 [details] [review]
Enable Shibboleth for Plack

https://bugs.koha-community.org/show_bug.cgi?id=17776
Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>

I am not sure about this patch. I managed to get it working on 16.11.10 but not on 16.11.13. apply patch, enable plack - shibboleth auth doesn't work. Disable plack and shib works again. Shib log seems to to be comparable during both login attempts.

For information, we are currently using it successfully on 16.11.12

Patch doesn't apply cleanly to 16.11.13 - could you help resolve?

I didn't have any problems applying to 16.11.13. However, I tested again and on 16.11.10 it works fine but as soon as I upgrade to 16.11.13 and re-apply the patch the shib error returns.

I had a really small conflict, but don't understand the code well enough to risk it.

ShibUseHeaders On|Off
Defaults to "Off", this turns on the use of request headers to publish attributes to applications. Use of this option should be avoided. Be sure to review the topic on spoof checking if you enable it.

You are suggesting to disable ShibUseEnvironment and enable ShibUseHeaders.
Please explain. It is not recommended..

Can someone explain how to reproduce the problem? It seems to me that if we need to inject ENV variables on a per-request basis we can write a Plack middleware to take care of that.

To reproduce the problem.
- create a koha instance with plack not enabled. 
- configure shibboleth as normal and confirm login works and maps correctly to a borrower in koha. 
- enable and start plack for the same instance
- observe shib login no longer works (after successful login at idp redirect to koha does not map to correct borrower and login option still available on koha)

Oddly, this patch has worked right up to 17.11.01 but since 17.11.02 it has stopped. Applied fine but functionality is no longer there - still trying to figure out why

CAUTION. The proposed fix for this issue, enabling request headers to convey Shibboelth attributes, opens a gaping security hole unless other compensating controls are applied. From https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPAttributeAccess:

----------
Unfortunately, not all web servers currently expose a mechanism to create custom variables from within server extensions. This is a bug; all web servers should support this in some way, but IIS and Sun/iPlanet do not.

On these platforms, the SP is forced to substitute the use of custom HTTP request headers. This is convenient, in that the CGI requires custom headers to be passed along to applications, but is also dangerous and difficult to secure. The SP has had at least two separate major security patches resulting from this mechanism. This is because the header mechanism is really about passing information from the client to the application; any browser can be manipulated to supply arbitrary headers quite easily with little skill.

To defend against this, the SP has a number of protections designed to clear out any data supplied by the client that might overlap with the headers it creates. But this is very difficult to get right in practice, and recent versions include a much-enhanced NativeSPSpoofChecking mechanism for actually detecting and blocking requests that carry such headers.

When using headers, the main difference is that instead of using the names defined via the mapping process, the application must prefix them with "HTTP_", and in most tools upcase the rest of the name as well. The specifics vary by tool, and in the case of IIS and ASP.NET are even more bizarre because of serious flaws in IIS' CGI implementation.

A fair amount of detail on this can be found in the secadv_20090615 topic. The most particular point about ASP.NET is that it provides access to both the transformed headers (all caps, with the HTTP_ prefix) via the ServerVariables collection, and the untransformed input headers via the Headers collection. The latter is much safer to use.
----------

Thus enabling ShibUseHeaders without any other controls allows clients to spoof shibboleth attributes, thereby allowing them to completely bypass authentication and defeat any authorization controls in the worst case.

One adequate compensating control is the header spoof prevention facility described at https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPSpoofChecking.

(In reply to Marcel de Rooy from comment #24)
> ShibUseHeaders On|Off
> Defaults to "Off", this turns on the use of request headers to publish
> attributes to applications. Use of this option should be avoided. Be sure to
> review the topic on spoof checking if you enable it.
> 
> You are suggesting to disable ShibUseEnvironment and enable ShibUseHeaders.
> Please explain. It is not recommended..

The issue is IPC (Inter Process Communication). Koha relies upon a third party software to handle most of the complexities of Shibboleth/SAML.. The 'native service provider' package.. an apache plugin exists, mod_shibboleth, which we have been using to communicate between the native service provider code, Apache and finally koha. In CGI world, Koha runs a process per request under a forked Apache, and as such Apache and Koha share the same process environment. In the Plack world, Koha runs in a persistent process and requests are proxied from Apache to Plack (Koha); As such, no environment is shared and we have to utilize an alternative means of communicating between Koha and Apache (and therefore the native shibboleth service provider). The only other supported means of transporting that information is Headers (in mod_shibboleth).

So.. to do better than this patch we either need to get rid of Apache and the native shibboleth service provider package and write our own native shibboleth handling code.. or write a plack middleware that interfaces directly with the native service provider software.. that's a pretty long way outside of my own scope for this.

Personally, this isn't the 100% best fix, but it's the best we can do without basically re-writing Koha in my opinion.. With the NativeSPSpoofChecking guidance followed it's not as big an issue as many are making out in my opinion.

I completely agree with Martin's comment: this is the best we can come up with right now.

However, I'm not a system administrator, and this is a system issue.

(In reply to Martin Renvoize from comment #28)
> Personally, this isn't the 100% best fix, but it's the best we can do
> without basically re-writing Koha in my opinion.. With the
> NativeSPSpoofChecking guidance followed it's not as big an issue as many are
> making out in my opinion.

What about the concerns listed in comment27 ?

(In reply to Marcel de Rooy from comment #30)
> What about the concerns listed in comment27 ?

Well, we could document that if using plack we strongly recommend you read the documentation around NativeSPSpoofChecking and add a spoofKey as suggested there. I believe that's all handled in the configuration of the Native Service Provider package, so again somewhat outside of the scope of Koha code.. but I agree to adding a doc patch pointing people there.. Would that allay your concerns at all?

I do wish they would document what they mean by 'There are no known scenarios in which environment variables can't be used' on that NativeSPSpoofChecking page.. I certainly know of no way to get around our scenario.

About comment#27, I tried to spoof HTTP headers with firefox's "Modify Header Value (HTTP Headers)" extension ( https://addons.mozilla.org/fr/firefox/addon/modify-header-value ) and got the following message:

"opensaml::SecurityPolicyException

The system encountered an error at Fri Sep 28 08:33:58 2018

To report this problem, please contact the site administrator at
root@localhost.

Please include the following message in any email:

opensaml::SecurityPolicyException at
(https://catalogue.koha-shib/cgi-bin/koha/opac-user.pl)

Attempt to spoof header (AJP_Login) was detected."


So basic spoofing doesn't work.

However, I'm no security expert, so if anyone thinks that we should add more control mechanisms to the stack we recommand (Apache / mod_shib / plack), please say so.

For all the other stacks (IIS, Sun/iPlanet, etc.), we should clearly mention in the documentation that control mechanisms are needed.

This patchset leaves the decision open to the user/IT staff outside of Koha. We can either enable shib with plack, or disable plack and use environment variables depending on their unique security needs. Additional spoof protection would be on the maintainer of the Koha server (via apache or other configuration) so is again outside of Koha.

We should document that anyone enabling headers should read up on spoof protection.

Finding a middleware solution might be a great future enhancement (or just make everyone use CAS :-) ), but I think this is a workable interim solution

No blocker for me.

Created attachment 79572 [details] [review]
Bug 17776: Enable Shibboleth for Plack

https://bugs.koha-community.org/show_bug.cgi?id=17776

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>

Created attachment 79573 [details] [review]
Bug 17776: (followup) Add note about NativeSPSpoofChecking

Is the intention to update the documentation to discuss security implications before closing this issue?

(In reply to Marvin Addison from comment #36)
> Is the intention to update the documentation to discuss security
> implications before closing this issue?

Yes, we are going to add the option to use headers, and add a warning for anyone who chooses to do so and leave the final decision to the Koha admin/end user

Should we also update the wiki + the manual? At least the wiki has a page for Shibboleth already: 
https://wiki.koha-community.org/wiki/Shibboleth_Configuration

Also we should add "Text to go in the release notes" here in bugzille. 

Really glad to see this moving.

Are we ready to move this from FQA to Signed off yet?

or even PQA perhaps?

QA: Having another look now

Created attachment 80000 [details] [review]
Bug 17776: Enable Shibboleth for Plack

https://bugs.koha-community.org/show_bug.cgi?id=17776

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80001 [details] [review]
Bug 17776: (follow-up) Add note about NativeSPSpoofChecking

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended the text a bit:
Hopefully, an uppercase important attracts slightly more attention :)
Added the bug number too.

Created attachment 80002 [details] [review]
Bug 17776: (QA follow-up) Consistent regex for Plack detection

Synchronizing:
C4/Auth_with_shibboleth.pm:    if ( any { /(^psgi|^plack)/i } keys %ENV ) {
Koha/AuthUtils.pm:    if ( ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) && $ENV{SCRIPT_NAME} =~ m,^/(intranet|opac)(.*), ) {
about.pl:if ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) {

Actually we should move it to a subroutine. New report please.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80003 [details] [review]
Bug 17776: (QA follow-up) Remove shibboleth package variables

This is about $shib and $shib_login.
We move in the right direction by calling get_login_shib in
get_template_and_user and checkauth. In the same line we can do the
shib_ok check at that time (just checking cached values). This paves
the way for the third subroutine using the two package vars: checkpw.
Note that checkpw is also called outside Auth.pm. So I would be more
comfortable if we do the same calls like in checkauth and remove both
variables from the package level (especially under Plack of course).

The former changes actually justify a 'use C4::Auth_with_shibboleth'
instead of the current require and import.

Note: When calling checkpw from checkauth, we are calling get_login_shib
twice now. But the time involved for doing so is around zero (cache), so
not really an argument for extra parameters and complexer code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Waiting for a confirm from Martin

Created attachment 80023 [details] [review]
Bug 17776: Enable Shibboleth for Plack

https://bugs.koha-community.org/show_bug.cgi?id=17776

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 80024 [details] [review]
Bug 17776: (follow-up) Add note about NativeSPSpoofChecking

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended the text a bit:
Hopefully, an uppercase important attracts slightly more attention :)
Added the bug number too.

Created attachment 80025 [details] [review]
Bug 17776: (QA follow-up) Consistent regex for Plack detection

Synchronizing:
C4/Auth_with_shibboleth.pm:    if ( any { /(^psgi|^plack)/i } keys %ENV ) {
Koha/AuthUtils.pm:    if ( ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) && $ENV{SCRIPT_NAME} =~ m,^/(intranet|opac)(.*), ) {
about.pl:if ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) {

Actually we should move it to a subroutine. New report please.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 80026 [details] [review]
Bug 17776: (QA follow-up) Remove shibboleth package variables

This is about $shib and $shib_login.
We move in the right direction by calling get_login_shib in
get_template_and_user and checkauth. In the same line we can do the
shib_ok check at that time (just checking cached values). This paves
the way for the third subroutine using the two package vars: checkpw.
Note that checkpw is also called outside Auth.pm. So I would be more
comfortable if we do the same calls like in checkauth and remove both
variables from the package level (especially under Plack of course).

The former changes actually justify a 'use C4::Auth_with_shibboleth'
instead of the current require and import.

Note: When calling checkpw from checkauth, we are calling get_login_shib
twice now. But the time involved for doing so is around zero (cache), so
not really an argument for extra parameters and complexer code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

As requested I've tested the followups and confirmed them working from here. Nice little bit of code golf there, thanks Marcel.

Awesome work all!

Pushed to master for 18.11

Enhancement, will not be backported to 18.05.x series.