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

[Xuelei Fan]

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()

At present we only support PKIXRevocationChecker. But once in the future
we want to expose more provider implicit checkers, we may not have to
define new CertPathBuilder APIs (such as
getAlgorithmConstraintsChecker()) any more.

Then the new example would look like:
  CertPathBuilder cpb = CertPathBuilder.getInstance("PKIX");
  List<CertPathChecker> checkers = cpb.getDefaultCheckers();
  for (CertPathChecker checker : checkers) {
      if (checker instanceof PKIXRevocationChecker) {
          PKIXRevocationChecker rc =
              (PKIXRevocationChecker)checker;
          rc.setOptions(EnumSet.of(Option.PREFER_CRLS));
          params.addCertPathChecker(rc);
      }
  }
  CertPathBuilderResult cpbr = cpb.build(params);


We may need a better name than "getDefaultCheckers".

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.

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.

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

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

I will try to look into other update tomorrow.

Xuelei

On 5/23/2012 12:09 AM, Sean Mullan wrote:
> Vinnie,
> 
> Can you please review my changes for 6854712 (JEP 124). It also includes
> fixes for a couple of other CRs: 6637288 and 7126011. Other reviewers
> are welcome too.
> 
> The webrev:
> http://cr.openjdk.java.net/~mullan/webrevs/6854712_6637288_7126011/webrev.00/
> 
> 
> The CRs:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6854712
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6637288
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7126011
> 
> There are a large number of changes, so please let me know if you need
> additional time, but I would like to push the changes by early next
> week, if possible.
> 
> Thanks,
> Sean
>