JDK 8 Review Request for 6854712 (JEP 124), 6637288 and 7126011

[Sean Mullan]

On 5/24/12 9:05 PM, Xuelei Fan wrote:
> On 5/24/2012 3:26 AM, Sean Mullan wrote:
>>> I will try to look into other update tomorrow.
> A. Can I extend a PKIXRevocationChecker?
>    Both the cert path builder and validator depends on CertPathParameters.
>    CertPathBuilder.build(CertPathParameters params)
>    CertPathValidator.validate(CertPath, CertPathParameters params)
> 
>    However, the checker is independent from CertPathParameters. The
> check method only has one parameter, the certificate to be checked.
>    CertPathChecker.check(Certificate)
> 
>    When I want to implement a customized checker by extending
> PKIXRevocationChecker, the impl of check() may need to access the CRLs
> in the cert store specified by CertPathParameters.  I was wondering we
> may need to add a new method to have the checker know the
> CertPathParameters.
>    CertPathChecker.check(Certificate, CertPathParameters)

Right. I see your point. A custom RevocationChecker should ideally have access
to the CertStores in the CertPathParameters that may contain CRLs.

As a workaround, if you are creating a custom checker that requires additional
parameters, these can be specified as parameters of the constructor, ex:

MyPKIXRevocationChecker extends PKIXRevocationChecker {
    MyPKIXRevocationChecker(CertStore crlStore) ...
}

I actually think that is an acceptable workaround. But I will think about this
some more. Instead of your suggestion, I would consider adding an overloaded
init method that takes a CertPathParameters object. So, something like:

void init(boolean forward, CertPathParameters params) ...

> B. Can I extend a PKIXRevocationChecker (another aspect)?
> 
>    In the implementation of PKIXCertPathValidator, when the revocation
> checker in CertPathParameters is instance of *PKIXRevocationChecker*,
> the checker will be cast to RevocationChecker, and the default
> revocation checking mechanisms will not be used any more.
> 
>    if (checker instanceof PKIXRevocationChecker) {
>        // initialize it
>       ((RevocationChecker)checker).init(anchor, params);
>       revCheckerAdded = true;
>    }
> 
>    In the implementation of SunCertPathBuilder, when the revocation
> checker in CertPathParameters is instance of *RevocationChecker*, the
> checker will be used to replace of the default revocation checking
> mechanisms.
> 
>    if (ckr instanceof RevocationChecker) {
>        // initialize it
>        ((RevocationChecker)ckr).init(builder.trustAnchor, buildParams);
>        revCheckerAdded = true;
>    }
> 
>    I don't see the reason why one checks the instance of
> PKIXRevocationChecker, while another one of RevocationChecker. Maybe a typo.
> 
>    I think the user may extend PKIXRevocationChecker as:
>    public class MyPKIXRevocationChecker extends PKIXRevocationChecker {
>        ...
>    }
> 
>    I think there may be two problems here. The 1st one is that the
> object of the extended class cannot be cast to RevocationChecker. The
> 2nd problem is the question whether the extended checker should override
> the default revocation checking mechanisms, or not.

It should. This is related to your prior email where I said that if you add a
PKIXRevocationChecker it should override the default checker and be used,
regardless of the PKIXParameters.isRevocationEnabled value.

The code has a bug and is a good catch on your part. The code should be (for
both Builder and Validator):

            if (checker instanceof PKIXRevocationChecker) {
                revCheckerAdded = true;
                // if it's our own, initialize it
                if (checker instanceof RevocationChecker)
                    ((RevocationChecker)checker).init(anchor, params);
            }

> C. PKIXRevocationChecker.java
>    //@@@ FIXME need to deep-copy the extensions
>    I think you may want to fix it before integration the code.

Yes, will fix.

> That's my comment on specification.  I may look into the implementation
> update next Monday.

Thanks, great comments.

--Sean