Code review request: JDK-8046295 - Support Trusted CA Indication extension
Xuelei Fan
xuelei.fan at oracle.com
Fri Aug 11 15:24:03 UTC 2017
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 <mbalao at redhat.com
> <mailto:mbalao at redhat.com>> 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 <xuelei.fan at oracle.com
> <mailto:xuelei.fan at oracle.com>> 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
> <xuelei.fan at oracle.com <mailto:xuelei.fan at oracle.com>
> <mailto: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>
>
> <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
> <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:
>
> 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>>>
> <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
> <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