Server side key gen password complexity #4950

fmarco76 · 2025-02-04T18:24:02Z

I have implemented a new constraint to enforce the password for p12 export profile.

Added a description of the new options into the file docs/changes/v11.6.0/Server-Changes.adoc.

To enable this policy constraint in a profile with input serverKeygenInputImpl and output pkcs12OutputImpl, the following policy should be added to the set (the number could be different):

policyset.userCertSet.11.constraint.class_id=p12ExportPasswordConstraintImpl
policyset.userCertSet.11.constraint.name=PKCS12 Password Constraint
policyset.userCertSet.11.constraint.params.password.minSize=12
policyset.userCertSet.11.constraint.params.password.minCharCategory=2,2,2,2
policyset.userCertSet.11.constraint.params.password.substringMatch=4
policyset.userCertSet.11.constraint.params.password.maxRepeatedChar=4
policyset.userCertSet.11.constraint.params.password.cracklibCheck=true
policyset.userCertSet.11.default.class_id=noDefaultImpl
policyset.userCertSet.11.default.name=No Default

@ladycfu and @edewata I have not modified the current policies but we could add at least the minimum size for the password. Is this OK?

This is for BZ #2196889

Key derivation from pkcs12 password was implemented during the default population operation and then the password was deleted. To implement a new constraint for the password this operation has to move to a later step so no computation is wasted in case the password do not satisfy its constraints.

edewata

I have some comments and questions below but there's no major issue. I'll defer the final review to @ladycfu.

Do we need to add these params into the profiles that we ship?
Do we need an upgrade script to update the registry.cfg in existing instances?
Is there a doc/wiki page that shows how to modify the existing registry/profile files in existing instances?

It might be nice if we have a unit test or CI test for this, but that's optional.

edewata · 2025-02-04T19:08:08Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

+            String transportNickname = kraConnectorConfig.getString("transportCertNickname", "KRA Transport Certificate");
+            transCert = cm.findCertByNickname(transportNickname);
+        } catch (Exception e) {
+            logger.debug(method + "'KRA transport certificate' not found in nssdb; need to be manually setup for Server-Side keygen enrollment");


This is a misconfiguration so I think we should log the error message using logger.error().

edewata · 2025-02-04T19:13:14Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

+            /* future; cert nickname can't be controlled yet at import in jss
+            logger.debug(method + "KRA transport certificate not found in nssdb; getting from CS.cfg");
+            transportCertStr = connectorsConfig.getString("KRA.transportCert", "");
+            logger.debug(method + "transportCert found in CS.cfg: " + transportCertStr);
+
+            byte[] transportCertB = Utils.base64decode(transportCertStr);
+            logger.debug(method + "transportCertB.length=" + transportCertB.length);
+            // hmmm, can't yet control the nickname
+            transCert = cm.importCACertPackage(transportCertB);
+            logger.debug(method + "KRA transport certificate imported");
+            */


I don't think we should support storing the transport cert in CS.cfg anymore. The transport cert should already be stored in NSS database when the server is started and we should not assume we can modify the NSS database when the server is running.

@edewata The majority of the code in convertP12Password was actually moved from ServerKeygenUserKeyDefault.java (see lines 286-386 in this PR below) so that the code is not run before the p12 password gets checked by the constraint. I'm therefore to be blamed for the "future" comment (wink). I agree that we shouldn't assume that we can modify the NSS db on the fly. The comment should be removed.

edewata · 2025-02-04T19:16:48Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

+            if (ct == null)
+                logger.debug(method + "crypto token null");


We probably should throw an exception if the token is not available. Alternatively, we probably can assume that the internal token is always available so no need to check for null.

Actually, there is no action beside the log. If the token is null there will be exceptions later in the code.

I have investigated some other places where the token is retrieved and this is never verified. I am removing this line. If a log or exception is needed then it can be in the getCryptoToken(<name>) which log or raise an exception if it cannot be retrieved.

edewata · 2025-02-04T19:17:14Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

+            boolean useOAEP = caCfg.getUseOAEPKeyWrap();
+
+            KeyWrapAlgorithm wrapAlgorithm = KeyWrapAlgorithm.RSA;
+            if(useOAEP == true) {


The == true is redundant.

edewata · 2025-02-04T19:19:37Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+ * parameters: graceBefore and graceAfter
+ *
+ * @author Christina Fu
+ * @version $Revision$, $Date$


We don't use $Revision$ or $Date$ so I think we can remove the @version.

feel free to remove or replace the @author too! lol. Also, replace the "This class supports renewal grace period, which has two parameters: graceBefore and graceAfter" with 5 params and their names.

edewata · 2025-02-04T19:25:57Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+                }
+
+            } catch (IOException e) {
+                logger.error(method + "impossible check password with cracklib.", e);


If cracklib check is enabled and it's failing I think we should throw an exception here.

ladycfu

some suggestions for now.

ladycfu · 2025-02-04T23:33:31Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

@@ -531,4 +532,113 @@ public void execute(Request request) throws EProfileException, ERejectException
        }
    }



