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

[Xuelei Fan]

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 <xuelei.fan at oracle.com 
> <mailto:xuelei.fan at oracle.com>> 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
>         <xuelei.fan at oracle.com <mailto:xuelei.fan at oracle.com>
>         <mailto:xuelei.fan at oracle.com <mailto:xuelei.fan at oracle.com>>>
>         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.-
> 
>