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

[Steven Read]

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

> 
> On 6/8/2012 8:31 PM, Steven Read wrote:
> > 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.
> > 
> Before the reply, I want to explain that the fix only impact the default
> cipher suites preference of Oracle provider. It does not impact the user
> customized cipher suites via SSLXxxxx.setEnabledCipherSuites().  Hope we
> are on the same page on this point.
> 
> Right. It's not a part of java specification. It's a behavior of Oracle
> provider. It may not apply third parties' providers.
> 
> Different provider may define different preferences, it is hard to
> define what's the preference exactly. And it is hard to tell which
> provider provides a better preference for a certain circumstance.
> Cross-provider applications which care about the exactly order should
> not reply on providers preference.
> 
> > 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.
> If the application uses the default cipher suites, every provider has
> its own default preference.  If the application uses the customized
> cipher suites, the preference is defined by the application, providers
> should not (should not, is it really true for every provider? I'm not
> sure) adjust the users preference.  So I'm not sure I understand in
> which case the user will run into the confusion you mentioned above.
> 
> > Perhaps it is possible to do a docs change on the abstract classes to
> > indicate that the order is not guaranteed across JSSE implementations.
> > 
> I think maybe no words to guarantee the order is to say that it may be
> ordered, may be not. IMHO, I did not see a very strong reason to declare
> a maybe-or-maybe-not spec.
> 
> Thank you very much for looking into the issue and the feedback.
> 
> Thanks & Regards,
> Xuelei
> 
> > Best regards
> > Steve
> > 
> > 
> > 
> 
> 

Hi Xuelei, yes I agree that the reordering doesn't affect those ciphers set 
using SetEnabledCipherSuites, and that yes other providers can have different 
preference orders returned by getSupportedCipherSuites. It is also true that 
they won't necessarily sort by an order at all.

In the Sun/Oracle JDK 5 and 6, was the order by preference? As far as I can 
see from the commit log this is new functionality introduced in change 6976117.

My concern is for anyone trying to dynamically use the order of 
getSupportedCipherSuites to enable a subset of ciphers. Order is not 
guaranteed, so I cannot see where order would be used besides in printing it 
or similar. For consistent behaviour regarding cipher preference a developer 
must reference a list of their own ciphers instead, even if it is based on a 
JSSE provider's list.

If getSupportedCipherSuites returns an unsorted list in JDK5 and JDK6, then 
there should in my opinion be measures to stop compilation of code written 
against the sorted list functionality with a JDK5/6 compiler. I therefore feel 
that a new method call would be better in that it cannot be mis-used in older 
JDK versions and JDKs by other vendors. That way, the documentation for the 
new method can then say that the order of the ciphers is sorted - and people 
can trust it. 

>From my investigations it appears to me the ciphers from 
getSupportedCipherSuites have never been sorted in released code, even if the 
TreeSet was originally intended in place of the ArrayList. Your original 
response to Max gave me the impression this feature existed already, but on 
closer inspecition it looks like the initial commit was an ArrayList and my 
experiences on JDK6 (Sun and IBM) have suggested that the order is unsorted. I 
have only skimmed over code so please tell me if the original code was ordered 
as well as filtered.

I hope I have managed to explain my concerns clearly. I think that an ordered 
list of ciphers is great, but am concerned it is done in a way that is not 
dangerous to those running old/different JDKs.

Best regards
Steve