Integrated: 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint

[Sean Mullan]

Michael,

Thanks for the comments - a couple of observations of mine below --

On 2/10/21 12:53 AM, Michael StJohns wrote:
> On 2/9/2021 9:02 PM, Weijun Wang wrote:
>> On Wed, 10 Feb 2021 01:39:15 GMT, Weijun Wang<weijun at openjdk.org>  wrote:
>>
>>> Print out "no limit" instead. This is the words RFC 5280 uses: "Where pathLenConstraint does not appear, no limit is imposed".
>>>
>>> No regression test. Trivial.
>> This pull request has now been integrated.
>>
>> Changeset: 4619f372
>> Author:    Weijun Wang<weijun at openjdk.org>
>> URL:https://git.openjdk.java.net/jdk/commit/4619f372
>> Stats:     11 lines in 1 file changed: 8 ins; 1 del; 2 mod
>>
>> 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
>>
>> Reviewed-by: jnimeh
>>
>> -------------
>>
>> PR:https://git.openjdk.java.net/jdk/pull/2493
> 
> 
> Sorry - not quite right, it's not quite that trivial a fix.
> 
> The definition for BasicConstraints is
> 
>> BasicConstraints ::= SEQUENCE {
>>          cA                      BOOLEAN DEFAULT FALSE,
>>          pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
> 
> If pathLenConstraint is not present, then the path length is infinite.   
> The flag value for that looks like it was encoded as both any negative 
> number (and set to -1 to start) and Integer.MAX_VALUE.  I'm not quite 
> sure why it was done that way, but there's a problem doing that - 
> actually a bunch of them.
> 
> You really ought to get the same encoding coming and going (e.g. 
> creating an object from DER should encode back to the exact same DER).  
> The current code does not do that.
> 
> 1) It's not valid to encode or decode pathLenConstraint in the DER as a 
> negative number.   This isn't a problem for encoding, but the 
> BasicConstraintsException(Boolean critical, Object value) needs a value 
> check after line 157 to make sure that the decoded pathLengthConstraint 
> field is positive - i'd throw an IOException on failure.

I agree.

>    I'd also 
> change line 149 to just return - the initial value of pathLen is -1 and 
> that's an appropriate flag for a missing field.

Some of this is due to the API for 
X509Certificate.getBasicConstraints(), and this code tries to return 
values that comply with that API. That API returns an integer which is 
specified as:

"the value of pathLenConstraint if the BasicConstraints extension is 
present in the certificate and the subject of the certificate is a CA, 
otherwise -1. If the subject of the certificate is a CA and 
pathLenConstraint does not appear, Integer.MAX_VALUE is returned to 
indicate that there is no limit to the allowed length of the 
certification path."

We need to be careful that for any changes we make, we still comply with 
the API.

> 2) I'm not sure why the set/get methods were added.  I think it was to 
> provide access for the PathValidation stuff? Given that they are 
> present, I'd insert a line after line 222 (set) : "if (pathLen < 0) 
> pathLen = -1;" // treat any negative value as unconstrained path length

In this case, I would throw an Exception. If the pathLenConstraint is 
unconstrained, it should not call this method at all.

The internal X509 classes have set/get/delete methods dating back to the 
original design.

> 3) And since the only place pathLen is available externally is in the 
> get method, I'd change line 237 to "return (pathLen < 0) ? 
> Integer.MAX_VALUE : Integer.valueOf(pathLen);"   I think this is more 
> correct than returning -1.

I think it is better to ensure that in this class, pathLen is never < 0, 
and maybe create an extra boolean flag to indicate if there is a 
pathLenConstraint field, or alternatively use an Integer object where 
null means there is no field. I think a lot of these issues are due to 
the fact that in this class -1 can mean either a bad value, or an 
unconstrained length.

> 4) And to fix the problem that led to this discussion, change line 176 
> to 'pathLenAsString = " unconstrained"' and delete lines 177-178.  E.g. 
> there's no such thing as undefined here - both a negative number and 
> MAX_VALUE mean unconstrained length in the previous version of the code.
> 
> 5) One more - in the other constructor, change line 108 to "this.pathLen 
> = (len < 0 || len == Integer.MAX_VALUE) ? -1 : len;"
> 
> 6) *sigh* Delete lines 197-201.  I have no idea why they are overriding 
> the specified value of critical based on whether ca is true or not, but 
> it's wrong.    Conversely, the call to the constructor at line 95 is 
> correct.

That does seem a little weird.

--Sean