I like how you group the code into a method to handle processing of the p12 password and return in a Map collection. The method "convertP12Password" could use a comment briefly describing what it does. Also, I think instead of "convertP12Password", it might be more appropriate to call it "processP12Password" since you are not changing or generating the password. It's just been processed.

Also, it appears that the code in this method was moved from def/ServerKeygenUserKeyDefault.java. I assume this is to avoid the P12 password being "processed" unnecessarily if it were to fail the password checks in the constraint.

I like how you group the code into a method to handle processing of the p12 password and return in a Map collection. The method "convertP12Password" could use a comment briefly describing what it does. Also, I think instead of "convertP12Password", it might be more appropriate to call it "processP12Password" since you are not changing or generating the password. It's just been processed.

I was using convert because in my mind it convert from a String to a corresponding key but I agree it is not a real conversion to I have update the names.

ladycfu · 2025-02-05T00:12:56Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

+            /* future; cert nickname can't be controlled yet at import in jss
+            logger.debug(method + "KRA transport certificate not found in nssdb; getting from CS.cfg");
+            transportCertStr = connectorsConfig.getString("KRA.transportCert", "");
+            logger.debug(method + "transportCert found in CS.cfg: " + transportCertStr);
+
+            byte[] transportCertB = Utils.base64decode(transportCertStr);
+            logger.debug(method + "transportCertB.length=" + transportCertB.length);
+            // hmmm, can't yet control the nickname
+            transCert = cm.importCACertPackage(transportCertB);
+            logger.debug(method + "KRA transport certificate imported");
+            */


@edewata The majority of the code in convertP12Password was actually moved from ServerKeygenUserKeyDefault.java (see lines 286-386 in this PR below) so that the code is not run before the p12 password gets checked by the constraint. I'm therefore to be blamed for the "future" comment (wink). I agree that we shouldn't assume that we can modify the NSS db on the fly. The comment should be removed.

ladycfu · 2025-02-05T00:15:39Z

base/ca/src/main/java/com/netscape/cms/profile/common/CAEnrollProfile.java

-             * They will be put back at SSK_STAGE_KEY_RETRIEVE below
-             */
-            transWrappedSessionKey = request.getExtDataInByteArray("serverSideKeygenP12PasswdTransSession");
+            Map<String, byte[]> passwordsGenerated = convertP12Password(request);


see my comment below at the method "convertP12Password". For the same reason (password was not changed or generated), how about changing the name "passwordsGenerated" to something like "p12PasswordInfo"?

ladycfu · 2025-02-05T00:34:56Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+// with this program; if not, write to the Free Software Foundation, Inc.,
+// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+//
+// (C) 2007 Red Hat, Inc.


Do we want to change the copy right to a more recent year? ;-)

ladycfu · 2025-02-05T00:37:50Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+ * parameters: graceBefore and graceAfter
+ *
+ * @author Christina Fu
+ * @version $Revision$, $Date$


feel free to remove or replace the @author too! lol. Also, replace the "This class supports renewal grace period, which has two parameters: graceBefore and graceAfter" with 5 params and their names.

ladycfu · 2025-02-05T00:52:50Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+    public static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(P12ExportPasswordConstraint.class);
+
+    public static final String CONFIG_PASSWORD_MIN_SIZE = "password.minSize";
+    public static final String CONFIG_PASSWORD_MIN_CHAR_CATEGORY = "password.minCharCategory";


