RFR 8165274: SHA1 certpath constraint check fails with OCSP certificate

[Sean Mullan]

On 10/12/2016 04:06 PM, Anthony Scarpino wrote:
> 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?

So, I think what you should do is skip the constraints check if it 
contains the jdkCA constraint and the trust anchor is null, because you 
need the trust anchor in order to do the check. I would also log a 
warning with a debug message in this case.

--Sean

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