[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-security-dev
Subject:    Re: Code review request: JDK-8046295 - Support Trusted CA Indication extension
From:       Xuelei Fan <xuelei.fan () oracle ! com>
Date:       2017-06-21 0:33:58
Message-ID: da2d4af3-147c-762a-31cc-ef6dfc606c63 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]