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 <mba...@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....@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....@oracle.com >>> <mailto:xuelei....@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....@oracle.com <mailto:xuelei....@oracle.com> >>> <mailto:xuelei....@oracle.com <mailto:xuelei....@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....@oracle.com <mailto:xuelei....@oracle.com> >>> <mailto:xuelei....@oracle.com <mailto:xuelei....@oracle.com>> >>> <mailto:xuelei....@oracle.com >>> <mailto:xuelei....@oracle.com> <mailto:xuelei....@oracle.com >>> <mailto:xuelei....@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.- >>> >>> >>> >>> >
