Code review request: JDK-8046295 - Support Trusted CA Indication extension
Martin Balao
mbalao at redhat.com
Fri Jun 29 17:03:48 UTC 2018
Hi Xuelei,
Thanks for your notification.
This is on my backlog again but don't have an ETA yet.
Kind regards,
Martin.-
On Thu, Jun 28, 2018 at 1:14 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> Hi Martin,
>
> The TLS 1.3 implementation was integrated into the mainline.
>
> I know you have multiple contributions were pending because of the re-org
> of the JSSE implementation. Would you mind to check if your design and
> implementation need some adjustment?
>
> I may not reply for your contributions in other email threads. Please
> trigger the code review again if the code is ready for the new JSSE
> implementation.
>
> Thanks & Regards,
> Xuelei
>
>
> On 8/11/2017 8:24 AM, Xuelei Fan wrote:
>
>> 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-80
>>> 46295/webrev.03/
>>> * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-80
>>> 46295/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_truste
>>> d_ca/2017_07_18/8046295.webrev.03/
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_ca/2017_07_18/8046295.webrev.03/>
>>> (browse online)
>>> *
>>> http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_truste
>>> d_ca/2017_07_18/8046295.webrev.03.zip
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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_truste
>>> d_ca/2017_06_15/8046295.webrev.02/
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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_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/>
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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>
>>> <http://people.redhat.com/mbalaoal/webrevs/jdk_8046295_trust
>>> ed_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>>
>>> <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.-
>>>
>>>
>>>
>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180629/13c5b023/attachment.htm>
More information about the security-dev
mailing list