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

[Xuelei Fan]

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
> 
> 
>