Code review request: 7113275, compatibility issue with MD2 trust anchor and old X509TrustManager

[Sean Mullan]

On 11/21/11 10:56 PM, Xuelei Fan wrote:
> updated webrev: http://cr.openjdk.java.net/~xuelei/7113275/webrev.01/

>> I believe you could add an optimization that if the TrustManager is an
>> instance of sun.security.ssl.X509TrustManagerImpl, then there is no need
>> to perform the additional certpath constraint checks because the PKIX
>> CertPathValidator will have already checked them.
>>
> The sun.security.ssl.X509TrustManagerImpl has already been an extension
> of X509ExtendedTrustManager. If the TM is Oracle JDK's implementation,
> including the implementation in deploy workspace, the implementation
> should extend X509ExtendedTrustManager. And then the calling should not
> fall into the AbstractTrustManagerWrapper.

Ok, sounds good.

>> I think the checkAlgorithmConstraints method can be marked static.
>>
> The method calls none-static method getAcceptedIssuers(), or in the new
> webrev, it need to call the none-static class attribute. So I think it
> cannot be static.

Ok.

>> Cycling through every root certificate each time to find a match in the
>> chain could be costly. Another potential optimization is to copy the
>> root certs into a Map, where the key is simply the Certificate. The
>> value can be null. Then you can just check if the last certificate is in
>> the Map.
>>
> Good point. Instead of a Map, I use a HashSet in the new webrev. I think
> it should be the same on performance as your suggestion.

Right.

>> But even better would be to add a method to TrustManager (or something
>> like that) that returned the trusted Certificate that was used to anchor
>> the chain that was just validated.
> The current TrustManager are defined to be none-state. As means that it
> can work in multiple threads safely without synchronization. Above
> suggestion may not comply to the requirement. We will face new
> bottleneck if we need to support synchronized calling.
> 
> However, it sounds like good if we update the existing method to return
> the trusted certificate:
> -    public void checkClientTrusted()
> +    public X509Certificate checkClientTrusted()
> 
> It may be challenging to update the return type of existing APIs.
> Anyway, I will think of the possible enhancement later on.

I think a better approach would be to add check{Client,Server}Trusted methods
that return a result. For example, something like (I believe that changing the
X509Certificate[] parameter allows us to overload the method with a different
return type):

public TrustManagerResult checkClientTrusted(CertPath chain, String authType,
Socket socket) ...

public interface TrustManagerResult {
    public TrustAnchor getTrustAnchor();
}

public interface CertPathTrustManagerResult extends TrustManagerResult {
    public CertPathValidatorResult getCertPathValidatorResult();
}

--Sean