[Bug 64715] New: PasswordValidationCallback not supported

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[Bug 64715] New: PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

            Bug ID: 64715
           Summary: PasswordValidationCallback not supported
           Product: Tomcat 9
           Version: 9.0.37
          Hardware: PC
                OS: All
            Status: NEW
          Severity: minor
          Priority: P2
         Component: JASPIC
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: -----

The JASPIC 1.1 specification (section 4.9.2) requires a runtime to provide a
CallbackHandler that supports the PasswordValidationCallback. This callback is
not implemented in Tomcat.

I would like to provide a patch for this, but would like to check some details
first.

The callback has to be implemented in the CallbackHandlerImpl. This is
relatively straightforward but as we need the realm associated with the current
context to be able to check the password it can't stay a singleton.

So what I propose:
- change CallbackHandlerImpl from singleton to standard class (one per context)
- add parameter to constructor to pass the current context to the handler (not
the realm because this would break changing the associated realm through JMX)
- update initialization code in AuthenticatorBase accordingly
- implement the callback by calling context.getRealm().authenticate(user, pass)

(optional)
- when dynamic initialization of a CallbackHandler is used (see
jaspicCallbackHandlerClass config parameter of AuthenticatorBase), use
introspection to search for a "setContext" and pass the context to the handler

Any comments are wellcome.

Questions:
- Should I check some annotations (e.g. @Ressource) for the injection of the
context in case of dynamic instantiation?
- How about instantiating the default CallbackHandler the same way as the
dynamic class (no duplicate instantiation code, only a default class name)?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #1 from Robert Rodewald <[hidden email]> ---
Created attachment 37434
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37434&action=edit
Proposed patch for bug

- CallbackHandlerImpl changed from singleton to regular class
- added parameter context in constructor of CallbackHandlerImpl
- implemented PasswordValidationCallback in CallbackHandlerImpl
- updated initialization code for the callbackHandler in AuthenticatorBase
- removed direct initialization of CallbackHandlerImpl

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Robert Rodewald <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37434|Proposed patch for bug      |Proposed patch for bug
        description|                            |64715

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|minor                       |enhancement

--- Comment #2 from Mark Thomas <[hidden email]> ---
Section 4.9.2 is part of the SOAP profile. Tomcat only targets the Servlet
Container profile. Looking at the requirements for the SOAP profile, it does
not look to be directly implementable in Tomcat. Therefore, I am wondering what
is the purpose of this enhancement?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #3 from Robert Rodewald <[hidden email]> ---
Sorry, I got the section number wrong, it's section 3.5

Chapter 3 is Servlet Container Profile.

Here is an excerpt from section 3.5:
The CallbackHandler passed to ServerAuthModule.initialize is determined by the
handler argument passed in the AuthConfigProvider.getServerAuthConfig call that
acquired the corresponding authentication context configuration object. The
handler argument must not be null, and the argument handler and the
CallbackHandler passed to ServerAuthModule.initialize MUST support the
following callbacks:

• CallerPrincipalCallback
• GroupPrincipalCallback
• PasswordValidationCallback


So it is a bug, if Tomcat claims to be JASPIC 1.1 compatible in my opinion.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|enhancement                 |normal

--- Comment #4 from Mark Thomas <[hidden email]> ---
Yes, that does make it a bug.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #5 from Mark Thomas <[hidden email]> ---
Reviewing the patch:
- It doesn't handle all combinations of
  - Constructor with/without Context
  - Class defined in web app / in container
- The call to the "with Context" constructor will always fail (no Context)
- Use of an interface would be cleaner. The Contained interface is a good fit.

