Can you please explain the purpose of the deniedAttributes? Why is it necessary, what is the usecase for?

Denied Attributes is the internal term of attributes, for which access is denied to. Those attributes could never be exposed as user attributes in the Principal's attributes map. Basically, this applies to attributes/fields that contain the user's password. Requesting such an attribute causes a userAttributeAccessDenied message to be logged.

Both MemoryRealm and UserDatabaseRealm provide all possible (and actually used, MemoryRealm does not use the group attribute) XML attributes of the <user ...> entry in tomcat-users.xml. Obviously, the password attribute is sensitive and should not be exposed so, these Realms make the password attribute a denied attribute.

DataSourceRealm makes the column name configured in userCredCol a denied attribute and JNDIRealm does this for the attribute name configured in userPassword.

Can you please explain the purpose of the deniedAttributes? Why is it necessary, what is the usecase for?

Denied Attributes is the internal term of attributes, for which access is denied to. Those attributes could never be exposed as user attributes in the Principal's attributes map. Basically, this applies to attributes/fields that contain the user's password. Requesting such an attribute causes a userAttributeAccessDenied message to be logged.

Both MemoryRealm and UserDatabaseRealm provide all possible (and actually used, MemoryRealm does not use the group attribute) XML attributes of the <user ...> entry in tomcat-users.xml. Obviously, the password attribute is sensitive and should not be exposed so, these Realms make the password attribute a denied attribute.

DataSourceRealm makes the column name configured in userCredCol a denied attribute and JNDIRealm does this for the attribute name configured in userPassword.

So you basically want to protect the admin/developer to shoot in the foot?

So you basically want to protect the admin/developer to shoot in the foot?

Yes, as it may hurt badly. Also, only these denied attributes makes using the (otherwise quite comfortable) wildcard character * possible/safe.

This looks good enough to put it in. I can probably think about enhancements late. The only thing that I'm really wondering about is why the UserDatabaseRealm did not get arbitrary attributes.

The only thing that I'm really wondering about is why the UserDatabaseRealm did not get arbitrary attributes.

My first focus was on UserDatabaseRealm and JDNIRealm. Arbitrary user attributes for UserDatabaseRealm sounds interesting. However, I did not yet think about it in detail. AFAIK, there is a schema file for tomcat-users.xml. Some new Digester rules must be defined. Actually, the attributes come from the User instance. Likely this class should get an attributes map as well (which we then can provide through UserDatabasePrincipal).

I'm almost finished with all the changes caused by that code review. Also, I've added support for SQL types ARRAY, BLOB, and CLOB. I will push my latest commits soon.

Still open/pending:

• Using linked hash maps (yes/no)
• Defensive copies (how paranoid shall we go?)
• Documentation (likely my job)

The latter might be quite challenging. Needs additions in config/realm.html and likely in realm-howto.html. Since, at current, only TomcatPrincipal supports the attribute stuff, maybe a code example in one of the HTML sites will be required.

Has anyone thought about putting these methods to a more official interface jakarta.servlet.Principal? Since it is now jakarta, is Apache or Tomcat now responsible for the Servlet Standard?

No, the Tomcat project is not responsible for Jakarta and has no control over it, so we cannot make that change.

I'm perfectly fine as it is, you're not supposed to do everything by yourself and I'm interested in improving things a bit too if I can.

Please, wait at least until I've pushed my latest changes. Also, JDBC stuff in DataSourceRealm needs some adjustments in order not to put arbitrary java.sql objects into the map. I'd really love to make ends meet for things that I've started...

I've committed the changes that we've agreed on during the code review. However, I don't know what to do with the still unresolved conversations.

What are the next steps? The recent review process was suddenly aborted and nothing more happened till now. Should a re-request another review?

@markt-asf Could you please share your ideas on defensive copies? (as suggested/requested by @michael-o)
@ChristopherSchultz Your ideas on defensive copies are appreciated as well, of course.

@rmaucher Is there anything I could do to get things moving?

@cklein05 I am waiting for other committers to review.

Still looks good to me, needs a super minor merging for UserDatabaseRealm.

For defensive copies, I see several approaches:

1. GenericPrincipal always returns defensive copies
2. GenericPrincipal returns defensive copies if running under a SecurityManager
3. GenericPrincipal is documented to require that any attributes added to it are safe to expose to applications.

