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