SEC-2078: Pre-authentication fails when using check for principal change and using non String principals

[spring-projects-issues]

Henrik Baastrup (Migrated from SEC-2078) said:

The problem occurs when using pre-authentication with "check for principal change" set and the class there extends org.springframework.security.web.authentication.preauth.AbstractPreAuthenticatedProcessingFilter using non String principals but e.g. java.security.Principal.
The problem is that the authentication manager will always authenticate even the principal has no changed, this can give problems with the authentication provider, and performance in the code.

The error is in line 145 in the AbstractPreAuthenticatedProcessingFilter class, the code:

if (currentUser.getName().equals(principal)) {
return false;
}

should be changed to something like:

if (principal instanceof Principal) {
return !currentUser.getName().equals(((Principal)principal).getName());
}
else {
return !currentUser.getName().equals(principal.toString());
}

The original code will only function when the passed principal parameter is of the type String. The code suggested will function for all type of objects there either implements the java.security.Principal interface or override the toString method.

[spring-projects-issues]

Rob Winch said:

Rather than getting the name from the principal, we can just check to see if the Principals are equal. For example:

if ((principal instanceof String) && currentUser.getName().equals(principal)) {
  return false;
}

if(principal != null && principal.equals(currentUser.getPrincipal())) {
  return false;
}

[spring-projects-issues]

Rob Winch said:

Fixed as outlined in my previous comment.

[spring-projects-issues]

Henrik Baastrup said:

I do not think calling the equals method direct on the Principal is a good idea, that should be last resort, using getName is more correct as it must be override, and as a programmer that is the method I would call to control a Principal.
So maybe the code should look like this:

if (principal==null) return true;