Option one will create unnecessary copies in some scenarios but it should be impossible for a security sensitive internal object to be exposed to a potentially untrusted application.
Option two may create some unnecessary copies but only when working with untrusted applications (on the assumption that anyone running an untrusted application should be using a SecurityManager).
Option three allows any unnecessary copying (either because the objects are safe to share or because the application is trusted) to be avoided at the risk of it being possible to expose an internal object if the configuration/coding is incorrect.
The other thing I like about option 3 is it enables the caller to figure out the best way to create any required defensive copy rather than trying to write a generic deep object clone solution in GenericPrincipal.

On defensive copies:

For my understanding, defensive copies do not prevent exposing sensitive user information to (potentially untrusted) applications due to an inappropriate configuration. So, the Realm always needs to be configured carefully with regards to keeping secrets secret. That has not much to do with defensive copies.

The only thing defensive copies could help is to prevent that an evil application is able to modify a cached attribute value (we've agreed on that attribute values should be immutable). However, in that case, GenericPrincipal cannot trust the caller and has to generically create these copies itself.

I guess, the question is whether we rely on Java's understanding of immutable objects (position A, e. g. String is an immutable type so, no copy is needed) or whether we account for that that's not true with reflection (position B, e. g. a String can be changed inline with reflection or native code). With position A, we could at least save some copies for immutable types (according to Java's definition of immutable).

After all, since we can expect that most applications don't request the Principal's attributes too frequently, we could easily go with option 1 and copy every object returned.

When considering defensive copying, one must remember that no other code can be trusted, but our own code can be trusted. So structures passed-into the realm must be copied to protect our code from a malicious (or incompetent) client breaking things. When returning structures, there is more flexibility depending upon how much you trust the JVM and whether or not you want to protect against truly malicious code.

For example, for most cases, it's reasonable to return Collections.unmodifiableCollection to ensure that foreign code doesn't mess with your structure. But, reflection is a possibility and therefore it's not entirely safe.

If it's reasonable to return Collections.unmodifiable[Collection](foo) for a certain getFoo() operation, and the strcuture needs to be unmodifiable locally, anyway, then making a defensive and unmodifiable copy during setFoo (or similar) is reasonable. Then we can avoid additional allocations on every getFoo() call.

But again, it depends upon hos security-sensitive these items are. Unfortunately, it's impossible to know how security-sensitive these things can be, because they are application-defined. Having a user-attribute "locale" is unlikely to be highly security-sensitive. But maybe "totp-seed" may be. Or "user-is-unrestricted-administrator" or something similar.

Security features are always a trade-off, and most applications don't need this kind of uber-paranoid data-handling. But some environments may require it. Since nobody has been beating-down the door asking for user-attributes on realms in the first place, perhaps engineering a solution to include uber-paranoia is over-engineering. Anything that can be written can be re-written, improved, etc.

I suggest we do the minimum possible to build the useful feature and ship it. If "casual" damage to structures would be Bad, let's prevent that from happening. But let's not worry too much about an application using native code to modify the internal char array of a java.lang.String value at this point.

Okay, let's make some decisions (Bob Ross? Here's a class, and it has a friend...)

Since there's no public setFoo() method in GenericPrincipal (attributes are only set through the constructor and retrieved by getAttribute(String name)), I don't see a big need for using Collections.unmodifiableMap(). Again, with reflection one could access the hash map and modify entries, but that would also be uber-paranoid (BTW, did you mean German über-paranoid?). On the other hand, wrapping the map with an unmodifiable view will not hurt... So, it's up to you to decide.

Both DataSourceRealm and JNDIRealm are the only components that actually add attributes to GenericPrincipal´s attributes map (UserDatabaseRealm and MemoryRealm work differently by relying on the User instance). So, defensively copying attributes on input is not really required, right? The attributes values are obtained from either JDBC or JNDI so, no application code should have another reference to these.

With defensive copies, we could do two things:

1. Remove object duplication in getAttribute(String name) for all types
   Pros: simple, fast
   Cons: shallow copy vs. deep copy problem

2. Remove object duplication in getAttribute(String name) for known immutable types only
   Pros: no shallow copy vs. deep copy problem
   Cons: more complex code, slow, more CPU load on attribute retrieval, can't tell whether a generic class is immutable

Although option 2 has much more cons, that may not be a big issue in practice, since most of the attributes are actually Strings, Numbers and Booleans only (e.g. Sun's standard LDAP provider returns Strings only). The only types for which this may hurt a bit are arrays (both DataSourceRealm and JNDIRealm may gather multi-value attributes).

With option 2, I would just modify method GenericPrincipal.copyObject and leave GenericPrincipal.copySerializableObject untouched. The only difference will be, that copyObject does a simple return obj; for many or most of the commonly object types. So, no much code is saved with option 2 but, there are likely no extra allocations needed for most of the attribute values returned.

So, what to do? (checked my personal choices)

• use Collections.unmodifiableMap()
• copy values on input
• no defensive copies for all types
• no defensive copies for known immutable types only

Sun's standard LDAP provider returns Strings only)

...no. I use byte arrays with this all the time.

In its default configuration it does. You can configure it for byte array as well, I know.

Security concerns only arise with untrusted apps. Untrusted apps have to run under a SecurityManager else they have access to reflection and can do pretty much whatever they like. Therefore, we don't need to worry about what an app can do via reflection. I'd be happy with a requirement that any attributes passed to the GenericPrincipal constructor should be safe to expose to a potentially untrusted app (e.g. immutable, already a defensive copy, etc).
I've no objection to a little defence in depth - such as using Collections.unmodifiableMap() but I don't think we need to do too much here.
I remain concerned about the potential complexity / fragility of any solution that attempts to provide defensive copies automatically. Any issues in such could are likely to result in security vulnerabilities.

So, I guess, it's best to just remove any defensive copies from GenericPrincipal. I just added these due to the recommendations on the User's mailing list in order to make user attributes behave immutable.

Since attributes are added by the Realms only, which obtain these values directly from either JDBC or JNDI, I believe, that there's no need to defensively copy these when they are added to the map.

Although it would be cheap and simple, I do not expect any help from wrapping the attributes map in GenericPrincipal with Collections.unmodifiableMap, so I recommend not adding this right now.

If you agree, I will remove methods copyObject and copySerializableObject from GenericPrincipal and adjust method getAttribute accordingly (as well as documentation for TomcatPrincipal#getAttribute(String)) and commit changes to this PR.

Awaiting your responses.

Looking back at this since I thin I have time to merge it. Shouldn't some of the changes be dropped and focused only on the realms that show a benefit ?

• JNDIRealm: Very useful, that's likely the whole point of this new feature.
• DataSourceRealm: This does something useful but less so and it is complex.
• MemoryRealm: This adds some complexity for a fixed list of attributes which do not seem that useful.
• UserDatabaseRealm: Same.

Rémy,

I agree with you that, for MemoryRealm and UserDatabaseRealm, this feature is not that much useful. However, for the sake of completeness (aka all realms should behave the same, in a way) these do support additional user attributes as well. Nevertheless, the feature is primarily intended to provide a user friendly user name easily. The UserDatabase supports the fullname attribute, so, this is still a simple way to provide this to the application.

With DataSourceRealm it's nearly as much as useful than with JNDIRealm, since one can always add many interesting columns to the tomcat_users database table. In my company, we have several production applications that relay on DataSourceRealm connected to a PostgreSQL user database.

I agreed with Mark and Michael to remove the whole defensive copy stuff. However, that's not yet pushed back onto my GitHub repository. Shall I do so?

Now, the code should be as we've discussed before.

I will additionally provide some initial documentation and a change log entry soon. You are always welcome to improve these while merging.

Inspired by Rémy's suggestion (@rmaucher), I've added support for arbitrary extra attributes to the User object used with the UserDatabase. Now, these can also be queried by the UserDatabaseRealm. While doing so, I stumbled over this:

1. The User's fullName property is loaded from either the fullName or the fullname XML attribute. However, when writing back to XML in MemoryUser.toXML(), this property is always written to the fullName XML attribute. Seems like fullname is just an undocumented alias to fullName. However, in tomcat-users.xsd, there is only attribute fullname defined. That's a bit confusing and I actually don't know, which of the two XML attributes is the "official" one or the "correct" one.

2. What is the exact purpose of tomcat-users.xsd? The Tomcat (aka the UserDatabase) does not use this XML schema for validation (and I did not find an option to enable XML validation for the user database). Is it correct, that this schema definition file is intended to assist users editing tomcat-users.xml in their XML editor only?

With my current arbitrary attributes implementation, one can just add any extra attributes to the <user username="... entry. Every unknown attribute (those not named username, fullName, password, etc). ends up in the User's attributes. However, I'm not able to figure out how to add these generic extra attributes to the schema file along with the already defined ones (if that's even possible). On the other hand, if the schema file is intended for local file editing only, users could just add their extra attributes to the XSD file as well, just to make their editor happy (if they don't, they must live with validation errors in the editor).

Using nested <userAttribute name="my_attribute" value="123" /> entries inside a <user ...> entry may be more XSD-friendly. However, that way it is much more complex to implement.

If the attirbute unofficial and undocumented, it should probably be removed on master and deprecated for the rest.

The code should be finished and in good shape now. UserDatabaseRealm now supports querying arbitrary attributes added to <user ... /> entries in tomcat-users.xml:

  <user username="tomcat" password="tomcat" roles="admin" fullName="Tomcat User"
        home="The Apache Software Foundation" description="User has full admin rights" />

Obviously, this required some changes in User, AbstractUser and MemoryUser and MemoryUserDatabase.
@rmaucher Hope you get that merged into the tree despite the recent changes you've applied.

The JSP example application under /examples/jsp/security/protected already lists available user attributes (if any). In order to make that work out of the box with a freshly installed Tomcat, we should consider to add some more attributes to the user entries in tomcat-users.xml intended for use with the example applications. My suggestion is the fullName attribute as well as a new one like roomNumber or phone.

In order to make the example application get these attributes, we should as well consider adding the new userAttributes attribute to the UserDatabaseRealm's configuration (likely it's best to use the * wildcard character in that case). Otherwise, these steps should be described in the docs and maybe in the example application itself, if no user attributes have been queried:

No user attributes have been queried. You could configure some in your Realm's configuration. Learn more about it (link to the docs)

That's it for now. Is anyone willing to merge and port back? :)

I really need this with JDBCRealm. We have a bunch of customers, that do not want to use LDAP/AD but only JDBC based user database. Why should there be no benefit from having extra attribute fields from a user database table/view? Also, wasn't it kind of your recommendation to have arbitrary user attributes from tomcat-users.xml? You were wondering why there's no support for such arbitrary attributes. Now, after I've implemented these, you don't want it anymore?

If you really want to do this, then let's focus on adding this to JNDIRealm for now, since this is where this has obvious usefulness. If everything is merged ok, then maybe we can move on to DataSourceRealm. But there, it only seems to be adding a lot of code which will have lower performance. Your customer can live with a custom extended realm for now IMO.

I obviously never wanted any arbitrary user attributes in tomcat-users.xml. I said adding the feature to the other realms is pointless with a fixed list of attribute. Somehow, you seem to understand it as: let's add more. That was the opposite: I requested to avoid changing the other realms (except JNDIRealm).

Why? I don't understand your position. This makes sense on any realm since the principal source shouldn't really matter. I have been using a similiar approach for the last 10 years with my own ActiveDirectoryRealm. We should at least provide the optional on an interface level.

@rmaucher
Why? I don't understand your position. This makes sense on any realm since the principal source shouldn't really matter. I have been using a similiar approach for the last 10 years with my own ActiveDirectoryRealm. We should at least provide the option on an interface level.

@rmaucher
have a look at you comment
#428 (comment)

However, maybe I just got you wrong :(

It seems by September I had already changed my mind on this after seeing some actual code. Anyway, the focus should be on JNDIRealm and then later DataSourceRealm.

Maybe this should be done in stages (PRs)?

1. Principals
2. RealmBase groundwork
3. Each Realm impl seperately?

@michael-o
Rebase is done and the enhancement was successfully tested.

Waiting for @rmaucher answer for my last proposal.

@rmaucher
I'm with @michael-o here. Having these extra attributes with the Principal is useful per se. Since any Realm can log you on and create a Principal, it should not matter, which Realm queries and provides these attributes. The more Realms can do so, the better.

You want to avoid changing the other Realms except JNDIRealm? Why? What's wrong with my code? Focusing on DataSourceRealm you're saying

... it only seems to be adding a lot of code which will have lower performance.

It does not only add a lot of code. It actually works as it should. I've only added the code required to actually query these attributes in a safe way.

Also, it does not really lower performance. If there are no attributes defined, it only takes two extra method calls with a total of two null checks, except for the first login attempt in the Realm's lifetime. For the normal case that is not really a performance killer, is it?

If there are attributes defined, there is one extra SQL query executed for the normal case (plus one extra SQL query executed for the first login attempt in the Realm's lifetime). Of course, that takes some time. However, if the Realm didn't perform that query, the application will have to do that after login, which in turn will take the same time or even more, since the application must use another JDBC connection from the pool. After all, I don't see an effective performance problem with the DataSourceRealm code here. If you do, could you please explain in more detail?

You where saying

adding the feature to the other realms is pointless with a fixed list of attribute.

Yes, that may be right. However, even that fixed list of attributes gives the application the opportunity to obtain the User's fullName property (if defined), which otherwise was just not accessible. Actually, the user's full name (aka display name) is one of the most important attributes users may want, as it is much more end-user friendly than a cryptic logon name. Now, with support for arbitrary attributes, the feature is even more useful.

I personally do not need these attributes with the UserDatabaseRealm, since we actually do not use tomcat-users.xml in any setup. However, since that is Tomcat's default Realm, I believe it should support these attributes as well. E. g. I've extended example application at /examples/jsp/security/protected to list additional user attributes from the Principal. I guess it would be just nice, if people can test these without configuring another Realm first.

Nevertheless, doing this in steps is an option. How to proceed? Are you able to merge the PR partially?

I will oppose changes to the MemoryRealm and the UserDatabase realm, and I will ignore this topic for the two other realms (so I will not veto it either).

@michael-o It should be easy to merge only the files you want to merge, all the realms are independent.

@michael-o files to skip:

java/org/apache/catalina/User.java
java/org/apache/catalina/realm/MemoryRealm.java
java/org/apache/catalina/realm/MemoryRuleSet.java
java/org/apache/catalina/realm/UserDatabaseRealm.java
java/org/apache/catalina/users/AbstractUser.java
java/org/apache/catalina/users/MemoryUser.java
java/org/apache/catalina/users/MemoryUserDatabase.java
test/org/apache/catalina/users/MemoryUserDatabaseTests.java

Also, remove attribute userAttributes from MemoryRealm and UserDatabaseRealm in file
java/org/apache/catalina/realm/mbeans-descriptors.xml

Adjust change log entry (remove MemoryRealm and UserDatabaseRealm) in file
webapps/docs/changelog.xml

Remove description of config option userAttributes from MemoryRealm and UserDatabaseRealm in file
webapps/docs/config/realm.xml

Adjust new text (remove references to the now unsupported Realms) in file
webapps/docs/realm-howto.xml

@cklein05 I know, it is quite some work, but can we stick to my proposal here:
#428 (comment)?

This will break up this PR in at least three making it much easier to come an agreement.

@michael-o
Yes, we could. What exactly do you mean with RealmBase groundwork? There is not much in RealmBase. It's only the parseUserAttributes and validateUserAttributes methods, the latter only used by JDBCRealm (as well as the now unsupported ones). JNDIRealm uses it's own validate method. For example, in which PR do you expect method validateUserAttributes? IMO, this should come with the DataSourceRealm stuff, right?

@cklein05 Let's start with the principals (interfaces) first.

@michael-o You mean the TomcatPrincipal interface and the GenericPrincipal class? Or really only the interfaces? TomcatPrincipal is the only interface changed by this enhancement. But after changing only that interface, the project will no longer build :-p

That's what you meant? Did not yet create a PR in your repo.

@michael-o
That's what you meant? Did not yet create a PR in your repo.

https://github.com/cklein05/tomcat/tree/user-attributes-principal

@michael-o That's what you meant? Did not yet create a PR in your repo.

https://github.com/cklein05/tomcat/tree/user-attributes-principal

Yes, that is what I have expected because with this change you are already able to solve your problem within your realm or modifying Tomcat provided realms.

So, lets move to new PR #463

@michael-o So, lets move to new PR #463

Closing this as it has been superseded.