Would it be simpler (to the users to understand, and to the coding) to have individual config parameter for each category? e.g. CONFIG_PASSWORD_MIN_UPPER_LETTER, CONFIG_PASSWORD_MIN_LOWER_LETTER, CONFIG_PASSWORD_MIN_NUMBER, CONFIG_PASSWORD_MIN_SPECIAL_CHAR, and CONFIG_PASSWORD_MIN_TOTAL_CHAR ?

ladycfu · 2025-02-05T00:57:08Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+                catChars[0] = passwordSubset.split("(?<=\\p{Upper})", -6).length -1;
+                catChars[1] = passwordSubset.split("(?<=\\p{Lower})", -6).length -1;
+                catChars[2] = passwordSubset.split("(?<=\\d)", -6).length -1;
+                catChars[3] = passwordSubset.split("(?<=\\p{Punct})", -6).length -1;


If you were to have separate config param for each category, you could have easier (to read) and less error-prone coding here, Something like:
Pattern upper = Pattern.compile("[A-Z]");
Pattern lower = Pattern.compile("[a-z]");
Pattern digit = Pattern.compile("[0-9]");
Pattern special = Pattern.compile("[^a-zA-Z0-9]");
then
if (countIt(password, upper) < minUpper ||
countIt(password, lower) < minLower ||
countIt(password, digit) < minDigit ||
countIt(password, special) < minSpecial) {
return false; (or throw exception)
}
private static int countIt(String str, Pattern pattern) {
Matcher matched = pattern.matcher(str);
int c = 0;
while (matched.find()) {
c++;
}
return c;
}

ladycfu · 2025-02-05T01:22:59Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+ *
+ * @author Christina Fu
+ * @version $Revision$, $Date$
+ */


Here is a thought. Could we maybe use a password checker somewhere else later? Like maybe KRA key recovery? If so, maybe we could extract the relevant code into a generic PasswordChecker class and apply it in this constraint instead?

ladycfu · 2025-02-05T01:36:23Z

2196889

@fmarco76 Actually, I think it's best to have defaults set in the constraint class itself as well as the profile. Also, see my comment regarding having separate config for each of the 4 elements in minCharCategory. Shouldn't the minSize be 20?

ladycfu

Thanks for the update. Most comments for your consideration.

ladycfu · 2025-02-05T22:31:21Z

base/server/src/main/resources/UserMessages.properties

 CMS_PASSWORD_MISSING_LOWER_CASE_1=The password requires at least {0} lower case letter(s)
+CMS_PASSWORD_SEQUENCE=PKCS12 password cannot have repeated sequences (check also inverted).
+CMS_PASSWORD_REPEATED_CHAR=PKCS12 password cannot contain the same character more then {0} times.
+CMS_PASSWORD_CRACKLIB_FAILS=PKCS12 password did not pass cracklib test.


Since the PasswordChecker is now a generic password quality checker, perhaps drop the word "PKCS12" from the above 3 CMS_PASSWORD_ messages?

ladycfu · 2025-02-05T23:23:16Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+            StringBuilder base = new StringBuilder(password.replaceAll("^\\p{Upper}*", ""));
+            base.reverse();
+            basePassword = base.toString().replaceAll("^\\d*", "");
+        }


I might have missed something, but it looks like it's skipping over the leading upper letters and ending digits? Don't they need to be checked too?

Investigating other password checker there are several consideration on the initial and final letters. As an example, from passwdc readme (it is an alternative to cracklib in many distros):

When calculating the number of character classes, upper-case letters used as the first character and digits used as the last character of a password are not counted.

I guess it is to avoid putting a password without digit and add a counter at the end at each password update.
However, to start simple we could remove this for now and count all the characters, is it OK?

Yeah, I think it's safer that way. Simpler too.

