RFR 8223482: Unsupported ciphersuites may be offered by a TLS client

[Xuelei Fan]

On 5/28/2019 3:14 PM, Martin Balao wrote:
>> If we could check the transformation directly, it might be better to
>> have the check in the SSLCipher constructors, using immutable enum field.
>>
>> Putting them together:
>>
>> SSLCipher.java:
>>      private SSLCipher(String transformation,
>>        ...
>>        this.isAvailable =
>> -         allowed && isUnlimited(keySize, transformation);
>> +         allowed && isUnlimited(keySize, transformation) &&
>> +         isAvailable(transformation);
>>      }
>>
>> +   private static boolean isAvailable(String transformation) {
>> +      if (!transformation.equals("NULL")) {
>> +          ...  // check the instance
>> +      } else {
>> +          ...
>> +      }
>> +   }
> 
>> SSLContextImpl.java:
>>   382    if (!suite.supports(protocol) ||
>> -383            !suite.bulkCipher.supports(protocol)) {
>> +383            !suite.isAvailable()) {
>>
> * I guess you meant suite.bulkCipher.isAvailable().
> 
No, I meant suite.isAvailable(), but suite.bulkCipher.isAvailable() also 
works for the update.  I will leave it to you for the final decision, 
not more code review required to me.

> Yes, this was something I considered at the beginning but had some
> design concerns. The Read/WriteCipherGenerator is chosen according to
> the protocol, and is the one that may have problems with the
> transformation when creating an instance. This generic design gives me
> the idea that there may be different cipher implementations (per
> protocol) and even per write or read operation. We can simplify based on
> the current implementation but, at some point, I believe we are going
> against the generic design. Anyways, I'll take it.
> 
With "!suite.supports(protocol) || !suite.isAvailable()", the protocol 
specific is checked with suite.supports(protocol), while the 
availability is checked with suite.isAvailable().  I think the design 
should be fine.

> It's still possible to have the "available transformations" cache but I
> guess, per your first comment, that you prefer not to have it.
> 
I'm about 50% to 50% to use cache in enum constructor: a balance of 
memory or CPU.  Once the enum get constructed, the cache is useless.  So 
I prefer your current update unless the duplicated transformations is 
not a few.

> 
> Here it's Webrev.03 with all the previous changes:
> 
>   * http://cr.openjdk.java.net/~mbalao/webrevs/8223482/8223482.webrev.03/
> 
It looks good to me.

Thank you very much for taking my comments!

Regards,
Xuelei