[Pki-devel] [PATCH] 0016-2 - Fix BasicConstraints min/max path length check
Fraser Tweedale
ftweedal at redhat.com
Tue Nov 25 00:57:55 UTC 2014
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
More information about the Pki-devel
mailing list