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
