|Summary:||Failed LDAP connection triggers lockout realm|
|Product:||Tomcat 8||Reporter:||Ben <ben>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
isUp idea from comment 9
Description Ben 2016-10-04 15:45:54 UTC
Comment 1 Mark Thomas 2016-10-04 16:05:52 UTC
Given the way that the LockOutRealm works (as a Realm wrapper with no insight into the inner workings of the Realm it wraps) there is no easy way to fix this as it has no way to dertermine why an authentication has failed. Additionally, most LDAP services implement their own lock out policy for failed passwords so use of the LockOutRealm may not be necessary in this case.
Comment 2 Christopher Schultz 2016-10-04 17:02:48 UTC
Perhaps the Realm interface could be adjusted to throw a variety of exceptions: - AuthenticationException (triggers lock-out) - CommunicationException (does not trigger lock-out) Honestly, if a Realm throws an exception during authentication, I would think that should always be considered a transient failure and NOT trigger lock-out. If the LDAPRealm is returning <false> when a communications exception occurs, I think it should instead throw an exception.
Comment 3 Remy Maucherat 2016-10-04 17:17:02 UTC
The realm doesn't and shouldn't throw exceptions at all :) Look at the lock out realm code.
Comment 4 Christopher Schultz 2016-10-04 18:41:41 UTC
(In reply to Remy Maucherat from comment #3) > The realm doesn't and shouldn't throw exceptions at all :) Look at the lock > out realm code. How should the Realm behave if there is a communications error? I assert that throwing an exception is a perfectly reasonable thing to do. The current Realm code is irrelevant. It will behave however we want it to behave. I'm suggesting a change to the behavior, not asking why it doesn't behave in a way that it wasn't programmed to do. It's not LockOutRealm that you need to read, it's LDAPRealm, which shows that it catches exceptions like CommunicationException (a transient, non-authentication-related problem) and returns null which makes it look the same as an authentication problem, which is isn't.
Comment 5 Remy Maucherat 2016-10-04 18:46:48 UTC
So I want it to behave the way it does, mostly, and especially not fail requests like that. -1
Comment 6 Ben 2016-10-06 14:39:59 UTC
I agree with Christopher here. Possibly this is a two-part fix, one to adjust the LDAPRealm to treat connection failures differently than authentication failures, and one to allow the LockoutRealm to know the difference.
Comment 7 Remy Maucherat 2016-10-06 14:47:40 UTC
This is not useful, we can understand all you care about is having your issue fixed, regardless of the collateral damage. Exception propagation is clearly not the right way to do it, since the request should go on without an authenticated user and then generate a response according to the security constraints (if any). Something that could be possible: return a singleton token principal in case of some failure that is not an auth failure. The downside is that it's a hack that is risky, and I'm not convinced it is worth trying.
Comment 8 Ben 2016-10-06 15:21:33 UTC
I'm not asking for a hack fix and I'm not asking for a fix that might break something down the line. I'm asking for an improvement that will make the application work better implemented in whatever way the development community decides is best for the project.
Comment 9 Remy Maucherat 2016-10-06 16:34:01 UTC
It's not magical and at this point the thinking is that we have to change something and it would be problematic. Something that could work better is having a "Realm.isUp" method or something like this. That's not quite the way this realm is designed to work with its connection at the moment.
Comment 10 Christopher Schultz 2016-10-08 19:33:47 UTC
Remy, I just want to take the opportunity to point out that, in *this* bug you claim that throwing an IOException (essentially what is happening, here) would be some kind of major moral violation yet in bug #59876 you argue that "hey, sometimes IOExceptions happen and you just have to deal with them". It sounds like your position is that code should never change after its been written. -1 to your attitude :( I'm reopening this bug for further discussion.
Comment 11 Remy Maucherat 2016-10-10 06:38:49 UTC
I am not the one who closed this report in the first place, hence my -1 is seconded. You don't need to reopen a bug to have further discussion as well. Your argument on exceptions doesn't make sense. In realms, the authentication must look as if it fails to see what the request will do, regardless on what happens in the realm. Elsewhere, exception are propagated. Async, as in the other report, is something else, the user component receives the exception and can do anything with it (so it is an opportunity for processing, logging, etc).
Comment 12 Remy Maucherat 2016-10-10 13:00:37 UTC
Created attachment 34352 [details] isUp idea from comment 9 Using LifecycleState.FAILED is also a possibility but it could have side effects so I didn't try it.
Comment 13 Remy Maucherat 2016-10-21 08:53:00 UTC
Created attachment 34396 [details] v2 I still had the changes locally, so I added the checks to the other similar realms. It's not accurate for a transient connection failure, but it should be good enough since the only consumer of this is the lockout realm.
Comment 14 Ben 2016-10-22 01:31:56 UTC
That looks like a pretty elegant solution. How would it interact with third-party realms?
Comment 15 Remy Maucherat 2016-10-24 13:56:56 UTC
(In reply to Ben from comment #14) > That looks like a pretty elegant solution. How would it interact with > third-party realms? A third party realm would have to implement the new API method.
Comment 16 Ben 2016-10-25 20:52:19 UTC
(In reply to Remy Maucherat from comment #15) > (In reply to Ben from comment #14) > > That looks like a pretty elegant solution. How would it interact with > > third-party realms? > > A third party realm would have to implement the new API method. Last question (bear with me since I'm not a developer): Am I correct that the default isAvailable() you added in Realm.java would return true if a 3rd-party realm did not have its own isAvailable() method? If so, this looks like minimal fallout and a good solution to my problem. I'll defer to the community on whether to accept the change but it has my vote.
Comment 17 Remy Maucherat 2016-11-09 09:59:49 UTC
I included the change for 8.5.9 and 9.0.0.M14. I don't plan to backport to earlier versions since it's an API change.