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: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20170721/7bce6150/attachment-0001.html>


More information about the security-dev mailing list