RFR: Re-enable support for non-Principal implementations of PrincipalComparator

[Sean Mullan]

Hi,

On 02/21/2013 07:13 AM, Neil Richards wrote:
> Hi,
> The change made for the RFE 7019834 [1] [2] [3] is built upon the
> assertion that:
>          "All PrincipalComparator implementations should already
>          implement Principal".
>
> However, the Javadoc for com.sun.security.auth.PrincipalComparator in
> the JAAS specification [4] states that:
>          "Although classes that implement this interface typically also
>          implement the java.security.Principal interface, it is not
>          required. In other words, classes may implement the
>          java.security.Principal interface by itself, the
>          PrincipalComparator interface by itself, or both at the same
>          time."
>
> Therefore, it is to be expected that there exist user implementations of
> PrincipalComparator which do not also implement Principal, and which are
> therefore broken by the change made by 7019834.

Do you have any evidence that there are user implementations of
PrincipalComparator which do not also implement Principal? I'm 
sympathetic to the compatibility issue, but would really like to see 
some evidence of real use cases.

> One example of this (prior to its modification by 7019834) was the jtreg
> test test/sun/security/provider/PolicyFile/Comparator.java, which
> (legitimately) had several classes implementing only
> PrincipalComparator.

Yes, though I would argue that the JavaPolicy implementation actually 
had a bug in it and was not compliant with the policy syntax [1]. These 
should be classes that implement the java.security.Principal interface.

> The changes made by 7019834 effectively both deprecate the use of
> PrincipalComparator - which is arguably fair enough (though the Javadoc
> for PrincipalComparator should to modified to reflect this change) - AND
> remove the support for its function, all in one fell swoop, which I
> suggest is not legitimate to do (within the same release as the initial
> deprecation).

Preserving compatibility is certainly important, so I do think its 
important that we understand if there are actual scenarios that might 
break, but I don't think we should add this extra code for something 
that is not compliant with the documented JavaPolicy syntax.

> Given this, I have prepared a suggested change [5] to re-enable support
> for honoring the JAAS spec in this regard for implementations of
> PrincipalComparator that do not also implement Principal.
>
> Trying to conform to the spirit of the 7019834 change, I've implemented
> this using reflection, so the code is tolerant of situations where
> PrincipalComparator is not available.
>
> For jtreg tests, I've copied the original form of the "Comparator" test,
> changing only its name (to "CompatibleComparator").
>
>
> Please review this change and let me know what you think.
>
> I'd be particularly interested in any suggestions regarding the
> Class.forName() call to find the PrincipalComparator class in the static
> initializer block - should it be wrapped in a doPrivileged block, or fed
> some specific ClassLoader ?
>
> Also, could a new bug id be generated for this, please ?
>
> Thanks,
> Neil
>
> [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7019834
> [2] http://mail.openjdk.java.net/pipermail/security-dev/2012-December/006270.html
> [3] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/bf6d0bca5ea7
> [4] http://docs.oracle.com/javase/7/docs/jre/api/security/jaas/spec/com/sun/security/auth/PrincipalComparator.html
> [5] http://cr.openjdk.java.net/~ngmr/comparator/webrev.00/
>

Thanks,
Sean

[1] 
http://docs.oracle.com/javase/7/docs/technotes/guides/security/PolicyFiles.html