auth_proxy, native login fails when LDAP unavailable

[John Merriam]

Confirmed in 2.9. Problem likely in earlier versions and probably also in later versions up through 2.12 and master branch.

When using the auth_proxy module with both the native authenticator and the LDAP authenticator, native authentication fails if the LDAP server can not be reached.

In our consortium, all of our libraries but one authenticate with the native authenticator. For the one other library, we proxy their login requests to their LDAP server. In our opensrf.xml we have the native authenticator enabled with the default configuration. The only other configured autheticator is the configuration for the client library LDAP server which has the appropriate <org_units> section to limit it to their org unit.

Recently the LDAP server could not be reached due to an outage at the library that we proxy LDAP logins for. This caused a failure in our Evergreen system where many people could not log in that were not associated with the LDAP library.

After looking at the code I suspected that what may be happening is that at least some logins come through without an org unit set. When that happens it will try all the authenticators no matter what. I thought through the flow of how that information comes in to the auth_proxy module and came up with the following very simple fix:

--- AuthProxy.pm 2017-09-06 09:20:56.401002057 -0400
+++ AuthProxy.pm.ldap_unavail_fix 2017-09-06 09:22:03.258049213 -0400
@@ -217,7 +217,7 @@
         if ($authenticator->login_types and $args->{'type'}) {
             next unless grep(/^(all|$args->{'type'})$/, @{$authenticator->{'login_types'}});
         }
- if ($authenticator->org_units and $args->{'org'}) {
+ if ($authenticator->org_units) {
             next unless grep(/^(all|$args->{'org'})$/, @{$authenticator->{'org_units'}});
         }

That is based on the master branch. Applying that patch to our system fixes the problem. I am not sure if that is the best way to fix it or not though.

[John Merriam (jmerriam)]

Confirmed in Evergreen 2.12.8

Likely also affects later releases including 3.0 and master branch.

After upgrading to 2.12 last weekend, we retested this bug. Without the patch, adding a null route for the LDAP server IP address so it can not be reached reproduces the bug where no one can log in, even local users. Remove the null route to the LDAP server and all logins work again.

With the patch, local users can log in even with the null route to the LDAP server in place.

[Dan Wells (dbw2)]

John, thank you for digging into this. I have a few questions:

1) Is your LDAP-enabled library using LDAP logins for OPAC, staff, or both?

2) Would you be able to post your auth_proxy app_settings contents (with passwords, etc. redacted)?

I think I understand why your change works for you, but it does seem contrary to the intention of the code, i.e. treating lack of an org argument as an 'undefined' case where org filters are not applied. In other words, I believe this change would break some cases the other way, but I am not quite sure yet.

I think a safer fix is ultimately going to be making the LDAP request a little more graceful in failure, and finding cases were we could but are not yet passing in an org argument would be a good idea as well, if such places exist.

Dan

[John Merriam (jmerriam)]

Hi Dan.

1) I believe that they use the LDAP logins for their patrons to access
the OPAC but have native/local logins for their staff.

2) auth_proxy app_settings in our opensrf.xml:

<open-ils.auth_proxy>
   ...
   <app_settings>
     <enabled>true</enabled>
     <authenticators>
       <authenticator>
         <name>ldap</name>
         <module>OpenILS::Application::AuthProxy::LDAP_Auth</module>
         <hostname>somehost.somedomain.org</hostname>
         <basedn>dc=somedomain,dc=org</basedn>
         <authid>cn=admin_abc,cn=users,dc=somedomain,dc=org</authid>
         <id_attr>AccountName</id_attr>
         <password>Password</password>
         <login_types>
           <type>staff</type>
           <type>opac</type>
         </login_types>
         <org_units>
           <unit>123</unit>
         </org_units>
       </authenticator>
       <authenticator>
         <name>native</name>
       </authenticator>
     </authenticators>
   </app_settings>
</open-ils.auth_proxy>

Let me know if you need me to make that in to an attachment instead.

What didn't make sense to me at first when looking at the code was why
would it be trying the LDAP authenticator at all unless the org_unit
equaled 123?

When I looked at it long enough, it seems to me that what the original code:

   if ($authenticator->org_units and $args->{'org'})

is saying is only run this next test:

   next unless grep(/^(all|$args->{'org'})$/,
@{$authenticator->{'org_units'}})

if we received an $args->{'org'}.

But, if you look at that 'next unless' test, I don't think we want to
run it only if we received an $args->{'org'}, I think we want to run it
no matter what if there is an $authenticator->org_units

I think the result of the original code is that the system may try all
authenticators if there is not an $args->{'org'} regardless of if there
is an $authenticator->org_units set. Basically ignoring the fact that
there is an <org_units> for the <authenticator> in the config.

I think what my patch does is it says run the 'next unless' test if
there is an $authenticator->org_units regardless of if there is an
$args->{'org'}

But, maybe I'm reading it wrong or there is a better way.

John

[Dan Wells (dbw2)]

John, thank you for providing the requested details. I will make every effort to push on this and see what I can make of it.

"I think the result of the original code is that the system may try all authenticators if there is not an $args->{'org'} regardless of if there is an $authenticator->org_units set."

I fully grant that being the author of this code does not guarantee I still understand it :) That said, this behavior is intentional. The system is designed to be inclusive by default, running every authenticator in the absence of more selective parameters (either 'org' or 'type'). In other words, passing in 'org' is a way to *skip* (filter) some authenticators, not to include certain ones. We could invert the logic, but it was written in this direction for ease of use and compatibility reasons.

Making the LDAP authenticator fail gracefully is worth doing in any case, and with fewer consequences to our current functional promises.

[John Merriam (jmerriam)]

That's fine, we'll just carry around a local patch to invert the behavior.

Once I discovered this, my thinking also gravitates to the security aspect
in that in addition to needing to correct the bug, I also don't want to be
sending off any login credentials to an LDAP server that I don't have
control over unless they are really supposed to go to that server.

Anyway, if you need me to test any patches, let me know.

[Jeff Davis (jdavis-sitka)]

I'm marking this bug as High priority since it can break login globally.

My consortium is in a similar situation to John's. We have one library that wants to use LDAP, and the rest should always use native login. The current inclusive-by-default behavior, where every authenticator is applied when no org argument is supplied, is highly undesirable for us. There should be a setting that governs whether auth_proxy is inclusive or exclusive. I'll see if I can put together a branch for this.

Regarding org-less logins, I presume that initial staff client login (prior to registering a workstation) does not have an org parameter? So if you're not running all authenticators by default, you'd need to use native login in order to register a workstation. That's a reasonable tradeoff for my organization, but it would be cool if we had a way to supply an org param for that use case.

I agree with Dan that LDAP should fail gracefully regardless.

[Jeff Davis (jdavis-sitka)]

Well, this branch should preserve the current default behavior while giving the option to skip an authenticator when no context org is supplied:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jeffdavis/lp1715396-non-inclusive-authproxy

However, the new code is even more confusing than the original code. Maybe that means we should re-think what we're trying to achieve here? (Or maybe I just need to call it a day...)