Code review request, CR 7174244, NPE in Krb5ProxyImpl.getServerKeys()

[Steven Read]

Xuelei Fan <xuelei.fan at ...> writes:

> 
> On 6/6/2012 9:11 PM, Weijun Wang wrote:
> > Hi Xuelei
> > 
> > I don't quite understand the bug report. Certainly it's good to prevent
> > that NPE, but why must the cipher suites be ordered? It seems it's not
> > ordered before the new HashMap.
> No, it was not ordered because of a regression.
> 
> > Is this a regression or an RFE?
> It's a regression. We used to use TreeSet to sort the cipher suites. But
> later it is miss-updated to use ArrayList rather then TreeSet in a bug
> fix. Then the order was lost.
> 
> We need to sort the cipher suites because the order is the preference
> when TLS handshaking selects a proper cipher suite. Please refer to TLS
> specification about the order, page 41, RFC5246:
> 
>    cipher_suites
>       This is a list of the cryptographic options supported by the
>       client, with the client's first preference first.
> 
> > You'd
> > better add some explanation in the bug report.
> > 
> I will.
> 
> Thanks,
> Xuelei
> 
> > Thanks
> > Max
> > 
> > On 06/06/2012 08:27 PM, Xuelei Fan wrote:
> >> Hi Weijun,
> >>
> >> Would you please review the fix for CR 7174244.
> >>      http://cr.openjdk.java.net./~xuelei/7174244/webrev.00/
> >>
> >> There are mainly two updates in the fix. One is to have
> >> Krb5ProxyImpl.getServerKeys()  to check null return value of
> >> Krb5Util.getServiceCreds(). The other one is to have cipher suite order
> >> to be independent from HashMap implementation.  TreeSet is used to
> >> replace ArrayList so that the cipher suites order are sorted
> >> automatically.
> >>
> >> Thanks,
> >> Xuelei
> 
> 

Hi Xuelei,

My only comment is that providing the ciphers in preference order goes beyond
what the API call specifies. I would agree it makes a lot of sense to have them
returned in order, but other implementations of JSSE do not necessarily provide
them in preference order as they are not required to - certainly looking at
JDK1.6.0 on IBM earlier today it was not.

Therefore, anyone wishing to write cross-JDK compatible code would be wrong to
assume that they don't need to order the ciphers by preference themselves as
well. Perhaps it is possible to do a docs change on the abstract classes to
indicate that the order is not guaranteed across JSSE implementations.

Best regards
Steve