You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@tomcat.apache.org by Carsten Klein <c....@datagis.com> on 2021/05/26 11:00:43 UTC
Enhancement: Additional user attributes queried by (some) realms
Hi there,
as already discussed here:
http://mail-archives.apache.org/mod_mbox/tomcat-users/202104.mbox/ajax/%3Cb9a2a913-f00f-f5bf-ca05-8ea4f8663ca9%40datagis.com%3E
I'm implementing an enhancement for querying configurable extra user
attributes through some of the Realm classes from the "user database"
configured for that Realm.
Before starting off, I want do clarify some questions.
The implementation basically of consists two parts. One is to enable the
GenericPrincipal class (and/or TomcatPrincipal interface) to hold these
additional user attributes. The other part is to query these attributes
from the Realm's configured user database an to store these in the newly
created Principal instance.
The second part is quite clear and changes are only local to the Realm
classes. But some decisions must be made for the first part.
The Principal's attributes will be stored in a Map<String, Object> hash
map. Since attributes may come from various sources, using an
Object-typed value seems appropriate to me. That map will also be added
to internal class SerializablePrincipal in order to be persisted properly.
Now, my questions:
1. How to access the Principal's new attributes
Simplest is to provide a getter method, that actually returns the map
(optionally with a read-only parameter):
/**
* The set of additional attributes associated with the user represented
* by this Principal.
*/
protected final Map<String, Object> attributes;
/**
* Returns the user's additional attributes as a Map.
*
* @param readonly
* if <code>true</code>, the returned map is an unmodifiable
* view of the map; otherwise the underlying map is returned
* unchanged
* @return the user's additional attributes as an unmodifiable Map
*/
public Map<String, Object> getAttributes(boolean readonly) {
if (readonly) {
return Collections.unmodifiableMap(attributes);
}
return attributes;
}
/**
* Returns the user's additional attributes as an unmodifiable Map.
*
* @return the user's additional attributes as an unmodifiable Map
*/
public Map<String, Object> getAttributes() {
return getAttributes(true);
}
However, in the jakarta.servlet classes, there are also some entities
that have attributes. There, attributes are accessed by these methods:
Object getAttribute(String name);
Enumeration<String> getAttributeNames();
void setAttribute(String name, Object o);
Would you, for the sake of uniformity, prefer using these more
servlet-spec like methods?
2. Should I add attributes-related public methods to the TomcatPrincipal
interface as well?
According to the documentation of TomcatPrincipal I likely should:
Defines additional methods implemented by {@link Principal}s created by
Tomcat's standard {@link Realm} implementations.
However, this interface seems only being used with GSS (most other
instanceof tests are performed against GenericPrincipal).
So, I guess the attributes stuff should better NOT be added to
TomcatPrincipal?
3. Class UserDatabasePrincipal in UserDatabaseRealm
Although neither documented nor used in Tomcat's shipped standard
tomcat-users.xml, the UserDatabaseRealm supports the additional
attribute "fullname" for a user entry. This makes this Realm a good
candidate for supporting additional user attributes (actually, the
authenticated user's display name was the initial intention for this
enhancement).
The UserDatabaseRealm creates a Principal with an explicitly set
userPrincipal (why? e.g. DataSourceRealm and JNDIRealm don't) of type
UserDatabasePrincipal. The latter does not extend GenericPrincipal class
and so, will not have the new attributes-related methods by default.
However, since Request.getUserPrincipal returns that "inner"
userPrincipal by calling getUserPrincipal(), attributes should be
available for that UserDatabasePrincipal as well.
My Questions:
Why does UserDatabaseRealm pass a userPrincipal of type
UserDatabasePrincipal? Can't we just drop that and do it like JNDIRealm
or DataSourceRealm?
If not, I need to add the attributes-related methods to the
UserDatabasePrincipal as well.
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Chris,
On 28/05/2021 23:16, Christopher Schultz wrote:
</snip>
> Yeah, about that...
>
> https://openjdk.java.net/jeps/411
>
> IMO this is a Bad Thing for Java. If someone was looking for a reason to
> abandon the whole Java ecosystem, this would be it. Well, we had a good
> run.
>
> Now we can all run node.js, Python, or Go where security is not a
> problem because the languages are "safe" so nothing Bad can happen,
> right? *facepalm*
Safe all the way down... including type safety :-p
</snip>
> For now, Tomcat can rely on the SecurityManager doing its job. That
> means we only need to rely on the encapsulation strategies the language
> and the standard library provide, which are (currently) sufficient.
I will try to come up with a solution that uses defensive copying
whenever possible. That will include a couple of hard-coded special
cases as well as try to use Serializable if available. I will not
consider Cloneable due to the risk of shallow copies.
If none of the tried methods will work, getAttribute(String name) should
return the result of the object's toString() method. We'll likely
"loose" some of the requested attributes that way but, need not care
about preventing reflection with a (soon missing) SecurityManager.
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/28/21 04:13, Mark Thomas wrote:
> On 28/05/2021 07:22, Carsten Klein wrote:
>>
>> Chris, Mark,
>>
>> On 27/05/2021 22:11, Christopher Schultz wrote:
>>
>> </snip>
>>
>> After re-reading this, you mentioned reflection while asking how much
>> we trust in Collections.unmodifiableMap(). I didn't get that right, my
>> bad.
>>
>> However, I thought of reflection in order to implement a deep copy
>> mechanism. Maybe anyone already coded one on jpm? :)
>>
>>> If we return Collections.unmodifiableMap() to return
>>> Map<String,String> that makes things simpler. But how much do we
>>> trust Collections.unmodifiableMap if the underlying data are
>>> security-sensitive? With reflection, is it possible to monkey your
>>> way through the references and swap-out the underlying references?
>>> That might be Bad.
>>
>> The question is, what kind of security problem are we actually trying
>> to prevent? It is clearly not leak of information, since the
>> administrator is responsible for what attributes are configured to be
>> gathered. The administrator should clearly not provide sensitive
>> information that should not be seen by the public. If so, his bad :-p
>>
>> Needless to say that I plan to deny querying passwords with that new
>> Realm feature? Attribute names, for which I know they refer to fields
>> containing passwords (like whats set for DataSourceRealm's option
>> userCredCol), will *never* be queried.
>>
>> So, security here is about preventing that someone can modify that
>> information during that Principal's lifetime. Can you modify an
>> immutable object with reflection? Not sure.
>
> You can. But. In a scenario where that is a risk you need to mitigate,
> you would be running under a SecurityManager which would block access to
> reflection.
Yeah, about that...
https://openjdk.java.net/jeps/411
IMO this is a Bad Thing for Java. If someone was looking for a reason to
abandon the whole Java ecosystem, this would be it. Well, we had a good run.
Now we can all run node.js, Python, or Go where security is not a
problem because the languages are "safe" so nothing Bad can happen,
right? *facepalm*
> Generally, the "running untrusted apps" risk isn't one that most users
> face but it a use case that Tomcat aims to support so we have to keep it
> in mind.
>
> In this case, ensuring that the caller can't modify the source data and
> thereby influence the result of future calls - e.g. by making defensive
> copies or ensuring objects are immutable - is sufficient to maintain
> Tomcat's security model.
+1
For now, Tomcat can rely on the SecurityManager doing its job. That
means we only need to rely on the encapsulation strategies the language
and the standard library provide, which are (currently) sufficient.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 28/05/2021 07:22, Carsten Klein wrote:
>
> Chris, Mark,
>
> On 27/05/2021 22:11, Christopher Schultz wrote:
>
> </snip>
>
> After re-reading this, you mentioned reflection while asking how much we
> trust in Collections.unmodifiableMap(). I didn't get that right, my bad.
>
> However, I thought of reflection in order to implement a deep copy
> mechanism. Maybe anyone already coded one on jpm? :)
>
>> If we return Collections.unmodifiableMap() to return
>> Map<String,String> that makes things simpler. But how much do we trust
>> Collections.unmodifiableMap if the underlying data are
>> security-sensitive? With reflection, is it possible to monkey your way
>> through the references and swap-out the underlying references? That
>> might be Bad.
>
> The question is, what kind of security problem are we actually trying to
> prevent? It is clearly not leak of information, since the administrator
> is responsible for what attributes are configured to be gathered. The
> administrator should clearly not provide sensitive information that
> should not be seen by the public. If so, his bad :-p
>
> Needless to say that I plan to deny querying passwords with that new
> Realm feature? Attribute names, for which I know they refer to fields
> containing passwords (like whats set for DataSourceRealm's option
> userCredCol), will *never* be queried.
>
> So, security here is about preventing that someone can modify that
> information during that Principal's lifetime. Can you modify an
> immutable object with reflection? Not sure.
You can. But. In a scenario where that is a risk you need to mitigate,
you would be running under a SecurityManager which would block access to
reflection.
Generally, the "running untrusted apps" risk isn't one that most users
face but it a use case that Tomcat aims to support so we have to keep it
in mind.
In this case, ensuring that the caller can't modify the source data and
thereby influence the result of future calls - e.g. by making defensive
copies or ensuring objects are immutable - is sufficient to maintain
Tomcat's security model.
Mark
> But, you could always use JNI and a C++ module to do nearly everything
> with the bytes in memory. Should we think that far? That would be a
> killer argument against using any sensitive data with Tomcat, Java or
> even computers in general :-)
>
> Carsten
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: users-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Chris, Mark,
On 27/05/2021 22:11, Christopher Schultz wrote:
</snip>
After re-reading this, you mentioned reflection while asking how much we
trust in Collections.unmodifiableMap(). I didn't get that right, my bad.
However, I thought of reflection in order to implement a deep copy
mechanism. Maybe anyone already coded one on jpm? :)
> If we return Collections.unmodifiableMap() to return Map<String,String>
> that makes things simpler. But how much do we trust
> Collections.unmodifiableMap if the underlying data are
> security-sensitive? With reflection, is it possible to monkey your way
> through the references and swap-out the underlying references? That
> might be Bad.
The question is, what kind of security problem are we actually trying to
prevent? It is clearly not leak of information, since the administrator
is responsible for what attributes are configured to be gathered. The
administrator should clearly not provide sensitive information that
should not be seen by the public. If so, his bad :-p
Needless to say that I plan to deny querying passwords with that new
Realm feature? Attribute names, for which I know they refer to fields
containing passwords (like whats set for DataSourceRealm's option
userCredCol), will *never* be queried.
So, security here is about preventing that someone can modify that
information during that Principal's lifetime. Can you modify an
immutable object with reflection? Not sure.
But, you could always use JNI and a C++ module to do nearly everything
with the bytes in memory. Should we think that far? That would be a
killer argument against using any sensitive data with Tomcat, Java or
even computers in general :-)
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Carsten,
On 5/28/21 01:48, Carsten Klein wrote:
> Chris, Mark,
>
> On 27/05/2021 22:11, Christopher Schultz wrote:
>
> </snip>
> </reordered>
>
>> What's the primary use-case for these kinds of attributes?
>
> This has been described in detail here:
>
> http://mail-archives.apache.org/mod_mbox/tomcat-users/202104.mbox/ajax/%3Cb9a2a913-f00f-f5bf-ca05-8ea4f8663ca9%40datagis.com%3E
>
>
> Trying brief:
>
> Basically, it is about getting some user-friendly user related
> attributes, like display name, department, phone nuber, Email address
> into our applications. Typically, such attributes live in the same user
> database, that has already been configured for the Realm for
> authentication and authorization. That database might be an SQL database
> (DataSourceRealm) or any directory server (JNDIRealm).
>
> Those attributes are often intended for display only (which user is
> logged on?). Another common case is sending mails from an application,
> which requires a valid From: address of the logged on user.
>
> The Realm will have a new configuration option, e. g.
> additionalAttributes, which takes a comma separated field (attribute)
> list. The Realm, which already knows how to access that user database,
> is the responsible for querying that attributes from the "user table" in
> that database (e. g. in JNDI, that is the User's DirContext).
>
> Without knowing anything about SQL or JNDI/LDAP queries, users can
> simply configure, what additional user attributes they like to get.
>
> Without that, users need to implement a "session initialized"-hook and
> access that user database "manually" and need some knowledge of that
> database. Also, credentials for accessing that database must be provided
> separately for that hook method.
Right. This is literally how every application in the world does things
right now. It always works and always has the information the
application needs :)
>> Unfortunately, Clonable and Serializable have both fallen out of favor
>> in the Java world because both ideas seem to have some serious issues.
>> If we pick one versus the other, we may have a lot of push-back.
>
> Too bad. In order to get it bullet proof, my idea is to have a real deep
> copy cloning mechanism in Java. That may be even quite slow, since we
> could check for a couple of simple but fast cases up front (e. g.
> objects that are by design immutable, like String, or some other
> commonly used objects, for which we can provide a fast clone with a copy
> constructor).
>
> Most of the attributes will likely be strings or be of any of the
> commonly used types so, only few object must go through the slow deep
> copy clone mechanism.
>
> Actually, my idea was to use Serializable for that. What's the point
> with Serializable? Isn't Tomcat using that for clustering/HA as well?
> The documentation of Serializable is quite promising...
>
>
>> If we restrict attribute values to Strings, is that too limiting?
>
> I consider that a last resort option, only. Maybe we could agree on
> String plus a limited number of well-known, commonly used and (by
> design) immutable objects like Number, Boolean, Date (is Date actually
> immutable?).
Date is most definitely NOT immutable.
Maybe java.lang.*, any primitive array (especially byte[], which will
certainly be important), and anything that is either Clonable or
Serializable. That pretty much means "Object*" where the "*" means
"well, some kinds of objects".
>> If we return Collections.unmodifiableMap() to return
>> Map<String,String> that makes things simpler. But how much do we trust
>> Collections.unmodifiableMap if the underlying data are
>> security-sensitive? With reflection, is it possible to monkey your way
>> through the references and swap-out the underlying references? That
>> might be Bad.
>
> Reflection was another idea. This my slow down things noticeably but, if
> we have enough well-known fast-cloning object types defined, that should
> not be too bad.
Honestly, a reflective-attack could just as easily monkey-around with
the principal itself, traversing private members and breaking things in
there. So I'm sorry to have brought-up the idea of messing-around with
an unmodifiableMap. It just moves the problem.
> On npm there are likely ten thousands of packages that support deep
> copying JavaScript objects. Where's the jpm for Java? Just kidding...
It exists, and it's called Maven Central. Except it's not the cesspool
that is npm.
> However, the deep copy problem should have been solved already, likely
> many many times. Isn't there a common utility function available for
> Java somewhere on the Internet?
Deep copy was "solved" with the Clonable interface way back in the
initial release of Java. But it only works if classes actually implement
it. See my note above about its use falling out of favor. If users don't
implement Clonable, then objects of those classes cannot be "cloned".
You might be able to make a copy in some other way, but it's not
universal the way Clonable was supposed to be.
https://jarombek.com/blog/may-15-2018-java-clone
You can also use Serializable as a cloning mechanism: simply serialize
to byte[] and then back again. But it requires the object to be
Serializable, of course, and every object that object refers to, and so on.
The reason Javascript objects are so easy to copy is because the
language was designed to support that kind of thing. Java developers
found Clonable confusing and scary and so they ditched it without
thinking about the consequences. Anyone who says "just use copy
constructors" has never written middleware.
> What about Apache Commons? Can we use these in Tomcat?
commons-lang SerializationUtils.clone uses serialization
Evidently, there are some other hacks. Amusingly, some of them use JSON
as an intermediary.
https://www.baeldung.com/java-deep-copy
The Tomcat team, for good reason, have decided to keep dependencies to a
minimum. Where needed, we must "shade" libraries so that they never
conflict with anything a web application might want to load.
So we are likely to opt for something that doesn't require heroics so we
can implement the solution in Tomcat simply, without the need for an
external library.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Chris, Mark,
On 27/05/2021 22:11, Christopher Schultz wrote:
</snip>
</reordered>
> What's the primary use-case for these kinds of attributes?
This has been described in detail here:
http://mail-archives.apache.org/mod_mbox/tomcat-users/202104.mbox/ajax/%3Cb9a2a913-f00f-f5bf-ca05-8ea4f8663ca9%40datagis.com%3E
Trying brief:
Basically, it is about getting some user-friendly user related
attributes, like display name, department, phone nuber, Email address
into our applications. Typically, such attributes live in the same user
database, that has already been configured for the Realm for
authentication and authorization. That database might be an SQL database
(DataSourceRealm) or any directory server (JNDIRealm).
Those attributes are often intended for display only (which user is
logged on?). Another common case is sending mails from an application,
which requires a valid From: address of the logged on user.
The Realm will have a new configuration option, e. g.
additionalAttributes, which takes a comma separated field (attribute)
list. The Realm, which already knows how to access that user database,
is the responsible for querying that attributes from the "user table" in
that database (e. g. in JNDI, that is the User's DirContext).
Without knowing anything about SQL or JNDI/LDAP queries, users can
simply configure, what additional user attributes they like to get.
Without that, users need to implement a "session initialized"-hook and
access that user database "manually" and need some knowledge of that
database. Also, credentials for accessing that database must be provided
separately for that hook method.
> Unfortunately, Clonable and Serializable have both fallen out of favor
> in the Java world because both ideas seem to have some serious issues.
> If we pick one versus the other, we may have a lot of push-back.
Too bad. In order to get it bullet proof, my idea is to have a real deep
copy cloning mechanism in Java. That may be even quite slow, since we
could check for a couple of simple but fast cases up front (e. g.
objects that are by design immutable, like String, or some other
commonly used objects, for which we can provide a fast clone with a copy
constructor).
Most of the attributes will likely be strings or be of any of the
commonly used types so, only few object must go through the slow deep
copy clone mechanism.
Actually, my idea was to use Serializable for that. What's the point
with Serializable? Isn't Tomcat using that for clustering/HA as well?
The documentation of Serializable is quite promising...
> If we restrict attribute values to Strings, is that too limiting?
I consider that a last resort option, only. Maybe we could agree on
String plus a limited number of well-known, commonly used and (by
design) immutable objects like Number, Boolean, Date (is Date actually
immutable?).
> If we return Collections.unmodifiableMap() to return Map<String,String>
> that makes things simpler. But how much do we trust
> Collections.unmodifiableMap if the underlying data are
> security-sensitive? With reflection, is it possible to monkey your way
> through the references and swap-out the underlying references? That
> might be Bad.
Reflection was another idea. This my slow down things noticeably but, if
we have enough well-known fast-cloning object types defined, that should
not be too bad.
On npm there are likely ten thousands of packages that support deep
copying JavaScript objects. Where's the jpm for Java? Just kidding...
However, the deep copy problem should have been solved already, likely
many many times. Isn't there a common utility function available for
Java somewhere on the Internet? What about Apache Commons? Can we use
these in Tomcat?
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/27/21 12:22, Mark Thomas wrote:
> On 27/05/2021 15:04, Christopher Schultz wrote:
>> Mark,
>>
>> On 5/27/21 04:59, Mark Thomas wrote:
>>> On 27/05/2021 07:32, Carsten Klein wrote:
>>>> On 26/05/2021 19:56, Mark Thomas wrote:
>>>>
>>>>> Given that the attributes may well be security related, you would
>>>>> need to make sure neither the Map nor any of the keys/values could
>>>>> be modified. Protecting the Map is easy. Protecting the keys/values
>>>>> is a little trickier. For that reason I'd lean towards the solution
>>>>> below.
>>>>
>>>> Oh yes, these attributes should likely be immutable. Since I still
>>>> believe that Enumerations are kind of uncomfortable (and outdated?),
>>>> what about strictly relying on Collections.unmodifiableMap?
>>>
>>> The issue while objects can't be added to the Map or removed from the
>>> Map, the objects can still be mutated. For that reason I prefer the
>>> getAttribute() approach where an appropriate defensive copy can be
>>> made before returning any attribute value.
>>
>> Does that mean this needs to be Map<String,<? extends Clonable>>? Or
>> Map<String,<? extends Serializable>>?
>>
>> How do you expect to perform defensive-copies of arbitrary objects?
>
> Good point. I think that means attribute values can't be any old object.
> Limiting things to <? extends Clonable> seems reasonable to me as that
> makes the defensive copy easy. Although that does lead to a deep clone
> vs shallow cone discussion. I think the simplest way to deal with that
> is to document how the defensive copy is made and note that users of the
> API need to be sure any attributes they provide are safe given how the
> defensive copy is made.
Unfortunately, Clonable and Serializable have both fallen out of favor
in the Java world because both ideas seem to have some serious issues.
If we pick one versus the other, we may have a lot of push-back.
What's the primary use-case for these kinds of attributes?
If we restrict attribute values to Strings, is that too limiting?
If we return Collections.unmodifiableMap() to return Map<String,String>
that makes things simpler. But how much do we trust
Collections.unmodifiableMap if the underlying data are
security-sensitive? With reflection, is it possible to monkey your way
through the references and swap-out the underlying references? That
might be Bad.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 27/05/2021 15:04, Christopher Schultz wrote:
> Mark,
>
> On 5/27/21 04:59, Mark Thomas wrote:
>> On 27/05/2021 07:32, Carsten Klein wrote:
>>> On 26/05/2021 19:56, Mark Thomas wrote:
>>>
>>>> Given that the attributes may well be security related, you would
>>>> need to make sure neither the Map nor any of the keys/values could
>>>> be modified. Protecting the Map is easy. Protecting the keys/values
>>>> is a little trickier. For that reason I'd lean towards the solution
>>>> below.
>>>
>>> Oh yes, these attributes should likely be immutable. Since I still
>>> believe that Enumerations are kind of uncomfortable (and outdated?),
>>> what about strictly relying on Collections.unmodifiableMap?
>>
>> The issue while objects can't be added to the Map or removed from the
>> Map, the objects can still be mutated. For that reason I prefer the
>> getAttribute() approach where an appropriate defensive copy can be
>> made before returning any attribute value.
>
> Does that mean this needs to be Map<String,<? extends Clonable>>? Or
> Map<String,<? extends Serializable>>?
>
> How do you expect to perform defensive-copies of arbitrary objects?
Good point. I think that means attribute values can't be any old object.
Limiting things to <? extends Clonable> seems reasonable to me as that
makes the defensive copy easy. Although that does lead to a deep clone
vs shallow cone discussion. I think the simplest way to deal with that
is to document how the defensive copy is made and note that users of the
API need to be sure any attributes they provide are safe given how the
defensive copy is made.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/27/21 04:59, Mark Thomas wrote:
> On 27/05/2021 07:32, Carsten Klein wrote:
>> On 26/05/2021 19:56, Mark Thomas wrote:
>>
>>> Given that the attributes may well be security related, you would
>>> need to make sure neither the Map nor any of the keys/values could be
>>> modified. Protecting the Map is easy. Protecting the keys/values is a
>>> little trickier. For that reason I'd lean towards the solution below.
>>
>> Oh yes, these attributes should likely be immutable. Since I still
>> believe that Enumerations are kind of uncomfortable (and outdated?),
>> what about strictly relying on Collections.unmodifiableMap?
>
> The issue while objects can't be added to the Map or removed from the
> Map, the objects can still be mutated. For that reason I prefer the
> getAttribute() approach where an appropriate defensive copy can be made
> before returning any attribute value.
Does that mean this needs to be Map<String,<? extends Clonable>>? Or
Map<String,<? extends Serializable>>?
How do you expect to perform defensive-copies of arbitrary objects?
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Martin Grigorov <mg...@apache.org>.
On Tue, Jun 1, 2021 at 12:46 PM Carsten Klein <c....@datagis.com> wrote:
>
>
> On 01/06/2021 10:18, Mark Thomas wrote:
>
> > I don't know if you can. I suspect not. By all means see if you can. I'm
> > mildly curious to find out the answer. Whether you can or not, you don't
> > need to.
>
> I found nothing to re-trigger the Travis CI build so far. However, now
> the CI test is 'successful'. Did you do anything about it?
>
I've restarted it few hours ago.
Only members of github.com/apache/tomcat team can restart builds via the
TravisCI UI. And most probably Apache Infra team members.
>
> Carsten
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: users-help@tomcat.apache.org
>
>
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
On 01/06/2021 10:18, Mark Thomas wrote:
> I don't know if you can. I suspect not. By all means see if you can. I'm
> mildly curious to find out the answer. Whether you can or not, you don't
> need to.
I found nothing to re-trigger the Travis CI build so far. However, now
the CI test is 'successful'. Did you do anything about it?
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 01/06/2021 08:39, Carsten Klein wrote:
> Mark,
>
> On 01/06/2021 09:28, Mark Thomas wrote:
>
>> We have been seeing that a lot lately. As far as I can tell, it is an
>> issue with Travis CI.
>
> Can you use the PR anyway?
Yes. We don't have a strict CI must pass rule. Whether or not a PR is
applied a decision that is made by a committer. The tools are useful and
inform that decision but the final say is made by a committer using
their judgement.
> Can/must I re-trigger the Travis build?
I don't know if you can. I suspect not. By all means see if you can. I'm
mildly curious to find out the answer. Whether you can or not, you don't
need to.
Thanks,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Mark,
On 01/06/2021 09:28, Mark Thomas wrote:
> We have been seeing that a lot lately. As far as I can tell, it is an
> issue with Travis CI.
Can you use the PR anyway? Can/must I re-trigger the Travis build?
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 29/05/2021 13:28, Carsten Klein wrote:
> Mark,
>
> On 27/05/2021 18:56, Carsten Klein wrote:
>
> Concerning removal of class UserDatabaseRealm.UserDatabasePrincipal:
>
>> I will provide a PR and file a corresponding issue in Bugzilla soon.
>
> My PR and Bugzilla issue are present. However, Travis CI build failed
> on arm64 architecture for the PR with "An error occurred while
> generating the build script." All other tests/jobs (including my local
> builds) were fine. Currently, I have no idea how to deal with that.
We have been seeing that a lot lately. As far as I can tell, it is an
issue with Travis CI.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Mark,
On 27/05/2021 18:56, Carsten Klein wrote:
Concerning removal of class UserDatabaseRealm.UserDatabasePrincipal:
> I will provide a PR and file a corresponding issue in Bugzilla soon.
My PR and Bugzilla issue are present. However, Travis CI build failed
on arm64 architecture for the PR with "An error occurred while
generating the build script." All other tests/jobs (including my local
builds) were fine. Currently, I have no idea how to deal with that.
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Mark,
On 27/05/2021 18:19, Mark Thomas wrote:
> I will note that it isn't uncommon to have to log out and back in again
> to pick up newly allocated groups/roles in other environments.
Yes, you are right. Didn't see it that way so far. We're talking about
live updates for a session during its lifetime, which we can surely
drop. Only few other Realms will support this (if any).
> If we go the removal route then I'd treat that as a separate PR /
> bugzilla issue so any discussion remains focussed on the relevant issue.
The removal route seems simple. Both the UserDatabasePrincipal and the
hasRole()-overwrite can be removed. That should do the trick.
I will provide a PR and file a corresponding issue in Bugzilla soon.
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 27/05/2021 12:49, Carsten Klein wrote:
> On 27/05/2021 10:59, Mark Thomas wrote:
>
> <snip/>
>
>> As far as I can tell, removing UserDatabasePrincipal, relying on
>> GenericPrincipal and User remaining an internal object not exposed via
>> the Servlet API would achieve the same result with less code.
>>
>> At this point I am looking for a reason not to remove
>> UserDatabasePrincipal and I'm not seeing one.
>
> Since User and Role instances support dynamic updates (have a look at
> methods addGroup/Role and removeGroup/Role), the special handling in
> UserDatabaseRealm.hasRole() is required. Although that feature is
> currently not used by Tomcat (is it?), those possible live updates must
> be taken into account in the code.
You can just as easily add roles in a database for the DataSourceRealm
or in LDAP for the JNDIRealm and we don't handle the dynamic update case
for either of those. I don't have a strong view of whether we should
handle this or shouldn't but I do have a strong view that we should try
to be consistent.
I will note that it isn't uncommon to have to log out and back in again
to pick up newly allocated groups/roles in other environments.
<snip/>
>> I think it would be worth handling this is a separate commit to give
>> folks the chance to review it before proceeding to add attribute support.
>
> +1
>
> Are you talking about a separate commit in my additional-user-attributes
> branch? Or is that worth an extra BUG/case/issue?
If we go the removal route then I'd treat that as a separate PR /
bugzilla issue so any discussion remains focussed on the relevant issue.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
On 27/05/2021 10:59, Mark Thomas wrote:
<snip/>
> As far as I can tell, removing UserDatabasePrincipal, relying on
> GenericPrincipal and User remaining an internal object not exposed via
> the Servlet API would achieve the same result with less code.
>
> At this point I am looking for a reason not to remove
> UserDatabasePrincipal and I'm not seeing one.
Since User and Role instances support dynamic updates (have a look at
methods addGroup/Role and removeGroup/Role), the special handling in
UserDatabaseRealm.hasRole() is required. Although that feature is
currently not used by Tomcat (is it?), those possible live updates must
be taken into account in the code.
If the UserDatabase was immutable, we could simply go ahead with the
pre-resolved effective roles list and GenericPrincipal. But actually,
those effective roles may be wrong after an update of the UserDatabase.
So, I suggest not resolving effective roles up front in
UserDatabaseRealm.getPrincipal() and use null as the roles list when
constructing the new GenericPrincipal instance.
The special handling in UserDatabaseRealm.hasRole() is triggered, if the
Principal is a GenericPrincipal with a userPrincipal of type
UserDatabasePrincipal:
if (principal instanceof GenericPrincipal) {
GenericPrincipal gp = (GenericPrincipal) principal;
if (gp.getUserPrincipal() instanceof UserDatabasePrincipal) {
principal = database.findUser(gp.getName());
}
}
The current test requires an extra UserDatabasePrincipal class. Making
that class support attributes and serializable is possible.
However, a simple default/package visible boolean flag
'userDatabasePrincipal' in GenericPrincipal would work as well and lets
us finally drop class UserDatabasePrincipal.
> I think it would be worth handling this is a separate commit to give
> folks the chance to review it before proceeding to add attribute support.
+1
Are you talking about a separate commit in my additional-user-attributes
branch? Or is that worth an extra BUG/case/issue?
> I suspect the users that worry about that sort of thing aren't using the
> UserDatabaseRealm but it would be nice to fix that anyway.
That will be fixed after having added attribute support either way.
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 27/05/2021 07:32, Carsten Klein wrote:
> On 26/05/2021 19:56, Mark Thomas wrote:
>
>> Given that the attributes may well be security related, you would need
>> to make sure neither the Map nor any of the keys/values could be
>> modified. Protecting the Map is easy. Protecting the keys/values is a
>> little trickier. For that reason I'd lean towards the solution below.
>
> Oh yes, these attributes should likely be immutable. Since I still
> believe that Enumerations are kind of uncomfortable (and outdated?),
> what about strictly relying on Collections.unmodifiableMap?
The issue while objects can't be added to the Map or removed from the
Map, the objects can still be mutated. For that reason I prefer the
getAttribute() approach where an appropriate defensive copy can be made
before returning any attribute value.
> Since UserDatabasePrincipal is a private inner class of
> UserDatabaseRealm, no instanceof checks are possible to determine
> whether attributes may be available or not. So, I was thinking of a new
> interface
>
> public interface AttributedPrincipal extends Principal {
>
> public Object getAttribute(String name);
>
> Enumeration<String> getAttributeNames();
>
> }
>
> That interface has a slightly more meaningful name for consumers of
> additional user attributes. But adding these methods to TomcatPrincipal
> is good as well.
I don't see a need either now or in the future to differentiate between
TomcatPrimcipal and AttributePrincipal. If we can think of a valid use
case for separate interfaces I'd support that - but at the moment I
don't see one.
>>>> 3. Class UserDatabasePrincipal in UserDatabaseRealm
<snip/>
> So, the only thing UserDatabasePrincipal does, is to hide the fact that
> groups have already been resolved to a single list of effective roles,
> the Principal is working with during its lifetime. Did I overlook
> something?
I don't think so.
I think I have figured it out.
User implements Principal and exposes various fields including password.
User was used as the UserPrincipal stored in GenericPrincipal
Some time ago, getPassword() was removed from GenericPrincipal. In order
to prevent access to User.getPassword(), UserDatabasePrincipal was
introduced.
As far as I can tell, removing UserDatabasePrincipal, relying on
GenericPrincipal and User remaining an internal object not exposed via
the Servlet API would achieve the same result with less code.
At this point I am looking for a reason not to remove
UserDatabasePrincipal and I'm not seeing one.
I think it would be worth handling this is a separate commit to give
folks the chance to review it before proceeding to add attribute support.
> Additionally, class UserDatabasePrincipal is NOT serializable. That
> means, it gets dropped when sessions and principals get persisted and
> reloaded. That applies to session persistence across restarts (with
> persistAuthentication set to true) and likely to clustering/HA. That's
> not fatal, but after a restart or when running on a different cluster
> node, users suddenly get a full-blown GenericPrincipal instance when
> they call Request.getUserPrincipal(). However, since nobody complained
> about it, the UserDatabasePrincipal is likely not so important :-p
I suspect the users that worry about that sort of thing aren't using the
UserDatabaseRealm but it would be nice to fix that anyway.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Carsten Klein <c....@datagis.com>.
Hi Mark,
thanks for sharing your ideas :)
On 26/05/2021 19:56, Mark Thomas wrote:
> Given that the attributes may well be security related, you would need
> to make sure neither the Map nor any of the keys/values could be
> modified. Protecting the Map is easy. Protecting the keys/values is a
> little trickier. For that reason I'd lean towards the solution below.
Oh yes, these attributes should likely be immutable. Since I still
believe that Enumerations are kind of uncomfortable (and outdated?),
what about strictly relying on Collections.unmodifiableMap?
But, I'm ok with getAttribute(String name) and getAttributeNames() pair
as well. You decide :)
My first idea was to allow users to put their own attributes as well.
But then, we'd need to track Realm-sourced attribute names and make
those read-only in a setAttribute method. Since users could as well put
their stuff into session attributes, making the whole map immutable is
possible and clearly much simpler.
>> 2. Should I add attributes-related public methods to the
>> TomcatPrincipal interface as well?
>
> Yes. So far the only Tomcat specific extensions are for SPNEGO but this
> is in the same category. The other option would be a new interface but I
> don't see a need for that.
Since UserDatabasePrincipal is a private inner class of
UserDatabaseRealm, no instanceof checks are possible to determine
whether attributes may be available or not. So, I was thinking of a new
interface
public interface AttributedPrincipal extends Principal {
public Object getAttribute(String name);
Enumeration<String> getAttributeNames();
}
That interface has a slightly more meaningful name for consumers of
additional user attributes. But adding these methods to TomcatPrincipal
is good as well.
>>> 3. Class UserDatabasePrincipal in UserDatabaseRealm
>>> Why does UserDatabaseRealm pass a userPrincipal of type
>>> UserDatabasePrincipal? Can't we just drop that and do it like
>>> JNDIRealm or DataSourceRealm?
>>
>> I don't see any obvious reason. I'll do some digging in the source
>> history to see if I can find out why. Absent a good reason, I'd say
>> drop it.
>
> There is a good reason for it, but I think it should be possible to drop
> it.
>
> It is there because the UserDatabaseRealm supports the concepts of
> groups. Users can have roles assigned directly or users can be assigned
> to a group and inherit the roles of the group. This means hasRole() is a
> little more complicated and the UserDatabasePrincipal is used to
> determine if this additional processing is required.
>
> I think this could be replaced by a
> "org.apache.catalina.realm.UserDatabaseRealm.groups" attribute which
> would remove the need for the dedicated UserDatabasePrincipal
Honestly, I don't see that in the code. What I see is, that
UserDatabaseRealm resolves roles coming from groups up front in its
getPrincipal method. After that, it just creates an ordinary
GenericPrincipal instance with a single list of effective roles.
The associated UserDatabasePrincipal does nothing special with
hasRole(). Its only method is getName(). So, it just removes or "hides"
any special GenericPrincipal methods like hasRole() and getRoles() from
the user (aka from the Servlet code, internally Tomcat still uses the
surrounding GenericPrincipal, e. g. for authorization).
So, the only thing UserDatabasePrincipal does, is to hide the fact that
groups have already been resolved to a single list of effective roles,
the Principal is working with during its lifetime. Did I overlook something?
Actually, when declaring the attributes-related methods in a public
interface (either TomcatPrincipal or AttributedPrincipal), adding that
interface to UserDatabasePrincipal is not expensive. So, we could always
decide to leave that UserDatabasePrincipal in place and let it implement
that interface.
However, in order to not "pollute" UserDatabasePrincipal with unneeded
methods, I'd vote for using a new
org.apache.catalina.AttributedPrincipal interface.
Additionally, class UserDatabasePrincipal is NOT serializable. That
means, it gets dropped when sessions and principals get persisted and
reloaded. That applies to session persistence across restarts (with
persistAuthentication set to true) and likely to clustering/HA. That's
not fatal, but after a restart or when running on a different cluster
node, users suddenly get a full-blown GenericPrincipal instance when
they call Request.getUserPrincipal(). However, since nobody complained
about it, the UserDatabasePrincipal is likely not so important :-p
Carsten
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2021 18:56, Mark Thomas wrote:
> On 26/05/2021 12:00, Carsten Klein wrote:
<snip/>
>> Why does UserDatabaseRealm pass a userPrincipal of type
>> UserDatabasePrincipal? Can't we just drop that and do it like
>> JNDIRealm or DataSourceRealm?
>
> I don't see any obvious reason. I'll do some digging in the source
> history to see if I can find out why. Absent a good reason, I'd say drop
> it.
There is a good reason for it, but I think it should be possible to drop it.
It is there because the UserDatabaseRealm supports the concepts of
groups. Users can have roles assigned directly or users can be assigned
to a group and inherit the roles of the group. This means hasRole() is a
little more complicated and the UserDatabasePrincipal is used to
determine if this additional processing is required.
I think this could be replaced by a
"org.apache.catalina.realm.UserDatabaseRealm.groups" attribute which
would remove the need for the dedicated UserDatabasePrincipal
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org
Re: Enhancement: Additional user attributes queried by (some) realms
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2021 12:00, Carsten Klein wrote:
<snip/>
> 1. How to access the Principal's new attributes
>
> Simplest is to provide a getter method, that actually returns the map
> (optionally with a read-only parameter):
Given that the attributes may well be security related, you would need
to make sure neither the Map nor any of the keys/values could be
modified. Protecting the Map is easy. Protecting the keys/values is a
little trickier. For that reason I'd lean towards the solution below.
> However, in the jakarta.servlet classes, there are also some entities
> that have attributes. There, attributes are accessed by these methods:
>
> Object getAttribute(String name);
>
> Enumeration<String> getAttributeNames();
>
> void setAttribute(String name, Object o);
>
> Would you, for the sake of uniformity, prefer using these more
> servlet-spec like methods?
Yes, but without setAttribute(). The attributes need to be provided at
construction time and should be immutable from that point onwards.
> 2. Should I add attributes-related public methods to the TomcatPrincipal
> interface as well?
Yes. So far the only Tomcat specific extensions are for SPNEGO but this
is in the same category. The other option would be a new interface but I
don't see a need for that.
> 3. Class UserDatabasePrincipal in UserDatabaseRealm
<snip/>
> Why does UserDatabaseRealm pass a userPrincipal of type
> UserDatabasePrincipal? Can't we just drop that and do it like JNDIRealm
> or DataSourceRealm?
I don't see any obvious reason. I'll do some digging in the source
history to see if I can find out why. Absent a good reason, I'd say drop it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org