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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
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):


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

--
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
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-8046295/webrev.00/ 
> (browse online)
>   *
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
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.-

--

On Thu, Jun 8, 2017 at 1:35 PM, Xuelei Fan <[hidden email]> 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-8046295/webrev.00/ (browse online)
  * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
On 6/8/2017 3:09 PM, Martin Balao wrote:

> 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.
>
I did not get the point why setting SSLParametes is not useful.

When an SSLSocket in created, the SSL connection has not established.
Creating a SSLSocket, and then setting the SSLParameters, and then
negotiating the SSL connection (handshake).  That's the general routines
to use SSLParameters.

        // connect
        SSLSocket sslSocket = sslSocketFactory.createSocket(...);

        // configure
        SSLParameters sslParameters = ...
        sslSocket.setParameters(sslParameters);

        // negotiate
        sslSocket.startHandshake()/sslSocket I/O.

I think if trusted ca indication is configured with SSLParameters, it
should can be used.

>   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?
>
Per my understanding, the significant benefit of this extension is to
help the server choose the right server cert if the server supports
multiple certificates.  For example, the server has 10 RSA certs issued
by 8 CAs, while the client only trust one CA.

I may add a pair of SSLParametersmethod, or define a system property:
      public boolean getUseTrustedIndication();
      public void setUseTrustedIndication(boolean useTrustedIndication);

or define a system property:
      jdk.tls.useTrustedIndication = true/false.

or use system property:
      jdk.tls.extensions = +/-trusted_ca_keys

As this is a mandatory TLS Extensions of NIST SP 800-52, more
flexibility may be not desired.  I would prefer to use one system
property only.

The trusted authorities can be get from client trust manager.  Server
can choose the best matching of server certificate of the client
requested trusted authorities.

In JSSE, the benefits pre_agreed option can get by customizing the
key/trust manager, so I did not see too much benefits to support this
option in JDK.  The values of other three options can get from the
key/trust manager certificates.

What do you think?  Can it meet your requirements?

> 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).
> Please feel free to file a bug or submit the change.

Thanks & Regards,
Xuelei

> Kind regards,
> Martin.-
>
> --
> [1] - https://github.com/openssl/openssl/issues/3029
>
> On Thu, Jun 8, 2017 at 1:35 PM, Xuelei Fan <[hidden email]
> <mailto:[hidden email]>> 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-8046295/webrev.00/
>         <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.00/>
>         (browse online)
>            *
>         http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.00/8046295.webrev.00.zip
>         <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/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
>         <https://bugs.openjdk.java.net/browse/JDK-8046295>
>         [2] -
>         http://www.oracle.com/technetwork/community/oca-486395.html#r
>         <http://www.oracle.com/technetwork/community/oca-486395.html#r>
>         [3] - https://tools.ietf.org/html/rfc6066#page-12
>         <https://tools.ietf.org/html/rfc6066#page-12>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2


On 6/8/2017 8:36 PM, Xuelei Fan wrote:
> The trusted authorities can be get from client trust manager.  Server
> can choose the best matching of server certificate of the client
> requested trusted authorities.
 >
I missed the point that the key manager need to know the client
requested trusted authorities for the choosing.  So may need a new
SSLSession attribute (See similar method in ExtendedSSLSession).

Xuelei
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
Hi Xuelei,

I didn't notice that some of the SSLSocket contructors did not establish the connection, so SSLParameters can be effective for Trusted CA Indication. This was an invalid argument on my side, sorry.

As for the configuration to enable the extension, it's probably not necessary on the Server side because -as you mentioned- it is mandatory and there is no harm in supporting it. However, it has to be configurable on the Client side because -as we previously discussed- the client may cause a handshake failure if the server does not support the extension. I'd prefer the Client configuring the SSLSocket through SSLParameters instead of a system-wide property -which has even more impact than the TrustManager approach-. Would this work for you?

> In JSSE, the benefits pre_agreed option can get by customizing the key/trust manager, so I did not see too much benefits to support this option in JDK

I understand your point and will remove support for "pre_agreed".


On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan <[hidden email]> wrote:


On 6/8/2017 8:36 PM, Xuelei Fan wrote:
The trusted authorities can be get from client trust manager.  Server can choose the best matching of server certificate of the client requested trusted authorities.
>
I missed the point that the key manager need to know the client requested trusted authorities for the choosing.  So may need a new SSLSession attribute (See similar method in ExtendedSSLSession).

Xuelei


Yes, an attribute on SSLSession may do the job (both when Key Manager receives a SSLSocket and a SSLEngine).

Kind regards,
Martin.- 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
I'm OK to use SSLParameters.  Thank you very much for considering a new
design.

Xuelei

On 6/9/2017 1:10 PM, Martin Balao wrote:

> Hi Xuelei,
>
> I didn't notice that some of the SSLSocket contructors did not establish
> the connection, so SSLParameters can be effective for Trusted CA
> Indication. This was an invalid argument on my side, sorry.
>
> As for the configuration to enable the extension, it's probably not
> necessary on the Server side because -as you mentioned- it is mandatory
> and there is no harm in supporting it. However, it has to be
> configurable on the Client side because -as we previously discussed- the
> client may cause a handshake failure if the server does not support the
> extension. I'd prefer the Client configuring the SSLSocket through
> SSLParameters instead of a system-wide property -which has even more
> impact than the TrustManager approach-. Would this work for you?
>
>  > In JSSE, the benefits pre_agreed option can get by customizing the
> key/trust manager, so I did not see too much benefits to support this
> option in JDK
>
> I understand your point and will remove support for "pre_agreed".
>
>
> On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>
>     On 6/8/2017 8:36 PM, Xuelei Fan wrote:
>
>         The trusted authorities can be get from client trust manager.
>         Server can choose the best matching of server certificate of the
>         client requested trusted authorities.
>
>     >
>     I missed the point that the key manager need to know the client
>     requested trusted authorities for the choosing.  So may need a new
>     SSLSession attribute (See similar method in ExtendedSSLSession).
>
>     Xuelei
>
>
>
> Yes, an attribute on SSLSession may do the job (both when Key Manager
> receives a SSLSocket and a SSLEngine).
>
> Kind regards,
> Martin.-
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
Hi Xuelei,

The new webrev.01 is ready:


The following changes have been implemented since the previous webrev.00:

 * Pre-agreed support removed from server-side
  * Unnecessary overhead and minium benefits for JSSE.

 * Enabling the use of Trusted CA Indication extension for clients through TrustManager objects was reverted. Trusted CA Indication extension can now be enabled through: 1) SSLEngine, 2) SSLSocket, or 3) SSLParameters (which can be applied to both SSLEngine and SSLSocket objects). Trusted CA Indication extension is mandatory for servers.

 * SunX509KeyManagerImpl old key manager ("SunX509" algorithm) is now out of scope. This key manager does not support other TLS extensions as Server Name Indication (SNI), which is far more relevant than Trusted CA Indication. The new X509KeyManagerImpl key manager ("PKIX" algorithm) is now in scope.

 * Client requested indications are now an ExtendedSSLSession attribute. ServerHandshaker gets the information from the Client Hello message (now parsed by TrustedCAIndicationExtension class instead of TrustedAuthorityIndicator) and sets it in the ExtendedSSLSession (SSLSessionImpl object). The key manager (i.e.: X509KeyManagerImpl), when choosing a server alias, may now get the information from the ExtendedSSLSession object and guide the certificate selection based on it.
  * In order to allow multiple key managers to use Trusted Authority Indicators information and to allow multiple Trusted Authority Indicators implementations, TrustedAuthorityIndicator has now been split in an abstract class (TrustedAuthorityIndicator, located in javax.net.ssl) and an implementation class (TrustedAuthorityIndicatorImpl, located in sun.security.ssl). No coupling was added between javax.net.ssl and sun.security.ssl packages.

 * Documentation extended and improved.
 
 * Test cases (server and client) updated to reflect the new interface and supported key manager.

Look forward to your new review!

Kind regards,
Martin.-



On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan <[hidden email]> wrote:
I'm OK to use SSLParameters.  Thank you very much for considering a new design.

Xuelei


On 6/9/2017 1:10 PM, Martin Balao wrote:
Hi Xuelei,

I didn't notice that some of the SSLSocket contructors did not establish the connection, so SSLParameters can be effective for Trusted CA Indication. This was an invalid argument on my side, sorry.

As for the configuration to enable the extension, it's probably not necessary on the Server side because -as you mentioned- it is mandatory and there is no harm in supporting it. However, it has to be configurable on the Client side because -as we previously discussed- the client may cause a handshake failure if the server does not support the extension. I'd prefer the Client configuring the SSLSocket through SSLParameters instead of a system-wide property -which has even more impact than the TrustManager approach-. Would this work for you?

 > In JSSE, the benefits pre_agreed option can get by customizing the key/trust manager, so I did not see too much benefits to support this option in JDK

I understand your point and will remove support for "pre_agreed".


On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan <[hidden email] <mailto:[hidden email]>> wrote:



    On 6/8/2017 8:36 PM, Xuelei Fan wrote:

        The trusted authorities can be get from client trust manager.         Server can choose the best matching of server certificate of the
        client requested trusted authorities.

    >
    I missed the point that the key manager need to know the client
    requested trusted authorities for the choosing.  So may need a new
    SSLSession attribute (See similar method in ExtendedSSLSession).

    Xuelei



Yes, an attribute on SSLSession may do the job (both when Key Manager receives a SSLSocket and a SSLEngine).

Kind regards,
Martin.-


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
Hi Martin,

The big picture of the design looks pretty good to me, except a few
comment about the JSSE conventions.  I appreciate it very much.  By the
way, I need more time to look into the details of the specification and
implementation.


In order to keep the APIs simple and small, SSLParameters is preferred
as the only configuration port for common cases.   I may suggest to
remove the s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket.

The identify type is defined as an enum
TrustedAuthorityIndicator.IdentifierType.  In the future, if more type
is added, we need to update the specification by adding a new enum item.
  Enum is preferred in JDK, but for good extensibility, in general JSSE
does not use enum in public APIs for extensible properties.  I may
suggest to use String (or integer/byte, I prefer to use String) as the
type.  The standard trusted authority indicator algorithm (identifier)
can be documented in the "Java Cryptography Architecture Standard
Algorithm Name Documentation"[1].

In TrustedAuthorityIndicator class, some methods, like
getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(),
getLength(), equals() and hashCode() look more like implementation
logic.  I may suggest remove them from public APIs.

I did not see the benefit to have X509Certificate in the
TrustedAuthorityIndicator class.  The class is mainly used for server
side certificate selection.  X509Certificate could be unknown for a
indicator.  I may suggestion remove the related methods and properties.

