Request for comment on BZ 59750 (authentication listener)

[Christopher Schultz <ch...@christopherschultz.net>]

All,

For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750

I've got a proposal (in patch form) attached to that BZ issue.

Ralf's enhancement request is fairly terse, but this is something I'd
like to have as well.

The requirement is to be able to log failed authentication attempts. As
it stands, using container-managed authentication does not allow this
(as far as I can tell).

My proposal is essentially a new listener interface that authenticator
classes will invoke (if registered) when an authentication event occurs
(success or failure). The Request object and username are currently
arguments to the two methods on the interface.

You can read the entire current path without even scrolling your screen.

Before attempting to publish a more complete patch, I wanted to know if
there was any appetite for this kind of thing, or any objections.

Thanks,
-chris

Re: Request for comment on BZ 59750 (authentication listener)

[Coty Sutherland <cs...@redhat.com>]

On Thu, Mar 29, 2018 at 11:41 AM, Rémy Maucherat <re...@apache.org> wrote:
> On Thu, Mar 29, 2018 at 3:48 PM, Christopher Schultz <
> chris@christopherschultz.net> wrote:
>
>> All,
>>
>> For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750
>>
>> I've got a proposal (in patch form) attached to that BZ issue.
>>
>> Ralf's enhancement request is fairly terse, but this is something I'd
>> like to have as well.
>>
>> The requirement is to be able to log failed authentication attempts. As
>> it stands, using container-managed authentication does not allow this
>> (as far as I can tell).
>>
>> My proposal is essentially a new listener interface that authenticator
>> classes will invoke (if registered) when an authentication event occurs
>> (success or failure). The Request object and username are currently
>> arguments to the two methods on the interface.
>>
>> You can read the entire current path without even scrolling your screen.
>>
>> Before attempting to publish a more complete patch, I wanted to know if
>> there was any appetite for this kind of thing, or any objections.
>>
>
> Ok with the idea, but the patch is indeed very incomplete.

+1, I like the idea too.

>
> Rémy
>
>>
>> Thanks,
>> -chris
>>
>>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Request for comment on BZ 59750 (authentication listener)

[Christopher Schultz <ch...@christopherschultz.net>]

Mark,

On 3/29/18 4:11 PM, Mark Thomas wrote:
> On 29/03/18 19:07, Christopher Schultz wrote:
>> Rémy,
>>
>> On 3/29/18 11:41 AM, Rémy Maucherat wrote:
>>> On Thu, Mar 29, 2018 at 3:48 PM, Christopher Schultz <
>>> chris@christopherschultz.net> wrote:
>>>
>>>> All,
>>>>
>>>> For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750
>>>>
>>>> I've got a proposal (in patch form) attached to that BZ issue.
>>>>
>>>> Ralf's enhancement request is fairly terse, but this is something I'd
>>>> like to have as well.
>>>>
>>>> The requirement is to be able to log failed authentication attempts. As
>>>> it stands, using container-managed authentication does not allow this
>>>> (as far as I can tell).
>>>>
>>>> My proposal is essentially a new listener interface that authenticator
>>>> classes will invoke (if registered) when an authentication event occurs
>>>> (success or failure). The Request object and username are currently
>>>> arguments to the two methods on the interface.
>>>>
>>>> You can read the entire current path without even scrolling your screen.
>>>>
>>>> Before attempting to publish a more complete patch, I wanted to know if
>>>> there was any appetite for this kind of thing, or any objections.
> 
> I think this is better than changing the existing Realm.authenticate(*)
> methods to add Request.

Yes, I like this better, too. The separation-of-concerns is
architecturally appropriate.

> My biggest concern at this point is how does the listener get
> registered?