ladycfu · 2025-02-05T23:54:05Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+    private int minPunctuationChar = 0;
+    private int substringMatch = 0;
+    private int maxRepeatedChar = 0;
+    private boolean cracklibCheck = false;
    /**


Something to consider (maybe later?). Since there are existing calls to the previous version of PasswordChecker, we could leave these default values here. However, I'm thinking maybe it's a good idea to allow overwriting these values in maybe CS.cfg instead of individual profiles so that the password quality restriction is uniformly configured, even at places where profiles are not involved. Maybe just add a comment for now if to be punted.

on second thought, if we are more leaning towards setting these in CS.cfg, it might be a good idea to do it now while the class being being introduced so as to avoid backward compatibility and profile regression support etc.

@fmarco76 @edewata I asked support the question (on Slack) regarding whether to put the controls in CS.cfg or not. Please see their response on slack. I'd like to hear your opinions. Thanks!

I have added now in both. The configuration in CS.cfg is the default. The profile can overwrite the value.

ladycfu · 2025-02-05T23:56:25Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+                    return PasswordQuality.CMS_PASSWORD_SEQUENCE;
+                }
+            }
+        }


I'm still thinking about defaults. For a password length of 20, it might be a good idea to set substringMatch to 5 or 6? Anyway, we can think about it later.

Also, it appears that substringMatch is to detect repeated sequences of a length, would something like "seqLength" be a more telling name?

ladycfu · 2025-02-06T00:15:27Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+            int mostRepeatedChar = password.chars().mapToObj(x -> (char) x)
+                    .collect(Collectors.groupingBy(x -> x, Collectors.counting()))
+                    .entrySet().stream().max(Map.Entry.comparingByValue())
+                    .get().getValue().intValue();


If I'm not mistaken, if this is the attempt to catch " repeated characters", I suspect that's misunderstood. I think "repeated characters" means something like "aaa", "bb" etc. instead of, say, "takuaba", where 'a' appears 3 times but it's not a meaningful word and doesn't have consecutive repeated characters so should be fine.

In fact, I think the previous check (substringMatch) should catch repeated characters already, right? If so, maybe remove this check. I think we can call this "Phase 1" and add improvements later.

I do not think the substringMatch capture repeated characters. If a password contains aaaaabbbbb and we check with substring of 6 then it is OK but we could have max repeated of 3 which should fails. I'll update the check.

yeah, maybe you can change the logic so that the value assigned to substringMatch means that it will check with substringMatch, substringMatch-1, and substringMatch-2, etc.

I do not think we have to check with 1 character but we could go to min(3, maxRepeated).

ladycfu · 2025-02-06T00:30:58Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+                crackIn.write(password);
+                crackIn.close();
+
+                String crackResult = crackOut.readLine().substring(password.length() + 2);


does cracklib check for substrings of the password? If not, perhaps you could do some minimal processing first to extract consecutive string of characters from password to pass into cracklib.
We can do more complex checks in phase 2.

It does some substring check but not sure how much it is effective but get consecutive characters does not work then I replace a character with a symbol. The word matches to dictionary if taken entirely. I think this extraction should go to phase 2.

ladycfu · 2025-02-06T00:45:47Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

-        return pwd != null && pwd.length() >= MIN_LEN;
+        try {
+            return checkPassword(pwd) == PasswordQuality.CMS_PASSWORD_GOOD;
+        } catch (EPasswordCheckException e) {


We only have a few places that call isGoodPassword now. Could we maybe just throw an exception with the reason instead, and get rid of getReason()? See comment for getReason() below.

ladycfu · 2025-02-06T00:48:15Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

-            EPasswordCheckException e = new EPasswordCheckException(
-                    CMS.getUserMessage("CMS_PASSWORD_INVALID_LEN", "" + MIN_LEN));
+        try {
+            PasswordQuality pq = checkPassword(pwd);


This is making the existing calls to isGoodPassword() and then getReason() call checkPassword() twice. See comment at isGoodPassword. If we change it so that isGoodPassword throws password quality failure reason in an exception it would be more efficient. It would require some minor code changes to the callers, but I think it's only at a few places.

Yes, it is not optimal. My idea was not to modify the code currently using the checker and update/improve in a second step but we can go fix all in one.

In my idea was not to return the reason and the output but caching latest check so the reason can be retrieved without a check. If reason is not needed the boolean for the check is better. I'll update the code.

@ladycfu I have updated the code. I still have to fully test, add update version script and eventually CI. But you can get a look if it is in the right direction

The new *p12ExportPasswordConstraintImpl* constraint allows to check: * `password.minSize` - the minimum size for the password; * `password.minUpperLetter` - the minimum number of capital letters; * `password.minLowerLetter` - the minimum number of lower letters; * `password.minNumber` - the minimum number of digits; * `password.minSpecialChar` - the minimum number of punctuation characters; * `password.seqLength` - the size of substring sequence which cannot be repeated; * `password.maxRepeatedChar` - maximum number of repeating for each character; * `password.cracklibCheck` - a boolean to request an additional check with *cracklib* (it has to be installed if not present). The same option can be configured in the CS.cfg replacing `password.*` with `passwordChecker.*`. The configuration in CS.cfg is used for all the passwords but the profile can overwrite to have stronger or weaker configuration.

sonarqubecloud · 2025-02-06T18:38:16Z

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.9% Duplication on New Code

See analysis details on SonarQube Cloud

ladycfu

I think this is pretty much it. I only have a few very minor comments now. The rest is up to your testing. Thanks!

ladycfu · 2025-02-06T22:15:50Z

base/ca/src/main/java/com/netscape/cms/profile/constraint/P12ExportPasswordConstraint.java

+ *   - password.minLowerLetter
+ *   - password.minNumber
+ *   - password.minSpecialChar
+ *   - password.substringMatch


@fmarco76 Could we unified the names between the two classes (I put the ones I prefer in bold)? so
substringMatch becomes seqLength
and
minPunctuationChar becomes minSpecialChar
This way you can document things in one place and not to confuse people.

ladycfu · 2025-02-06T22:46:53Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+                    last = c;
+                }
+            }
+        }


That's much better.

ladycfu · 2025-02-06T22:52:53Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

+                CMS.getUserMessage(loc,"CMS_PASSWORD_MISSING_NUMERIC", "" + minNumber)).toString();
+        case CMS_PASSWORD_MISSING_PUNCTUATION -> new EPasswordCheckException(
+                CMS.getUserMessage(loc,"CMS_PASSWORD_MISSING_PUNCTUATION", "" + minPunctuationChar)).toString();
+        case CMS_PASSWORD_REPEATED_CHAR -> new EPasswordCheckException(


I think you are missing CMS_PASSWORD_SEQUENCE.

ladycfu · 2025-02-06T22:56:31Z

base/server/src/main/java/com/netscape/cms/password/PasswordChecker.java

-        if (pwd == null || pwd.length() == 0) {
-            EPasswordCheckException e = new EPasswordCheckException(
-                    CMS.getUserMessage("CMS_PASSWORD_EMPTY_PASSWORD"));
+    public String getReason(Locale loc) {


Not sure where to put this, but shouldn't be have a user message completes with the usage that tells users the requirements of the passwords?

ladycfu · 2025-02-06T23:00:11Z

docs/changes/v11.6.0/Server-Changes.adoc

+`password.*` with `passwordChecker.*`. The configuration in `CS.cfg`
+are used for all the passwords but each profile can overwrite to allow
+stronger or weaker passwords.
+


Shouldn't be put this in the Admin's guide as well for both CS.cfg and profile configuration?

fmarco76 requested review from edewata and ladycfu February 4, 2025 18:24

edewata reviewed Feb 4, 2025

View reviewed changes

ladycfu reviewed Feb 5, 2025

View reviewed changes

fmarco76 force-pushed the ServerSideKeyGenPasswordComplexity branch from 7cec1df to ffbabfc Compare February 5, 2025 10:58

fmarco76 marked this pull request as draft February 5, 2025 10:58

fmarco76 force-pushed the ServerSideKeyGenPasswordComplexity branch 3 times, most recently from 058435f to bf717d1 Compare February 5, 2025 19:47

ladycfu reviewed Feb 6, 2025

View reviewed changes

fmarco76 force-pushed the ServerSideKeyGenPasswordComplexity branch from bf717d1 to 3e9d347 Compare February 6, 2025 12:21

fmarco76 force-pushed the ServerSideKeyGenPasswordComplexity branch from 3e9d347 to c0d104b Compare February 6, 2025 18:26

ladycfu reviewed Feb 6, 2025

View reviewed changes

		@@ -531,4 +532,113 @@ public void execute(Request request) throws EProfileException, ERejectException
		}
		}

Server side key gen password complexity #4950

Are you sure you want to change the base?

Server side key gen password complexity #4950

Conversation

fmarco76 commented Feb 4, 2025

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu commented Feb 5, 2025

ladycfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 6, 2025

Quality Gate passed

ladycfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladycfu Feb 6, 2025 •

edited

Loading