After that, as there is no requirement to instantiate
TrustedAuthorityIndicator class in application code, looks like it may
be enough to use an interface to represent a trusted authorities:
    public interface TrustedAuthorityIndicator {
         // identifier type, standard algorithm name
         String/int/Byte getType();

        // identifier
         byte[] getEncoded();
    }

What do you think?


Thanks & Regards,
Xuelei

[1]
https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html


On 6/13/2017 3:41 PM, Martin Balao wrote:

> Hi Xuelei,
>
> The new webrev.01 is ready:
>
>   *
> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/ 
> (browse online)
>   *
> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip 
> (zip, download)
>
> The following changes have been implemented since the previous webrev.00:
>
>   * Pre-agreed support removed from server-side
>    * Unnecessary overhead and minium benefits for JSSE.
>
>   * Enabling the use of Trusted CA Indication extension for clients
> through TrustManager objects was reverted. Trusted CA Indication
> extension can now be enabled through: 1) SSLEngine, 2) SSLSocket, or 3)
> SSLParameters (which can be applied to both SSLEngine and SSLSocket
> objects). Trusted CA Indication extension is mandatory for servers.
>
>   * SunX509KeyManagerImpl old key manager ("SunX509" algorithm) is now
> out of scope. This key manager does not support other TLS extensions as
> Server Name Indication (SNI), which is far more relevant than Trusted CA
> Indication. The new X509KeyManagerImpl key manager ("PKIX" algorithm) is
> now in scope.
>
>   * Client requested indications are now an ExtendedSSLSession
> attribute. ServerHandshaker gets the information from the Client Hello
> message (now parsed by TrustedCAIndicationExtension class instead of
> TrustedAuthorityIndicator) and sets it in the ExtendedSSLSession
> (SSLSessionImpl object). The key manager (i.e.: X509KeyManagerImpl),
> when choosing a server alias, may now get the information from the
> ExtendedSSLSession object and guide the certificate selection based on it.
>    * In order to allow multiple key managers to use Trusted Authority
> Indicators information and to allow multiple Trusted Authority
> Indicators implementations, TrustedAuthorityIndicator has now been split
> in an abstract class (TrustedAuthorityIndicator, located in
> javax.net.ssl) and an implementation class
> (TrustedAuthorityIndicatorImpl, located in sun.security.ssl). No
> coupling was added between javax.net.ssl and sun.security.ssl packages.
>
>   * Documentation extended and improved.
>   * Test cases (server and client) updated to reflect the new interface
> and supported key manager.
>
> Look forward to your new review!
>
> Kind regards,
> Martin.-
>
>
>
> On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     I'm OK to use SSLParameters.  Thank you very much for considering a
>     new design.
>
>     Xuelei
>
>     On 6/9/2017 1:10 PM, Martin Balao wrote:
>
>         Hi Xuelei,
>
>         I didn't notice that some of the SSLSocket contructors did not
>         establish the connection, so SSLParameters can be effective for
>         Trusted CA Indication. This was an invalid argument on my side,
>         sorry.
>
>         As for the configuration to enable the extension, it's probably
>         not necessary on the Server side because -as you mentioned- it
>         is mandatory and there is no harm in supporting it. However, it
>         has to be configurable on the Client side because -as we
>         previously discussed- the client may cause a handshake failure
>         if the server does not support the extension. I'd prefer the
>         Client configuring the SSLSocket through SSLParameters instead
>         of a system-wide property -which has even more impact than the
>         TrustManager approach-. Would this work for you?
>
>           > In JSSE, the benefits pre_agreed option can get by
>         customizing the key/trust manager, so I did not see too much
>         benefits to support this option in JDK
>
>         I understand your point and will remove support for "pre_agreed".
>
>
>         On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email] <mailto:[hidden email]>>>
>         wrote:
>
>
>
>              On 6/8/2017 8:36 PM, Xuelei Fan wrote:
>
>                  The trusted authorities can be get from client trust
>         manager.         Server can choose the best matching of server
>         certificate of the
>                  client requested trusted authorities.
>
>              >
>              I missed the point that the key manager need to know the client
>              requested trusted authorities for the choosing.  So may
>         need a new
>              SSLSession attribute (See similar method in
>         ExtendedSSLSession).
>
>              Xuelei
>
>
>
>         Yes, an attribute on SSLSession may do the job (both when Key
>         Manager receives a SSLSocket and a SSLEngine).
>
>         Kind regards,
>         Martin.-
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
Hi Martin,

I think more about the new TrustedAuthorityIndicator class.  Maybe, it
can be replaced with the existing java.util.Map.Entry class (using
java.util.AbstractMap.SimpleImmutableEntry for the implementation).

ExtendedSSLSession.java
    List<Map.Entry<Byte, byte[]>> getRequestedTrustedCAs();


Xuelei

On 6/14/2017 3:17 PM, Xuelei Fan wrote:

> Hi Martin,
>
> The big picture of the design looks pretty good to me, except a few
> comment about the JSSE conventions.  I appreciate it very much.  By the
> way, I need more time to look into the details of the specification and
> implementation.
>
>
> In order to keep the APIs simple and small, SSLParameters is preferred
> as the only configuration port for common cases.   I may suggest to
> remove the s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket.
>
> The identify type is defined as an enum
> TrustedAuthorityIndicator.IdentifierType.  In the future, if more type
> is added, we need to update the specification by adding a new enum item.
>   Enum is preferred in JDK, but for good extensibility, in general JSSE
> does not use enum in public APIs for extensible properties.  I may
> suggest to use String (or integer/byte, I prefer to use String) as the
> type.  The standard trusted authority indicator algorithm (identifier)
> can be documented in the "Java Cryptography Architecture Standard
> Algorithm Name Documentation"[1].
>
> In TrustedAuthorityIndicator class, some methods, like
> getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(),
> getLength(), equals() and hashCode() look more like implementation
> logic.  I may suggest remove them from public APIs.
>
> I did not see the benefit to have X509Certificate in the
> TrustedAuthorityIndicator class.  The class is mainly used for server
> side certificate selection.  X509Certificate could be unknown for a
> indicator.  I may suggestion remove the related methods and properties.
>
> After that, as there is no requirement to instantiate
> TrustedAuthorityIndicator class in application code, looks like it may
> be enough to use an interface to represent a trusted authorities:
>     public interface TrustedAuthorityIndicator {
>          // identifier type, standard algorithm name
>          String/int/Byte getType();
>
>      // identifier
>          byte[] getEncoded();
>     }
>
> What do you think?
>
>
> Thanks & Regards,
> Xuelei
>
> [1]
> https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html 
>
>
>
> On 6/13/2017 3:41 PM, Martin Balao wrote:
>> Hi Xuelei,
>>
>> The new webrev.01 is ready:
>>
>>   *
>> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/ 
>> (browse online)
>>   *
>> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip 
>> (zip, download)
>>
>> The following changes have been implemented since the previous webrev.00:
>>
>>   * Pre-agreed support removed from server-side
>>    * Unnecessary overhead and minium benefits for JSSE.
>>
>>   * Enabling the use of Trusted CA Indication extension for clients
>> through TrustManager objects was reverted. Trusted CA Indication
>> extension can now be enabled through: 1) SSLEngine, 2) SSLSocket, or
>> 3) SSLParameters (which can be applied to both SSLEngine and SSLSocket
>> objects). Trusted CA Indication extension is mandatory for servers.
>>
>>   * SunX509KeyManagerImpl old key manager ("SunX509" algorithm) is now
>> out of scope. This key manager does not support other TLS extensions
>> as Server Name Indication (SNI), which is far more relevant than
>> Trusted CA Indication. The new X509KeyManagerImpl key manager ("PKIX"
>> algorithm) is now in scope.
>>
>>   * Client requested indications are now an ExtendedSSLSession
>> attribute. ServerHandshaker gets the information from the Client Hello
>> message (now parsed by TrustedCAIndicationExtension class instead of
>> TrustedAuthorityIndicator) and sets it in the ExtendedSSLSession
>> (SSLSessionImpl object). The key manager (i.e.: X509KeyManagerImpl),
>> when choosing a server alias, may now get the information from the
>> ExtendedSSLSession object and guide the certificate selection based on
>> it.
>>    * In order to allow multiple key managers to use Trusted Authority
>> Indicators information and to allow multiple Trusted Authority
>> Indicators implementations, TrustedAuthorityIndicator has now been
>> split in an abstract class (TrustedAuthorityIndicator, located in
>> javax.net.ssl) and an implementation class
>> (TrustedAuthorityIndicatorImpl, located in sun.security.ssl). No
>> coupling was added between javax.net.ssl and sun.security.ssl packages.
>>
>>   * Documentation extended and improved.
>>   * Test cases (server and client) updated to reflect the new
>> interface and supported key manager.
>>
>> Look forward to your new review!
>>
>> Kind regards,
>> Martin.-
>>
>>
>>
>> On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     I'm OK to use SSLParameters.  Thank you very much for considering a
>>     new design.
>>
>>     Xuelei
>>
>>     On 6/9/2017 1:10 PM, Martin Balao wrote:
>>
>>         Hi Xuelei,
>>
>>         I didn't notice that some of the SSLSocket contructors did not
>>         establish the connection, so SSLParameters can be effective for
>>         Trusted CA Indication. This was an invalid argument on my side,
>>         sorry.
>>
>>         As for the configuration to enable the extension, it's probably
>>         not necessary on the Server side because -as you mentioned- it
>>         is mandatory and there is no harm in supporting it. However, it
>>         has to be configurable on the Client side because -as we
>>         previously discussed- the client may cause a handshake failure
>>         if the server does not support the extension. I'd prefer the
>>         Client configuring the SSLSocket through SSLParameters instead
>>         of a system-wide property -which has even more impact than the
>>         TrustManager approach-. Would this work for you?
>>
>>           > In JSSE, the benefits pre_agreed option can get by
>>         customizing the key/trust manager, so I did not see too much
>>         benefits to support this option in JDK
>>
>>         I understand your point and will remove support for "pre_agreed".
>>
>>
>>         On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
>>         <[hidden email] <mailto:[hidden email]>
>>         <mailto:[hidden email] <mailto:[hidden email]>>>
>>         wrote:
>>
>>
>>
>>              On 6/8/2017 8:36 PM, Xuelei Fan wrote:
>>
>>                  The trusted authorities can be get from client trust
>>         manager.         Server can choose the best matching of server
>>         certificate of the
>>                  client requested trusted authorities.
>>
>>              >
>>              I missed the point that the key manager need to know the
>> client
>>              requested trusted authorities for the choosing.  So may
>>         need a new
>>              SSLSession attribute (See similar method in
>>         ExtendedSSLSession).
>>
>>              Xuelei
>>
>>
>>
>>         Yes, an attribute on SSLSession may do the job (both when Key
>>         Manager receives a SSLSocket and a SSLEngine).
>>
>>         Kind regards,
>>         Martin.-
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
In reply to this post by Xuelei Fan-2
Hi Xuelei,