Why not attach it to the Authenticator valve? Most people don't
explicitly declare their authenticator valve and let Tomcat auto-select
based upon the auth-method. But most people don't need this new listener
(not many people have mentioned this before, which is surprising ...
maybe everyone's been using Spring Security forever), but if they do,
they'll have to manually-configure the Valve.

> Might this be better as ContainerListener and two new
> events? That reduces the plumbing required to make this work.

That would definitely require a dependency upon Tomcat code. I think
it's worth the trade-off of plumbing-code versus dependencies.

>>> Ok with the idea, but the patch is indeed very incomplete.
>>
>> Absolutely. I wanted to make sure there were no -1s before I spent much
>> time on it. That represents maybe 5 minutes of work :)
>>
>> Some specific questions:
>>
>> For FormAuthenticator: there are several calls to authenticate() in
>> doAuthenticate. I chose to ignore the call at the top of the method
>> because I didn't understand the purpose. Something about
>> re-authentication of a previous-authentication? Would it be appropriate
>> to also fire an authentication event at that time?
> 
> I don't think so.

Okay, thanks for the confirmation.

>> For Basic/DigestAuthenticator: since technically the user is being
>> authenticated at *every* request, should we bother trying to avoid
>> spamming the Listener, or should we let the Listener decide how to
>> handle the huge number of events it will likely get? Does Tomcat know
>> whether the authentication is a re-authentication or not? If so, should
>> it let the Listener know this is a re-authentication?
> 
> Let the listener solve that problem. There may be different ways that
> are appropriate for different use cases (client IP, session ID, etc.)

+1

This was my nominal position as well... especially because it's less
work in Tomcat.

>> Implementing a listener in a webapp: can a listener be registered from
>> within the web application without any ClassLoader weirdness?
> 
> Yes.

I was thinking about something that needed to depend upon the Tomcat
API, but ...

>> What options exist for writing a listener that doesn't require a
>> compile-time dependency on Tomcat? (I suspect this is unavoidable,
>> because even if reflection is used to invoke the listener's method
>> by *name* and not via an interface-call, the Tomcat-specific
>> Request class is a parameter to the interface methods.
> 
> Use HttpServletRequest? Or do you need access to Tomcat specific internals?

I don't see a particular need to access Tomcat-specific internals at
this point. I was just being lazy with the initial patch I guess.

Thanks,
-chris

Re: Request for comment on BZ 59750 (authentication listener)

[Mark Thomas <ma...@apache.org>]

On 29/03/18 19:07, Christopher Schultz wrote:
> Rémy,
> 
> On 3/29/18 11:41 AM, Rémy Maucherat wrote:
>> On Thu, Mar 29, 2018 at 3:48 PM, Christopher Schultz <
>> chris@christopherschultz.net> wrote:
>>
>>> All,
>>>
>>> For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750
>>>
>>> I've got a proposal (in patch form) attached to that BZ issue.
>>>
>>> Ralf's enhancement request is fairly terse, but this is something I'd
>>> like to have as well.
>>>
>>> The requirement is to be able to log failed authentication attempts. As
>>> it stands, using container-managed authentication does not allow this
>>> (as far as I can tell).
>>>
>>> My proposal is essentially a new listener interface that authenticator
>>> classes will invoke (if registered) when an authentication event occurs
>>> (success or failure). The Request object and username are currently
>>> arguments to the two methods on the interface.
>>>
>>> You can read the entire current path without even scrolling your screen.
>>>
>>> Before attempting to publish a more complete patch, I wanted to know if
>>> there was any appetite for this kind of thing, or any objections.

I think this is better than changing the existing Realm.authenticate(*)
methods to add Request.

My biggest concern at this point is how does the listener get
registered? Might this be better as ContainerListener and two new
events? That reduces the plumbing required to make this work.

>> Ok with the idea, but the patch is indeed very incomplete.
> 
> Absolutely. I wanted to make sure there were no -1s before I spent much
> time on it. That represents maybe 5 minutes of work :)
> 
> Some specific questions:
> 
> For FormAuthenticator: there are several calls to authenticate() in
> doAuthenticate. I chose to ignore the call at the top of the method
> because I didn't understand the purpose. Something about
> re-authentication of a previous-authentication? Would it be appropriate
> to also fire an authentication event at that time?

I don't think so.

> For Basic/DigestAuthenticator: since technically the user is being
> authenticated at *every* request, should we bother trying to avoid
> spamming the Listener, or should we let the Listener decide how to
> handle the huge number of events it will likely get? Does Tomcat know
> whether the authentication is a re-authentication or not? If so, should
> it let the Listener know this is a re-authentication?

