[Pki-devel] [PATCH] 0016-2 - Fix BasicConstraints min/max path length check

[Endi Sukma Dewata]

On 11/24/2014 12:03 AM, Fraser Tweedale wrote:
> New patch attached.
>
> Re point 1., despite the improved ``EPropertyException`` thrown in
> the new code, the error reported by the CLI is ``PKIException: Error
> changing profile data``.  I filed
> https://fedorahosted.org/pki/ticket/1212 to address this later.
>
> 3. is not an issue and 2. and 4. have been addressed in the new
> patch.

I'm not sure if we should do this:

   String origValue = getConfig(name);
   cs.putString(name, value);
   try {
       validateConfig();
   } catch (EPropertyException e) {
       cs.putString(name, origValue);
       throw e;
   }

In case validateConfig() throws a different exception, the origValue may 
not be restored, so the new (possibly invalid) value will remain in the 
config store.

Another thing, in the following code suppose the CONFIG_MIN_PATH_LEN 
contains an invalid value, the CONFIG_MAX_PATH_LEN will not be read:

   int minLen = -1;
   int maxLen = -1;
   try {
       minLen = getConfigInt(CONFIG_MIN_PATH_LEN);
       maxLen = getConfigInt(CONFIG_MAX_PATH_LEN);
   } catch (NumberFormatException e) {
       // one of the path len configs is empty - treat as unset
   }

It may not make a difference now since the validation is only done if 
both variables are not negative, but in case the validation code is 
expanded to check for other things, the maxLen might not have the right 
value. Also, I think the original code would throw an error if the value 
is not a valid integer while here it's swallowed.

I think the code should either validate each parameter as it gets added, 
or validate all parameters after all parameters get added. So in this 
case the setConfig() might look like this:

   validateParam(name, value);
   cs.putString(name, value);

and the validateParam() might look like this:

   // if the new param is minLen
   if (name == CONFIG_MIN_PATH_LEN) {

       // if minLen is unset, return
       if (value is empty) return;

       // validate minLen is an integer
       int minLen = getInt(value);

       // if minLen is negative, return
       if (minLen < 0) return;

       // if maxLen is unset, return
       String maxLenString = getConfig(CONFIG_MAX_PATH_LEN);
       if (maxLenString is empty) return;

       // if maxLen is negative, return
       int maxLen = getInt(maxLenString);
       if (maxLen < 0) return;

       // validate minLen is less than maxLen
       if (minLen >= maxLen)
           throw exception
   }

Same thing for maxLen. So the code is a bit longer, but I'm not sure it 
can be simplified without compromising the validation.

By the way, this is a separate issue and feel free to open a ticket. I 
think the validation should check for minLen > maxLen instead of >=. 
This way the minLen can be equal to maxLen, meaning the parameter has to 
be a specific length.

-- 
Endi S. Dewata