The new webrev.02 is ready:


The following changes have been implemented since the previous webrev.01:

 * s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket and in SSLEngineImpl/SSLSocketImpl removed. s/getSSLParameters is now the only way to set or get the use of the Trusted CA Indication extension. An exception is no longer thrown if trying to disable the extension for a server, but the change takes no effect as the extension is mandatory for servers. X509KeyManagerImpl modified to use SSLParameters to get information regarding if Trusted CA Indication is enabled and should guide the certificate choice.

 * TrustedAuthorityIndicator.IdentifierType has been moved from enum to String, to follow JSSE conventions. I understand how important is to be consistent. However, I still believe that an enum is a better fit for this value and does not prevent from future extension. We are choosing from a closed set (strictly defined by the RFC) and that's what enum allows to express. From the client point of view/API, it's very handy that the type gives you information regarding the allowed choices for the parameter. You don't necessarily have to look for implementation details or documentation but you can just leverage on the strongly typed language. It's also likely that enums are faster for comparisons than strings, but that's not the main point here.

 * Removed X509Certificate from TrustedAuthorityIndicator class (method and property). It was there for informational purposes (when TrustedAuthorityIndicator was built from a certificate by a client and the whole extension indicators converted to String). 

 * "equals" and "hashCode" methods moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

 * "getLength" method removed from TrustedAuthorityIndicator class. It's possible to get the encoded buffer and the length from there.

 * "getData" method renamed to "getEncoded" in TrustedAuthorityIndicator class.

 * "trustedAuthorityEncodedData" renamed to "encodedData" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes

 * "identifier" and "encodedData" instance variables moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

 * "getEncoded" and "getIdentifier" are now abstract methods in TrustedAuthorityIndicator, and their implementation is in TrustedAuthorityIndicatorImpl class.

 * "getIdentifier" method renamed to "getType" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes ("identifier" instance variable and parameter in TrustedAuthorityIndicatorImpl class renamed to "type").

 * Test cases (server and client) updated to reflect the new interface (enabling the use of the extension through SSLParameters)

However, some changes are still not implemented and I have some concerns:

1) I still believe that identifier type information has to be on TrustedAuthorityIndicator class somehow, and implementations restricted on what they can return as part of "getType" method. This is strictly specified by the RFC TrustedAuthorityIndicator class represents, and I find desirable that any implementation is enforced to be compliant to that. If we remove all of that (including the enum), TrustedAuthorityIndicator looks too generic and does not reflect (in my opinion) what it really is. It'd also be chaotic if different implementations call pre-agreed type as "preagreed", "pre-agreed", "PRE_AGREED", etc. I prefer stricter and more explicit interfaces.

2) I agree that type mappings can be seen as part of an implementation, but they were in TrustedAuthorityIndicator (as protected) because every implementation is highly likely to need them and we can avoid the necessity for repeated code/mappings. The same for "type" and "encodedData" variables or even "hashCode" and "equals" methods. That's why I was thinking more of an abstract class and not an interface, as it happens (in example) with SNIServerName.

3) I think that "implies" method on TrustedAuthorityIndicator should be also part of the class/interface, because that's the whole point of a Trusted Authority Information: to allow queries for a given certificate. This is, in fact, the only thing a server wants from one of these objects. My concern is that if we remove this requirement for an implementation, the interface looks more like a byte buffer holder.

I'd appreciate if you can re-consider these items.

Thanks,
Martin.-

On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan <[hidden email]> wrote:
Hi Martin,

The big picture of the design looks pretty good to me, except a few comment about the JSSE conventions.  I appreciate it very much.  By the way, I need more time to look into the details of the specification and implementation.


In order to keep the APIs simple and small, SSLParameters is preferred as the only configuration port for common cases.   I may suggest to remove the s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket.

The identify type is defined as an enum TrustedAuthorityIndicator.IdentifierType.  In the future, if more type is added, we need to update the specification by adding a new enum item.  Enum is preferred in JDK, but for good extensibility, in general JSSE does not use enum in public APIs for extensible properties.  I may suggest to use String (or integer/byte, I prefer to use String) as the type.  The standard trusted authority indicator algorithm (identifier) can be documented in the "Java Cryptography Architecture Standard Algorithm Name Documentation"[1].

In TrustedAuthorityIndicator class, some methods, like getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(), getLength(), equals() and hashCode() look more like implementation logic.  I may suggest remove them from public APIs.

I did not see the benefit to have X509Certificate in the TrustedAuthorityIndicator class.  The class is mainly used for server side certificate selection.  X509Certificate could be unknown for a indicator.  I may suggestion remove the related methods and properties.

After that, as there is no requirement to instantiate TrustedAuthorityIndicator class in application code, looks like it may be enough to use an interface to represent a trusted authorities:
   public interface TrustedAuthorityIndicator {
        // identifier type, standard algorithm name
        String/int/Byte getType();

        // identifier
        byte[] getEncoded();
   }

What do you think?


Thanks & Regards,
Xuelei

[1] https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html


On 6/13/2017 3:41 PM, Martin Balao wrote:
Hi Xuelei,

The new webrev.01 is ready:

  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/ (browse online)
  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip (zip, download)

The following changes have been implemented since the previous webrev.00:

  * Pre-agreed support removed from server-side
   * Unnecessary overhead and minium benefits for JSSE.

  * Enabling the use of Trusted CA Indication extension for clients through TrustManager objects was reverted. Trusted CA Indication extension can now be enabled through: 1) SSLEngine, 2) SSLSocket, or 3) SSLParameters (which can be applied to both SSLEngine and SSLSocket objects). Trusted CA Indication extension is mandatory for servers.

  * SunX509KeyManagerImpl old key manager ("SunX509" algorithm) is now out of scope. This key manager does not support other TLS extensions as Server Name Indication (SNI), which is far more relevant than Trusted CA Indication. The new X509KeyManagerImpl key manager ("PKIX" algorithm) is now in scope.

  * Client requested indications are now an ExtendedSSLSession attribute. ServerHandshaker gets the information from the Client Hello message (now parsed by TrustedCAIndicationExtension class instead of TrustedAuthorityIndicator) and sets it in the ExtendedSSLSession (SSLSessionImpl object). The key manager (i.e.: X509KeyManagerImpl), when choosing a server alias, may now get the information from the ExtendedSSLSession object and guide the certificate selection based on it.
   * In order to allow multiple key managers to use Trusted Authority Indicators information and to allow multiple Trusted Authority Indicators implementations, TrustedAuthorityIndicator has now been split in an abstract class (TrustedAuthorityIndicator, located in javax.net.ssl) and an implementation class (TrustedAuthorityIndicatorImpl, located in sun.security.ssl). No coupling was added between javax.net.ssl and sun.security.ssl packages.

  * Documentation extended and improved.
  * Test cases (server and client) updated to reflect the new interface and supported key manager.

Look forward to your new review!

Kind regards,
Martin.-



On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan <[hidden email] <mailto:[hidden email]>> wrote:

    I'm OK to use SSLParameters.  Thank you very much for considering a
    new design.

    Xuelei

    On 6/9/2017 1:10 PM, Martin Balao wrote:

        Hi Xuelei,

        I didn't notice that some of the SSLSocket contructors did not
        establish the connection, so SSLParameters can be effective for
        Trusted CA Indication. This was an invalid argument on my side,
        sorry.

        As for the configuration to enable the extension, it's probably
        not necessary on the Server side because -as you mentioned- it
        is mandatory and there is no harm in supporting it. However, it
        has to be configurable on the Client side because -as we
        previously discussed- the client may cause a handshake failure
        if the server does not support the extension. I'd prefer the
        Client configuring the SSLSocket through SSLParameters instead
        of a system-wide property -which has even more impact than the
        TrustManager approach-. Would this work for you?

          > In JSSE, the benefits pre_agreed option can get by
        customizing the key/trust manager, so I did not see too much
        benefits to support this option in JDK

        I understand your point and will remove support for "pre_agreed".


        On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
        <[hidden email] <mailto:[hidden email]>
        <mailto:[hidden email] <mailto:[hidden email]>>>
        wrote:



             On 6/8/2017 8:36 PM, Xuelei Fan wrote:

                 The trusted authorities can be get from client trust
        manager.         Server can choose the best matching of server
        certificate of the
                 client requested trusted authorities.

             >
             I missed the point that the key manager need to know the client
             requested trusted authorities for the choosing.  So may
        need a new
             SSLSession attribute (See similar method in
        ExtendedSSLSession).

             Xuelei



        Yes, an attribute on SSLSession may do the job (both when Key
        Manager receives a SSLSocket and a SSLEngine).

        Kind regards,
        Martin.-



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
In reply to this post by Xuelei Fan-2
Hi Xuelei,

On Thu, Jun 15, 2017 at 3:09 PM, Xuelei Fan <[hidden email]> wrote:

I think more about the new TrustedAuthorityIndicator class.  Maybe, it can be replaced with the existing java.util.Map.Entry class (using java.util.AbstractMap.SimpleImmutableEntry for the implementation).

ExtendedSSLSession.java
   List<Map.Entry<Byte, byte[]>> getRequestedTrustedCAs();


This looks a bit implicit and cryptic to me. I'd go for a more explicit, extensible and OO interface...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
In reply to this post by Martin Balao
Hi Martin,

The TLS 1.3 spec is replacing the Trusted CA Indication
(trusted_ca_keys) extension with a new Certificate Authorities
(certificate_authorities) extension.  See more details about the
specification in the TLS 1.3 draft:
     https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4

Both serves a similar purpose, but the trusted_ca_keys extension will
not be used in TLS 1.3 any more.  The "trusted_ca_keys" extension will
only be used for legacy protocol versions (TLS 1.2/1.1/1.0).

There are two options to me:
1. Supports the certificate_authorities, but not trusted_ca_keys extension.
It is acceptable to me as trusted_ca_keys is for legacy use only and the
certificate_authorities extension is the future.  Plus, the
certificate_authorities extension can also be used for TLS 1.2 and
previous versions.

2. Supports both the certificate_authorities and trusted_ca_keys extensions.
As far as I know, I did not see much benefit of this option unless the
trusted_ca_keys extension is widely used in practice.

If I did not miss something, the APIs you designed can still be used for
the certificate_authorities extension, with a little bit update.

What do you think?

Thanks & Regards,
Xuelei


On 6/15/2017 12:05 PM, Martin Balao wrote:

> Hi Xuelei,
>
> The new webrev.02 is ready:
>
>   *
> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/ 
> (browse online)
>   *
> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip 
> (zip, download)
>
> The following changes have been implemented since the previous webrev.01:
>
>   * s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket and in
> SSLEngineImpl/SSLSocketImpl removed. s/getSSLParameters is now the only
> way to set or get the use of the Trusted CA Indication extension. An
> exception is no longer thrown if trying to disable the extension for a
> server, but the change takes no effect as the extension is mandatory for
> servers. X509KeyManagerImpl modified to use SSLParameters to get
> information regarding if Trusted CA Indication is enabled and should
> guide the certificate choice.
>
>   * TrustedAuthorityIndicator.IdentifierType has been moved from enum to
> String, to follow JSSE conventions. I understand how important is to be
> consistent. However, I still believe that an enum is a better fit for
> this value and does not prevent from future extension. We are choosing
> from a closed set (strictly defined by the RFC) and that's what enum
> allows to express. From the client point of view/API, it's very handy
> that the type gives you information regarding the allowed choices for
> the parameter. You don't necessarily have to look for implementation
> details or documentation but you can just leverage on the strongly typed
> language. It's also likely that enums are faster for comparisons than
> strings, but that's not the main point here.
>
>   * Removed X509Certificate from TrustedAuthorityIndicator class (method
> and property). It was there for informational purposes (when
> TrustedAuthorityIndicator was built from a certificate by a client and
> the whole extension indicators converted to String).
>
>   * "equals" and "hashCode" methods moved from TrustedAuthorityIndicator
> to TrustedAuthorityIndicatorImpl class.
>
>   * "getLength" method removed from TrustedAuthorityIndicator class.
> It's possible to get the encoded buffer and the length from there.
>
>   * "getData" method renamed to "getEncoded" in
> TrustedAuthorityIndicator class.
>
>   * "trustedAuthorityEncodedData" renamed to "encodedData" in
> TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes
>
>   * "identifier" and "encodedData" instance variables moved from
> TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.
>
>   * "getEncoded" and "getIdentifier" are now abstract methods in
> TrustedAuthorityIndicator, and their implementation is in
> TrustedAuthorityIndicatorImpl class.
>
>   * "getIdentifier" method renamed to "getType" in
> TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes
> ("identifier" instance variable and parameter in
> TrustedAuthorityIndicatorImpl class renamed to "type").
>
>   * Test cases (server and client) updated to reflect the new interface
> (enabling the use of the extension through SSLParameters)
>
> However, some changes are still not implemented and I have some concerns:
>
> 1) I still believe that identifier type information has to be on
> TrustedAuthorityIndicator class somehow, and implementations restricted
> on what they can return as part of "getType" method. This is strictly
> specified by the RFC TrustedAuthorityIndicator class represents, and I
> find desirable that any implementation is enforced to be compliant to
> that. If we remove all of that (including the enum),
> TrustedAuthorityIndicator looks too generic and does not reflect (in my
> opinion) what it really is. It'd also be chaotic if different
> implementations call pre-agreed type as "preagreed", "pre-agreed",
> "PRE_AGREED", etc. I prefer stricter and more explicit interfaces.
>
> 2) I agree that type mappings can be seen as part of an implementation,
> but they were in TrustedAuthorityIndicator (as protected) because every
> implementation is highly likely to need them and we can avoid the
> necessity for repeated code/mappings. The same for "type" and
> "encodedData" variables or even "hashCode" and "equals" methods. That's
> why I was thinking more of an abstract class and not an interface, as it
> happens (in example) with SNIServerName.
>
> 3) I think that "implies" method on TrustedAuthorityIndicator should be
> also part of the class/interface, because that's the whole point of a
> Trusted Authority Information: to allow queries for a given certificate.
> This is, in fact, the only thing a server wants from one of these
> objects. My concern is that if we remove this requirement for an
> implementation, the interface looks more like a byte buffer holder.
>
> I'd appreciate if you can re-consider these items.
>
> Thanks,
> Martin.-
>
> On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi Martin,
>
>     The big picture of the design looks pretty good to me, except a few
>     comment about the JSSE conventions.  I appreciate it very much.  By
>     the way, I need more time to look into the details of the
>     specification and implementation.
>
>
>     In order to keep the APIs simple and small, SSLParameters is
>     preferred as the only configuration port for common cases.   I may
>     suggest to remove the s/getUseTrustedCAIndication() methods in
>     SSLEngine/SSLSocket.
>
>     The identify type is defined as an enum
>     TrustedAuthorityIndicator.IdentifierType.  In the future, if more
>     type is added, we need to update the specification by adding a new
>     enum item.  Enum is preferred in JDK, but for good extensibility, in
>     general JSSE does not use enum in public APIs for extensible
>     properties.  I may suggest to use String (or integer/byte, I prefer
>     to use String) as the type.  The standard trusted authority
>     indicator algorithm (identifier) can be documented in the "Java
>     Cryptography Architecture Standard Algorithm Name Documentation"[1].
>
>     In TrustedAuthorityIndicator class, some methods, like
>     getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(),
>     getLength(), equals() and hashCode() look more like implementation
>     logic.  I may suggest remove them from public APIs.
>
>     I did not see the benefit to have X509Certificate in the
>     TrustedAuthorityIndicator class.  The class is mainly used for
>     server side certificate selection.  X509Certificate could be unknown
>     for a indicator.  I may suggestion remove the related methods and
>     properties.
>
>     After that, as there is no requirement to instantiate
>     TrustedAuthorityIndicator class in application code, looks like it
>     may be enough to use an interface to represent a trusted authorities:
>         public interface TrustedAuthorityIndicator {
>              // identifier type, standard algorithm name
>              String/int/Byte getType();
>
>              // identifier
>              byte[] getEncoded();
>         }
>
>     What do you think?
>
>
>     Thanks & Regards,
>     Xuelei
>
>     [1]
>     https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
>     <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>
>
>
>     On 6/13/2017 3:41 PM, Martin Balao wrote:
>
>         Hi Xuelei,
>
>         The new webrev.01 is ready:
>
>            *
>         http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
>         <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>
>         (browse online)
>            *
>         http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
>         <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>
>         (zip, download)
>
>         The following changes have been implemented since the previous
>         webrev.00:
>
>            * Pre-agreed support removed from server-side
>             * Unnecessary overhead and minium benefits for JSSE.
>
>            * Enabling the use of Trusted CA Indication extension for
>         clients through TrustManager objects was reverted. Trusted CA
>         Indication extension can now be enabled through: 1) SSLEngine,
>         2) SSLSocket, or 3) SSLParameters (which can be applied to both
>         SSLEngine and SSLSocket objects). Trusted CA Indication
>         extension is mandatory for servers.
>
>            * SunX509KeyManagerImpl old key manager ("SunX509" algorithm)
>         is now out of scope. This key manager does not support other TLS
>         extensions as Server Name Indication (SNI), which is far more
>         relevant than Trusted CA Indication. The new X509KeyManagerImpl
>         key manager ("PKIX" algorithm) is now in scope.
>
>            * Client requested indications are now an ExtendedSSLSession
>         attribute. ServerHandshaker gets the information from the Client
>         Hello message (now parsed by TrustedCAIndicationExtension class
>         instead of TrustedAuthorityIndicator) and sets it in the
>         ExtendedSSLSession (SSLSessionImpl object). The key manager
>         (i.e.: X509KeyManagerImpl), when choosing a server alias, may
>         now get the information from the ExtendedSSLSession object and
>         guide the certificate selection based on it.
>             * In order to allow multiple key managers to use Trusted
>         Authority Indicators information and to allow multiple Trusted
>         Authority Indicators implementations, TrustedAuthorityIndicator
>         has now been split in an abstract class
>         (TrustedAuthorityIndicator, located in javax.net.ssl) and an
>         implementation class (TrustedAuthorityIndicatorImpl, located in
>         sun.security.ssl). No coupling was added between javax.net.ssl
>         and sun.security.ssl packages.
>
>            * Documentation extended and improved.
>            * Test cases (server and client) updated to reflect the new
>         interface and supported key manager.
>
>         Look forward to your new review!
>
>         Kind regards,
>         Martin.-
>
>
>
>         On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email] <mailto:[hidden email]>>>
>         wrote:
>
>              I'm OK to use SSLParameters.  Thank you very much for
>         considering a
>              new design.
>
>              Xuelei
>
>              On 6/9/2017 1:10 PM, Martin Balao wrote:
>
>                  Hi Xuelei,
>
>                  I didn't notice that some of the SSLSocket contructors
>         did not
>                  establish the connection, so SSLParameters can be
>         effective for
>                  Trusted CA Indication. This was an invalid argument on
>         my side,
>                  sorry.
>
>                  As for the configuration to enable the extension, it's
>         probably
>                  not necessary on the Server side because -as you
>         mentioned- it
>                  is mandatory and there is no harm in supporting it.
>         However, it
>                  has to be configurable on the Client side because -as we
>                  previously discussed- the client may cause a handshake
>         failure
>                  if the server does not support the extension. I'd
>         prefer the
>                  Client configuring the SSLSocket through SSLParameters
>         instead
>                  of a system-wide property -which has even more impact
>         than the
>                  TrustManager approach-. Would this work for you?
>
>                    > In JSSE, the benefits pre_agreed option can get by
>                  customizing the key/trust manager, so I did not see too
>         much
>                  benefits to support this option in JDK
>
>                  I understand your point and will remove support for
>         "pre_agreed".
>
>
>                  On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
>                  <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email] <mailto:[hidden email]>>
>                  <mailto:[hidden email]
>         <mailto:[hidden email]> <mailto:[hidden email]
>         <mailto:[hidden email]>>>>
>                  wrote:
>
>
>
>                       On 6/8/2017 8:36 PM, Xuelei Fan wrote:
>
>                           The trusted authorities can be get from client
>         trust
>                  manager.         Server can choose the best matching of
>         server
>                  certificate of the
>                           client requested trusted authorities.
>
>                       >
>                       I missed the point that the key manager need to
>         know the client
>                       requested trusted authorities for the choosing.
>         So may
>                  need a new
>                       SSLSession attribute (See similar method in
>                  ExtendedSSLSession).
>
>                       Xuelei
>
>
>
>                  Yes, an attribute on SSLSession may do the job (both
>         when Key
>                  Manager receives a SSLSocket and a SSLEngine).
>
>                  Kind regards,
>                  Martin.-
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao
Hi,

Given that 1) Trusted CA Indication TLS Extension does not appear to be widely implemented today and 2) it will be replaced by Certificate Authorities TLS extension in TLS 1.3, it looks more beneficial to me supporting only the latter -and avoid paying the cost for a larger code-base-.

Here it is my proposal for Certificate Authorities TLS extension:


Implementation based on TLS 1.3 - draft 20 [1]. Patch based on JDK-10 (rev 76ff72bb6a4a).

