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

[Martin Balao]

On 7/6/20 5:45 PM, Martin Balao wrote:
> 
> Oh, good point! Hmm... let me give this some more thought, because I
> still find it confusing.
> 

Hi,

I'd like to propose Webrev.02:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8245474/8245474.webrev.02/

Diff between Webrev.01 and Webrev.02:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8245474/v2.patch

The main driver behind Webrev.02 was to align the Kerberos Key Exchange
implementation to the new TLS engine, while addressing the Java 8 SE
Embedded compatibility concerns you raised in response to my previous
Webrev.00 comments. Other improvements were also included as part of
this change. For reviewing purposes, I suggest to apply a 'new
implementation' approach instead of trying to track changes to old
TLS-Kerberos vestiges.

The most relevant classes in the new design are:

 * KrbClientKeyExchange::KrbClientKeyExchangeProducer
  * Used by TLS clients to produce a Kerberos Client Key Exchange
message, which contains the encrypted pre-master key for a TLS server.
Encryption is done with the Kerberos ticket session key.
  * This class is analogous to
RSAClientKeyExchange::RSAClientKeyExchangeProducer

 * KrbClientKeyExchange::KrbClientKeyExchangeConsumer
  * Used by TLS servers to receive a Kerberos Client Key Exchange
message, which contains the encrypted pre-master key from a TLS client.
Decryption is done with the Kerberos ticket session key.
  * This class is analogous to
RSAClientKeyExchange::RSAClientKeyExchangeConsumer

 * KrbClientKeyExchange::KrbClientKeyExchangeMessage
  * Kerberos Client Key Exchange message, used by the TLS client to
create a new message and by the TLS server to decode a message coming
from the network stream.
   * Messages have information such as the encrypted pre-master secret,
the decrypted pre-master secret, the Kerberos local principal and the
Kerberos remote principal
  * All the Kerberos-specific processing is done by the
sun.security.ssl.krb5.KrbClientKeyExchangeHelperImpl helper class
(through the KrbClientKeyExchangeHelper interface), dynamically loaded
when available
   * Processing includes getting Kerberos tickets and using the Kerberos
session key to encrypt or decrypt the pre-master secret
  * This class is analogous to
RSAClientKeyExchange::RSAClientKeyExchangeMessage

 * KrbClientKeyExchangeHelper
  * Interface to expose Kerberos-specific functionality to TLS code.
This class has no analogous class in other key exchange schemes.

 * KrbClientKeyExchangeHelperImpl
  * Implementation of the KrbClientKeyExchangeHelper interface from the
Kerberos side. This class has no analogous class in other key exchange
schemes.

 * KrbKeyExchange::KrbPossessionGenerator
  * For a Kerberos cipher-suite to be usable by a TLS server, the
Service credentials must be retrieved. In other words, the TLS server
must be able to authenticate against a KDC. In Webrev.01, a Kerberos
cipher-suite was considered available without checking this condition.
This was a regression from previous functionality in the old TLS engine.
Credentials are held as a possession, in a
KrbKeyExchange::KrbServiceCreds instance which is added to the Server
Handshake Context. It's tempting to think that
KrbKeyExchange::KrbServiceCreds should inherit from SSLCredentials
(instead of SSLPossession), but server possessions is used to decide
whether or not a ciphersuite is available. See
T12ServerHelloProducer::chooseCipherSuite method (ServerHello.java).

 * KrbKeyExchange::KrbServiceCreds
  * Service credentials used by the TLS Server. These credentials
contain the Kerberos session key used to decrypt the pre-master secret,
sent by the TLS client. See KrbKeyExchange::KrbPossessionGenerator.

 * KrbKeyExchange::KrbPremasterSecret
  * Class that represents the pre-master secret. Has methods to create a
new secret instance or to decode and validate a secret received from a
network stream.

As a consequence of the new design, we no longer need to expose TLS
interfaces, classes, fields or methods to Kerberos-specific classes.
There is a strict separation with the goal of processing TLS stuff on
the TLS side only. In example, the pre-master secret validation happens
on the TLS side and there is no need for Kerberos classes to access the
ProtocolVersion class, ProtocolVersion::minor field,
ProtocolVersion::major field or ProtocolVersion::compare method -which
remain package-private-.

