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

[Martin Balao]

Hi Xuelei,

Thanks for your feedback.

On 5/28/19 2:57 PM, Xuelei Fan wrote:
> 
> 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)

I've not seen any performance degradation with current benchmarks but
this is always good to know. Thanks.

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

Yes, I guess we can.

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

Yes, it's sufficient now because of the current implementation. However,
the existence of ReadCipherGenerator and WriteCipherGenerator as
separate classes suggests to me that there may be a difference. I have a
similar comment below.

>  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()) {
>
* I guess you meant suite.bulkCipher.isAvailable().

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.

It's still possible to have the "available transformations" cache but I
guess, per your first comment, that you prefer not to have it.

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

Yes, sure.

Here it's Webrev.03 with all the previous changes:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8223482/8223482.webrev.03/

Thanks,
Martin.-