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

Xuelei Fan xuelei.fan at oracle.com
Wed Jun 21 00:33:58 UTC 2017


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


More information about the security-dev mailing list