The rest looks good.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #6 from Robert Rodewald <[hidden email]> ---
(In reply to Mark Thomas from comment #5)

> - It doesn't handle all combinations of
>   - Constructor with/without Context
I don't think a constructor for CallbackHandlerImpl without Context is needed.
As this is a Tomcat internal class and AuthenticatorBase can always provide a
context with realm (this is stated in the usage constraints of
AuthenticatorBase). Or am I misinterpreting you?

>   - Class defined in web app / in container
That's a case I don't seem to understand. Could you please explain?

> - The call to the "with Context" constructor will always fail (no Context)
Ugh. Missing argument. I'll change this when the other fixes are clear.

> - Use of an interface would be cleaner. The Contained interface is a good
> fit.
Are you suggesting to pass the Realm (which is a Contained) to the
CallbackHandler? Wouldn't that break dynamic configuration through JMX? I could
use Container though.

I also saw that minor modifications to the tests will be necessary too. In
addition I would add one or two tests for the Callbacks.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #7 from Mark Thomas <[hidden email]> ---
Users may wish to use a 3rd party custom CallbackHandler that knows nothing
about Tomcat internals. A no-arg Constructor needs to be supported.

There are multiple class loaders involved and while the default configuration
avoids most of the complexities, the non-default configs need to be handled.

Some users reconfigure the class loaders so they look like this:
http://tomcat.apache.org/tomcat-4.1-doc/class-loader-howto.html

and may want to put the custom CallbackHandler in the Catalina loader. This
boils down to you need to try and load the specified class first with the web
app class loader (TCCL) and then with the class loader that loaded the current
class.

For adding the Context I'm suggesting something like:

if (callbackHandler instanceof Contained) {
    ((Contained) callbackHandler).setContainer(context);
}

Tomcat can then do:
if (callbackHandler instanceof Contained) {
    getContainer().getRealm()...
}

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Robert Rodewald <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37434|0                           |1
        is obsolete|                            |

--- Comment #8 from Robert Rodewald <[hidden email]> ---
Created attachment 37438
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37438&action=edit
Proposed patch for bug 64715 (second attempt)

Changes to the first version:

- added Contained interface to CallbackHandlerImpl and changed access logic
- implemented check for Contained interface instead of different constructors
in AuthenticatorBase
- added test cases for the three supported Callbacks


A questions that occurred to me:
- Should we throw UnsupportedCallbackException for unsupported callbacks
instead of ignoring them? This breaks compatibility but would enable the user
to check for unimplemented callbacks.

The use of the Contained interface is definitely a big plus. Thanks for the
great feedback Mark.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #9 from Robert Rodewald <[hidden email]> ---
Slightly off topic, but could someone explain why the package imports in
CallbackHandlerImpl switched from

import javax.security.auth.callback.UnsupportedCallbackException;
import javax.security.auth.message.callback.CallerPrincipalCallback;
import javax.security.auth.message.callback.GroupPrincipalCallback;

in Tomcat 9 to the corresponding jakarta.security.auth equivalents in Tomcat
10? Is that a JASPIC 2.0 change?

Would we have to support both in Tomcat 10 so that the implementation is
backwards compatible or am I mislead? I stumbled upon this while implementing
the tests for my patch and I am quite sure that my own SAM implementation
written for Tomcat 9 would not work with Tomcat 10 because of this.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Robert Rodewald <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37438|Proposed patch for bug      |Proposed patch for bug
        description|64715 (second attempt)      |64715 (second attempt),
                   |                            |Tomcat 10

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

Robert Rodewald <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #37438|0                           |1
        is obsolete|                            |

--- Comment #10 from Robert Rodewald <[hidden email]> ---
Created attachment 37439
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37439&action=edit
Proposed patch for bug 64715 (second attempt), Tomcat 10

- corrected a spelling error in Locale.properties

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #11 from Robert Rodewald <[hidden email]> ---
Created attachment 37440
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37440&action=edit
Proposed patch for bug 64715 (second attempt), Tomcat 9

The Tomcat 9.0.x version of the patch.

Main difference is the difference in the package names for the callbacks.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #12 from Christopher Schultz <[hidden email]> ---
(In reply to Mark Thomas from comment #7)

> Users may wish to use a 3rd party custom CallbackHandler that knows nothing
> about Tomcat internals. A no-arg Constructor needs to be supported.
>
> There are multiple class loaders involved and while the default
> configuration avoids most of the complexities, the non-default configs need
> to be handled.
>
> Some users reconfigure the class loaders so they look like this:
> http://tomcat.apache.org/tomcat-4.1-doc/class-loader-howto.html
>
> and may want to put the custom CallbackHandler in the Catalina loader. This
> boils down to you need to try and load the specified class first with the
> web app class loader (TCCL) and then with the class loader that loaded the
> current class.
>
> For adding the Context I'm suggesting something like:
>
> if (callbackHandler instanceof Contained) {
>     ((Contained) callbackHandler).setContainer(context);
> }
>
> Tomcat can then do:
> if (callbackHandler instanceof Contained) {
>     getContainer().getRealm()...
> }

Doesn't this tie the implementation class to Tomcata internals? It would be
nice to implement a CallbackHandler which can be built (and run) independently
of Tomcat classes. Or do I have an unrealistic expectation, here, of the way
CallbackHandlers would be implemented? Is it possible for them to be
container-agnostic?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #13 from Christopher Schultz <[hidden email]> ---
(In reply to Robert Rodewald from comment #9)
> Slightly off topic, but could someone explain why the package imports in
> CallbackHandlerImpl switched from
>
> import javax.security.auth.callback.UnsupportedCallbackException;
> import javax.security.auth.message.callback.CallerPrincipalCallback;
> import javax.security.auth.message.callback.GroupPrincipalCallback;
>
> in Tomcat 9 to the corresponding jakarta.security.auth equivalents in Tomcat
> 10? Is that a JASPIC 2.0 change?

I don't know about the JASPIC version implications, but Java EE recently change
to Jakarta EE an all the package names changed for their forthcoming versions.
Presumably, this includes JASPIC because it's included in the Java EE
specifications which have been "migrated" to the new Jakarta umbrella. [1]

For all the affected APIs, Tomcat has basically done a search-and-replace for
javax.(api).whatever to jakarta.(api).whatever.

It is not (currently, maybe ever) expected that web applications written for
Tomcat 9 can be directly run under Tomcat 10: they will require migration to
the new API package names. That includes all servlets, etc.[2]

There is an experimental migration tool available[3] which aims to migrate your
web applications from Java EE to Jakarta EE. I'd be interested to see if it
works for you and gives you artifacts you can successfully deploy into Tomcat
10.

[1] https://jakarta.ee/specifications/authentication/1.1/apidocs/
[2] http://tomcat.apache.org/migration-10.html#Specification_APIs
[3] https://github.com/apache/tomcat-jakartaee-migration

> Would we have to support both in Tomcat 10 so that the implementation is
> backwards compatible or am I mislead?

I don't believe any backward-compatibility would be expected. We toyed with
that idea, but it seems like a maintenance nightmare so we decided not to do
it.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #14 from Robert Rodewald <[hidden email]> ---
(In reply to Christopher Schultz from comment #12)
> Doesn't this tie the implementation class to Tomcata internals? It would be
> nice to implement a CallbackHandler which can be built (and run)
> independently of Tomcat classes. Or do I have an unrealistic expectation,
> here, of the way CallbackHandlers would be implemented? Is it possible for
> them to be container-agnostic?

In my understanding the CallbackHandler cannot be decoupled from Tomcat (the
"runtime" in the JASPIC specification), because it represents the "glue code"
to the runtime. A replacement of the implementing class is not intended
anywhere in the JASPIC specification as far as I can see, but is a nice thing
to have. This way the user can implement some very special non-standard
Callbacks.

The inner workings of the CallbackHandler are not well defined in the specs,
neither are the standard callbacks.

For example the PasswordValidationCallback:
- Shall it set the principal in the client subject or shall it just return true
or false for the getResult method?
- The password field may be null (as per spec) but how should the
CallbackHandler handle this case? Set the roles/groups of the principal anyway?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #15 from Robert Rodewald <[hidden email]> ---
I found an interesting differentiation on this page:
https://github.com/wildfly/wildfly/blob/master/docs/src/main/asciidoc/_elytron/Elytron_and_Java_Authentication_SPI_for_Containers-JASPI.adoc

If a SAM requires access to the configured identity management (the realm) of
the runtime is uses "integrated" mode (this would be a CallbackHandler
implementation that implements Contained).

If it establishes the identity and roles of the user by itself it can use
"non-itegrated" mode (no access to the realm) and is what was supported by
Tomcat before the patch (CallbackHandler does not implement Contained and has
no access to the Realm).

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #16 from Mark Thomas <[hidden email]> ---
All valid points regarding the expected behaviour of CallbackHandlers. I'd
recommend raising issues against the Jakarta Authentication spec:
https://github.com/eclipse-ee4j/authentication/issues

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 64715] PasswordValidationCallback not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=64715

--- Comment #17 from Robert Rodewald <[hidden email]> ---
(In reply to Mark Thomas from comment #16)
> All valid points regarding the expected behaviour of CallbackHandlers. I'd
> recommend raising issues against the Jakarta Authentication spec:
> https://github.com/eclipse-ee4j/authentication/issues

I raised the issue. Here is the link:
https://github.com/eclipse-ee4j/authentication/issues/110

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12