In addition to these changes, I analyzed how the Kerberos Client Key
Exchange functionality was coupled to the previous TLS engine and how
that is addressed now:

 * KRB5 + TLS 1.2 (or below)

  * When Key Exchange is KRB5, no server Certificate message is received
(Client side)
   * Ok: the client consumes the ServerHello message in
T12ServerHelloConsumer::consume (ServerHello.java). When establishing a
new session, the SSL Key Exchange instance (ke) is obtained and
ke.getRelatedHandshakers is called to push handshake consumers to the
list. Execution goes to SSLKeyExchange::getRelatedHandshakers method.
SSLKeyExchange::getRelatedHandshakers calls ::getRelatedHandshakers
methods for 'authentication' and 'key agreement' instances. Given that
SSLKeyExchange::SSLKeyExKRB5EXPORT.KE is assigned to an SSLKeyExchange
instance that has X509Authentication authentication = null, a
Certificate consumer is not returned there. Note:
X509Authentication::getRelatedHandshakers is the method that returns
both a SSLHandshake.CERTIFICATE and a CERTIFICATE_REQUEST consumer.
T12KeyAgreement::getRelatedHandshakers method, on the other side, gets
called but does not return a CERTIFICATE or a CERTIFICATE_REQUEST consumer.

  * When key Exchange is KRB5, no server Certificate Request message is
received (Client side)
   * Ok: for the same reason that a Certificate consumer is not pushed
to the list.

  * When Key Exchange is KRB5, no server Server Key Exchange message is
received (Client side)
   * OK: In T12KeyAgreement::getHandshakeConsumers, a
SERVER_KEY_EXCHANGE consumer is not returned for KRB Key Exchange. This
is called from SSLKeyExchange::getHandshakeConsumers, which is in turn
called from ServerKeyExchangeConsumer::consume (ServerKeyExchange.java).
A SERVER_KEY_EXCHANGE consumer may still be pushed to the list -or, in
other words, ServerKeyExchangeConsumer::consume may still be called-.
Why? T12KeyAgreement::getRelatedHandshakers may return a
SERVER_KEY_EXCHANGE because possessionGenerator != null for KRB Key
Exchange. T12KeyAgreement::getRelatedHandshakers is called from
SSLKeyExchange::getRelatedHandshakers, which is in turn called from
T12ServerHelloConsumer::consume for new SSL sessions. The reason why
this is okay is because once ServerKeyExchangeConsumer::consume gets
called, no SERVER_KEY_EXCHANGE consume is found associated to the Key
Exchange and a "Unexpected ServerKeyExchange handshake message."
exception is finally thrown. In the previous TLS engine,
ClientHandshaker::processMessage method has a
'HandshakeMessage.ht_server_key_exchange' switch-case scenario in which
an exception is thrown if Key Exchange is K_KRB5 or K_KRB5_EXPORT.

  * Server does not send Certificate message when Key Exchange is KRB5
(Server side)
   * OK: a Certificate producer is never pushed to the list of producers
because SSLKeyExchange::SSLKeyExKRB5EXPORT.KE is assigned to an
SSLKeyExchange instance that has X509Authentication authentication =
null. X509Authentication::getHandshakeProducers (called from
SSLKeyExchange::getHandshakeProducers) is the method that would return a
SSLHandshake.CERTIFICATE producer. SSLKeyExchange::getHandshakeProducers
also calls T12KeyAgreement::getHandshakeProducers but no CERTIFICATE
producer is returned from there. SSLKeyExchange::getHandshakeProducers
is called from T12ServerHelloProducer::produce (ServerHello.java), which
gets the producers from the SSL Key Exchange instance through a
'ke.getHandshakeProducers' call.

  * No Server Key Exchange sent for KRB5 Key Exchange (Server side)
   * OK: for KRB Key Exchange (server side),
T12KeyAgreement::getHandshakeProducers does not return any
SERVER_KEY_EXCHANGE producer. This is called from
SSLKeyExchange::getHandshakeProducers, which is in turn called from
T12ServerHelloProducer::produce ('ke.getHandshakeProducers(shc)' call).
SSLKeyExchange::getHandshakeProducers does not call
X509Authentication::getHandshakeProducers because
SSLKeyExchange::SSLKeyExKRB5EXPORT.KE is assigned to an SSLKeyExchange
instance that has X509Authentication authentication = null.

  * Server sends a CertificateRequest message only if Key Exchange is
