RFR 8165274: SHA1 certpath constraint check fails with OCSP certificate

[Anthony Scarpino]

On 10/12/2016 12:15 PM, Sean Mullan wrote:
> On 10/12/2016 01:47 PM, Anthony Scarpino wrote:
>> New webrev: http://cr.openjdk.java.net/~ascarpino/8165274/webrev.02/
>
> * DisabledAlgorithmConstraints
>
>   700                         "Algorithm constraints check failed on
> keysize limits."
>   701                                 + algorithm + " " + size + "bit
> used with " +
>
> Looks like you could insert a couple of spaces above, one after
> "limits." and one before "bit ...". Also do you want to say " bit key ...".
>
> * OCSP
>
> 129             responderURI, new OCSPResponse.IssuerInfo(null,
> issuerCert), null,

Ack.. I thought I only had one of these so that's why I discounted it.. 
  I need to add what I did below with a TrustAnchor object using issuerCert.

>
> This will still throw NPE.
>
>   169                 new TrustAnchor(issuerCert.getSubjectX500Principal(),
>   170                         issuerCert.getPublicKey(), null),
>
> Why are you using the issuerCert's key and dn as the trust anchor here?
> We don't actually know if that is the trust anchor, so I would just use
> null.

Later in the verify(), AlgorithmChecker needs a TrustAnchor object.  In 
this case, because it's the old method that deploy is using, I have to 
manufacture a TrustAnchor until they can use the new method with the 
real TrustAnchor.  Either way, if I pass null for the trust anchor, 
IssuerInfo will need to create a TrustAnchor from the same data.  Do you 
want me to add a comment what the TrustAnchor object is?

Unless there is further discussion about TrustAnchor part, I can easily 
fix the above two comments without a new webrev.

Tony



> --Sean
>
>>
>> On 10/12/2016 07:55 AM, Sean Mullan wrote:
>>> * AlgorithmChecker
>>>
>>> Not sure why these changes are necessary or why the check method has
>>> been made non-static. Isn't the previous code sufficient?
>>>
>>
>> Yeah, that change doesn't appear to be necessary anymore..
>>
>>> * OCSP
>>>
>>> 129             responderURI, new OCSPResponse.IssuerInfo(null,
>>> issuerCert), null,
>>>
>>> Passing null to OCSPResponse.IssuerInfo will throw an NPE. (but see
>>> comment below)
>>>
>>
>> You must have loaded the page just before I refreshed the webrev.  I
>> fixed.
>>
>> I also added some changes in the exception messages to
>> DisabledAlgorithmConstraints to give the cert subject, algorithm and/or
>> keysize if used..
>>
>>
>>> * OCSPResponse
>>>
>>> For IssuerInfo, you don't always have/know the TrustAnchor, so shouldn't
>>> it be optional?
>>
>> RevocationChecker always has a TrustAnchor as PKIXCertPathValidator
>> passes it. AlgorithmChecker always needs a TrustAnchor, which
>> PKIXCertPathValidator call.  So I don't see a situation where we don't
>> always have an TrustAnchor.
>>
>>>
>>> 1061                 return anchor;
>>>
>>> should be indented 4 spaces
>>>
>>> --Sean
>>>
>>> On 10/10/2016 02:53 PM, Anthony Scarpino wrote:
>>>> Hi,
>>>>
>>>> I need a review of a fix to JEP 288 were certpath algorithm checking
>>>> wasn't checking OCSP certs against the jdkCA keyword.
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8165274/webrev/
>>>>
>>>> thanks
>>>>
>>>> Tony
>>