Code review request: JDK-8046295 - Support Trusted CA Indication extension

Thu Jun 8 22:09:15 UTC 2017

Hi Xuelei,

Thanks for your time reviewing this implementation.

You have a point in interoperability considerations. A client may decide
not to send this extension to avoid an error on the server: either because
it does not know the server implementation and assumes that it won't
support the extension or because it knows that server does not support this
extension. Given that the extension is not widely implemented (as far as I
know OpenSSL does not support it [1]), I imagine that the first use cases
are going to be in scenarios where the user has control of both the client
and the server. In fact, this may be a good reason to choose Java on both
the server and the client -and thus the value of implementing both sides at
the same time-.

Your design concerns were also mine. I'll try to explain why I went that
way, and I'm open to discuss and re-work all of them.

1.

I understand your point and agree that TrustedManager and KeyManager are
better to remain immutable. The reason why I decided to go for a change in
X509KeyManager/X509TrustManager API (instead of a change in the factory
API/constructor) was to minimize the scope of the change as this is X509
oriented. I agree with you that if multiple threads use the same trust
objects, any change will impact the others. If they require isolation, they
would need to instantiate different factories and get different trust
objects.

2.

As it was implemented now, it'd be necessesary to instantiate a new
KeyManagerFactory/TrustedManagerFactory to have a different configuration
regarding pre-agreed certificates or the use of Trusted CA Indication
extension. Once the factory is instantiated, it will return the same
TrustManager/KeyManager object.

My first implementation was aligned to have this information more tighted
to the connection. But I faced a few issues with that:

 1. When you create a SSLSocket from the client side, the socket
automatically connects to the server. So, the use of Trusted CA Indication
needs to be there at the very beginning -if not, the extension won't be
used-. Getting a socket and setting SSLParameters is not useful in this
case.

 2. Setting this information through the SSLContext would be a hard
interface change -init method?-, but we can explore that path.

The fact that the information that is going to be used (when the extension
is enabled) is inside the TrustManager is what made me decide to go for the
option I implemented. Both the "switch" to use it and the information are
together. This is related to #3.

3.

The client sends all the certificates in the TrustManager as trusted CA
indication data because those are the certificates in which it trusts in
order to validate the connection. These certificates are not necessarily
the same that the certificates that the server has in its KeyManager. If
the client previously knows that the server has the same certificates, I
agree with you that it makes no sense to use this extension. That is going
to depend on each use-case.

A different alternative for #1 and #2 would be to set this information
through SSLServerSocketFactory (SSLSocketImpl) and SSLEngine
(SSLEngineImpl).

Do you have any preference or any other idea regarding the interface design
for enabling the extension?

BTW, there is an additional issue I've recently noticed related to the
aliases retrieval in the server side that I'll fix in the next Webrev. To
briefly summarize, getServerAliases method (invoked from
ServerHanshaker.java) is not enough when the KeyManager is implemented by
X509KeyManagerImpl.java (instead of SunX509KeyManagerImpl.java). The reason
is that chooseServerAlias does a lot more stuff in this implementation than
just getting the alias from a cache (i.e.: use of server name indications).

Kind regards,
Martin.-

--
[1] - https://github.com/openssl/openssl/issues/3029

On Thu, Jun 8, 2017 at 1:35 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:

> Hi Martin,
>
> Thanks for the contribution of the extension implementation.
>
> Do you know the real requirements or user cases for this extension?
>
>
> There is a vague point for this extension (Section 6, RFC 6066):
>
>    Servers that receive a client hello containing the "trusted_ca_keys"
>    extension MAY use the information contained in the extension to guide
>    their selection of an appropriate certificate chain to return to the
>    client.
>
> For better interop, a vendor may not use this extension if no appropriate
> certificate matching the trusted CA indication.  This could make the
> benefits of this extension discounted a lot.
>
> I have some concerns about the design:
> 1. The trust manager and key manager should be immutable, otherwise there
> are multiple threading issues.  It implies that the set methods should not
> be used for the trust/key managers.
>
> 2. The trusted ca indication is connection-oriented attribute, while
> trust/key managers are context wild configuration.  So it may be not the
> right place to configure the trusted ca indication in trust/key manager.
> For example, every connection may have its own pre-agreed indication. While
> the pre-agreed configuration in key manager means all connections in the
> context have the same pre-agreed indication.
>
> 3. In the implementation, if the extension is enabled, the client will use
> all trusted certificates as the trusted ca indication.  As all trusted CA
> are indicated, the extension is actually redundant.  Use it or not, no
> impact on the final handshaking result.  I would think it twice to use this
> extension if there is no significant benefits.
>
> Thanks & Regards,
> Xuelei
>
>
> On 6/7/2017 6:37 AM, Martin Balao wrote:
>
>> Hi,
>>
>> I'd like to propose a patch for bug ID JDK-8046295 - Support Trusted CA
>> Indication extension [1] and ask for reviews. I have the OCA signed since
>> I'm a Red Hat employee and this is part of my work [2].
>>
>> Webrev (jdk10 jdk10+10 based):
>>
>>   * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-80
>> 46295/webrev.00/ (browse online)
>>   * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-80
>> 46295/webrev.00/8046295.webrev.00.zip (zip download)
>>
>> Trusted CA Indication TLS extension (for TLS v1.2+) has been implemented
>> on both client and server sides, according to RFC 6066 [3]. Implementation
>> assumes the use of X.509 certificates.
>>
>> Client side
>>
>>   * The extension can be enabled by invoking 'setUseTrustedCAIndication'
>> method on the 'X509TrustManager' instance used for establishing a TLS
>> channel.
>>   * When enabled, a SHA-1 hash for each certificate managed by the
>> TrustManager instance is sent to the server as a "Trusted CA Indication"
>> data element. This happens during the Client Hello stage of the TLS
>> Handshake.
>>    * Note: SHA-1 key hash, Distinguished Name and pre-agreed methods
>> specified by RFC 6066 to identify a certificate were not implemented on the
>> client side.
>>
>> Server side
>>
>>   * The extension is always enabled on the server side.
>>   * When a client sends Trusted CA Indication data elements during the
>> Client Hello stage (TLS Handshake), the server tries to choose a
>> certificate from its 'X509KeyManager' instance based on that information.
>> If a certificate is not found, the TLS channel cannot be established.
>>   * A certificate chain on a 'X509KeyManager' instance can be set as
>> 'pre-agreed' trusted (see RFC 6066) invoking the 'setPreAgreedCertificate'
>> method
>>   * This is the procedure through which the server chooses a certificate:
>>    * Cipher suites iterated as usual (in preferred order)
>>    * If the client has sent Trusted CA Indication data elements:
>>     * All the certificate chains for the chosen cipher suite algorithm
>> are retrieved from the 'X509KeyManager' instance and iterated
>>      * For each certificate on a chain (starting from root):
>>       * For each Trusted CA Indication data element:
>>        * If there is a match between the Trusted CA Indication data
>> element and the certificate in the server's chain, the certificate chain is
>> chosen.
>>        * If Trusted CA Indication data element is of "pre-agreed" type
>> and the certificate chain was set as "pre-agreed", the certificate chain is
>> chosen.
>>   * As a consequence of the previous procedure, a client may trust in an
>> intermediate certificate and the server will be able to choose a
>> certificate chain that contains that intermediate certificate.
>>   * SHA-1 certificate hash, SHA-1 key hash, Distinguished Name and
>> pre-agreed methods specified by RFC 6066 are supported.
>> Test cases implemented for both client and server sides.
>>
>> Thanks in advanced,
>> Martin Balao.-
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8046295
>> [2] - http://www.oracle.com/technetwork/community/oca-486395.html#r
>> [3] - https://tools.ietf.org/html/rfc6066#page-12
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20170608/d05e62f3/attachment.htm>