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

[Fraser Tweedale]

On Fri, Dec 05, 2014 at 03:58:32AM +0700, Endi Sukma Dewata wrote:
> On 11/26/2014 1:02 PM, Fraser Tweedale wrote:
> >v3 patch.
> >
> >In this patch, validateConfig() is short-circuited until all params
> >have been set, to avoid repetition of logic in the validity check.
> >
> >Also, a config that causes validation to fail is now reverted on any
> >exception, not just EPropertyException.
> >
> >Finally, the profile still gets created when there is bad config so
> >I filed another ticket for that:
> >https://fedorahosted.org/pki/ticket/1215
> 
> This is getting much more complicated than I anticipated :)
> The patch still has some issues:
> 
> 1. To ensure we revert all invalid config changes, the code should catch
> Throwable instead of Exception when calling the validateConfig(), but see
> below.
> 
> 2. I'm not sure if allConfigsSet() is the right way to do this. Let's say we
> add a config with an invalid value, it may actually accept the invalid value
> because the validation only happens after all configs are set. If a
> validation error happens later, it will only revert the last config set, not
> the config with the invalid value. Let's say the profile doesn't specify all
> defined configs, the validation may never happen. It's also kind of
> inefficient because it's iterating through all defined configs on each
> config set.
> 
> I think ideally the validation should happen after the loop that calls
> setConfig(), for example:
> 
>   for (String name : names) {
>       String value = ...
>       constraint.setConfig(name, value);
>   }
>   constraint.validateConfig();
> 
> We may not even need to revert the invalid config if the code discards the
> constraint object.
> 
> If you'd like, feel free to push the original patch since it's already
> ACKed. We can improve the constraint config validation as a separate ticket.
> 
Thanks Endi, I'll push the original patch, give this thread a good
shake and file tickets for whatever falls out.

Ideally objects would be initialised (or throw if invalid) at
construction, and be immutable thereafter.  We need to be couragous
and push the code in this direction to prevent these sorts of bugs
and improve maintainability.  Changing even one interface such as
IPolicyConstraint would be a big change, but it's worth doing for
the long run.

Fraser