not KRB5 (Server side)
   * OK: The server can add a CertificateRequest producer to the list in
T12ServerHelloProducer::produce. To decide that, it calls
SSLKeyExchange::getRelatedHandshakers and expects a CERTIFICATE producer
to be received. SSLKeyExchange::SSLKeyExKRB5EXPORT.KE is assigned to an
SSLKeyExchange instance that has X509Authentication authentication =
null, so X509Authentication::getRelatedHandshakers is not called from
SSLKeyExchange::getRelatedHandshakers.
SSLKeyExchange::getRelatedHandshakers then calls
T12KeyAgreement::getRelatedHandshakers.
T12KeyAgreement::getRelatedHandshakers does not return a CERTIFICATE
handshake producer.

  * Server tries to set the KRB5 key exchange if setupKerberosKeys succeeds
   * OK: When the server decides whether or not the KRB key exchange is
available, KrbKeyExchange::KrbPossessionGenerator::createPossession is
called and performs what was done by setupKerberosKeys in the old TLS
engine. Only after succeeding is that the KRB ciphersuites are
considered available. This was fixed from Webrev.01.

  * Client Hello processing (Server side)
   * Session resumption
    * "// validate subject identity" logic in ServerHandshaker.java
     * OK: verified in T12ClientHelloConsumer::consume (new TLS engine)
      * Changed SSL log from "session" to "ssl,handshake,verbose" which
seems a better fit for the context

  * Server Hello processing (Client side)
   * Session resumption
    * "// validate subject identity" logic in ClientHandshaker.java
     * OK: verified in T12ServerHelloConsumer::consume (new TLS engine)
      * SSL log changes as in Client Hello processing

  * Client Key Exchange Message (send after Server Hello Done - Client side)
   * KerberosClientKeyExchange -> send logic
    * Ok: KrbClientKeyExchange::KrbClientKeyExchangeMessage has the
sending logic
   * Calculate keys with preMasterSecret
    * Ok: the pre-master secret is held in
KrbKeyExchange::KrbPremasterSecret, and
KrbKeyExchange::KrbPremasterSecret::createPremasterSecret method
generates the pre-master secret

  * Server receives a ht_client_key_exchange when Key Exchange is KRB5
(Server side)
   * KerberosClientKeyExchange -> receive logic
    * Server gets the preMasterSecret to calculate keys
     * Ok: When Key Exchange is KRB5, the
KrbClientKeyExchange::krbHandshakeConsumer consumer will handle the
incoming message. This consumer was returned by
SSLKeyExchange::getHandshakeConsumers, that calls
T12KeyAgreement::getHandshakeConsumers.
KrbClientKeyExchange::KrbClientKeyExchangeMessage has the message decode
logic.

  * Calling getPeerCertificates, getPeerCertificateChain or
getCertificateChain on a SSLSession where Key Exchange is KRB5, throws
an exception
   * OK: I implemented the check in SSLSessionImpl.java for a more
descriptive exception message.

  * Calling getPeerPrincipal on a SSLSession where keyExchange is KRB5
should return a non-null peerPrincipal
   * OK: implemented in SSLSessionImpl.java. Minor doc change. I believe
the "cipherSuite.keyExchange == K_KRB5*" check is not needed because
peerPrincipal shouldn't be set for non KRB5 key exchanges and peerCerts
should be null for KRB5 (no CertificateMessage allowed); no need for a
redundant -product- run-time check.

  * Calling getLocalPrincipal on a SSLSession where keyExchange is KRB5
should return a localPrincipal (may be null)
   * OK: implemented in SSLSessionImpl.java. Minor doc change. I believe
the "cipherSuite.keyExchange == K_KRB5*" check is not needed because
localPrincipal shouldn't be set for non KRB5 key exchanges and
localCerts should be null for KRB5 (no CertificateMessage allowed); no
need for a redundant -product- run-time check.


I'd appreciate if you can have a look, given your knowledge after
working on Webrev.00 and Webrev.01. I guess we might still need an
independent Reviewer here to proceed, once you are okay with the changes.

NOTE 1: I'll be revieweing this myself one more time, but wanted to
share it ahead of time for you to start looking at the changes, as there
are many.

NOTE 2: Tests were not run. This should not be a problem as test MUST
pass in next steps. If you have them ready, I'd appreciate any feedback
on that front :-)

Thanks,
Martin.-