[Pki-devel] [PATCH] 0016-3 - Fix BasicConstraints min/max path length check
Fraser Tweedale
ftweedal at redhat.com
Wed Nov 26 06:02:23 UTC 2014
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
Cheers,
Fraser
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.
>
> 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
-------------- next part --------------
>From e0a374e49957bc97a2865153aa7beb63804ed88f Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftweedal at redhat.com>
Date: Thu, 25 Sep 2014 01:39:40 -0400
Subject: [PATCH] Fix BasicConstraints min/max path length check
The BasicConstraintsExtConstraint min/max path length validity check
ensures that the max length is greater than the min length, however,
when a negative value is used to represent "no max", the check
fails.
Only compare the min and max length if all config parameters are set
and both the min and max length can be parsed and both are
non-negative integers. Also run the check when either value is
modified, since setting the min path length could invalidate the
configuration.
Ticket #1035
---
.../constraint/BasicConstraintsExtConstraint.java | 54 ++++++++++++++--------
.../cms/profile/constraint/EnrollConstraint.java | 17 +++++++
2 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/base/server/cms/src/com/netscape/cms/profile/constraint/BasicConstraintsExtConstraint.java b/base/server/cms/src/com/netscape/cms/profile/constraint/BasicConstraintsExtConstraint.java
index ca2668f7db305122f330fca058b27801820a75b4..4449a8ddf5e71379f24f8cef3e7f324a6c796ffb 100644
--- a/base/server/cms/src/com/netscape/cms/profile/constraint/BasicConstraintsExtConstraint.java
+++ b/base/server/cms/src/com/netscape/cms/profile/constraint/BasicConstraintsExtConstraint.java
@@ -195,30 +195,46 @@ public class BasicConstraintsExtConstraint extends EnrollConstraint {
public void setConfig(String name, String value)
throws EPropertyException {
+ IConfigStore cs = mConfig.getSubStore("params");
- if (mConfig.getSubStore("params") == null) {
+ if (cs == null) {
CMS.debug("BasicConstraintsExt: mConfig.getSubStore is null");
- //
} else {
-
CMS.debug("BasicConstraintsExt: setConfig name " + name + " value " + value);
-
- if (name.equals(CONFIG_MAX_PATH_LEN)) {
-
- String minPathLen = getConfig(CONFIG_MIN_PATH_LEN);
-
- int minLen = getInt(minPathLen);
-
- int maxLen = getInt(value);
-
- if (minLen >= maxLen) {
- CMS.debug("BasicConstraintExt: minPathLen >= maxPathLen!");
-
- throw new EPropertyException("bad value");
- }
-
+ String origValue = getConfig(name);
+ cs.putString(name, value);
+ try {
+ validateConfig();
+ } catch (Exception e) {
+ cs.putString(name, origValue);
+ throw e;
}
- mConfig.getSubStore("params").putString(name, value);
+ }
+ }
+
+ private void validateConfig() throws EPropertyException {
+ if (!allConfigsSet())
+ return;
+
+ int minLen = -1;
+ int maxLen = -1;
+ try {
+ minLen = getConfigInt(CONFIG_MIN_PATH_LEN);
+ } catch (NumberFormatException e) {
+ CMS.debug("BasicConstraintExt: minPathLen not a number");
+ throw new EPropertyException("Min path length is not an integral number.");
+ }
+ try {
+ maxLen = getConfigInt(CONFIG_MAX_PATH_LEN);
+ } catch (NumberFormatException e) {
+ CMS.debug("BasicConstraintExt: maxPathLen not a number");
+ throw new EPropertyException("Max path length is not an integral number.");
+ }
+ if (maxLen >= 0 && maxLen >= 0 && minLen >= maxLen) {
+ CMS.debug("BasicConstraintExt: minPathLen >= maxPathLen!");
+ throw new EPropertyException(
+ "Max path length (" + maxLen +
+ ") must be greater than min path length (" + minLen + ").");
}
}
}
diff --git a/base/server/cms/src/com/netscape/cms/profile/constraint/EnrollConstraint.java b/base/server/cms/src/com/netscape/cms/profile/constraint/EnrollConstraint.java
index eb3eb14f67a6dff5bcd8b048eba316daf6223cb4..a29b9409a27f00662cc8a86c7b38f6c90fd441df 100644
--- a/base/server/cms/src/com/netscape/cms/profile/constraint/EnrollConstraint.java
+++ b/base/server/cms/src/com/netscape/cms/profile/constraint/EnrollConstraint.java
@@ -102,6 +102,23 @@ public abstract class EnrollConstraint implements IPolicyConstraint {
return "";
}
+ protected boolean allConfigsSet() {
+ if (mConfig == null)
+ return false;
+ IConfigStore cs = mConfig.getSubStore("params");
+ if (cs == null)
+ return false;
+
+ for (String name : mConfigNames) {
+ try {
+ cs.getString(name);
+ } catch (EBaseException e) {
+ return false;
+ }
+ }
+ return true;
+ }
+
public void init(IProfile profile, IConfigStore config)
throws EProfileException {
mConfig = config;
--
1.9.3
More information about the Pki-devel
mailing list