Pending / blocked (in descending priority order):

High

 * The extension applies only to TLSv1.3+
  * Blocked because TLSv1.3 is still not supported in JSSE.
  * Impact: the extension cannot be used in TLS 1.2 (high impact on the client-side).
  * Action: replace "useTLS12PlusSpec" with "useTLS13PlusSpec" in the patch when available.

Medium

 * Server can send the CertificateAuthorities extension to the client in a CertificateRequest message (feature)
  * Blocked by: Server is still not able to send EncryptedExtensions message in CertificateRequest
  * Impact: feature not supported on the server side. The extension can still work in production environments. (medium). 
  * Action: implement EncryptedExtensions message in CertificateRequest and then implement this feature.

Low

 * Update documentation to refer the final TLS 1.3 RFC (draft 20 is currently referred)
  * Blocked by: publication of the final TLS 1.3 RFC
  * Impact: documentation is not accurate. (low)
  * Action: replace "https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4" with the final link in the patch. 

 * Update bug id in "CertificateAuthoritiesClientTest.java" and "CertificateAuthoritiesServerTest.java"
  * Blocked by: there is no bug id for Certificate Authorities TLS extension
  * Impact: internal tests (very low).
  * Action: replace "@bug 8046295" with the new bug id in the patch. Open a new bug id for Certificate Authorities TLS extension.
 
Look forward to your comments.

Kind regards,
Martin.-

--

On Tue, Jun 20, 2017 at 9:33 PM, Xuelei Fan <[hidden email]> wrote:
Hi Martin,

The TLS 1.3 spec is replacing the Trusted CA Indication (trusted_ca_keys) extension with a new Certificate Authorities (certificate_authorities) extension.  See more details about the specification in the TLS 1.3 draft:
    https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4

Both serves a similar purpose, but the trusted_ca_keys extension will not be used in TLS 1.3 any more.  The "trusted_ca_keys" extension will only be used for legacy protocol versions (TLS 1.2/1.1/1.0).

There are two options to me:
1. Supports the certificate_authorities, but not trusted_ca_keys extension.
It is acceptable to me as trusted_ca_keys is for legacy use only and the certificate_authorities extension is the future.  Plus, the certificate_authorities extension can also be used for TLS 1.2 and previous versions.

2. Supports both the certificate_authorities and trusted_ca_keys extensions.
As far as I know, I did not see much benefit of this option unless the trusted_ca_keys extension is widely used in practice.

If I did not miss something, the APIs you designed can still be used for the certificate_authorities extension, with a little bit update.

What do you think?

Thanks & Regards,
Xuelei


On 6/15/2017 12:05 PM, Martin Balao wrote:
Hi Xuelei,

The new webrev.02 is ready:

  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/ (browse online)
  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip (zip, download)

The following changes have been implemented since the previous webrev.01:

  * s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket and in SSLEngineImpl/SSLSocketImpl removed. s/getSSLParameters is now the only way to set or get the use of the Trusted CA Indication extension. An exception is no longer thrown if trying to disable the extension for a server, but the change takes no effect as the extension is mandatory for servers. X509KeyManagerImpl modified to use SSLParameters to get information regarding if Trusted CA Indication is enabled and should guide the certificate choice.

  * TrustedAuthorityIndicator.IdentifierType has been moved from enum to String, to follow JSSE conventions. I understand how important is to be consistent. However, I still believe that an enum is a better fit for this value and does not prevent from future extension. We are choosing from a closed set (strictly defined by the RFC) and that's what enum allows to express. From the client point of view/API, it's very handy that the type gives you information regarding the allowed choices for the parameter. You don't necessarily have to look for implementation details or documentation but you can just leverage on the strongly typed language. It's also likely that enums are faster for comparisons than strings, but that's not the main point here.

  * Removed X509Certificate from TrustedAuthorityIndicator class (method and property). It was there for informational purposes (when TrustedAuthorityIndicator was built from a certificate by a client and the whole extension indicators converted to String).

  * "equals" and "hashCode" methods moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

  * "getLength" method removed from TrustedAuthorityIndicator class. It's possible to get the encoded buffer and the length from there.

  * "getData" method renamed to "getEncoded" in TrustedAuthorityIndicator class.

  * "trustedAuthorityEncodedData" renamed to "encodedData" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes

  * "identifier" and "encodedData" instance variables moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

  * "getEncoded" and "getIdentifier" are now abstract methods in TrustedAuthorityIndicator, and their implementation is in TrustedAuthorityIndicatorImpl class.

  * "getIdentifier" method renamed to "getType" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes ("identifier" instance variable and parameter in TrustedAuthorityIndicatorImpl class renamed to "type").

  * Test cases (server and client) updated to reflect the new interface (enabling the use of the extension through SSLParameters)

However, some changes are still not implemented and I have some concerns:

1) I still believe that identifier type information has to be on TrustedAuthorityIndicator class somehow, and implementations restricted on what they can return as part of "getType" method. This is strictly specified by the RFC TrustedAuthorityIndicator class represents, and I find desirable that any implementation is enforced to be compliant to that. If we remove all of that (including the enum), TrustedAuthorityIndicator looks too generic and does not reflect (in my opinion) what it really is. It'd also be chaotic if different implementations call pre-agreed type as "preagreed", "pre-agreed", "PRE_AGREED", etc. I prefer stricter and more explicit interfaces.

2) I agree that type mappings can be seen as part of an implementation, but they were in TrustedAuthorityIndicator (as protected) because every implementation is highly likely to need them and we can avoid the necessity for repeated code/mappings. The same for "type" and "encodedData" variables or even "hashCode" and "equals" methods. That's why I was thinking more of an abstract class and not an interface, as it happens (in example) with SNIServerName.

3) I think that "implies" method on TrustedAuthorityIndicator should be also part of the class/interface, because that's the whole point of a Trusted Authority Information: to allow queries for a given certificate. This is, in fact, the only thing a server wants from one of these objects. My concern is that if we remove this requirement for an implementation, the interface looks more like a byte buffer holder.

I'd appreciate if you can re-consider these items.

Thanks,
Martin.-

On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan <[hidden email] <mailto:[hidden email]>> wrote:

    Hi Martin,

    The big picture of the design looks pretty good to me, except a few
    comment about the JSSE conventions.  I appreciate it very much.  By
    the way, I need more time to look into the details of the
    specification and implementation.


    In order to keep the APIs simple and small, SSLParameters is
    preferred as the only configuration port for common cases.   I may
    suggest to remove the s/getUseTrustedCAIndication() methods in
    SSLEngine/SSLSocket.

    The identify type is defined as an enum
    TrustedAuthorityIndicator.IdentifierType.  In the future, if more
    type is added, we need to update the specification by adding a new
    enum item.  Enum is preferred in JDK, but for good extensibility, in
    general JSSE does not use enum in public APIs for extensible
    properties.  I may suggest to use String (or integer/byte, I prefer
    to use String) as the type.  The standard trusted authority
    indicator algorithm (identifier) can be documented in the "Java
    Cryptography Architecture Standard Algorithm Name Documentation"[1].

    In TrustedAuthorityIndicator class, some methods, like
    getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(),
    getLength(), equals() and hashCode() look more like implementation
    logic.  I may suggest remove them from public APIs.

    I did not see the benefit to have X509Certificate in the
    TrustedAuthorityIndicator class.  The class is mainly used for
    server side certificate selection.  X509Certificate could be unknown
    for a indicator.  I may suggestion remove the related methods and
    properties.

    After that, as there is no requirement to instantiate
    TrustedAuthorityIndicator class in application code, looks like it
    may be enough to use an interface to represent a trusted authorities:
        public interface TrustedAuthorityIndicator {
             // identifier type, standard algorithm name
             String/int/Byte getType();

             // identifier
             byte[] getEncoded();
        }

    What do you think?


    Thanks & Regards,
    Xuelei

    [1]
    https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
    <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>


    On 6/13/2017 3:41 PM, Martin Balao wrote:

        Hi Xuelei,

        The new webrev.01 is ready:

           *
        http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
        <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>
        (browse online)
           *
        http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
        <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>
        (zip, download)

        The following changes have been implemented since the previous
        webrev.00:

           * Pre-agreed support removed from server-side
            * Unnecessary overhead and minium benefits for JSSE.

           * Enabling the use of Trusted CA Indication extension for
        clients through TrustManager objects was reverted. Trusted CA
        Indication extension can now be enabled through: 1) SSLEngine,
        2) SSLSocket, or 3) SSLParameters (which can be applied to both
        SSLEngine and SSLSocket objects). Trusted CA Indication
        extension is mandatory for servers.

           * SunX509KeyManagerImpl old key manager ("SunX509" algorithm)
        is now out of scope. This key manager does not support other TLS
        extensions as Server Name Indication (SNI), which is far more
        relevant than Trusted CA Indication. The new X509KeyManagerImpl
        key manager ("PKIX" algorithm) is now in scope.

           * Client requested indications are now an ExtendedSSLSession
        attribute. ServerHandshaker gets the information from the Client
        Hello message (now parsed by TrustedCAIndicationExtension class
        instead of TrustedAuthorityIndicator) and sets it in the
        ExtendedSSLSession (SSLSessionImpl object). The key manager
        (i.e.: X509KeyManagerImpl), when choosing a server alias, may
        now get the information from the ExtendedSSLSession object and
        guide the certificate selection based on it.
            * In order to allow multiple key managers to use Trusted
        Authority Indicators information and to allow multiple Trusted
        Authority Indicators implementations, TrustedAuthorityIndicator
        has now been split in an abstract class
        (TrustedAuthorityIndicator, located in javax.net.ssl) and an
        implementation class (TrustedAuthorityIndicatorImpl, located in
        sun.security.ssl). No coupling was added between javax.net.ssl
        and sun.security.ssl packages.

           * Documentation extended and improved.
           * Test cases (server and client) updated to reflect the new
        interface and supported key manager.

        Look forward to your new review!

        Kind regards,
        Martin.-



        On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan
        <[hidden email] <mailto:[hidden email]>
        <mailto:[hidden email] <mailto:[hidden email]>>>
        wrote:

             I'm OK to use SSLParameters.  Thank you very much for
        considering a
             new design.

             Xuelei

             On 6/9/2017 1:10 PM, Martin Balao wrote:

                 Hi Xuelei,

                 I didn't notice that some of the SSLSocket contructors
        did not
                 establish the connection, so SSLParameters can be
        effective for
                 Trusted CA Indication. This was an invalid argument on
        my side,
                 sorry.

                 As for the configuration to enable the extension, it's
        probably
                 not necessary on the Server side because -as you
        mentioned- it
                 is mandatory and there is no harm in supporting it.
        However, it
                 has to be configurable on the Client side because -as we
                 previously discussed- the client may cause a handshake
        failure
                 if the server does not support the extension. I'd
        prefer the
                 Client configuring the SSLSocket through SSLParameters
        instead
                 of a system-wide property -which has even more impact
        than the
                 TrustManager approach-. Would this work for you?

                   > In JSSE, the benefits pre_agreed option can get by
                 customizing the key/trust manager, so I did not see too
        much
                 benefits to support this option in JDK

                 I understand your point and will remove support for
        "pre_agreed".


                 On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
                 <[hidden email] <mailto:[hidden email]>
        <mailto:[hidden email] <mailto:[hidden email]>>
                 <mailto:[hidden email]
        <mailto:[hidden email]> <mailto:[hidden email]
        <mailto:[hidden email]>>>>
                 wrote:



                      On 6/8/2017 8:36 PM, Xuelei Fan wrote:

                          The trusted authorities can be get from client
        trust
                 manager.         Server can choose the best matching of
        server
                 certificate of the
                          client requested trusted authorities.

                      >
                      I missed the point that the key manager need to
        know the client
                      requested trusted authorities for the choosing.         So may
                 need a new
                      SSLSession attribute (See similar method in
                 ExtendedSSLSession).

                      Xuelei



                 Yes, an attribute on SSLSession may do the job (both
        when Key
                 Manager receives a SSLSocket and a SSLEngine).

                 Kind regards,
                 Martin.-




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Martin Balao

