Re: [Pki-devel] [PATCH] 0016-3 - Fix BasicConstraints min/max path length check

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