[8u] TLSv1.3 RFR: 8245474: Add TLS_KRB5 cipher suites support according to RFC-2712

[Martin Balao]

On 5/21/20 11:12 AM, Alexey Bakhtin wrote:
> Please review changes required to backport TLSv1.3 protocol from JDK11.0.7 to JDK8u

Hi Alexey,

A few comments / questions regarding Step 7 (8245474) v0:

 * Do we have tests covering this functionality?

 * CipherSuite.java

  * TLS_KRB5_WITH_3DES_EDE_CBC_SHA
   * Shouldn't this be enabled for protocols previous to TLS 1.2 too?
    * Note: I'm okay with TLS 1.2 being the upper limit.
   * Shouldn't we check whether SunJSSE::isFIPS is true or not before
setting isDefaultEnabled to 'true'?
    * I'm more inclined to set the isDefaultEnabled value to 'false',
even while not in FIPS mode. I've seen many ciphersuites which were 'N'
in the old TLS engine and are now isDefaultEnabled == false. Even if we
change the 'default behavior', I believe we should disable everything
not secure by default. That's precisely one of the points of upgrading
the TLS engine, and what we are doing for other non-KRB5 ciphersuites
already (since Step 1). What do you think?
     * Looks to me that jdk.tls.client.cipherSuites and
jdk.tls.server.cipherSuites properties can be used to override the default
      * We should document in a CSR all these changes so users can
understand how the default configuration is different from the previous
and how to enable a 'non-secure' but 'backward-compatible' behavior.
     * Please have a look at the tests that may be affected by any of
these changes.

  * With the previous comments in mind, please have a look at the other
KRB5 ciphersuites added.

 * ClientHello.java

  * I've seen there is a copyright line addition. If you want to keep it
there, please get approval from 8u maintainers because I'm not sure of
the policy there.
   * Same comment for SSLKeyExchange.java, SSLSessionImpl.java and
ServerHello.java
   * This comment does not apply to new files, which look okay to me

  * I suggest to be explicit in what we import, instead of "java.security.*"

  * I've seen a double-spacing in "} else  if (suite.keyExchange ==
K_KRB5 ||"

  * Are changes needed for previous versions of the protocol? TLS 1.1,
TLS 1.0, etc.

  * I suggest to follow the pattern. Create a new condition of type "if
(resumingSession && ..." instead of hooking "Validate that the cached
cipher suite" one

  * No gain in inserting a newline after "return"

 * ServerHello.java

  * Are changes needed for previous versions of the protocol? TLS 1.1,
TLS 1.0, etc.

 * HandshakeContext.java

  * I suggest to move sun/security/ssl/krb5/* files to sun/security/ssl
so we can keep HandshakeContext and other classes visibility unchanged.
This change will probably eliminate the need to add getMajor and
getMinor methods in ProtocolVersion, as well as changing valueOf method
visibility.

 * KrbClientKeyExchange.java

  * Can we make it more similar to DHClientKeyExchange.java /
ECDHClientKeyExchange.java / RSAClientKeyExchange.java?
   * I.e.: can we make the class final? can we have analogous comments
in the same locations? can we refer to the constructor through
KrbClientKeyExchangeConsumer instead of
KrbClientKeyExchange.KrbClientKeyExchangeConsumer? etc. None of these
examples are critical but I wish we could stick to the 'template' as
much as possible. More on this below.

 * Why do we have KerberosClientKeyExchange.java and
KrbClientKeyExchange.java files?
  * I see that for other key exchange methods there is only one file,
containing the class that inherits from HandshakeMessage. This makes me
think that KerberosClientKeyExchange class should probably be renamed to
KrbClientKeyExchangeMessage (having KerberosClientKeyExchange and
KrbClientKeyExchange classes simulateneously looks confusing to me and
does not follow the previous *ClientKeyExchange template).

 * KerberosClientKeyExchangeImpl.java
  * I wonder if it makes sense to have a separate "Impl" class. None of
the other key exchange methods are designed this way. I don't see how
KerberosClientKeyExchange::implClass could be assigned to null or to
something different than KerberosClientKeyExchangeImpl class. I could
track this to 6885204 and 6894643. Unless there is a good reason, I
suggest to get rid of this complexity and stick to the 'template'.

As a general criteria, I suggest to adapt-to/do-not-modify the new TLS
engine over keeping the old files/code unchanged. Looks to me that
handling this as a new feature is easier and more clear to review. This
would be a different strategy to what we have been doing in previous
Steps, and I see two reasons: 1) we are not re-introducing a specific
change because the TLS KRB5 has been in the old TLS engine since the
beginning of OpenJDK -so this cannot be a patch vs patch review-, and 2)
it's better for clarity and consistency to analyze this feature in the
context of the new engine, as an instance of a key exchange method such
as RSA, DH, etc.

There may be more comments but wish we could focus on these things
first. Thus, I expect at least a couple of iterations more.

Please feel free to comment / argue on any of the previous points before
working on a new webrev, so we are in the same page first.

Thanks,
Martin.-