Code review request: JDK-8046295 - Support Trusted CA Indication extension
Xuelei Fan
xuelei.fan at oracle.com
Thu Jun 28 16:14:22 UTC 2018
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-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