Code review request: JDK-8046295 - Support Trusted CA Indication extension
Xuelei Fan
xuelei.fan at oracle.com
Thu Jun 15 18:09:55 UTC 2017
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.-
>>
>>
More information about the security-dev
mailing list