RFR: 8154015 Apply algorithm constraints to timestamped code

[Anthony Scarpino]

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