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

[Martin Balao]

Hello Alexey,

Apologies for the delay. I was busy with the upcoming CPU, but my
expectation is that we can finish the TLS 1.3 work without any other
interruption now.

Comments inline.

Thanks,
Martin.-

--


On 6/16/20 3:07 PM, Alexey Bakhtin wrote:
>> * 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
> 

Ok.

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

Ok. Please keep in your backlog the CSR documentation of this change; so
any user affected knows why and what's the backward-compatibility
workaround for 'disabled by default' ciphers.

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

Ok, I talked to one of the 8u maintainers and as long as there are
significant/original changes into the file, a new copyright line added
should be fine. I'll leave this judgement up to you or to anyone else
who want to participate; I'd rather focus on more technical stuff for
this review.

>>  * I suggest to be explicit in what we import, instead of "java.security.*”
> OK. Added explicit declaration

Ok, thanks.

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

Ok.

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

That's right. My fault.

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

Ok.

>>
>>  * No gain in inserting a newline after “return”
> Fixed.

Ok.

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

You're right.

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

Oh, good point! Hmm... let me give this some more thought, because I
still find it confusing.

Thanks,
Martin.-