Let the listener solve that problem. There may be different ways that
are appropriate for different use cases (client IP, session ID, etc.)

> Implementing a listener in a webapp: can a listener be registered from
> within the web application without any ClassLoader weirdness?

Yes.

> What
> options exist for writing a listener that doesn't require a compile-time
> dependency on Tomcat? (I suspect this is unavoidable, because even if
> reflection is used to invoke the listener's method by *name* and not via
> an interface-call, the Tomcat-specific Request class is a parameter to
> the interface methods.

Use HttpServletRequest? Or do you need access to Tomcat specific internals?

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Request for comment on BZ 59750 (authentication listener)

[Christopher Schultz <ch...@christopherschultz.net>]

Rémy,

On 3/29/18 11:41 AM, Rémy Maucherat wrote:
> On Thu, Mar 29, 2018 at 3:48 PM, Christopher Schultz <
> chris@christopherschultz.net> wrote:
> 
>> All,
>>
>> For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750
>>
>> I've got a proposal (in patch form) attached to that BZ issue.
>>
>> Ralf's enhancement request is fairly terse, but this is something I'd
>> like to have as well.
>>
>> The requirement is to be able to log failed authentication attempts. As
>> it stands, using container-managed authentication does not allow this
>> (as far as I can tell).
>>
>> My proposal is essentially a new listener interface that authenticator
>> classes will invoke (if registered) when an authentication event occurs
>> (success or failure). The Request object and username are currently
>> arguments to the two methods on the interface.
>>
>> You can read the entire current path without even scrolling your screen.
>>
>> Before attempting to publish a more complete patch, I wanted to know if
>> there was any appetite for this kind of thing, or any objections.
>>
> 
> Ok with the idea, but the patch is indeed very incomplete.

Absolutely. I wanted to make sure there were no -1s before I spent much
time on it. That represents maybe 5 minutes of work :)

Some specific questions:

For FormAuthenticator: there are several calls to authenticate() in
doAuthenticate. I chose to ignore the call at the top of the method
because I didn't understand the purpose. Something about
re-authentication of a previous-authentication? Would it be appropriate
to also fire an authentication event at that time?

For Basic/DigestAuthenticator: since technically the user is being
authenticated at *every* request, should we bother trying to avoid
spamming the Listener, or should we let the Listener decide how to
handle the huge number of events it will likely get? Does Tomcat know
whether the authentication is a re-authentication or not? If so, should
it let the Listener know this is a re-authentication?

Implementing a listener in a webapp: can a listener be registered from
within the web application without any ClassLoader weirdness? What
options exist for writing a listener that doesn't require a compile-time
dependency on Tomcat? (I suspect this is unavoidable, because even if
reflection is used to invoke the listener's method by *name* and not via
an interface-call, the Tomcat-specific Request class is a parameter to
the interface methods. Changing that to "Object" kind of defeats the
purpose of the interface.)

Thanks,
-chris

Re: Request for comment on BZ 59750 (authentication listener)

[Rémy Maucherat <re...@apache.org>]

On Thu, Mar 29, 2018 at 3:48 PM, Christopher Schultz <
chris@christopherschultz.net> wrote:

> All,
>
> For reference: https://bz.apache.org/bugzilla/show_bug.cgi?id=59750
>
> I've got a proposal (in patch form) attached to that BZ issue.
>
> Ralf's enhancement request is fairly terse, but this is something I'd
> like to have as well.
>
> The requirement is to be able to log failed authentication attempts. As
> it stands, using container-managed authentication does not allow this
> (as far as I can tell).
>
> My proposal is essentially a new listener interface that authenticator
> classes will invoke (if registered) when an authentication event occurs
> (success or failure). The Request object and username are currently
> arguments to the two methods on the interface.
>
> You can read the entire current path without even scrolling your screen.
>
> Before attempting to publish a more complete patch, I wanted to know if
> there was any appetite for this kind of thing, or any objections.
>

Ok with the idea, but the patch is indeed very incomplete.

Rémy

>
> Thanks,
> -chris
>
>