new webrev: http://cr.openjdk.java.net/~xuelei/7188657/webrev.01/
On 9/4/2013 10:35 AM, Weijun Wang wrote:
> Mostly good, only some word/style issues.
>
> SSLParameters.java:
>
> 83 * server name matchers are set to <code>null</code>, cipher
> suites
> 84 * preference, wantClientAuth and needClientAuth are set to
> 85 * <code>false</code>.
>
> Why not just use "preferLocalCipherSuites" instead of "cipher suites
> preference"? Yes it looks ugly to refer to a variable name, but you've
> already used "wantClientAuth". Or, at least use "useCipherSuitesOrder"
> because that's used in the public method names.
>
Good catch. I will use "useCipherSuitesOrder".
> Handshaker.java:
>
> 148 // Whether local cipher suites preference in server side should be
> 149 // honored during handshaking?
> 150 boolean preferLocalCipherSuites = false;
>
> Since you apply the flag to both server and client, how about adding
> something like "(it's always honored in client side)".
>
Better to have such words.
// Whether local cipher suites preference should be honored during
// handshaking?
//
// Note that in this provider, this option only applies to server side.
// Local cipher suites preference is always honored in client side in
// this provider.
> 550 boolean isNegotiable(CipherSuite s) {
>
> You might need to update the doc for this method saying "within the
> current active cipher suites". You can even let it call the new
> isNegotiable(*,*) method.
>
Good suggestion.
> UseCipherSuitesOrder.java:
>
> 2 * Copyright (c) 2001, 2011, Oracle and/or its affiliates. All
> rights reserved.
>
> Should be "2001, 2013".
>
Should be 2013.
> 157 // client enabled cipher suites
> 158 private static String[] CliEnabledCipherSuites;
> 159
> 160 // server enabled cipher suites
> 161 private static String[] SrvEnabledCipherSuites;
>
> It looks weird for a variable to starts with a capital letter.
>
It looks weird to me, too. Not sure what I was thinking at that time.
Thanks,
Xuelei
> Thanks
> Max
>
> On 8/28/13 5:02 PM, Xuelei Fan wrote:
>> Hi,
>>
>> Please review this update to support cipher suites reorder:
>>
>> webrev: http://cr.openjdk.java.net/~xuelei/7188657/webrev.00/
>>
>> Two new methods are added to SSLParameters:
>> public final void setUseCipherSuitesOrder(boolean honorOrder);
>> public final boolean getUseCipherSuitesOrder();
>>
>> If SSLParameters.getUseCipherSuitesOrder() return true, the local cipher
>> suites order returned in SSLParameters.getCipherSuites() should be
>> honored during SSL/TLS handshaking.
>>
>> Considering the potential compatibility issues of third party's
>> implementation, I won't define the behaviors if
>> SSLParameters.getUseCipherSuitesOrder() return false. For Oracle
>> provider, SunJSSE, if getUseCipherSuitesOrder() returns false, the order
>> of SSLParameters.getCipherSuites() is honored in client side, and the
>> order of the requested cipher suites in client handshake message is
>> honored in server side.
>>
>> Thanks,
>> Xuelei
>>
