Code review request: JDK-8046295 - Support Trusted CA Indication extension
Martin Balao
mbalao at redhat.com
Fri Jul 21 14:20:56 UTC 2017
Webrev has been uploaded to 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> 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/ (browse online)
> * 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" 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
>
> On Tue, Jun 20, 2017 at 9:33 PM, Xuelei Fan <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
>>
>> 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_truste
>>> d_ca/2017_06_15/8046295.webrev.02/ (browse online)
>>> * http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_truste
>>> d_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/secur
>>> ity/StandardNames.html
>>> <https://docs.oracle.com/javase/8/docs/technotes/guides/secu
>>> rity/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_truste
>>> d_ca/2017_06_13/8046295.webrev.01/
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_ca/2017_06_13/8046295.webrev.01/>
>>> (browse online)
>>> *
>>> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_truste
>>> d_ca/2017_06_13/8046295.webrev.01.zip
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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.-
>>>
>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20170721/7bce6150/attachment.htm>
More information about the security-dev
mailing list