Unless otherwise specified below, it was accepted..

http://cr.openjdk.java.net/~ascarpino/8154015/webrev.02/

Tony

On 06/30/2016 12:00 PM, Sean Mullan wrote:
Just a few comments -

AlgorithmChecker.java:

79     private Date pkixdate = null;
81     private Timestamp jarTimestamp = null;

These can be marked final I think.

Bcause the below NPE issue, it make sense to make them final. I can't nest the constructors like I wanted to.

150      * {@code AlgorithmConstraints}.

s/AlgorithmConstraints/Timestamp/

153      * path for JAR files from deploy.

Avoid using "from deploy".

s/JAR files from deploy/signed JAR files that are timestamped./

159         this(certPathDefaultConstraints, jarTimestamp.getTimestamp());

This will throw NPE if jarTimestamp is null.

177     public AlgorithmChecker(AlgorithmConstraints constraints, Date
pkixdate) {

I think this should be private. You are only calling it within the
class. Also, the javadoc ctor description needs to be updated a little.
SunJSSE doesn't call this ctor AFAICT.

Actually the whole constructor isn't needed because of the previous NPE possibility


PKIX.java:

107                 this.params = ((PKIXTimestampParameters)
params).getPKIXBuilderParameters();

Shouldn't this be:

this.params = (PKIXBuilderParameters) params;

The passed in params doesn't itself contain the original PKIXBuilderParamters data. It's the wrapper with an internal PKIXBuilderParameter object it holds the data. getPKIXBuilderParameters() passes the orignal PKIXBuilderParameters object.

Without it being setup this way I don't see how I can get and set the timestamp.


201         Timestamp getTimestamp() {

Can you rename this to timestamp() to be consistent with rest of classes
methods that return params.

CertConstraintParameters.java

44     // Timestamp of the JAR file from deploy

s/JAR file from deploy/signed JAR file/

PKIXValidator.java

Since Timestamp is a new supported parameter, can you update the javadoc
of Validator.validate() to describe it?

Yes it should.  I didn't notice that


PKIXTimestampParameters.java

- missing copyright and class description

   30     public PKIXBuilderParameters getPKIXBuilderParameters() {

Do you need this method? See above comment on line 107 of PKIX.java

See response in PKIX..


   44     public void setTimestampTrustAnchors(Set<TrustAnchor> t)

Does anyone call this method? Can it be removed?

yes it can...


--Sean

On 06/28/2016 05:17 PM, Anthony Scarpino wrote:
Hi,

I need a review of the below code.  It's a continuation of the previous
certpath related changes.  Additional constraints checking on
timestamped jars being checked by the deploy code

http://cr.openjdk.java.net/~ascarpino/8154015/webrev.01/

thanks

Tony

Reply via email to