On Tue, Jul 18, 2017 at 2:12 PM, Martin Balao <[hidden email]> wrote:
Hi,

Given that 1) Trusted CA Indication TLS Extension does not appear to be widely implemented today and 2) it will be replaced by Certificate Authorities TLS extension in TLS 1.3, it looks more beneficial to me supporting only the latter -and avoid paying the cost for a larger code-base-.

Here it is my proposal for Certificate Authorities TLS extension:


Implementation based on TLS 1.3 - draft 20 [1]. Patch based on JDK-10 (rev 76ff72bb6a4a).

Pending / blocked (in descending priority order):

High

 * The extension applies only to TLSv1.3+
  * Blocked because TLSv1.3 is still not supported in JSSE.
  * Impact: the extension cannot be used in TLS 1.2 (high impact on the client-side).
  * Action: replace "useTLS12PlusSpec" with "useTLS13PlusSpec" in the patch when available.

Medium

 * Server can send the CertificateAuthorities extension to the client in a CertificateRequest message (feature)
  * Blocked by: Server is still not able to send EncryptedExtensions message in CertificateRequest
  * Impact: feature not supported on the server side. The extension can still work in production environments. (medium). 
  * Action: implement EncryptedExtensions message in CertificateRequest and then implement this feature.

Low

 * Update documentation to refer the final TLS 1.3 RFC (draft 20 is currently referred)
  * Blocked by: publication of the final TLS 1.3 RFC
  * Impact: documentation is not accurate. (low)
  * Action: replace "https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4" with the final link in the patch. 

 * Update bug id in "CertificateAuthoritiesClientTest.java" and "CertificateAuthoritiesServerTest.java"
  * Blocked by: there is no bug id for Certificate Authorities TLS extension
  * Impact: internal tests (very low).
  * Action: replace "@bug 8046295" with the new bug id in the patch. Open a new bug id for Certificate Authorities TLS extension.
 
Look forward to your comments.

Kind regards,
Martin.-

--

On Tue, Jun 20, 2017 at 9:33 PM, Xuelei Fan <[hidden email]> wrote:
Hi Martin,

The TLS 1.3 spec is replacing the Trusted CA Indication (trusted_ca_keys) extension with a new Certificate Authorities (certificate_authorities) extension.  See more details about the specification in the TLS 1.3 draft:
    https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4

Both serves a similar purpose, but the trusted_ca_keys extension will not be used in TLS 1.3 any more.  The "trusted_ca_keys" extension will only be used for legacy protocol versions (TLS 1.2/1.1/1.0).

There are two options to me:
1. Supports the certificate_authorities, but not trusted_ca_keys extension.
It is acceptable to me as trusted_ca_keys is for legacy use only and the certificate_authorities extension is the future.  Plus, the certificate_authorities extension can also be used for TLS 1.2 and previous versions.

2. Supports both the certificate_authorities and trusted_ca_keys extensions.
As far as I know, I did not see much benefit of this option unless the trusted_ca_keys extension is widely used in practice.

If I did not miss something, the APIs you designed can still be used for the certificate_authorities extension, with a little bit update.

What do you think?

Thanks & Regards,
Xuelei


On 6/15/2017 12:05 PM, Martin Balao wrote:
Hi Xuelei,

The new webrev.02 is ready:

  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/ (browse online)
  * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip (zip, download)

The following changes have been implemented since the previous webrev.01:

  * s/getUseTrustedCAIndication() methods in SSLEngine/SSLSocket and in SSLEngineImpl/SSLSocketImpl removed. s/getSSLParameters is now the only way to set or get the use of the Trusted CA Indication extension. An exception is no longer thrown if trying to disable the extension for a server, but the change takes no effect as the extension is mandatory for servers. X509KeyManagerImpl modified to use SSLParameters to get information regarding if Trusted CA Indication is enabled and should guide the certificate choice.

  * TrustedAuthorityIndicator.IdentifierType has been moved from enum to String, to follow JSSE conventions. I understand how important is to be consistent. However, I still believe that an enum is a better fit for this value and does not prevent from future extension. We are choosing from a closed set (strictly defined by the RFC) and that's what enum allows to express. From the client point of view/API, it's very handy that the type gives you information regarding the allowed choices for the parameter. You don't necessarily have to look for implementation details or documentation but you can just leverage on the strongly typed language. It's also likely that enums are faster for comparisons than strings, but that's not the main point here.

  * Removed X509Certificate from TrustedAuthorityIndicator class (method and property). It was there for informational purposes (when TrustedAuthorityIndicator was built from a certificate by a client and the whole extension indicators converted to String).

  * "equals" and "hashCode" methods moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

  * "getLength" method removed from TrustedAuthorityIndicator class. It's possible to get the encoded buffer and the length from there.

  * "getData" method renamed to "getEncoded" in TrustedAuthorityIndicator class.

  * "trustedAuthorityEncodedData" renamed to "encodedData" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes

  * "identifier" and "encodedData" instance variables moved from TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl class.

  * "getEncoded" and "getIdentifier" are now abstract methods in TrustedAuthorityIndicator, and their implementation is in TrustedAuthorityIndicatorImpl class.

  * "getIdentifier" method renamed to "getType" in TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl classes ("identifier" instance variable and parameter in TrustedAuthorityIndicatorImpl class renamed to "type").

  * Test cases (server and client) updated to reflect the new interface (enabling the use of the extension through SSLParameters)

However, some changes are still not implemented and I have some concerns:

1) I still believe that identifier type information has to be on TrustedAuthorityIndicator class somehow, and implementations restricted on what they can return as part of "getType" method. This is strictly specified by the RFC TrustedAuthorityIndicator class represents, and I find desirable that any implementation is enforced to be compliant to that. If we remove all of that (including the enum), TrustedAuthorityIndicator looks too generic and does not reflect (in my opinion) what it really is. It'd also be chaotic if different implementations call pre-agreed type as "preagreed", "pre-agreed", "PRE_AGREED", etc. I prefer stricter and more explicit interfaces.

2) I agree that type mappings can be seen as part of an implementation, but they were in TrustedAuthorityIndicator (as protected) because every implementation is highly likely to need them and we can avoid the necessity for repeated code/mappings. The same for "type" and "encodedData" variables or even "hashCode" and "equals" methods. That's why I was thinking more of an abstract class and not an interface, as it happens (in example) with SNIServerName.

3) I think that "implies" method on TrustedAuthorityIndicator should be also part of the class/interface, because that's the whole point of a Trusted Authority Information: to allow queries for a given certificate. This is, in fact, the only thing a server wants from one of these objects. My concern is that if we remove this requirement for an implementation, the interface looks more like a byte buffer holder.

I'd appreciate if you can re-consider these items.

Thanks,
Martin.-

