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

[Sean Mullan]

Xuelei,

Thanks for the comments.

On 5/23/12 2:13 AM, Xuelei Fan wrote:
> C1. flexibility of the API, maybe too later
> 
> It may be too later, but I was wondering we may be able to consider the
> CertPathBuilder/CertPathValidator spec changes a little more.
> 
> There are two types of cert path checker, one is specified by
> PKIXParameters.addCertPathChecker() explicit; the other one is enabled
> by the provider implicit, as the pre-defined checkers, including the
> revocation checker, the certificate basic constraints checker, the
> algorithm constraints checker in PKIX provider, etc.
> 
> Except the revocation checker, we may want to expose other pre-defined
> checker in the future. For example, expose the algorithm constraints
> checker.
> 
> So I was wondering, maybe it is more flexible to change the new API of
> CertPathBuilder from
>     public final CertPathChecker getRevocationChecker()
> 
> to
>     public final List<CertPathChecker> getDefaultCheckers()

I did consider something like this during development:

public CertPathChecker getCertPathChecker(String type)

where "type" would indicate what type of checker you wanted, ex: "Revocation",
"AlgorithmConstraints".

In the end I decided not to do that. If we find that it is desirable in the
future to expose more CertPathCheckers, then we could easily add a method like
the above -- but I don't feel it is necessary yet.

> C2. stateless or not?
> 
> For the new API,
>     public final CertPathChecker getRevocationChecker()
> 
> It is not defined whether the update on the return value will impact the
> behaviors of CertPathBuilder/CertPathValidator.  In another words, is
> the return value a reference or a cloned object?
> 
> I think it is a cloned (or new) object but not a reference. it would be
> better to describe it in the spec.

It should always be a new object I think. Let me think about it a bit more and I
will make that clarification.

> C3. Minor comments on PKIXCertPathChecker.java
> 
> It would be nice to add @Override to init(),
> isForwardCheckingSupported() and check(Certificate, Collection<String>)
> so that the compilers will help to check the declaration.

Good point, will fix. It does not apply to check(Certificate,
Collection<String>) though, since that is not a method of CertPathChecker.

> C4. Very minor comments on package.html
> Do you want to add OCSP RFC to the "Related Documentation" section?

I don't think it's necessary since it is already referenced in the package
specification. I actually think the reference to 5280 is redundant but I'll
leave it alone.

> C5. the interactions between PKIXRevocationChecker and
> PKIXParameters.isRevocationEnabled().
> 
> We may have two types of PKIXRevocationChecker instances. One is the
> default revocation checker, which may be got from
> CertPathBuilder.getRevocationChecker() (just a maybe, may be not
> retrievable) . Another one is the non-default revocation checker, which
> is set by PKIXParameters.addCertPathChecker().
> 
> According to the spec, PKIXParameters.isRevocationEnabled() should only
> impact the default revocation checker, but not the non-default
> revocation checker.
> 
>                          PKIXParameters.isRevocationEnabled()
> -------------------------------------------------------------
>                          |       true       |     false
> -------------------------------------------------------------
> default revo checker     |      enabled     |     disabled
> -------------------------------------------------------------
> non-default revo checker |      enabled     |     enabled
> -------------------------------------------------------------
> 
> That's OK. From the implementation, it seems that the non-default
> revocation checker will override the default revocation checker. I think
> we may need a description about the behavior.  Otherwise, we may need to
> use both PKIXParameters.isRevocationEnabled() and the non-default
> revocation checker.
> 
> In the class spec of PKIXRevocationChecker, it says, "When supplying a
> revocation checker in this manner, do not enable the default revocation
> checking mechanism (by calling {@link
> PKIXParameters#setRevocationEnabled}."  

Oops, good catch. That sentence is no longer correct. If you add a
PKIXRevocationChecker to your PKIXParameters, that's what is going to be used to
check revocation, regardless of the setting of the
PKIXParameters.isRevocationEnabled flag.

> But it does not define the
> behaviors about what happen if the default revocation checking mechanism
> is enabled.  As the default revocation checking mechanism is enabled by
> default (see PKIXParameters.setRevocationEnabled(boolean)), if we do not
> want to define the above behaviors, we must call the following two
> methods in couple:
>      PKIXParameters.setRevocationEnabled(false);
>      PKIXParameters.addCertPathChecker(PKIXRevocationChecker);
> 
> Otherwise, the behaviors are not defined.

Right, I did not want the caller to have to call
PKIXParameters.setRevocationEnabled(false); - it is too confusing.

Some clarifications are in order - will work on that.

> I will try to look into other update tomorrow.

Ok, thanks.

--Sean