Code review request: JDK-8046295 - Support Trusted CA Indication extension
Martin Balao
mbalao at redhat.com
Tue Jul 18 17:12:49 UTC 2017
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/
>> 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/>
>> (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/20170718/d87517c8/attachment.htm>
More information about the security-dev
mailing list