Code review request: JDK-8046295 - Support Trusted CA Indication extension

[Xuelei Fan]

Hi Martin,

I think more about the new TrustedAuthorityIndicator class.  Maybe, it 
can be replaced with the existing java.util.Map.Entry class (using 
java.util.AbstractMap.SimpleImmutableEntry for the implementation).

ExtendedSSLSession.java
    List<Map.Entry<Byte, byte[]>> getRequestedTrustedCAs();


Xuelei

On 6/14/2017 3:17 PM, Xuelei Fan 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 
> 
> 
> 
> 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/ 
>> (browse online)
>>   * 
>> 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>> 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>>>
>>         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.-
>>
>>