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

Tue Jun 12 00:24:02 UTC 2012

Hi Steven,

JSSE is a provider based framework.  And compatibility is one of the
most important facts users concerns.  That's to say, a provider
developed for JDK 6 is expected to work with JDK 7.  As implies that
sometimes, we cannot changes the specification for some certain cases.
Otherwise, we would run into compatibility issues. For this case, we did
not require supported cipher suites are ordered. Providers may not
implemented to support sorted supported cipher suites. If we update the
spec to require to sort supported cipher suites in preference, we would
face the compatibility issue, and the old providers would not be able to
be used any more. So IMHO, unless there is very very strong concerns, we
are just not able to update the specification.

The preference order is an option of JSSE providers. Although it is not
specified in API specification, SunJSSE providers would like to keep the
behaviors consistent between different releases. SunJSSE provider
defined the preference since 1.4.2 or earlier. But the preference may be
changed in different builds or releases. You may find that
"SSL_RSA_WITH_RC4_128_MD5" is the most preferable cipher suite in JDK
1.4.2, but in JDK 7, "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384" becomes
the most preferable one.

So unless you use the default cipher suites, it is not good to trust the
order of supported cipher suites.

Hope it answers your questions.

Regards,
Xuelei

On 6/11/2012 6:18 PM, Steven Read wrote:
> 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
> 