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

[Xuelei Fan]

updated webrev: http://cr.openjdk.java.net/~xuelei/7113275/webrev.01/

On 11/22/2011 5:59 AM, Sean Mullan wrote:
> On 11/20/2011 10:06 PM, Xuelei Fan wrote:
>> webrev: http://cr.openjdk.java.net/~xuelei/7113275/webrev.00/
>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7113275
>>
>> Test MD2InTrustAnchor.java is used to test that MD2 in trust anchor is
>> able to work with the default trust manager (X509ExtendedTrustManager).
>>
>> Test TrustTrustedCert.java is used to test that MD2 in trust anchor is
>> able to work with the un-extended trust manager (X509TrustManager).
>>
>> Some customized trust manages developed in JDK 6 did not know the
>> features in JDK 7, and may not check algorithm constraints. I think we
>> need the addition algorithm constraint check for un-extended trust
>> manager in order to ensure that the TM comply to security constraints
>> defined by security property, jdk.certpath.disabledAlgorithms.
>>
>> The algorithm check of certification chain is light weight, so even the
>> customized trust manager has already managed to check the algorithm
>> constraints during certification path validation, the performance hurt
>> is very limited.
> 
> 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.

> 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.

> A nit on line 922, change to:
> 
> checkedLength--;
> 
It's nice.

> 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.

> 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.

Thanks,
Xuelei

> This would be the most optimal
> solution. That might be a bit challenging given the API, but something
> to keep in mind if you enhance the API later on.
> 
> --Sean