On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan <[hidden email] <mailto:[hidden email]>> wrote:

    Hi Martin,

    The big picture of the design looks pretty good to me, except a few
    comment about the JSSE conventions.  I appreciate it very much.  By
    the way, I need more time to look into the details of the
    specification and implementation.


    In order to keep the APIs simple and small, SSLParameters is
    preferred as the only configuration port for common cases.   I may
    suggest to remove the s/getUseTrustedCAIndication() methods in
    SSLEngine/SSLSocket.

    The identify type is defined as an enum
    TrustedAuthorityIndicator.IdentifierType.  In the future, if more
    type is added, we need to update the specification by adding a new
    enum item.  Enum is preferred in JDK, but for good extensibility, in
    general JSSE does not use enum in public APIs for extensible
    properties.  I may suggest to use String (or integer/byte, I prefer
    to use String) as the type.  The standard trusted authority
    indicator algorithm (identifier) can be documented in the "Java
    Cryptography Architecture Standard Algorithm Name Documentation"[1].

    In TrustedAuthorityIndicator class, some methods, like
    getIdentifierTypeFromCode(), getCodeFromIdentifierType(), implies(),
    getLength(), equals() and hashCode() look more like implementation
    logic.  I may suggest remove them from public APIs.

    I did not see the benefit to have X509Certificate in the
    TrustedAuthorityIndicator class.  The class is mainly used for
    server side certificate selection.  X509Certificate could be unknown
    for a indicator.  I may suggestion remove the related methods and
    properties.

    After that, as there is no requirement to instantiate
    TrustedAuthorityIndicator class in application code, looks like it
    may be enough to use an interface to represent a trusted authorities:
        public interface TrustedAuthorityIndicator {
             // identifier type, standard algorithm name
             String/int/Byte getType();

             // identifier
             byte[] getEncoded();
        }

    What do you think?


    Thanks & Regards,
    Xuelei

    [1]
    https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
    <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>


    On 6/13/2017 3:41 PM, Martin Balao wrote:

        Hi Xuelei,

        The new webrev.01 is ready:

           *
        http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
        <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>
        (browse online)
           *
        http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
        <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>
        (zip, download)

        The following changes have been implemented since the previous
        webrev.00:

           * Pre-agreed support removed from server-side
            * Unnecessary overhead and minium benefits for JSSE.

           * Enabling the use of Trusted CA Indication extension for
        clients through TrustManager objects was reverted. Trusted CA
        Indication extension can now be enabled through: 1) SSLEngine,
        2) SSLSocket, or 3) SSLParameters (which can be applied to both
        SSLEngine and SSLSocket objects). Trusted CA Indication
        extension is mandatory for servers.

           * SunX509KeyManagerImpl old key manager ("SunX509" algorithm)
        is now out of scope. This key manager does not support other TLS
        extensions as Server Name Indication (SNI), which is far more
        relevant than Trusted CA Indication. The new X509KeyManagerImpl
        key manager ("PKIX" algorithm) is now in scope.

           * Client requested indications are now an ExtendedSSLSession
        attribute. ServerHandshaker gets the information from the Client
        Hello message (now parsed by TrustedCAIndicationExtension class
        instead of TrustedAuthorityIndicator) and sets it in the
        ExtendedSSLSession (SSLSessionImpl object). The key manager
        (i.e.: X509KeyManagerImpl), when choosing a server alias, may
        now get the information from the ExtendedSSLSession object and
        guide the certificate selection based on it.
            * In order to allow multiple key managers to use Trusted
        Authority Indicators information and to allow multiple Trusted
        Authority Indicators implementations, TrustedAuthorityIndicator
        has now been split in an abstract class
        (TrustedAuthorityIndicator, located in javax.net.ssl) and an
        implementation class (TrustedAuthorityIndicatorImpl, located in
        sun.security.ssl). No coupling was added between javax.net.ssl
        and sun.security.ssl packages.

           * Documentation extended and improved.
           * Test cases (server and client) updated to reflect the new
        interface and supported key manager.

        Look forward to your new review!

        Kind regards,
        Martin.-



        On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan
        <[hidden email] <mailto:[hidden email]>
        <mailto:[hidden email] <mailto:[hidden email]>>>
        wrote:

             I'm OK to use SSLParameters.  Thank you very much for
        considering a
             new design.

             Xuelei

             On 6/9/2017 1:10 PM, Martin Balao wrote:

                 Hi Xuelei,

                 I didn't notice that some of the SSLSocket contructors
        did not
                 establish the connection, so SSLParameters can be
        effective for
                 Trusted CA Indication. This was an invalid argument on
        my side,
                 sorry.

                 As for the configuration to enable the extension, it's
        probably
                 not necessary on the Server side because -as you
        mentioned- it
                 is mandatory and there is no harm in supporting it.
        However, it
                 has to be configurable on the Client side because -as we
                 previously discussed- the client may cause a handshake
        failure
                 if the server does not support the extension. I'd
        prefer the
                 Client configuring the SSLSocket through SSLParameters
        instead
                 of a system-wide property -which has even more impact
        than the
                 TrustManager approach-. Would this work for you?

                   > In JSSE, the benefits pre_agreed option can get by
                 customizing the key/trust manager, so I did not see too
        much
                 benefits to support this option in JDK

                 I understand your point and will remove support for
        "pre_agreed".


                 On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
                 <[hidden email] <mailto:[hidden email]>
        <mailto:[hidden email] <mailto:[hidden email]>>
                 <mailto:[hidden email]
        <mailto:[hidden email]> <mailto:[hidden email]
        <mailto:[hidden email]>>>>
                 wrote:



                      On 6/8/2017 8:36 PM, Xuelei Fan wrote:

                          The trusted authorities can be get from client
        trust
                 manager.         Server can choose the best matching of
        server
                 certificate of the
                          client requested trusted authorities.

                      >
                      I missed the point that the key manager need to
        know the client
                      requested trusted authorities for the choosing.         So may
                 need a new
                      SSLSession attribute (See similar method in
                 ExtendedSSLSession).

                      Xuelei



                 Yes, an attribute on SSLSession may do the job (both
        when Key
                 Manager receives a SSLSocket and a SSLEngine).

                 Kind regards,
                 Martin.-





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Xuelei Fan-2
Hi Martin,

Sorry for the delay.

I'd like to wait for finalization of TLS 1.3 specification, so that we
can get a stable specification of the Certificate Authorities extension.

For the current design, I did not see much benefit to add a new
CertificateAuthority API.  The CertificateAuthority.implies() may not
yet reach the threshold to be a public API.  A trust/key manager can
easily matching the distinguished name with the target certificate.  And
I did not see the cert matching behavior differences between different
providers and trust/key managers.

For the specification documentation part, I may suggest reword them a
little bit so that those developers who are not TLS specification
experts can easily catch the purpose or benefits of the API.

Xuelei

On 7/21/2017 7:20 AM, Martin Balao wrote:

> Webrev has been uploaded to cr.openjdk.java.net
> <http://cr.openjdk.java.net>:
>
>   *
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.03/
>   *
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8046295/webrev.03/8046295.webrev.03.zip
>
> Kind regards,
> Martin.-
>
> On Tue, Jul 18, 2017 at 2:12 PM, Martin Balao <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     Given that 1) Trusted CA Indication TLS Extension does not appear to
>     be widely implemented today and 2) it will be replaced by
>     Certificate Authorities TLS extension in TLS 1.3, it looks more
>     beneficial to me supporting only the latter -and avoid paying the
>     cost for a larger code-base-.
>
>     Here it is my proposal for Certificate Authorities TLS extension:
>
>       *
>     http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03/
>     <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03/>
>     (browse online)
>       *
>     http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03.zip
>     <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_07_18/8046295.webrev.03.zip>
>     (download)
>
>     Implementation based on TLS 1.3 - draft 20 [1]. Patch based on
>     JDK-10 (rev 76ff72bb6a4a).
>
>     Pending / blocked (in descending priority order):
>
>     High
>
>       * The extension applies only to TLSv1.3+
>        * Blocked because TLSv1.3 is still not supported in JSSE.
>        * Impact: the extension cannot be used in TLS 1.2 (high impact on
>     the client-side).
>        * Action: replace "useTLS12PlusSpec" with "useTLS13PlusSpec" in
>     the patch when available.
>
>     Medium
>
>       * Server can send the CertificateAuthorities extension to the
>     client in a CertificateRequest message (feature)
>        * Blocked by: Server is still not able to send
>     EncryptedExtensions message in CertificateRequest
>        * Impact: feature not supported on the server side. The extension
>     can still work in production environments. (medium).
>        * Action: implement EncryptedExtensions message in
>     CertificateRequest and then implement this feature.
>
>     Low
>
>       * Update documentation to refer the final TLS 1.3 RFC (draft 20 is
>     currently referred)
>        * Blocked by: publication of the final TLS 1.3 RFC
>        * Impact: documentation is not accurate. (low)
>        * Action: replace
>     "https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4
>     <https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>"
>     with the final link in the patch.
>
>       * Update bug id in "CertificateAuthoritiesClientTest.java" and
>     "CertificateAuthoritiesServerTest.java"
>        * Blocked by: there is no bug id for Certificate Authorities TLS
>     extension
>        * Impact: internal tests (very low).
>        * Action: replace "@bug 8046295" with the new bug id in the
>     patch. Open a new bug id for Certificate Authorities TLS extension.
>     Look forward to your comments.
>
>     Kind regards,
>     Martin.-
>
>     --
>     [1] -
>     https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4
>     <https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>
>
>     On Tue, Jun 20, 2017 at 9:33 PM, Xuelei Fan <[hidden email]
>     <mailto:[hidden email]>> wrote:
>
>         Hi Martin,
>
>         The TLS 1.3 spec is replacing the Trusted CA Indication
>         (trusted_ca_keys) extension with a new Certificate Authorities
>         (certificate_authorities) extension.  See more details about the
>         specification in the TLS 1.3 draft:
>         https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4 <https://tools.ietf.org/html/draft-ietf-tls-tls13-20#section-4.2.4>
>
>         Both serves a similar purpose, but the trusted_ca_keys extension
>         will not be used in TLS 1.3 any more.  The "trusted_ca_keys"
>         extension will only be used for legacy protocol versions (TLS
>         1.2/1.1/1.0).
>
>         There are two options to me:
>         1. Supports the certificate_authorities, but not trusted_ca_keys
>         extension.
>         It is acceptable to me as trusted_ca_keys is for legacy use only
>         and the certificate_authorities extension is the future.  Plus,
>         the certificate_authorities extension can also be used for TLS
>         1.2 and previous versions.
>
>         2. Supports both the certificate_authorities and trusted_ca_keys
>         extensions.
>         As far as I know, I did not see much benefit of this option
>         unless the trusted_ca_keys extension is widely used in practice.
>
>         If I did not miss something, the APIs you designed can still be
>         used for the certificate_authorities extension, with a little
>         bit update.
>
>         What do you think?
>
>         Thanks & Regards,
>         Xuelei
>
>
>         On 6/15/2017 12:05 PM, Martin Balao wrote:
>
>             Hi Xuelei,
>
>             The new webrev.02 is ready:
>
>                *
>             http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02/>
>             (browse online)
>                *
>             http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_15/8046295.webrev.02.zip>
>             (zip, download)
>
>             The following changes have been implemented since the
>             previous webrev.01:
>
>                * s/getUseTrustedCAIndication() methods in
>             SSLEngine/SSLSocket and in SSLEngineImpl/SSLSocketImpl
>             removed. s/getSSLParameters is now the only way to set or
>             get the use of the Trusted CA Indication extension. An
>             exception is no longer thrown if trying to disable the
>             extension for a server, but the change takes no effect as
>             the extension is mandatory for servers. X509KeyManagerImpl
>             modified to use SSLParameters to get information regarding
>             if Trusted CA Indication is enabled and should guide the
>             certificate choice.
>
>                * TrustedAuthorityIndicator.IdentifierType has been moved
>             from enum to String, to follow JSSE conventions. I
>             understand how important is to be consistent. However, I
>             still believe that an enum is a better fit for this value
>             and does not prevent from future extension. We are choosing
>             from a closed set (strictly defined by the RFC) and that's
>             what enum allows to express. From the client point of
>             view/API, it's very handy that the type gives you
>             information regarding the allowed choices for the parameter.
>             You don't necessarily have to look for implementation
>             details or documentation but you can just leverage on the
>             strongly typed language. It's also likely that enums are
>             faster for comparisons than strings, but that's not the main
>             point here.
>
>                * Removed X509Certificate from TrustedAuthorityIndicator
>             class (method and property). It was there for informational
>             purposes (when TrustedAuthorityIndicator was built from a
>             certificate by a client and the whole extension indicators
>             converted to String).
>
>                * "equals" and "hashCode" methods moved from
>             TrustedAuthorityIndicator to TrustedAuthorityIndicatorImpl
>             class.
>
>                * "getLength" method removed from
>             TrustedAuthorityIndicator class. It's possible to get the
>             encoded buffer and the length from there.
>
>                * "getData" method renamed to "getEncoded" in
>             TrustedAuthorityIndicator class.
>
>                * "trustedAuthorityEncodedData" renamed to "encodedData"
>             in TrustedAuthorityIndicator and
>             TrustedAuthorityIndicatorImpl classes
>
>                * "identifier" and "encodedData" instance variables moved
>             from TrustedAuthorityIndicator to
>             TrustedAuthorityIndicatorImpl class.
>
>                * "getEncoded" and "getIdentifier" are now abstract
>             methods in TrustedAuthorityIndicator, and their
>             implementation is in TrustedAuthorityIndicatorImpl class.
>
>                * "getIdentifier" method renamed to "getType" in
>             TrustedAuthorityIndicator and TrustedAuthorityIndicatorImpl
>             classes ("identifier" instance variable and parameter in
>             TrustedAuthorityIndicatorImpl class renamed to "type").
>
>                * Test cases (server and client) updated to reflect the
>             new interface (enabling the use of the extension through
>             SSLParameters)
>
>             However, some changes are still not implemented and I have
>             some concerns:
>
>             1) I still believe that identifier type information has to
>             be on TrustedAuthorityIndicator class somehow, and
>             implementations restricted on what they can return as part
>             of "getType" method. This is strictly specified by the RFC
>             TrustedAuthorityIndicator class represents, and I find
>             desirable that any implementation is enforced to be
>             compliant to that. If we remove all of that (including the
>             enum), TrustedAuthorityIndicator looks too generic and does
>             not reflect (in my opinion) what it really is. It'd also be
>             chaotic if different implementations call pre-agreed type as
>             "preagreed", "pre-agreed", "PRE_AGREED", etc. I prefer
>             stricter and more explicit interfaces.
>
>             2) I agree that type mappings can be seen as part of an
>             implementation, but they were in TrustedAuthorityIndicator
>             (as protected) because every implementation is highly likely
>             to need them and we can avoid the necessity for repeated
>             code/mappings. The same for "type" and "encodedData"
>             variables or even "hashCode" and "equals" methods. That's
>             why I was thinking more of an abstract class and not an
>             interface, as it happens (in example) with SNIServerName.
>
>             3) I think that "implies" method on
>             TrustedAuthorityIndicator should be also part of the
>             class/interface, because that's the whole point of a Trusted
>             Authority Information: to allow queries for a given
>             certificate. This is, in fact, the only thing a server wants
>             from one of these objects. My concern is that if we remove
>             this requirement for an implementation, the interface looks
>             more like a byte buffer holder.
>
>             I'd appreciate if you can re-consider these items.
>
>             Thanks,
>             Martin.-
>
>             On Wed, Jun 14, 2017 at 7:17 PM, Xuelei Fan
>             <[hidden email] <mailto:[hidden email]>
>             <mailto:[hidden email]
>             <mailto:[hidden email]>>> wrote:
>
>                  Hi Martin,
>
>                  The big picture of the design looks pretty good to me,
>             except a few
>                  comment about the JSSE conventions.  I appreciate it
>             very much.  By
>                  the way, I need more time to look into the details of the
>                  specification and implementation.
>
>
>                  In order to keep the APIs simple and small,
>             SSLParameters is
>                  preferred as the only configuration port for common
>             cases.   I may
>                  suggest to remove the s/getUseTrustedCAIndication()
>             methods in
>                  SSLEngine/SSLSocket.
>
>                  The identify type is defined as an enum
>                  TrustedAuthorityIndicator.IdentifierType.  In the
>             future, if more
>                  type is added, we need to update the specification by
>             adding a new
>                  enum item.  Enum is preferred in JDK, but for good
>             extensibility, in
>                  general JSSE does not use enum in public APIs for
>             extensible
>                  properties.  I may suggest to use String (or
>             integer/byte, I prefer
>                  to use String) as the type.  The standard trusted authority
>                  indicator algorithm (identifier) can be documented in
>             the "Java
>                  Cryptography Architecture Standard Algorithm Name
>             Documentation"[1].
>
>                  In TrustedAuthorityIndicator class, some methods, like
>                  getIdentifierTypeFromCode(),
>             getCodeFromIdentifierType(), implies(),
>                  getLength(), equals() and hashCode() look more like
>             implementation
>                  logic.  I may suggest remove them from public APIs.
>
>                  I did not see the benefit to have X509Certificate in the
>                  TrustedAuthorityIndicator class.  The class is mainly
>             used for
>                  server side certificate selection.  X509Certificate
>             could be unknown
>                  for a indicator.  I may suggestion remove the related
>             methods and
>                  properties.
>
>                  After that, as there is no requirement to instantiate
>                  TrustedAuthorityIndicator class in application code,
>             looks like it
>                  may be enough to use an interface to represent a
>             trusted authorities:
>                      public interface TrustedAuthorityIndicator {
>                           // identifier type, standard algorithm name
>                           String/int/Byte getType();
>
>                           // identifier
>                           byte[] getEncoded();
>                      }
>
>                  What do you think?
>
>
>                  Thanks & Regards,
>                  Xuelei
>
>                  [1]
>             https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
>             <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>
>                
>             <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
>             <https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html>>
>
>
>                  On 6/13/2017 3:41 PM, Martin Balao wrote:
>
>                      Hi Xuelei,
>
>                      The new webrev.01 is ready:
>
>                         *
>             http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>
>                    
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01/>>
>                      (browse online)
>                         *
>             http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>
>                    
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip
>             <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trusted_ca/2017_06_13/8046295.webrev.01.zip>>
>                      (zip, download)
>
>                      The following changes have been implemented since
>             the previous
>                      webrev.00:
>
>                         * Pre-agreed support removed from server-side
>                          * Unnecessary overhead and minium benefits for
>             JSSE.
>
>                         * Enabling the use of Trusted CA Indication
>             extension for
>                      clients through TrustManager objects was reverted.
>             Trusted CA
>                      Indication extension can now be enabled through: 1)
>             SSLEngine,
>                      2) SSLSocket, or 3) SSLParameters (which can be
>             applied to both
>                      SSLEngine and SSLSocket objects). Trusted CA Indication
>                      extension is mandatory for servers.
>
>                         * SunX509KeyManagerImpl old key manager
>             ("SunX509" algorithm)
>                      is now out of scope. This key manager does not
>             support other TLS
>                      extensions as Server Name Indication (SNI), which
>             is far more
>                      relevant than Trusted CA Indication. The new
>             X509KeyManagerImpl
>                      key manager ("PKIX" algorithm) is now in scope.
>
>                         * Client requested indications are now an
>             ExtendedSSLSession
>                      attribute. ServerHandshaker gets the information
>             from the Client
>                      Hello message (now parsed by
>             TrustedCAIndicationExtension class
>                      instead of TrustedAuthorityIndicator) and sets it
>             in the
>                      ExtendedSSLSession (SSLSessionImpl object). The key
>             manager
>                      (i.e.: X509KeyManagerImpl), when choosing a server
>             alias, may
>                      now get the information from the ExtendedSSLSession
>             object and
>                      guide the certificate selection based on it.
>                          * In order to allow multiple key managers to
>             use Trusted
>                      Authority Indicators information and to allow
>             multiple Trusted
>                      Authority Indicators implementations,
>             TrustedAuthorityIndicator
>                      has now been split in an abstract class
>                      (TrustedAuthorityIndicator, located in
>             javax.net.ssl) and an
>                      implementation class
>             (TrustedAuthorityIndicatorImpl, located in
>                      sun.security.ssl). No coupling was added between
>             javax.net.ssl
>                      and sun.security.ssl packages.
>
>                         * Documentation extended and improved.
>                         * Test cases (server and client) updated to
>             reflect the new
>                      interface and supported key manager.
>
>                      Look forward to your new review!
>
>                      Kind regards,
>                      Martin.-
>
>
>
>                      On Fri, Jun 9, 2017 at 6:15 PM, Xuelei Fan
>                      <[hidden email]
>             <mailto:[hidden email]> <mailto:[hidden email]
>             <mailto:[hidden email]>>
>                      <mailto:[hidden email]
>             <mailto:[hidden email]> <mailto:[hidden email]
>             <mailto:[hidden email]>>>>
>                      wrote:
>
>                           I'm OK to use SSLParameters.  Thank you very
>             much for
>                      considering a
>                           new design.
>
>                           Xuelei
>
>                           On 6/9/2017 1:10 PM, Martin Balao wrote:
>
>                               Hi Xuelei,
>
>                               I didn't notice that some of the SSLSocket
>             contructors
>                      did not
>                               establish the connection, so SSLParameters
>             can be
>                      effective for
>                               Trusted CA Indication. This was an invalid
>             argument on
>                      my side,
>                               sorry.
>
>                               As for the configuration to enable the
>             extension, it's
>                      probably
>                               not necessary on the Server side because
>             -as you
>                      mentioned- it
>                               is mandatory and there is no harm in
>             supporting it.
>                      However, it
>                               has to be configurable on the Client side
>             because -as we
>                               previously discussed- the client may cause
>             a handshake
>                      failure
>                               if the server does not support the
>             extension. I'd
>                      prefer the
>                               Client configuring the SSLSocket through
>             SSLParameters
>                      instead
>                               of a system-wide property -which has even
>             more impact
>                      than the
>                               TrustManager approach-. Would this work
>             for you?
>
>                                 > In JSSE, the benefits pre_agreed
>             option can get by
>                               customizing the key/trust manager, so I
>             did not see too
>                      much
>                               benefits to support this option in JDK
>
>                               I understand your point and will remove
>             support for
>                      "pre_agreed".
>
>
>                               On Fri, Jun 9, 2017 at 1:37 AM, Xuelei Fan
>                               <[hidden email]
>             <mailto:[hidden email]> <mailto:[hidden email]
>             <mailto:[hidden email]>>
>                      <mailto:[hidden email]
>             <mailto:[hidden email]> <mailto:[hidden email]
>             <mailto:[hidden email]>>>
>                               <mailto:[hidden email]
>             <mailto:[hidden email]>
>                      <mailto:[hidden email]
>             <mailto:[hidden email]>>
>             <mailto:[hidden email] <mailto:[hidden email]>
>                      <mailto:[hidden email]
>             <mailto:[hidden email]>>>>>
>                               wrote:
>
>
>
>                                    On 6/8/2017 8:36 PM, Xuelei Fan wrote:
>
>                                        The trusted authorities can be
>             get from client
>                      trust
>                               manager.         Server can choose the
>             best matching of
>                      server
>                               certificate of the
>                                        client requested trusted authorities.
>
>                                    >
>                                    I missed the point that the key
>             manager need to
>                      know the client
>                                    requested trusted authorities for the
>             choosing.         So may
>                               need a new
>                                    SSLSession attribute (See similar
>             method in
>                               ExtendedSSLSession).
>
>                                    Xuelei
>
>
>
>                               Yes, an attribute on SSLSession may do the
>             job (both
>                      when Key
>                               Manager receives a SSLSocket and a SSLEngine).
>
>                               Kind regards,
>                               Martin.-
>
>
>
>
>
Loading...