if (principal instanceof Principal) {
  if (currentUser.getPrincipal()!=null) {
    if (currentUser.getPrincipal().getName().equals(principal.getName()) return false;
    return !currentUser.getPrincipal().equals(principal);
  }
  return !currentUser.getName().equals(((Principal)principal).toString());
}
return !currentUser.getName().equals(principal.toString());

This code allow the programmer to use three methods around the Principal: getName (that must be override as it is part of the interface), equals and toString. Also the principal can be any kind of Object now.

[spring-projects-issues]

Rob Winch said:

I disagree that using getName() is more correct. Do you have any evidence or concrete examples of why you believe getName() is "more correct"?

The only time that getName() should be used to determine equality is when you are trying to compare the Principal to a String representation (which is already accounted for). Otherwise, the equals method should be used. This is the definition of the equals method after all.

[spring-projects-issues]

Henrik Baastrup said:

I could override toString if I want to compare two Principals as strings and then, if I follow your argumentation, the getName has no sense.

The Principal interface has only one method (property); the getName, this has sense, after all the principal represent an entity, and as such has a name, two principals with the same name, are in this sense, equal they represent the same entity.
When you have two principals the only value you can be sure to compare is the name, because that is the only method the programmer is force to write, it is not given that the equals method is overwritten and for this reason your code will fail even if the two principals where thought equals, as they had the same name. A principal is not a string, as the original code expected, but is a principal.

Last, it is not given you override the equals method, I believe, if you take a look at your own code, you have only a very little part of your objects, there actual override this method, even you compare properties within the objects. And what does it means that two instances of an object are equals? That depend of the programmer understanding of this. I strongly advice, that you use the getName method, as the code in question, is used as a framework by others and as such, you should write code, for what you could expect, and you can not expect that the equals method is overwritten.

[spring-projects-issues]

Rob Winch said:

In reply to comment #5:

I could override toString if I want to compare two Principals as strings and
then, if I follow your argumentation, the getName has no sense.

Overriding toString() has nothing to do with the equality of the two objects. This is just one way a human would visualize the Object.

The Principal interface has only one method (property); the getName, this has
sense, after all the principal represent an entity, and as such has a name, two
principals with the same name, are in this sense, equal they represent the same
entity.
When you have two principals the only value you can be sure to compare is the
name, because that is the only method the programmer is force to write, it is
not given that the equals method is overwritten and for this reason your code
will fail even if the two principals where thought equals, as they had the same
name. A principal is not a string, as the original code expected, but is a
principal.

Actually the getPreAuthenticatedPrincipal method allows for Object to be returned (not just Principal). Additionally, the User object accepts an Object as the principal that is injected into it. Since everything is an Object and Object defines the equals method, then everything (despite if it is a Princpal or not) will have an equals method.

Last, it is not given you override the equals method, I believe, if you take a
look at your own code, you have only a very little part of your objects, there
actual override this method, even you compare properties within the objects.

It is very frequent that I override the equals (and hashCode) methods. You must override the equals method if you are using standard Java Collections, an ORM like Hibernate, etc. This is a very common requirement.

And what does it means that two instances of an object are equals? That depend of
the programmer understanding of this.

Precisely why the equals method should be used. If it is not used, then the behavior cannot be changed by developers.

I strongly advice, that you use the
getName method, as the code in question, is used as a framework by others and as
such, you should write code, for what you could expect, and you can not expect
that the equals method is overwritten.

As mentioned previously, there are many other reasons that you must override the equals method. The equals method is a standard Java method that must be overridden if you want to determine equality (this is documented on the equals method as well).

[spring-projects-issues]

Henrik Baastrup said:

I do follow your way of thinking, and I do agree with you, that you have the equals and hasCode methods to override when needed. The problem is; that object is not just objects, they are thought to have behaviors, and the Principal class has a behavior around its name, see the "JAAS Reference Guide" and "JAAS and Java GSS-API Tutorials" authorization in Java, as I understand it, is based on Principal names and personal I have only seen the name property used when two Principals are compared.

I advice you again to use the name property to compare the two Principals, as that is, as I understand it, the natural behavior of the Principal object.

If you look at the code I suggest, it takes into account that the Principal might have a special behavior around its name and for this reason use the equals method. It also takes into account that the object has overwritten the toString method such as the String object. I do believe this code will be more useful for the framework than yours, but I cannot, and do not want to, force you to change your code, so I will start to override the equals method, even I think it is not a natural behavior, on my Principals when use the SPRING framework. You should document this someplace too.

The "Single Sign-on Using Kerberos in Java" is an other very interesting paper on the subject

[spring-projects-issues]

Rob Winch said:

Resolving as fixed. In summary, the fix will use the equals method for the following reasons:

• The getName() method should only be used to compare the principal against a String
• The return type of getPreAuthenticatedPrincipal is an Object. Similarly the principal argument for User is an Object. This means we should support types other than Principal. The equals method will work on Objects that do not implement Principal.
• The equals method gives us greater flexibility than using getName()
• It is documented by the JDK that this is the purpose of the equals method. Furthermore, the equals method must be overridden for many other reasons (i.e. to support using the Collections framework, ORM's like Hibernate, etc).

[spring-projects-issues]

Henrik Baastrup said:

Having taken a look of the Spring code again, I think it comes down to what you want to trig the re-authentication on: Do you want to trig on difference in objects, from the getPreAuthenticatedPrincipal method and what the currentUser object contains, then use equals, or do you want trig on change in entity, then use getName. Please see at my code below

class SimplePrincipal implements Principal {
    String name;

    public SimplePrincipal(String name) {
        this.name = name;
    }
    @Override
    public String getName() {
        return name;
    }
}
class ComplexPrincipal implements Principal {
    String person;
    int age;

    ComplexPrincipal(String person, int age) {
        this.person = person;
        this.age = age;
    }
    @Override
    public String getName() {
        return person;
    }
    @Override
    public boolean equals(Object obj) {
        if (!(obj instanceof ComplexPrincipal)) return false;
        if (!((ComplexPrincipal)obj).person.equals(this.person)) return false;
        if (((ComplexPrincipal)obj).age!=this.age) return false;
        return true;
    }
}

SimplePrincipal sp = new SimplePrincipal("Henrik Baastrup");
ComplexPrincipal cp = new ComplexPrincipal("Henrik Baastrup", 28); //That is not my age ;-)

if (sp.getName().equals(cp.getName())) System.out.println("The two principals present the same entity but Spring does not understand that!");

if (sp.equals(cp)) System.out.println("This should not be possibly as the two principals are of different types");
else System.out.println("Sping has failed to detect that the two principals contain the same entity as they are of different type");

Do not forget that it might be very likely that the objects are different as they come from different part of the code, and for this reason you should trig on change in entity.
My opinion is that you have not solved the bug, but only added complexity to the code, as it does not handle components from the java.security package correct.

[spring-projects-issues]

Rob Winch said:

If ComplexPrincipal and Principal are not equal, then it should trigger re-authentication. The detection determines if the Principal has changed, it is not intended to determine if the Principal name has changed or not (although this may be reflected in the equals of your Principal Object in which case it has the same effect as detecting changes in getName). If you want the two to be treated as equal, then you need to make your equals methods reflect that for both objects. Again this is no different than if you were using Java Collections, Hibernate, etc.

[spring-projects-issues]

Henrik Baastrup said:

I don't feel you understand the idea behind principals. They are objects there present entities and can be of different types, even they present the same entity. This is especial valid in the current case, as it is not the same code there set the principals for the current user, as that there is use for pri-authentication, so you risk different principal types presenting the same entity and you can not use equals. If I want to use Authenticator A (could be one of the Authenticators there comes with Spring) and have my pri-authentication object B (written by me), then A might use one type of principals and B an other, and your code is not able to deistic if the entity has changed.
Equals is normal used on the same type of objects: here is a good article on the subject: http://www.a2ztechguide.com/2011/12/java-how-to-override-equals-and.html

[spring-projects-issues]

Rob Winch said:

I do see your points, but I am hesitant to use the proposed solution because I am concerned when another user logs a JIRA stating they have two different Principal objects that return the same value for getName() but are different Principals. Going back to your JAAS reference, consider the Subject API which allows for multiple Principal objects to be placed on it. One Principal may represent the username and a second Principal may represent a Group. In this instance, the two Principals may have the same result for getName(), but not be equal. This is only one scenario where it makes sense to have getName() return the same value but represent different Principals, but I am sure there are others that I am missing.

Using the equals method solves this issue because the user can determine what it means to be equal. However, as I mentioned I still see your points. With that said, I propose that we provide a method that can be overridden in AbstractPreAuthenticatedProcessingFilter that allows users to customize when the principal changed. The default behavior of this method will be the original implementation. The method would look like:

protected boolean principalChanged(HttpServletRequest request, Authentication currentAuthentication) {
    Object principal = getPreAuthenticatedPrincipal(request);

    if (currentAuthentication.getName().equals(principal)) {
        return false;
    }

    logger.debug("Pre-authenticated principal has changed to " + principal + " and will be reauthenticated");
    return true;
}

This would allow you to solve your situation with very minimal effort without making any assumptions. It would also account for the future use case.

[spring-projects-issues]

Henrik Baastrup said:

I think this is an acceptable solution.
Making the principalChanged protected and this way let the programmer choses an alternative solution for trigging re-authentication can also help in other situations. We could even begin to discuss if it should be abstract, but that would properly brake the architecture around pre-authentication.
In the favor of using equals can be said; Sprint does no implement JAAS at pre-authentication level, so if you want to use the javax.security package, override principalChanged.

[spring-projects-issues]

Henrik Baastrup said:

I now see (again) that the currentUser in the requiresAuthentication method implementing the java.security.Principal interface for this reason the right solution is something like my first proposal:

if (principal instanceof Principal) { 
  return !currentUser.getName().equals(((Principal)principal).getName());+
}
else {
  return !currentUser.getName().equals(principal.toString()); 
}

The output from the getName method is always a String, and as the String only can compare other String objects, you can not use the equals method, but must use the toString method on not java.security.Principal objects and should use the getName on java.security.Principal objects.
Sorry that I did not noticed this before, I think I forgot it a long the long discussion we had.

[spring-projects-issues]

Rob Winch said:

The toString() method may allow you to compare the two objects but it is not intended for checking equality. If you wish to modify the default behavior, then you will be able to override the principalChanged method that is to be added.