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