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

[Alexey Bakhtin]

Hello Martin,

Thank you for review.
Please see my answer below.

Updated webrev at: http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245474/webrev.v1/

Regards Alexey

> On 16 Jun 2020, at 03:10, Martin Balao <mbalao at redhat.com> wrote:
> 
> 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
> 
Yes. The tests are located in sun/security/krb5/auto directory
It is part of JDK-8245681 subtask.
Also, some regular ciphersuite tests also test TLS_KRB cipher suites

>  * 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.
Yes. You are right. Fixed to enable for TLS_KRB for early TLS versions
Maximum supported version is TLSv1.2
>   * 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.
> 
Agree. These suites should be disabled by default. Fixed.
Also, the tests will be updated to enable TLS_KRB5 suites where it is required.

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

Sure. All TLS_KRB5 cipher suites are updated accordingly
> 
> * 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've added copyrights to the files with new TLS_KRB5 code, not a backport.
But sure, I’ll check if it is possible to add copyright lines to these files

> 
>  * I suggest to be explicit in what we import, instead of "java.security.*”
OK. Added explicit declaration
> 
>  * I've seen a double-spacing in "} else  if (suite.keyExchange ==
> K_KRB5 ||”
Fixed
> 
>  * Are changes needed for previous versions of the protocol? TLS 1.1,
> TLS 1.0, etc.
> 
T12ClientHelloConsumer is used for TLSv1.2 and prior protocol versions.
I think it is right place for all non-TLSv1.3 protocols

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

OK. Updated the code to follow the pattern.

> 
>  * No gain in inserting a newline after “return”
Fixed.
> 
> * ServerHello.java
> 
>  * Are changes needed for previous versions of the protocol? TLS 1.1,
> TLS 1.0, etc.

T12ServerHelloConsumer is used for TLSv1.2 and prior protocol versions.
I think it is right place for all non-TLSv1.3 protocols
> 
> * 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.
> 
Separation of the Kerberos functionality was done intentionally.
JDK8 compact profile1 does not include Kerberos API and implementation.
So, such separation allows to detect Kerberos presence at runtime
and support TLS_KRB cipher suites if implementation exist only.
Also, this is a reason we have different classes: KerberosClientKeyExchange,
KerberosClientKeyExchangeImpl and KrbClientKeyExchange
We can not merge them together without breaking compact profile.
I’ll try to make KrbClientKeyExchange more similar to template but
proxy and delegation classes are still required.
This issue and listed below are not included in updated webrev.

> * 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.-
> 
>