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

[Neil Richards]

Hi Sean,
Thanks for your quick response.

I admit, I hadn't spotted the description of the policy file syntax to
which you point.

(In my defence, it's a lot easier to overlook than the explicit wording
that I found at the top of PrincipalComparator's Javadoc).

Just for info, are there other scenarios where (non-Principal)
PrincipalComparator impls can (still) be used, which matches that class'
Javadoc ?

And do these other scenarios also (already) support the use of
Principal.implies() ?

I think your answer may have obviated my desire for using the suggested
fix. 
I'm asking around nearby to see if evidence of real use breakage can be
found, and will tug on this thread again if/when I have something more
to share on this.

Regards,
Neil

On Thu, 2013-02-21 at 17:38 -0500, Sean Mullan wrote:
> 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
> 


-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU