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

[Fraser Tweedale]

On Mon, Nov 24, 2014 at 01:08:26PM -0600, Endi Sukma Dewata wrote:
> 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.
> 
Good point - I will catch all exceptions.

> 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.
> 
Ah, the perils of objects whose initialisation/validity check is not
contained in the constructor - or even better, the types!

I think that validating each parameter when it is set is too much
code, and logic will be duplicated.  A single validity check guarded
by the condition that all parameters have been set is the approach I
will take (the second approach you mention above).

> 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.
> 
Yes, I noticed this but have preserved the original semantics.  I
agree; will file a new ticket.

Cheers,

Fraser

> -- 
> Endi S. Dewata