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

[Xuelei Fan]

Hi Martin,

Thank you for considering to make the improvement in this update.

The use of synchronizedMap may hurt the performance.  I may prefer to 
use ConcurrentHashMap and its computeIfAbsent​() method. (Read more, 
this map may be not necessary)

  621     interface CipherGenerator {
Could this interface be private?  (Read more, this interface may be not 
necessary)

  592         if (rcg != null && wcg != null) {
Per the current implementation, checking read or write cipher generator 
is sufficient.  If you want to check the read one only, there is no need 
to declare the CipherGenerator any more (line 621).  It could be placed 
in ReadCipherGenerator.  (Read more, may not need to update 
ReadCipherGenerator)

  589-597     boolean supports(ProtocolVersion protocolVersion) {
Together with the CipherGenerator interface, the logic of this block is 
actually about the transformation, not really about the cipher 
generator.  Is it simpler to check the transformation directly?


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()) {

BTW, the coding style looks a little bit uncommon to me (May not need 
this block, anyway just for your consideration).

  631   } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
  632   }
  633   cipherAlgorithms.put(algorithm, false);

Would you mind if not using blank catch block?
  631   } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
+          cipherAlgorithms.put(algorithm, false);
+          if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
+              ...
+          }
  632   }
-633   cipherAlgorithms.put(algorithm, false);

Thanks & Regards,
Xuelei

On 5/27/2019 2:00 PM, Martin Balao wrote:
> Hi Xuelei,
> 
> Thanks to you for raising these concerns and providing your feedback.
> 
> On 5/24/19 7:47 PM, Xuelei Fan wrote:
>> Good, I have no further comment for this update.  Please go ahead.
>>
>> I think there is a possible improvement by calling
>> Cipher.getInstance(algorithm) only one time for each transformation
>> algorithm.  But may not worthy as the duplicated transformation
>> algorithm number is still small.  I'm fine if you want to leave it as it
>> is.
> 
> Yes, I agree on both statements: there can be an improvement there but
> it won't be significant. Here it's Webrev.02:
> 
>   * http://cr.openjdk.java.net/~mbalao/webrevs/8223482/8223482.webrev.02/
> 
> At Webrev.02, Cipher.getInstance results are cached so we don't need to
> create instances unnecessary.
> 
> Benchmarks do not show any significant performance increase nor decrease
> compared to Webrev.01:
> 
> Benchmark                                      (testMode)   Mode  Cnt
>   Score     Error  Units
> SupportedCiphersuites.test_TLS12Communication        FIPS  thrpt   10
> 167.393 ±  35.659  ops/s
> SupportedCiphersuites.test_TLS12Communication    NON_FIPS  thrpt   10
> 593.441 ± 110.044  ops/s
> 
> FIPS_with_8223482_webrev02.txt average: 396.86
> NON_FIPS_with_8223482_webrev02.txt average: 888.05
> 
> Are you okay to go with Webrev.02?
> 
> Kind regards,
> Martin.-
>