Bug 17776 - Shibboleth Authentication is broken in plack
Summary: Shibboleth Authentication is broken in plack
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 8446
Blocks: 19625
  Show dependency treegraph
 
Reported: 2016-12-14 14:21 UTC by Martin Renvoize
Modified: 2018-10-15 13:40 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This enhancement adds support for using Shibboleth in a Plack environment. Caution should, however, be taken before enabling it as there are security implications to be aware of regarding header spoofing attacks that can be mitigated with additional care whilst configuring the native service provider and Apache: Please see https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPSpoofChecking for further details.


Attachments
Enable Shibboleth for Plack (1.89 KB, patch)
2017-03-02 16:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Enable Shibboleth for Plack (3.18 KB, patch)
2017-03-06 16:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Enable Shibboleth for Plack (3.18 KB, patch)
2017-03-22 07:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Enable Shibboleth for Plack (3.25 KB, patch)
2017-10-17 14:27 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 17776: Enable Shibboleth for Plack (3.30 KB, patch)
2018-09-28 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17776: (followup) Add note about NativeSPSpoofChecking (872 bytes, patch)
2018-09-28 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17776: Enable Shibboleth for Plack (3.38 KB, patch)
2018-10-05 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17776: (follow-up) Add note about NativeSPSpoofChecking (1.13 KB, patch)
2018-10-05 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17776: (QA follow-up) Consistent regex for Plack detection (1.29 KB, patch)
2018-10-05 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17776: (QA follow-up) Remove shibboleth package variables (4.30 KB, patch)
2018-10-05 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17776: Enable Shibboleth for Plack (3.36 KB, patch)
2018-10-05 10:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17776: (follow-up) Add note about NativeSPSpoofChecking (1.10 KB, patch)
2018-10-05 10:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17776: (QA follow-up) Consistent regex for Plack detection (1.32 KB, patch)
2018-10-05 10:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17776: (QA follow-up) Remove shibboleth package variables (4.34 KB, patch)
2018-10-05 10:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2016-12-14 14:21:44 UTC
Shibboleth authentication relies heavily on per request environment variable, which doesn't play nicely with persistent apps (plack).

We need to convert to using request headers in this case and update the documentation to reflect this alteration.
Comment 1 Katrin Fischer 2016-12-14 20:23:02 UTC
Should this be enh or more a bug?
Comment 2 Mirko Tietgen 2016-12-15 09:31:42 UTC
If it's broken it's a bug. ;)
Comment 3 Martin Renvoize 2017-03-02 16:43:37 UTC
Created attachment 60800 [details] [review]
Enable Shibboleth for Plack
Comment 4 Martin Renvoize 2017-03-02 16:47:42 UTC
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.
Comment 5 Zeno Tajoli 2017-03-03 10:33:16 UTC
Do you think we can use http://www.ssocircle.com/en/portfolio/publicidp/ for testing ?

Or is better to use https://www.testshib.org/ ?
Comment 6 Martin Renvoize 2017-03-06 16:43:34 UTC
Created attachment 60847 [details] [review]
Enable Shibboleth for Plack
Comment 7 Martin Renvoize 2017-03-06 16:47:05 UTC
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).
Comment 8 Martin Renvoize 2017-03-06 16:48:15 UTC
(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.
Comment 9 Mirko Tietgen 2017-03-20 13:03:59 UTC
There are two instances of get_shib_login in Auth.pm that probably should be get_login_shib instead?
Comment 10 Mirko Tietgen 2017-03-20 13:19:54 UTC
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>
Comment 11 Martin Renvoize 2017-03-22 07:27:04 UTC
Created attachment 61426 [details] [review]
Enable Shibboleth for Plack
Comment 12 Martin Renvoize 2017-03-22 07:32:38 UTC
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>
Comment 13 Martin Renvoize 2017-03-22 07:34:53 UTC
https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPApacheConfig is very helpful for understanding what the different apache directives do ;)
Comment 14 Mirko Tietgen 2017-03-22 15:48:54 UTC
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?
Comment 15 Martin Renvoize 2017-03-22 17:35:51 UTC
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
Comment 16 Katrin Fischer 2017-08-18 10:51:54 UTC
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...
Comment 17 Matthias Meusburger 2017-10-17 14:24:49 UTC
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.
Comment 18 Matthias Meusburger 2017-10-17 14:27:24 UTC
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>
Comment 19 Barry Cannon 2017-10-24 19:47:18 UTC
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.
Comment 20 Matthias Meusburger 2017-10-25 07:11:02 UTC
For information, we are currently using it successfully on 16.11.12
Comment 21 Katrin Fischer 2017-10-27 12:13:51 UTC
Patch doesn't apply cleanly to 16.11.13 - could you help resolve?
Comment 22 Barry Cannon 2017-10-29 14:30:07 UTC
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.
Comment 23 Katrin Fischer 2017-10-29 14:32:07 UTC
I had a really small conflict, but don't understand the code well enough to risk it.
Comment 24 Marcel de Rooy 2018-01-12 09:09:42 UTC
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..
Comment 25 Tomás Cohen Arazi 2018-02-06 19:02:47 UTC
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.
Comment 26 Barry Cannon 2018-02-16 09:13:37 UTC
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
Comment 27 Marvin Addison 2018-02-21 12:38:02 UTC
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.
Comment 28 Martin Renvoize 2018-09-27 08:47:14 UTC
(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.
Comment 29 Matthias Meusburger 2018-09-28 07:24:46 UTC
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.
Comment 30 Marcel de Rooy 2018-09-28 07:29:41 UTC
(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 ?
Comment 31 Martin Renvoize 2018-09-28 08:33:07 UTC
(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.
Comment 32 Matthias Meusburger 2018-09-28 08:43:59 UTC
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.
Comment 33 Nick Clemens 2018-09-28 12:28:09 UTC
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.
Comment 34 Martin Renvoize 2018-09-28 12:33:55 UTC
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>
Comment 35 Martin Renvoize 2018-09-28 12:33:59 UTC
Created attachment 79573 [details] [review]
Bug 17776: (followup) Add note about NativeSPSpoofChecking
Comment 36 Marvin Addison 2018-09-28 12:37:12 UTC
Is the intention to update the documentation to discuss security implications before closing this issue?
Comment 37 Nick Clemens 2018-09-28 12:46:23 UTC
(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
Comment 38 Katrin Fischer 2018-09-28 13:03:30 UTC
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?
Comment 39 Martin Renvoize 2018-09-28 13:17:21 UTC
or even PQA perhaps?
Comment 40 Marcel de Rooy 2018-10-05 07:28:00 UTC
QA: Having another look now
Comment 41 Marcel de Rooy 2018-10-05 09:21:05 UTC
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>
Comment 42 Marcel de Rooy 2018-10-05 09:21:11 UTC
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.
Comment 43 Marcel de Rooy 2018-10-05 09:21:16 UTC
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>
Comment 44 Marcel de Rooy 2018-10-05 09:21:21 UTC
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>
Comment 45 Marcel de Rooy 2018-10-05 09:22:47 UTC
Waiting for a confirm from Martin
Comment 46 Martin Renvoize 2018-10-05 10:51:51 UTC
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>
Comment 47 Martin Renvoize 2018-10-05 10:51:56 UTC
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.
Comment 48 Martin Renvoize 2018-10-05 10:52:00 UTC
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>
Comment 49 Martin Renvoize 2018-10-05 10:52:05 UTC
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>
Comment 50 Martin Renvoize 2018-10-05 10:53:02 UTC
As requested I've tested the followups and confirmed them working from here. Nice little bit of code golf there, thanks Marcel.
Comment 51 Nick Clemens 2018-10-09 16:26:43 UTC
Awesome work all!

Pushed to master for 18.11
Comment 52 Martin Renvoize 2018-10-15 13:40:58 UTC
Enhancement, will not be backported to 18.05.x series.