Added alternative KeyConstraint parameters to profile configuration f…#5338
Open
agaragna77 wants to merge 6 commits into
Open
Added alternative KeyConstraint parameters to profile configuration f…#5338agaragna77 wants to merge 6 commits into
agaragna77 wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces CSV ChangesAllowedKeys migration & validation
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "ProfileSelect (client)"
participant Store as "Config Store (profiles/.cfg)"
participant Server as "KeyConstraint (server)"
participant Upgrader as "Upgrade Script"
User->>UI: open profile select
UI->>Store: read profile keyParameters + allowedKeys.*
Store-->>UI: return base params + overrides
UI->>UI: applyAllowedKeysOverrides(), sortLengthsOrCurvesList()
UI-->>User: show filtered key size/curve options
Upgrader->>Store: (at upgrade time) migrate keyParameters -> allowedKeys
User->>Server: request key generation (alg, size/curve)
Server->>Store: read merged allowedKeys for profile
Store-->>Server: allowed set
Server->>Server: validate algorithm & param against allowed set
Server-->>User: accept or reject request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for ML-DSA in certificate profiles and the KeyConstraint class, implementing a mechanism to override allowed keys via allowedKeys.* properties. UI templates were also updated to support these changes with improved sorting and filtering logic. Review feedback highlights that several configuration files use an incorrect property format for ML-DSA allowed keys, which will cause them to be ignored. Additionally, it was suggested to implement active validation for ML-DSA parameters in the Java code to improve error handling.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
494-542:⚠️ Potential issue | 🔴 Critical
allowedKeys.*entries are never persisted and can silently wipekeyType/keyParameters.Three related defects in this rewritten
setConfig:
- When
namestarts withCONFIG_ALLOWED_KEYS_PREFIX, the method only validates (and only ifvalueequals"true") — it never callssuper.setConfig(name, value)to persist theallowedKeys.<ALG>_<VAL>leaf. So admin-UI edits to these new fields appear to succeed but are never written to the profile config.- Because neither branch at lines 519-531 matches an
allowedKeys.*name, control falls through to lines 536-537 which unconditionally writesuper.setConfig(CONFIG_KEY_TYPE, keyType)andsuper.setConfig(CONFIG_KEY_PARAMETERS, keyParams)using the static fields. After a previous(keyType, keyParameters)pair has completed, those statics were reset to""at lines 540-541, so the very nextsetConfig("allowedKeys.mldsa_44", "true")call will overwrite bothkeyTypeandkeyParametersin the profile with empty strings.value="false"is silently ignored (falls past thefilter("true"::equalsIgnoreCase)guard), so there is no way to disable an entry viasetConfig, and it still triggers the keyType/keyParameters rewrite in (2).🛠️ Suggested direction
Short-circuit the
allowedKeys.*branch so it persists viasuper.setConfig(name, value)and returns, without touching the statickeyType/keyParamsstate:- if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) - && Optional.ofNullable(value).map(String::trim).filter("true"::equalsIgnoreCase).isPresent() - && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() - ) { - String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); - String[] parts = leaf.split("_", 2); - if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { - logger.warn( - "KeyConstraint: allowedKeys property \"{}\" ignored: " - + "expected exactly two underscore-separated segments (prefix_value)", - leaf); - return; - } - String algType = parts[0].toUpperCase(); - String keyValue = parts[1]; - validateKeyParams(name, algType, keyValue); - } + if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) + && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length()) { + String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); + String[] parts = leaf.split("_", 2); + if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { + logger.warn("KeyConstraint: allowedKeys property \"{}\" ignored: " + + "expected exactly two underscore-separated segments (prefix_value)", leaf); + return; + } + String trimmed = Optional.ofNullable(value).map(String::trim).orElse(""); + if ("true".equalsIgnoreCase(trimmed)) { + validateKeyParams(name, parts[0].toUpperCase(Locale.ROOT), parts[1]); + } else if (!"false".equalsIgnoreCase(trimmed)) { + logger.warn("KeyConstraint: allowedKeys \"{}\" has non-boolean value \"{}\"; ignored", + name, value); + return; + } + super.setConfig(name, value); + return; + }Also consider: the class-level
staticmutability ofkeyType/keyParamsmakes this method non-thread-safe across concurrent admin-UI edits to different profiles — worth addressing in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 494 - 542, The allowedKeys branch in setConfig (check using CONFIG_ALLOWED_KEYS_PREFIX) currently only validates and never persists nor returns, causing allowedKeys.* entries to be ignored and then falling through to overwrite CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS with the static keyType/keyParams (which may be empty); fix by calling super.setConfig(name, value) and returning immediately from that branch (regardless of value being "true" or "false") after optionally calling validateKeyParams(…) where appropriate, so allowedKeys.<ALG>_<VAL> is persisted and does not touch the class-level keyType/keyParams or the later super.setConfig(CONFIG_KEY_TYPE, keyType)/super.setConfig(CONFIG_KEY_PARAMETERS, keyParams) calls.base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template (2)
462-488:⚠️ Potential issue | 🟡 MinorSelect the first emitted option, not the first source token.
If the first source value is filtered out, Line 486 never marks the first valid option as selected.
🐛 Proposed fix
var value = 0; var l = 0; + var selectedAdded = false; for (l = 0 ; l < lengthsOrCurves.length; l++) { @@ - if (l == 0) { + if (!selectedAdded) { options += ' SELECTED'; + selectedAdded = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 462 - 488, The current loop uses "if (l == 0)" to mark the selected option, which picks the first source token even if it was filtered out; add a boolean flag (e.g., firstSelected) initialized before the loop and, when you append an OPTION to the options string inside the "if (included)" block, check "if (!firstSelected)" to append ' SELECTED' and then set firstSelected = true; update the code around the lengthsOrCurves loop (where variables value, included and options are used) to use this flag instead of "if (l == 0)" so the first emitted option is selected.
210-230:⚠️ Potential issue | 🟠 MajorPreserve the configured default after sorting key types.
Sorting makes
RSA,ECdefault toECbecause Line 229 still selects index0. That can silently change the generated key type when users accept the default.🐛 Proposed fix
var keyTypesRet = keyTypes.split(","); var keyTypesTrimmed = []; + var defaultKeyType = ""; for (var kt = 0; kt < keyTypesRet.length; kt++) { var tok = String(keyTypesRet[kt]).trim(); if (tok) { + if (!defaultKeyType) { + defaultKeyType = tok; + } keyTypesTrimmed.push(tok); } } keyTypesTrimmed.sort(function(a, b) { return a.localeCompare(b); }); keyTypesRet = keyTypesTrimmed; @@ - for(types= 0 ; types < keyTypesRet.length ; types ++) { + var selectedDefault = false; + for(types= 0 ; types < keyTypesRet.length ; types ++) { @@ - if( types == 0 ) { + if(!selectedDefault && keyTypesRet[types] == defaultKeyType) { selected = "SELECTED"; + selectedDefault = true; } else { selected = ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 210 - 230, Before sorting, capture the configured default (e.g., var originalDefault = keyTypesRet[0]) and after you build keyTypesTrimmed and sort it, compute the selected index by finding originalDefault in the sorted array (e.g., selectedIndex = keyTypesRet.indexOf(originalDefault)); then in the options loop set selected only when the loop index equals selectedIndex instead of hardcoding types == 0, ensuring the original default key type is preserved even after sorting (make sure to compare against the raw values used in keyTypesRet, accounting for the EC -> "ECC" display mapping done in the loop).
🧹 Nitpick comments (3)
.gitignore (1)
17-19: Consider consistency in directory patterns.The ignore patterns are reasonable additions for IDE settings and local configs. However, there's a minor inconsistency:
.cursor/uses a trailing slash (directory-only) while.vscodeand.configsdo not.For clarity and consistency, consider adding trailing slashes to all directory patterns:
📝 Suggested consistency improvement
-.configs -.vscode +.configs/ +.vscode/ .cursor/Additionally,
.configsis a fairly generic pattern. If this is meant to ignore a specific local directory, the trailing slash makes that intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 17 - 19, The .gitignore entries are inconsistent: ".cursor/" uses a trailing slash while ".vscode" and ".configs" do not; update the patterns to consistently use directory-only syntax by changing ".vscode" and ".configs" to include trailing slashes (i.e., ".vscode/" and ".configs/") and, if ".configs" is meant to ignore a specific local config directory, make it more specific (rename to the exact directory name) to avoid over-broad ignores; update the file entries for ".configs", ".vscode", and ".cursor/" accordingly.base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
150-150: UsetoUpperCase(Locale.ROOT)for deterministic case folding.
parts[0].toUpperCase()and the matchingalgName.toUpperCase()at line 190 are locale-sensitive; in a Turkish default locale the dotless/dotted-I mapping can produce identifiers that never match the families ("MLDSA","RSA","EC") being looked up. Since these are configuration identifiers, uppercase them withLocale.ROOT.- String algType = parts[0].toUpperCase(); + String algType = parts[0].toUpperCase(Locale.ROOT);- String algType = algName.toUpperCase(); + String algType = algName.toUpperCase(Locale.ROOT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` at line 150, The locale-sensitive calls to toUpperCase() in KeyConstraint.java can produce non-deterministic identifiers; change the case-folding for parts[0] (assigned to algType) and the algName conversion to use toUpperCase(Locale.ROOT) so matching against families like "MLDSA", "RSA", and "EC" is deterministic across locales.base/ca/shared/profiles/ca/caMLDSAServerCert.cfg (1)
33-36: Redundant configuration.
keyParameters=44,65,87andallowedKeys.mldsa_44/65/87=trueencode the same allow-set. With the new precedence model described in the PR (allowedKeys.*takes precedence), consider either removingkeyParametersfor MLDSA-only profiles or omitting theallowedKeys.*entries that simply re-affirm what's already inkeyParameters, to avoid two sources of truth drifting apart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/profiles/ca/caMLDSAServerCert.cfg` around lines 33 - 36, This profile contains duplicate allow-listing: policyset.serverCertSet.3.constraint.params.keyParameters and the individual policyset.serverCertSet.3.constraint.params.allowedKeys.mldsa_44/65/87 entries; remove the redundant source of truth by deleting policyset.serverCertSet.3.constraint.params.keyParameters=44,65,87 so the allowedKeys.mldsa_44, allowedKeys.mldsa_65 and allowedKeys.mldsa_87 entries remain as the single precedence-driven configuration for MLDSA-only profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/ca/shared/profiles/ca/caInstallCACert.cfg`:
- Around line 35-37: The leaf names under
policyset.caCertSet.3.constraint.params.allowedKeys (mldsa44, mldsa65, mldsa87)
don't match the parser's expected "type_version" form
(KeyConstraint.mapAllowedKeys() splits on '_' into exactly two non-empty
segments), so rename those leaves to include the underscore (mldsa_44, mldsa_65,
mldsa_87) so they are recognized and not skipped; ensure the names now match the
tokens used by keyParameters in the same profile.
In `@base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg`:
- Around line 35-37:
policyset.auditSigningCertSet.3.constraint.params.allowedKeys entries use names
like "mldsa44" which KeyConstraint.mapAllowedKeys() will skip because it expects
an underscore-delimited pair; update the three keys under
policyset.auditSigningCertSet.3.constraint.params.allowedKeys to "mldsa_44",
"mldsa_65", and "mldsa_87" to match the keyParameter values in keyParameters and
the naming used by other profiles so they are parsed correctly by
KeyConstraint.mapAllowedKeys().
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 493-495: The template currently unconditionally injects the ML-DSA
"44" option when options.length == 0 and keyType == "MLDSA", which re-enables a
parameter even when overrides disable it; update the conditional to also check
the override flag (e.g., allowedKeys.mldsa_44) so that the options assignment
only occurs when keyType == "MLDSA" AND allowedKeys.mldsa_44 is true, targeting
the options variable and the keyType/"MLDSA" branch in ProfileSelect.template to
prevent adding the 44 option when overrides disallow it.
- Around line 448-460: The ML-DSA validity filter (using MLDSA_VALID) is
currently applied before calling applyAllowedKeysOverrides, which lets overrides
reintroduce invalid MLDSA values; move the MLDSA-specific filtering so it runs
after applyAllowedKeysOverrides (i.e., call
applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType) first, then
if keyType == "MLDSA" filter the resulting lengthsOrCurves against MLDSA_VALID),
and finally call sortLengthsOrCurvesList; update references to lengthsOrCurves,
MLDSA_VALID, applyAllowedKeysOverrides, and sortLengthsOrCurvesList accordingly.
---
Outside diff comments:
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 462-488: The current loop uses "if (l == 0)" to mark the selected
option, which picks the first source token even if it was filtered out; add a
boolean flag (e.g., firstSelected) initialized before the loop and, when you
append an OPTION to the options string inside the "if (included)" block, check
"if (!firstSelected)" to append ' SELECTED' and then set firstSelected = true;
update the code around the lengthsOrCurves loop (where variables value, included
and options are used) to use this flag instead of "if (l == 0)" so the first
emitted option is selected.
- Around line 210-230: Before sorting, capture the configured default (e.g., var
originalDefault = keyTypesRet[0]) and after you build keyTypesTrimmed and sort
it, compute the selected index by finding originalDefault in the sorted array
(e.g., selectedIndex = keyTypesRet.indexOf(originalDefault)); then in the
options loop set selected only when the loop index equals selectedIndex instead
of hardcoding types == 0, ensuring the original default key type is preserved
even after sorting (make sure to compare against the raw values used in
keyTypesRet, accounting for the EC -> "ECC" display mapping done in the loop).
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 494-542: The allowedKeys branch in setConfig (check using
CONFIG_ALLOWED_KEYS_PREFIX) currently only validates and never persists nor
returns, causing allowedKeys.* entries to be ignored and then falling through to
overwrite CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS with the static
keyType/keyParams (which may be empty); fix by calling super.setConfig(name,
value) and returning immediately from that branch (regardless of value being
"true" or "false") after optionally calling validateKeyParams(…) where
appropriate, so allowedKeys.<ALG>_<VAL> is persisted and does not touch the
class-level keyType/keyParams or the later super.setConfig(CONFIG_KEY_TYPE,
keyType)/super.setConfig(CONFIG_KEY_PARAMETERS, keyParams) calls.
---
Nitpick comments:
In @.gitignore:
- Around line 17-19: The .gitignore entries are inconsistent: ".cursor/" uses a
trailing slash while ".vscode" and ".configs" do not; update the patterns to
consistently use directory-only syntax by changing ".vscode" and ".configs" to
include trailing slashes (i.e., ".vscode/" and ".configs/") and, if ".configs"
is meant to ignore a specific local config directory, make it more specific
(rename to the exact directory name) to avoid over-broad ignores; update the
file entries for ".configs", ".vscode", and ".cursor/" accordingly.
In `@base/ca/shared/profiles/ca/caMLDSAServerCert.cfg`:
- Around line 33-36: This profile contains duplicate allow-listing:
policyset.serverCertSet.3.constraint.params.keyParameters and the individual
policyset.serverCertSet.3.constraint.params.allowedKeys.mldsa_44/65/87 entries;
remove the redundant source of truth by deleting
policyset.serverCertSet.3.constraint.params.keyParameters=44,65,87 so the
allowedKeys.mldsa_44, allowedKeys.mldsa_65 and allowedKeys.mldsa_87 entries
remain as the single precedence-driven configuration for MLDSA-only profiles.
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Line 150: The locale-sensitive calls to toUpperCase() in KeyConstraint.java
can produce non-deterministic identifiers; change the case-folding for parts[0]
(assigned to algType) and the algName conversion to use toUpperCase(Locale.ROOT)
so matching against families like "MLDSA", "RSA", and "EC" is deterministic
across locales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 119a1ea7-8cda-43d9-acf0-3a981608d646
📒 Files selected for processing (11)
.gitignorebase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java
Comment on lines
493
to
+495
| if (options.length == 0) { | ||
| if (keyType != "EC") { | ||
| if (keyType == "MLDSA") { | ||
| options = '<OPTION VALUE=44 SELECTED>44'; |
There was a problem hiding this comment.
Don’t re-enable ML-DSA 44 after overrides remove all options.
If allowedKeys.mldsa_44=false leaves no effective ML-DSA parameters, Line 495 still offers 44, contradicting the override and likely causing backend rejection.
🐛 Proposed fix
if (keyType == "MLDSA") {
- options = '<OPTION VALUE=44 SELECTED>44';
+ options = '<OPTION VALUE="" DISABLED SELECTED>No allowed ML-DSA parameter</OPTION>';
} else if (keyType != "EC") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (options.length == 0) { | |
| if (keyType != "EC") { | |
| if (keyType == "MLDSA") { | |
| options = '<OPTION VALUE=44 SELECTED>44'; | |
| if (options.length == 0) { | |
| if (keyType == "MLDSA") { | |
| options = '<OPTION VALUE="" DISABLED SELECTED>No allowed ML-DSA parameter</OPTION>'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 493 -
495, The template currently unconditionally injects the ML-DSA "44" option when
options.length == 0 and keyType == "MLDSA", which re-enables a parameter even
when overrides disable it; update the conditional to also check the override
flag (e.g., allowedKeys.mldsa_44) so that the options assignment only occurs
when keyType == "MLDSA" AND allowedKeys.mldsa_44 is true, targeting the options
variable and the keyType/"MLDSA" branch in ProfileSelect.template to prevent
adding the 44 option when overrides disallow it.
fmarco76
reviewed
Apr 22, 2026
Member
fmarco76
left a comment
fmarco76
left a comment
There was a problem hiding this comment.
There are some error in the modified profiles which need to be fixed.
Additionally, I think the approach with _ to separate key and value could generate error.
The resulting profile is a mixed approach I would prefer to avoid. The keyParameters can be dropped from the profile but still parsed with a warning for deprecation.
Not sure if the keytype should be the only accepted list and only these will be verified.
An alternative is to replace 44, 65, 87 with mldsa44, mldsa65 and mldsa87 but updated the parser to recognize them appropriately. Not sure this solve other issues.
Note: if AI is used to generate the code it should be specified in the commit message with field Assisted-by.
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
93e880a to
3a9a0af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
494-542:⚠️ Potential issue | 🔴 CriticalCritical:
setConfig("allowedKeys.X_Y", …)is never persisted and clobberskeyType/keyParameters.Two real problems on this path:
- The
allowedKeys.*leaf is never written back. The new branch (lines 500–516) only validates; it never callssuper.setConfig(name, value). There is also noreturnafter validation, so control falls through to lines 536–537, which writeCONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERSonly. Any admin-UI edit toallowedKeys.mldsa_44=trueis silently dropped.- Falling through wipes existing config. Because the static fields
keyType/keyParams(lines 71–72) are reset to""at the end of every call, a subsequentsetConfig("allowedKeys.mldsa_44", "true")reaches lines 536–537 with both statics empty and callssuper.setConfig(CONFIG_KEY_TYPE, "")/super.setConfig(CONFIG_KEY_PARAMETERS, ""), overwriting the profile’s previously-savedkeyTypeandkeyParameterswith empty strings.value="false"cannot be saved. The guardfilter("true"::equalsIgnoreCase)causes the wholeallowedKeysbranch to be skipped when the admin explicitly disables a key (…_44=false); the call then falls into the same clobbering path at 536–537 and also never persists thefalse.- Minor: when validation at line 515 fails,
EPropertyExceptionis raised withname="allowedKeys.X_Y"butvalidateKeyParamsreferencesCMS_INVALID_PROPERTYagainst the samename— worth double-checking the UI maps this to the correct field.Suggested shape:
🛠️ Proposed fix
if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) - && Optional.ofNullable(value).map(String::trim).filter("true"::equalsIgnoreCase).isPresent() && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() ) { String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); String[] parts = leaf.split("_", 2); if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { logger.warn( "KeyConstraint: allowedKeys property \"{}\" ignored: " + "expected exactly two underscore-separated segments (prefix_value)", leaf); return; } - String algType = parts[0].toUpperCase(); - String keyValue = parts[1]; - validateKeyParams(name, algType, keyValue); + String trimmed = value == null ? "" : value.trim(); + if (!"true".equalsIgnoreCase(trimmed) && !"false".equalsIgnoreCase(trimmed)) { + logger.warn("KeyConstraint: allowedKeys {} non-boolean value \"{}\"; ignored", name, value); + return; + } + if ("true".equalsIgnoreCase(trimmed)) { + validateKeyParams(name, parts[0].toUpperCase(), parts[1]); + } + super.setConfig(name, value); + return; }Please also add a unit test covering: (a) persistence of
allowedKeys.mldsa_44=true, (b) persistence ofallowedKeys.mldsa_44=false, and (c) that setting anallowedKeys.*entry does not modify the storedkeyType/keyParameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 494 - 542, The allowedKeys branch in setConfig(...) only validates and then falls through, never calling super.setConfig(name,value), and then clobbers keyType/keyParams because both static fields are reset; change the logic in setConfig to (1) treat names starting with CONFIG_ALLOWED_KEYS_PREFIX as their own case: validate via validateKeyParams(name, algType, keyValue), then call super.setConfig(name, value) and return immediately (do not fall through to the keyType/keyParams handling), (2) remove the restrictive filter("true"::equalsIgnoreCase) so allowedKeys entries with "false" are persisted, (3) only update CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS when name.equals(CONFIG_KEY_TYPE) or name.equals(CONFIG_KEY_PARAMETERS) respectively (and avoid writing empty values back if the partner param is not yet set), and (4) ensure keyType and keyParams statics are not reset until after successful writes; add unit tests for persisting allowedKeys.mldsa_44=true, allowedKeys.mldsa_44=false, and verifying an allowedKeys.* set does not modify stored keyType/keyParameters.
♻️ Duplicate comments (3)
base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template (3)
493-505:⚠️ Potential issue | 🟠 MajorMLDSA empty-options fallback still unconditionally offers
44.When
options.length == 0forMLDSA(e.g. admin setallowedKeys.mldsa_44=falseand didn't enable65/87, or the merged allow-set is otherwise empty), this branch still renders<OPTION VALUE=44 SELECTED>44, contradicting the constraint. The resulting request will be rejected by the server (checkAllowedAlgorithmwon’t find44in the allowed set). Either honor the allowlist (don’t offer a default) or pick a value that is still allowed.🐛 Proposed fix
- if (keyType == "MLDSA") { - options = '<OPTION VALUE=44 SELECTED>44'; - } else if (keyType != "EC") { + if (keyType == "MLDSA") { + options = '<OPTION VALUE="" DISABLED SELECTED>No allowed ML-DSA parameter</OPTION>'; + } else if (keyType != "EC") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 493 - 505, The MLDSA branch in ProfileSelect.template currently injects a hardcoded '<OPTION VALUE=44 SELECTED>44' when options.length == 0, which violates the allowlist; change the MLDSA branch in the options-fallback logic (the if (keyType == "MLDSA") block) to not emit a default value when no allowed MLDSA values exist—either leave options as an empty string (no OPTION added) or select a value from the actual allowed-set if one exists—so the rendered select honors the allowlist; ensure the code path that calls isIE() and the other branches remain unchanged.
448-460:⚠️ Potential issue | 🟠 MajorMLDSA whitelist still runs before overrides.
The earlier review asked for
MLDSA_VALIDfiltering to run afterapplyAllowedKeysOverridesso overrides can't reintroduce invalid variants (e.g.allowedKeys.mldsa_99=true). The PR was marked addressed in commit 3a9a0af, but in this revision the filter at 448–457 still executes before the call on line 459. Please re-apply the fix.🐛 Proposed fix
- if (keyType == "MLDSA") { - var mldsaFiltered = []; - for (var mf = 0; mf < lengthsOrCurves.length; mf++) { - var mv = String(lengthsOrCurves[mf]).trim(); - if (MLDSA_VALID[mv]) { - mldsaFiltered.push(mv); - } - } - lengthsOrCurves = mldsaFiltered; - } - lengthsOrCurves = applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType); + if (keyType == "MLDSA") { + var mldsaFiltered = []; + for (var mf = 0; mf < lengthsOrCurves.length; mf++) { + var mv = String(lengthsOrCurves[mf]).trim(); + if (MLDSA_VALID[mv]) { + mldsaFiltered.push(mv); + } + } + lengthsOrCurves = mldsaFiltered; + } lengthsOrCurves = sortLengthsOrCurvesList(keyType, lengthsOrCurves);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 448 - 460, The MLDSA validity filter is applied to lengthsOrCurves before applyAllowedKeysOverrides, allowing overrides to reintroduce invalid MLDSA variants; move the MLDSA-specific filtering so it runs after applyAllowedKeysOverrides and before sortLengthsOrCurvesList. Concretely, keep calls to applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType) and sortLengthsOrCurvesList(keyType, lengthsOrCurves) but relocate the MLDSA block that checks MLDSA_VALID and builds mldsaFiltered to execute between those two calls (and only when keyType == "MLDSA"), ensuring MLDSA_VALID is consulted last.
209-218:⚠️ Potential issue | 🟠 MajorAlphabetical sort of
keyTypesRetchanges the default selected key type.With
keyType="-"the code setskeyTypes="RSA,EC"(line 195); after sorting, the array becomes["EC","RSA"], so thetypes==0branch at line 229 now marks EC as SELECTED rather than RSA. Admins who deliberately orderedkeyType=RSA,ECto keep RSA as the default will silently see their UI default flip to EC.Either drop the sort for
keyTypesRet(it’s unrelated to the allowedKeys feature), or preserve the configured order. If deterministic ordering is desired, do it only after choosing the default.🛠️ Proposed fix
var keyTypesRet = keyTypes.split(","); var keyTypesTrimmed = []; for (var kt = 0; kt < keyTypesRet.length; kt++) { var tok = String(keyTypesRet[kt]).trim(); if (tok) { keyTypesTrimmed.push(tok); } } - keyTypesTrimmed.sort(function(a, b) { return a.localeCompare(b); }); keyTypesRet = keyTypesTrimmed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 209 - 218, The code sorts keyTypesRet (via keyTypesTrimmed.sort(...)) which reorders the configured allowed keyTypes and causes the default selection logic (the branch checking types==0) to pick the wrong default; fix by preserving the configured order for default selection: do not sort keyTypesRet before the default is chosen—either remove the sort entirely or perform the sort only after using the original keyTypesRet (or a copy) to compute the default (references: keyTypes, keyTypesRet, keyTypesTrimmed, and the selection logic that checks types==0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 219-237: The rejection message in checkAllowedAlgorithm currently
passes the raw value (which may be empty) into
CMS_PROFILE_KEY_PARAMS_NOT_MATCHED; change the thrown ERejectException to supply
a non-empty, informative token instead: compute a fallbackString such as algName
+ ":" + keySize or the allowedKeys set (from getAllowedKeysForAlgorithm) and
pass that to CMS.getUserMessage when value is null/empty; update
checkAllowedAlgorithm to use this fallbackString in the exception parameter so
the audit message always contains meaningful details.
---
Outside diff comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 494-542: The allowedKeys branch in setConfig(...) only validates
and then falls through, never calling super.setConfig(name,value), and then
clobbers keyType/keyParams because both static fields are reset; change the
logic in setConfig to (1) treat names starting with CONFIG_ALLOWED_KEYS_PREFIX
as their own case: validate via validateKeyParams(name, algType, keyValue), then
call super.setConfig(name, value) and return immediately (do not fall through to
the keyType/keyParams handling), (2) remove the restrictive
filter("true"::equalsIgnoreCase) so allowedKeys entries with "false" are
persisted, (3) only update CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS when
name.equals(CONFIG_KEY_TYPE) or name.equals(CONFIG_KEY_PARAMETERS) respectively
(and avoid writing empty values back if the partner param is not yet set), and
(4) ensure keyType and keyParams statics are not reset until after successful
writes; add unit tests for persisting allowedKeys.mldsa_44=true,
allowedKeys.mldsa_44=false, and verifying an allowedKeys.* set does not modify
stored keyType/keyParameters.
---
Duplicate comments:
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 493-505: The MLDSA branch in ProfileSelect.template currently
injects a hardcoded '<OPTION VALUE=44 SELECTED>44' when options.length == 0,
which violates the allowlist; change the MLDSA branch in the options-fallback
logic (the if (keyType == "MLDSA") block) to not emit a default value when no
allowed MLDSA values exist—either leave options as an empty string (no OPTION
added) or select a value from the actual allowed-set if one exists—so the
rendered select honors the allowlist; ensure the code path that calls isIE() and
the other branches remain unchanged.
- Around line 448-460: The MLDSA validity filter is applied to lengthsOrCurves
before applyAllowedKeysOverrides, allowing overrides to reintroduce invalid
MLDSA variants; move the MLDSA-specific filtering so it runs after
applyAllowedKeysOverrides and before sortLengthsOrCurvesList. Concretely, keep
calls to applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType) and
sortLengthsOrCurvesList(keyType, lengthsOrCurves) but relocate the MLDSA block
that checks MLDSA_VALID and builds mldsaFiltered to execute between those two
calls (and only when keyType == "MLDSA"), ensuring MLDSA_VALID is consulted
last.
- Around line 209-218: The code sorts keyTypesRet (via
keyTypesTrimmed.sort(...)) which reorders the configured allowed keyTypes and
causes the default selection logic (the branch checking types==0) to pick the
wrong default; fix by preserving the configured order for default selection: do
not sort keyTypesRet before the default is chosen—either remove the sort
entirely or perform the sort only after using the original keyTypesRet (or a
copy) to compute the default (references: keyTypes, keyTypesRet,
keyTypesTrimmed, and the selection logic that checks types==0).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12995138-0e53-43d5-bb08-5876a6c2c841
📒 Files selected for processing (11)
.gitignorebase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
🚧 Files skipped from review as they are similar to previous changes (2)
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
Comment on lines
+219
to
+237
| private void checkAllowedAlgorithm( | ||
| int keySize, | ||
| String value, | ||
| String[] baseTokens, | ||
| Map<String, Map<String, String>> allowedKeysMap, | ||
| String algName, | ||
| Request request | ||
| ) throws ERejectException { | ||
| Set<String> allowedKeys = getAllowedKeysForAlgorithm(baseTokens, allowedKeysMap, algName); | ||
| logger.info("{} KeySize is {}", algName, keySize); | ||
| if (!allowedKeys.contains(Integer.toString(keySize))) { | ||
| throw new ERejectException( | ||
| CMS.getUserMessage( | ||
| getLocale(request), | ||
| "CMS_PROFILE_KEY_PARAMS_NOT_MATCHED", | ||
| value)); | ||
| } | ||
| logger.debug("KeyConstraint.validate: {} key contraints passed.", algName); | ||
| } |
There was a problem hiding this comment.
Minor: error message uses raw keyParameters which may be empty.
When a profile relies solely on allowedKeys.* (e.g. caMLDSAInternalAuthSubsystemCert.cfg now has keyParameters= empty), a rejection here passes an empty value into CMS_PROFILE_KEY_PARAMS_NOT_MATCHED. Consider substituting the computed allowedKeys set (or algName + ":" + keySize) in the user message for a clearer audit trail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`
around lines 219 - 237, The rejection message in checkAllowedAlgorithm currently
passes the raw value (which may be empty) into
CMS_PROFILE_KEY_PARAMS_NOT_MATCHED; change the thrown ERejectException to supply
a non-empty, informative token instead: compute a fallbackString such as algName
+ ":" + keySize or the allowedKeys set (from getAllowedKeysForAlgorithm) and
pass that to CMS.getUserMessage when value is null/empty; update
checkAllowedAlgorithm to use this fallbackString in the exception parameter so
the audit message always contains meaningful details.
Contributor
Author
Thank you @fmarco76 |
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
a0e5998 to
9ffea25
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg (1)
86-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix double comma in signingAlgsAllowed parameter.
Line 86 contains a double comma between
SHA512withRSAandSHA256withEC, which is a syntax error.🐛 Proposed fix
-policyset.transportCertSet.8.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87 +policyset.transportCertSet.8.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg` at line 86, The signingAlgsAllowed property (policyset.transportCertSet.8.constraint.params.signingAlgsAllowed) contains a stray double comma between SHA512withRSA and SHA256withEC; remove the extra comma so the list is a valid comma-separated sequence (i.e., ensure only a single comma separates SHA512withRSA and SHA256withEC).base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
528-579:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical:
setConfignever persistsallowedKeys.*leaves.The only
super.setConfig(...)calls (lines 573-574) writeCONFIG_KEY_TYPEandCONFIG_KEY_PARAMETERS. Whennamestarts withCONFIG_ALLOWED_KEYS_PREFIX, the method runs validation but never stores the value through the config store, so anyallowedKeys.*change made via the admin/REST API is silently dropped. Combine this with the static-field reuse below and the new feature is effectively unsavable through the UI path.Also, the static class fields
keyType/keyParams/allowedKeys(lines 72-74) are mutable shared state across all instances and threads — the existing pattern is fragile, and the newallowedKeysfield exacerbates it: it stores the property value ("true"/"false"), is checked elsewhere (line 458) only vialength() > 0— which is always true regardless of value — and is never reset at lines 577-578. After the first call with anallowedKeys.*leaf, every subsequent profile validation will seeKeyConstraint.allowedKeys.length() > 0.Suggested direction:
- Persist
allowedKeys.*directly: whennamestarts withCONFIG_ALLOWED_KEYS_PREFIX, callsuper.setConfig(name, value)and return early after validation.- Use a presence flag (boolean) rather than storing the value string, and reset it in the same place as
keyType/keyParams, or — preferably — drop the static accumulator pattern entirely in favor of reading fromgetConfigStore()when validation needs cross-leaf checks.Suggested changes
`@Override` public void setConfig(String name, String value) throws EPropertyException { logger.info("KeyConstraint: Setting " + name + ": " + value); - if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX)) { - allowedKeys = value; - } - if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) - && Optional.ofNullable(value).map(String::trim).filter("true"::equalsIgnoreCase).isPresent() && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() ) { String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); String[] parts = leaf.split("\\.", 2); if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { throw new EPropertyException(CMS.getUserMessage("CMS_INVALID_PROPERTY", name)); } String algType = parts[0].toUpperCase(Locale.ROOT); validateAlgorithmName(algType); - String keyValue = parts[1]; - validateKeyParams(name, algType, keyValue); + // forbid mixing with deprecated keyParameters + String existingKeyParams = super.getConfig(CONFIG_KEY_PARAMETERS); + if (existingKeyParams != null && !existingKeyParams.isEmpty()) { + throw new EPropertyException(CMS.getUserMessage( + "CMS_MIXED_KEY_CONFIGURATION", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_PARAMETERS)); + } + super.setConfig(name, value); + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 528 - 579, setConfig currently validates allowedKeys.* leaves but never persists them and uses mutable static strings (keyType, keyParams, allowedKeys) which leak state; update setConfig in KeyConstraint so that when name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) you call super.setConfig(name, value) (after validation) and return early, and replace the allowedKeys string accumulator with a boolean presence flag (or drop the static accumulator and read the value from getConfigStore() when validateKeyParams needs cross-leaf info); also ensure the presence flag (or no-static design) is reset alongside keyType/keyParams after super.setConfig(CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS) or eliminate the static fields entirely to avoid shared mutable state across instances/threads.
♻️ Duplicate comments (2)
base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template (2)
493-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMLDSA fallback still re-enables 44 even when overrides disabled it (issue not yet addressed).
When
options.length == 0(e.g. becauseallowedKeys.mldsa.44=false,.65=false,.87=falsewere set), Line 495 unconditionally injects<OPTION VALUE=44 SELECTED>44, contradicting the override and likely producing a backend rejection. The previous review flagged this; the file still does the unconditional injection.🐛 Suggested fix — surface a "no allowed parameter" sentinel instead
if (options.length == 0) { if (keyType == "MLDSA") { - options = '<OPTION VALUE=44 SELECTED>44'; + options = '<OPTION VALUE="" DISABLED SELECTED>No allowed ML-DSA parameter</OPTION>'; } else if (keyType != "EC") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 493 - 505, The code block that builds the select options wrongly injects '<OPTION VALUE=44 SELECTED>44' when options.length == 0 and keyType == "MLDSA", ignoring overrides; change the logic in the options construction (the branch handling keyType == "MLDSA" inside the if (options.length == 0) block) to not unconditionally add 44 but instead emit a sentinel "no allowed parameter" option (e.g. a disabled/empty OPTION or a special VALUE that your backend recognizes) or leave options empty and set a flag indicating no allowed keys so the UI/submit flow can handle rejection gracefully; update any codepaths that expect a default selection (the options variable and any downstream form-submit handling) to treat the sentinel as “no valid choice” rather than a real key.
448-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMLDSA whitelist still bypassable via overrides (issue not yet addressed).
The current order is: MLDSA-filter at Lines 448-457 →
applyAllowedKeysOverridesat Line 459 → sort. So a configuration likeallowedKeys.mldsa.99=trueis added by the overrides after the MLDSA_VALID filter has run, and "99" ends up in the rendered options. The previous review flagged this and the proposed fix was to move the MLDSA filter to run after the overrides; the file under review still has the old order.🐛 Suggested fix — apply MLDSA filter after overrides
- if (keyType == "MLDSA") { - var mldsaFiltered = []; - for (var mf = 0; mf < lengthsOrCurves.length; mf++) { - var mv = String(lengthsOrCurves[mf]).trim(); - if (MLDSA_VALID[mv]) { - mldsaFiltered.push(mv); - } - } - lengthsOrCurves = mldsaFiltered; - } - - lengthsOrCurves = applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType); + lengthsOrCurves = applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType); + if (keyType == "MLDSA") { + var mldsaFiltered = []; + for (var mf = 0; mf < lengthsOrCurves.length; mf++) { + var mv = String(lengthsOrCurves[mf]).trim(); + if (MLDSA_VALID[mv]) { + mldsaFiltered.push(mv); + } + } + lengthsOrCurves = mldsaFiltered; + } lengthsOrCurves = sortLengthsOrCurvesList(keyType, lengthsOrCurves);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 448 - 460, The MLDSA whitelist filter runs before applyAllowedKeysOverrides so overrides can reintroduce disallowed values; change the order so applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType) runs first, then perform the MLDSA filtering using MLDSA_VALID against lengthsOrCurves when keyType == "MLDSA", and finally call sortLengthsOrCurvesList(keyType, lengthsOrCurves); update references to lengthsOrCurves, MLDSA_VALID, applyAllowedKeysOverrides, and sortLengthsOrCurvesList accordingly.
🧹 Nitpick comments (7)
base/ca/shared/profiles/ca/caFullCMCUserCert.cfg (1)
32-36: 💤 Low valueProperty ordering inconsistency (minor).
The
allowedKeys.rsa.*entries (lines 32-35) appear beforekeyType=RSA(line 36), whereas other profiles in this PR definekeyTypefirst. While property order typically doesn't affect Java properties parsing, maintaining consistent ordering across profiles improves maintainability.Suggested ordering for consistency
policyset.cmcUserCertSet.3.constraint.class_id=keyConstraintImpl policyset.cmcUserCertSet.3.constraint.name=Key Constraint +policyset.cmcUserCertSet.3.constraint.params.keyType=RSA policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.1024=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.2048=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.3072=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.4096=true -policyset.cmcUserCertSet.3.constraint.params.keyType=RSA policyset.cmcUserCertSet.3.default.class_id=userKeyDefaultImpl🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caFullCMCUserCert.cfg` around lines 32 - 36, The property ordering is inconsistent: the keyType property (policyset.cmcUserCertSet.3.constraint.params.keyType) should appear before the allowedKeys entries (policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.1024, .2048, .3072, .4096) to match other profiles; move the keyType=RSA line so it precedes the allowedKeys.rsa.* lines to maintain consistent ordering across profiles.base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
197-213: 💤 Low valueOptional refactor: simplify the verbose
Optionalchains.
Optional.ofNullable(propertyValue).map(String::trim).filter("true"::equalsIgnoreCase).isPresent()can be expressed more directly. This pattern is repeated four times (here and at line 539).♻️ Suggested simplification
- if (Optional.ofNullable(propertyValue) - .map(String::trim) - .filter("true"::equalsIgnoreCase) - .isPresent()) { + String trimmed = propertyValue == null ? "" : propertyValue.trim(); + if ("true".equalsIgnoreCase(trimmed)) { allowed.add(keyValue); - } else if (Optional.ofNullable(propertyValue) - .map(String::trim) - .filter("false"::equalsIgnoreCase) - .isPresent()) { + } else if ("false".equalsIgnoreCase(trimmed)) { allowed.remove(keyValue); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 197 - 213, The Optional chains checking propertyValue in KeyConstraint are verbose; replace them with a simple trimmed, null-safe string comparison: compute a trimmed value (e.g., String v = propertyValue == null ? null : propertyValue.trim()) and then use "true".equalsIgnoreCase(v) and "false".equalsIgnoreCase(v) to decide allowed.add(keyValue) or allowed.remove(keyValue), and keep the existing logger.warn branch for other values; apply the same simplification to the other occurrence around line 539. Ensure you reference the same variables/methods (propertyValue, keyValue, allowed, logger.warn) in the refactor.base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template (2)
313-359: 💤 Low valueHelper looks correct; consider switching to a single rebuild pass for readability.
The override removal path at Lines 348-354 rebuilds the array on every
falsetoken (O(n²)) and theinSetflag is then toggled tofalse, which works but couples two state variables. A single final pass that emits tokens whoseinSetflag is stilltruewould be clearer and avoid the inner loop. Not a correctness concern; flagging only as readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 313 - 359, The function applyAllowedKeysOverrides rebuilds the allowed array inside the loop for each "false" override which is O(n²); instead, when processing overrides (in applyAllowedKeysOverrides) mark additions with inSet[keyValue]=true and removals with inSet[keyValue]=false and also maintain an insertion-order list (reuse the existing allowed/order list when adding new keys), then after the overrides loop do a single pass over that insertion-order list to produce the final allowed array by including only entries where inSet[name] is true; update references to allowed, inSet, and the overrides handling in applyAllowedKeysOverrides accordingly to remove the inner rebuild and perform one final emit pass.
480-491: ⚡ Quick win
SELECTEDmay end up on no option when first array slot is filtered out.
l == 0at Line 486 is the array index, not the index of the first included option. IflengthsOrCurves[0]is excluded by the EC/numeric checks at Lines 469-478 (e.g. EC profile where the first item after sort failstranslateCurveName/IE filtering), no option in the rendered list is markedSELECTED, leaving the browser to its own default. Tracking the first included item makes the default deterministic.♻️ Suggested fix
var value = 0; var l = 0; + var firstIncluded = true; for (l = 0 ; l < lengthsOrCurves.length; l++) { var included = true; @@ if (included) { if (keyType == "EC") { options += '<OPTION VALUE="' + translateCurveName(value) + '"'; } else { options += '<OPTION VALUE="' + value + '"'; } - if (l == 0) { + if (firstIncluded) { options += ' SELECTED'; + firstIncluded = false; } options += '>' + value; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 480 - 491, The current logic uses the array index check (l == 0) to mark SELECTED which fails when the first element is filtered out; change it to track the first included option instead: introduce a boolean flag (e.g., firstIncludedFound) initialized false before the loop and when you append an option for an included item in the block guarded by "if (included)" check, if firstIncludedFound is false add ' SELECTED' to options and set firstIncludedFound=true; keep using translateCurveName and keyType logic as-is and remove the l == 0 check so the first actually rendered option is deterministically selected.base/server/upgrade/11.10.0/02-UpdateKeyParameters.py (3)
63-119: 💤 Low valueRewrite normalizes line endings and may add a trailing newline.
content.splitlines()plus'\n'.join(out)collapses any CRLF in the source to LF, and theif has_trailing_nl or out:branch unconditionally appends\nwhenever the migrated content is non-empty even if the original file had no terminator. On Linux deployments this is harmless, but it does diff every migrated file beyond the intended keyParameters change. If you want the diff to be surgical, preserve the original line separator and only append the trailing newline when the input had one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py` around lines 63 - 119, The migration currently normalizes CRLF to LF and unconditionally adds a trailing '\n' when any output exists; fix by detecting and preserving the original line separator and trailing newline: determine the original separator (check for '\r\n' first, then '\r', default to '\n') from the raw content, use that separator instead of '\n' when joining out (replace '\n'.join(out) with sep.join(out)), and only append the trailing separator when has_trailing_nl is True (do not rely on "or out" to add a newline). Update the body construction near the end of the function in UpdateKeyParameters.py (where content, lines, has_trailing_nl, out, and body are used) so diffs are surgical and CRLFs are preserved.
88-110: ⚡ Quick winPre-existing
allowedKeys.<family>.<value>=falsesilently swallows the migrated token.
all_keys(built once at Line 77) contains every property key — including any pre-existingallowedKeys.X=false. At Lines 96-102 the script then skips emitting the new line because the key already exists, which is correct only if the existing entry is=true. If an admin hadallowedKeys.rsa.2048=falsetogether withkeyParameters=2048,4096, the upgrade will quietly drop 2048 instead of either keeping the explicitfalse(current behavior) or warning the operator. Consider logging at WARN when the conflicting existing value isfalse, so admins can audit the outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py` around lines 88 - 110, The loop in classify_key_parameter_token handling skips emitting ak_key if it already exists in all_keys but doesn't detect when the existing entry is explicitly "=false"; modify the logic around ak_key (in the block that builds out and uses all_keys, classify_key_parameter_token, out, logger) to check the original properties map (the parsed properties dict used to populate all_keys) for the existing ak_key value and, if present and equal to "false", emit a logger.warning (or logger.warn) noting the conflict (include os.path.basename(path), k, ak_key and the existing "false" value) so operators are alerted, while preserving the existing false (i.e., do not overwrite the key); otherwise keep the current skip behavior for true/existing entries.
19-31: ⚖️ Poor tradeoffToken classification ignores sibling
keyType, risking misclassification of ambiguous tokens in custom profiles.
classify_key_parameter_tokendetermines token family (mldsa/rsa/ec) by token shape alone. Since ML-DSA sizes (44,65,87) are valid integers, a hypothetical RSA profile withkeyParameters=44,65,87would incorrectly map toallowedKeys.mldsa.*rather thanallowedKeys.rsa.*. The siblingkeyTypeconstraint is accessible via thebaseprefix within_build_rewritten_profile_content, so the function could be restructured to accept akeyTypeparameter and use it to disambiguate integer tokens, making the migration robust against custom profiles. Currently shipped and tested profiles don't mix MLDSA-sized tokens with non-MLDSA keyTypes, so this is a latent edge case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py` around lines 19 - 31, classify_key_parameter_token currently infers family solely from token shape and misclassifies numeric MLDSA sizes as RSA; change its signature to accept a keyType parameter and use it to disambiguate: if keyType == 'mldsa' prefer MLDSA_SIZES, if keyType == 'rsa' treat digits as rsa, otherwise fall back to existing shape-based logic (use MLDSA_SIZES, 'rsa', 'ec' families as before). Update the caller in _build_rewritten_profile_content to pass the sibling base['keyType'] (or equivalent keyType value) when invoking classify_key_parameter_token so token classification respects the profile's keyType. Ensure MLDSA_SIZES remains used for explicit ML-DSA matching and preserve prior behavior for empty/striped tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg`:
- Around line 32-38: The EC allowed-keys list in
policyset.auditSigningCertSet.3.constraint.params.allowedKeys currently includes
nistp256 and nistp521 but omits nistp384; update
policyset.auditSigningCertSet.3.constraint.params.allowedKeys.ec to include
nistp384 (match entries like
policyset.auditSigningCertSet.3.constraint.params.allowedKeys.ec.nistp384=true
used in other profiles) for consistency with caCMCocspCert.cfg/caOCSPCert.cfg,
or if the omission is intentional add a clear comment next to
policyset.auditSigningCertSet.3.constraint.params.keyType or the EC allowedKeys
block explaining the rationale for excluding nistp384.
In `@base/ca/shared/profiles/ca/caCMCocspCert.cfg`:
- Line 33: The policy entry
policyset.ocspCertSet.3.constraint.params.allowedKeys.rsa.1024=true enables
1024-bit RSA; confirm whether this is an intentional backward-compatibility
exception and, if not, change the setting to false (or remove it) across all
affected profiles; if it is intentional, add a documented override and
deprecation timeline in the CA policy docs and add a comment/flag in each
profile (e.g., referencing policyset.ocspCertSet.* and
constraint.params.allowedKeys.rsa.1024) so the exception is explicit and
includes a planned phase-out date and migration steps.
In `@base/ca/shared/profiles/ca/caInstallCACert.cfg`:
- Around line 40-42: KeyConstraint.validateAlgorithmName() currently allows
"ML-DSA-\\d+" but not the "MLDSA" token emitted by mapAllowedKeys() (prefix
"mldsa"), so update validateAlgorithmName() to accept MLDSA forms (e.g., allow
"MLDSA" and "ML-DSA-<digits>" or use a single regex like ^ML-?DSA(-\\d+)?$) so
entries under allowedKeys.mldsa.* are recognized; adjust the validation logic in
KeyConstraint.validateAlgorithmName() accordingly and add/update unit tests that
exercise mapAllowedKeys() → KeyConstraint to ensure MLDSA entries are accepted.
In `@base/ca/shared/profiles/ca/caOtherCert.cfg`:
- Line 33: Remove or disable the weak RSA 1024 entry by changing
policyset.otherCertSet.3.constraint.params.allowedKeys.rsa.1024 from true to
false (or delete the key) so RSA 1024 is no longer allowed; apply the same edit
to any other migrated profile files in this PR that still set
allowedKeys.rsa.1024=true to keep policies consistent.
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 209-218: The alphabetical sort on the trimmed key types
(keyTypesTrimmed.sort(...)) changes the original insertion order and thus flips
the default selected option; remove the sort and instead trim and dedupe while
preserving the original order: iterate over keyTypesRet, trim tokens into
keyTypesTrimmed only if non-empty and not already present (preserving the first
occurrence) so the first key listed in getKeyTypesOptionsForKeyGen() remains the
default; alternatively, if you want a deliberate bias (e.g., keep "RSA" as
default), implement explicit reordering after dedupe to move "RSA" to index 0
and document that behavior.
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 538-551: The current guard only parses and validates the leaf when
value.trim().equalsIgnoreCase("true"), letting malformed keys like
allowedKeys.foo.bar=false slip through; update the logic for
CONFIG_ALLOWED_KEYS_PREFIX so that whenever name != null &&
name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) you always parse the leaf (extract
leaf via name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()), split into parts,
verify parts length and non-empty and throw EPropertyException on bad format)
and call validateAlgorithmName(algType) and validateKeyParams(name, algType,
keyValue); then separately normalize/validate the value (accept only boolean
strings "true"/"false" or throw a validation exception) so non-boolean values
are rejected rather than bypassing leaf/name validation.
- Around line 134-164: mapAllowedKeys currently swallows ERejectException in its
broad catch, letting malformed allowedKeys entries pass silently; update the
outer catch in mapAllowedKeys to rethrow ERejectException (or narrow the catch)
so configuration validation fails loudly: e.g., in mapAllowedKeys's catch
(Exception e) check if (e instanceof ERejectException) throw (ERejectException)
e; otherwise keep the warning log for other exceptions (or replace the broad
Exception catch with specific exceptions like EBaseException). Ensure the unique
symbols referenced are mapAllowedKeys and ERejectException so the thrown
validation errors are not suppressed.
- Around line 324-336: In validate(), the empty-string descriptor default for
getConfig(CONFIG_KEY_PARAMETERS) produces keyParams == [""], causing spurious
deprecation/error handling; fix by normalizing the raw value first (String raw =
getConfig(CONFIG_KEY_PARAMETERS)), treat raw as empty when raw == null ||
raw.trim().isEmpty(), otherwise split raw on "," and trim/filter out empty
tokens to produce keyParams; keep the subsequent logic that checks
keyParams.length > 0 and compares with mapAllowedKeys()/allowedKeysMap so errors
are only raised when there are actual parameters.
- Around line 395-408: validateAlgorithmName currently rejects the uppercased
"MLDSA" token (causing allowedKeys.mldsa.* to fail) and contains a duplicated
alg.equals("DSA") branch; update validateAlgorithmName to explicitly accept
"MLDSA" (e.g., treat alg.equals("MLDSA") as valid) in addition to the existing
ML-DSA regex and RSA/DSA/EC checks, and remove the redundant second
alg.equals("DSA") branch so the method returns for RSA, DSA, EC, MLDSA, or
alg.matches("^ML-DSA-\\d+$") before throwing EPropertyException.
In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py`:
- Around line 122-162: The code in UpdateKeyParameters (see upgrade_subsystem
and _build_rewritten_profile_content) currently rewrites profiles and removes
params.keyParameters; to resolve the discrepancy, update the PR description and
any upgrade docs to state that the migration removes params.keyParameters from
instance profile files and that params.allowedKeys replaces it (allowedKeys
takes precedence), or alternatively change _build_rewritten_profile_content to
preserve the original keyParameters line and emit a deprecation warning if you
prefer to retain it—pick one approach and make the code/comments/docs consistent
with that choice.
---
Outside diff comments:
In `@base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg`:
- Line 86: The signingAlgsAllowed property
(policyset.transportCertSet.8.constraint.params.signingAlgsAllowed) contains a
stray double comma between SHA512withRSA and SHA256withEC; remove the extra
comma so the list is a valid comma-separated sequence (i.e., ensure only a
single comma separates SHA512withRSA and SHA256withEC).
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 528-579: setConfig currently validates allowedKeys.* leaves but
never persists them and uses mutable static strings (keyType, keyParams,
allowedKeys) which leak state; update setConfig in KeyConstraint so that when
name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) you call super.setConfig(name,
value) (after validation) and return early, and replace the allowedKeys string
accumulator with a boolean presence flag (or drop the static accumulator and
read the value from getConfigStore() when validateKeyParams needs cross-leaf
info); also ensure the presence flag (or no-static design) is reset alongside
keyType/keyParams after super.setConfig(CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS)
or eliminate the static fields entirely to avoid shared mutable state across
instances/threads.
---
Duplicate comments:
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 493-505: The code block that builds the select options wrongly
injects '<OPTION VALUE=44 SELECTED>44' when options.length == 0 and keyType ==
"MLDSA", ignoring overrides; change the logic in the options construction (the
branch handling keyType == "MLDSA" inside the if (options.length == 0) block) to
not unconditionally add 44 but instead emit a sentinel "no allowed parameter"
option (e.g. a disabled/empty OPTION or a special VALUE that your backend
recognizes) or leave options empty and set a flag indicating no allowed keys so
the UI/submit flow can handle rejection gracefully; update any codepaths that
expect a default selection (the options variable and any downstream form-submit
handling) to treat the sentinel as “no valid choice” rather than a real key.
- Around line 448-460: The MLDSA whitelist filter runs before
applyAllowedKeysOverrides so overrides can reintroduce disallowed values; change
the order so applyAllowedKeysOverrides(lengthsOrCurves, allowedKeysMap, keyType)
runs first, then perform the MLDSA filtering using MLDSA_VALID against
lengthsOrCurves when keyType == "MLDSA", and finally call
sortLengthsOrCurvesList(keyType, lengthsOrCurves); update references to
lengthsOrCurves, MLDSA_VALID, applyAllowedKeysOverrides, and
sortLengthsOrCurvesList accordingly.
---
Nitpick comments:
In `@base/ca/shared/profiles/ca/caFullCMCUserCert.cfg`:
- Around line 32-36: The property ordering is inconsistent: the keyType property
(policyset.cmcUserCertSet.3.constraint.params.keyType) should appear before the
allowedKeys entries
(policyset.cmcUserCertSet.3.constraint.params.allowedKeys.rsa.1024, .2048,
.3072, .4096) to match other profiles; move the keyType=RSA line so it precedes
the allowedKeys.rsa.* lines to maintain consistent ordering across profiles.
In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template`:
- Around line 313-359: The function applyAllowedKeysOverrides rebuilds the
allowed array inside the loop for each "false" override which is O(n²); instead,
when processing overrides (in applyAllowedKeysOverrides) mark additions with
inSet[keyValue]=true and removals with inSet[keyValue]=false and also maintain
an insertion-order list (reuse the existing allowed/order list when adding new
keys), then after the overrides loop do a single pass over that insertion-order
list to produce the final allowed array by including only entries where
inSet[name] is true; update references to allowed, inSet, and the overrides
handling in applyAllowedKeysOverrides accordingly to remove the inner rebuild
and perform one final emit pass.
- Around line 480-491: The current logic uses the array index check (l == 0) to
mark SELECTED which fails when the first element is filtered out; change it to
track the first included option instead: introduce a boolean flag (e.g.,
firstIncludedFound) initialized false before the loop and when you append an
option for an included item in the block guarded by "if (included)" check, if
firstIncludedFound is false add ' SELECTED' to options and set
firstIncludedFound=true; keep using translateCurveName and keyType logic as-is
and remove the l == 0 check so the first actually rendered option is
deterministically selected.
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 197-213: The Optional chains checking propertyValue in
KeyConstraint are verbose; replace them with a simple trimmed, null-safe string
comparison: compute a trimmed value (e.g., String v = propertyValue == null ?
null : propertyValue.trim()) and then use "true".equalsIgnoreCase(v) and
"false".equalsIgnoreCase(v) to decide allowed.add(keyValue) or
allowed.remove(keyValue), and keep the existing logger.warn branch for other
values; apply the same simplification to the other occurrence around line 539.
Ensure you reference the same variables/methods (propertyValue, keyValue,
allowed, logger.warn) in the refactor.
In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py`:
- Around line 63-119: The migration currently normalizes CRLF to LF and
unconditionally adds a trailing '\n' when any output exists; fix by detecting
and preserving the original line separator and trailing newline: determine the
original separator (check for '\r\n' first, then '\r', default to '\n') from the
raw content, use that separator instead of '\n' when joining out (replace
'\n'.join(out) with sep.join(out)), and only append the trailing separator when
has_trailing_nl is True (do not rely on "or out" to add a newline). Update the
body construction near the end of the function in UpdateKeyParameters.py (where
content, lines, has_trailing_nl, out, and body are used) so diffs are surgical
and CRLFs are preserved.
- Around line 88-110: The loop in classify_key_parameter_token handling skips
emitting ak_key if it already exists in all_keys but doesn't detect when the
existing entry is explicitly "=false"; modify the logic around ak_key (in the
block that builds out and uses all_keys, classify_key_parameter_token, out,
logger) to check the original properties map (the parsed properties dict used to
populate all_keys) for the existing ak_key value and, if present and equal to
"false", emit a logger.warning (or logger.warn) noting the conflict (include
os.path.basename(path), k, ak_key and the existing "false" value) so operators
are alerted, while preserving the existing false (i.e., do not overwrite the
key); otherwise keep the current skip behavior for true/existing entries.
- Around line 19-31: classify_key_parameter_token currently infers family solely
from token shape and misclassifies numeric MLDSA sizes as RSA; change its
signature to accept a keyType parameter and use it to disambiguate: if keyType
== 'mldsa' prefer MLDSA_SIZES, if keyType == 'rsa' treat digits as rsa,
otherwise fall back to existing shape-based logic (use MLDSA_SIZES, 'rsa', 'ec'
families as before). Update the caller in _build_rewritten_profile_content to
pass the sibling base['keyType'] (or equivalent keyType value) when invoking
classify_key_parameter_token so token classification respects the profile's
keyType. Ensure MLDSA_SIZES remains used for explicit ML-DSA matching and
preserve prior behavior for empty/striped tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5762ff73-e8b2-45a1-99d8-56cbbc0238f6
📒 Files selected for processing (93)
base/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
2 times, most recently
from
86da621 to
6638e95
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg (1)
34-37: ⚡ Quick winPrefer explicit
keyType=MLDSAwhen only MLDSA variants are enabledLine 34 remains
keyType=-while Line 35-Line 37 only enable MLDSA variants. SettingkeyType=MLDSAwould make the constraint intent unambiguous and align with the server profile pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg` around lines 34 - 37, The policy constraint currently leaves policyset.serverCertSet.3.constraint.params.keyType set to '-' while only MLDSA variants are enabled (policyset.serverCertSet.3.constraint.params.allowedKeys.mldsa.44/65/87=true); update keyType to 'MLDSA' to make intent explicit and consistent with the server profile pattern by replacing the '-' value for policyset.serverCertSet.3.constraint.params.keyType with MLDSA so the constraint clearly targets MLDSA keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg`:
- Around line 34-37: The policy constraint currently leaves
policyset.serverCertSet.3.constraint.params.keyType set to '-' while only MLDSA
variants are enabled
(policyset.serverCertSet.3.constraint.params.allowedKeys.mldsa.44/65/87=true);
update keyType to 'MLDSA' to make intent explicit and consistent with the server
profile pattern by replacing the '-' value for
policyset.serverCertSet.3.constraint.params.keyType with MLDSA so the constraint
clearly targets MLDSA keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 933c1f2b-db2c-4380-becc-4ca468e10429
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (53)
- .gitignore
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- base/ca/shared/profiles/ca/AdminCert.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caCACert.cfg
- base/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caCMCECUserCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caAdminCert.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caECDualCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caRAserverCert.cfg
- base/ca/shared/profiles/ca/caSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caStorageCert.cfg
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
🚧 Files skipped from review as they are similar to previous changes (31)
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caDirBasedDualCert.cfg
- base/ca/shared/profiles/ca/caCMCcaCert.cfg
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caSignedLogCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCMCocspCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template
- base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java
- base/server/upgrade/11.10.0/02-UpdateKeyParameters.py
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
6638e95 to
00cd4fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg`:
- Line 40: Remove the weak RSA 1024-bit allowance by deleting the configuration
key policyset.userCertSet.3.constraint.params.allowedKeys.rsa.1024=true from the
profile; ensure no other entries re-enable 1024-bit RSA and that the minimum
allowed RSA key size remains 2048 (e.g.,
policyset.userCertSet.3.constraint.params.allowedKeys.rsa.2048=true or
equivalent) so user certificates cannot be issued with 1024-bit keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa103054-7529-4211-94ad-19a0a567ccdc
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (51)
- .gitignore
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCert.cfg
- base/ca/shared/profiles/ca/caRARouterCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/server/src/main/resources/UserMessages.properties
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
🚧 Files skipped from review as they are similar to previous changes (34)
- base/ca/shared/profiles/ca/caCACert.cfg
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
- base/ca/shared/profiles/ca/caRAserverCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caECAdminCert.cfg
- base/ca/shared/profiles/ca/caECDualCert.cfg
- base/ca/shared/profiles/ca/caCMCocspCert.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/caDualCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/ca/shared/profiles/ca/caSignedLogCert.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caStorageCert.cfg
- base/server/upgrade/11.10.0/02-UpdateKeyParameters.py
- base/ca/shared/profiles/ca/acmeServerCert.cfg
- base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCMCcaCert.cfg
- base/ca/shared/profiles/ca/AdminCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfg
- base/ca/shared/profiles/ca/caSubsystemCert.cfg
- base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
523-574:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
setConfigforallowedKeys.*does not persist and overwriteskeyType/keyParameters.When
setConfig("allowedKeys.<alg>.<value>", "true")is invoked:
- Line 530 stashes the value in the static
allowedKeysfield.- Lines 533–546 validate the leaf shape but do not return.
- The early-return guards at lines 549 and 556 are keyed on
name.equals(CONFIG_KEY_TYPE/CONFIG_KEY_PARAMETERS)— neither matches anallowedKeys.*name.- Execution falls through to
validateKeyParams(name, keyType, keyParams)at line 565 and then tosuper.setConfig(CONFIG_KEY_TYPE, keyType)/super.setConfig(CONFIG_KEY_PARAMETERS, keyParams)at lines 568–569 with whatever the static fields currently hold (typically""). That blanks any previously storedkeyType/keyParameters, and the actualallowedKeys.<alg>.<value>leaf is never written viasuper.setConfig(name, value)— so the new flag is lost.There is also a follow-on hazard: the static
allowedKeysfield is set on line 530 but never reset to""(unlikekeyType/keyParamsat lines 572–573), sovalidateKeyParamsline 453 will subsequently seeallowedKeys.length() > 0and fire the “mixed configuration” error for the next legitimatekeyParameterswrite in the same JVM.🐛 Suggested fix sketch
if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) && Optional.ofNullable(value).map(String::trim).filter("true"::equalsIgnoreCase).isPresent() && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() ) { String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); String[] parts = leaf.split("\\.", 2); if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { throw new EPropertyException(CMS.getUserMessage("CMS_INVALID_PROPERTY", name)); } String algType = parts[0].toUpperCase(Locale.ROOT); validateAlgorithmName(algType); String keyValue = parts[1]; validateKeyParams(name, algType, keyValue); + // persist the leaf and don't fall through to the keyType/keyParameters writers + super.setConfig(name, value); + allowedKeys = ""; + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 523 - 574, setConfig mishandles names starting with CONFIG_ALLOWED_KEYS_PREFIX: it stores into the static allowedKeys but then falls through and clobbers keyType/keyParams and never persists the allowedKeys entry. Fix setConfig by treating allowedKeys.* as a separate branch: when name startsWith CONFIG_ALLOWED_KEYS_PREFIX and the leaf validates (use the existing validation logic that extracts algType and keyValue and calls validateAlgorithmName/validateKeyParams), call super.setConfig(name, value) to persist that specific leaf, reset allowedKeys = "" afterward, and then return early so the later keyType/keyParams logic (and the final super.setConfig(CONFIG_KEY_TYPE, ...) / super.setConfig(CONFIG_KEY_PARAMETERS, ...)) does not run for allowedKeys entries; ensure existing behavior for CONFIG_KEY_TYPE and CONFIG_KEY_PARAMETERS remains unchanged and continue to call validateKeyParams only for those specific names.
♻️ Duplicate comments (1)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
134-157:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
mapAllowedKeysstill swallows the validation exceptions it throws.The
throw new ERejectException(...)at line 145 and thevalidateAlgorithmName(algType)call at line 148 (which throwsEPropertyException) are both inside thetryblock, and the outercatch (Exception e)at line 153 catches them and downgrades them to alogger.warn. The result: a malformedallowedKeys.<bad>leaf or an unknown algorithm prefix is logged and then silently ignored —validate(...)proceeds with a partial allowlist instead of failing loudly.The previous review on this hunk was marked "Addressed", but the swallowing pattern is still present in the code under review. Either rethrow
ERejectException/EPropertyExceptionfrom the catch, or move the enumeration call out of the protective try and let the per-leaf validations propagate.🐛 Suggested fix
- try { - Map<String, String> properties = store.getProperties(); - for (String leaf : properties.keySet()) { - String[] parts = leaf.split("\\.", 2); - if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { - throw new ERejectException(CMS.getUserMessage("CMS_PROFILE_INVALID_CONFIGURATION_PARAM", leaf)); - } - String algType = parts[0].toUpperCase(Locale.ROOT); - validateAlgorithmName(algType); - String keyValue = parts[1]; - String propValue = properties.get(leaf); - result.computeIfAbsent(algType, k -> new HashMap<>()).put(keyValue, propValue); - } - } catch (Exception e) { - logger.warn("KeyConstraint: failed to enumerate allowedKeys: {}", e.getMessage()); - } + Map<String, String> properties; + try { + properties = store.getProperties(); + } catch (EBaseException e) { + logger.warn("KeyConstraint: failed to enumerate allowedKeys: {}", e.getMessage()); + return result; + } + for (Map.Entry<String, String> e : properties.entrySet()) { + String leaf = e.getKey(); + String[] parts = leaf.split("\\.", 2); + if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { + throw new ERejectException(CMS.getUserMessage("CMS_PROFILE_INVALID_CONFIGURATION_PARAM", leaf)); + } + String algType = parts[0].toUpperCase(Locale.ROOT); + try { + validateAlgorithmName(algType); + } catch (EPropertyException ex) { + throw new ERejectException(ex.getMessage()); + } + result.computeIfAbsent(algType, k -> new HashMap<>()).put(parts[1], e.getValue()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 134 - 157, mapAllowedKeys currently catches Exception around the whole enumeration, which swallows ERejectException and EPropertyException from validateAlgorithmName and the explicit throw, causing invalid config to be ignored; modify mapAllowedKeys so that only non-validation errors are caught (e.g., call getAllowedKeysStore() and store.getProperties() inside a try/catch, but perform the per-leaf parsing/validation (validateAlgorithmName and the explicit throw of ERejectException) outside that catch) or rethrow ERejectException/EPropertyException if caught in the existing catch block; reference method mapAllowedKeys, the validateAlgorithmName call, and the thrown ERejectException/EPropertyException so the fix ensures validation exceptions propagate instead of being logged and swallowed.
🧹 Nitpick comments (3)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
489-494: 💤 Low valueMLDSA branch silently logs but does not validate tokens.
In the legacy
keyParameterspath, the newMLDSAbranch only emits an info log per token without any validation, so e.g.keyType=MLDSA+keyParameters=foo,baris accepted byvalidateKeyParams. RSA/EC tokens still get validated. If the intent is "tokens are not used for ML-DSA validation" (per the comment), prefer rejecting non-{44,65,87}tokens (or non-numeric tokens) here so that misconfigurations surface at admin time instead of at request time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 489 - 494, The MLDSA branch currently only logs tokens; change it to validate each token in the same way RSA/EC branches do: inside the keyType.equals("MLDSA") block (in class KeyConstraint, method validateKeyParams), iterate params, trim and skip null/empty, verify each token is numeric (parse to int) and that its value is one of 44, 65 or 87; if any token is non-numeric or not in {44,65,87}, fail validation (throw or return false consistent with the method's error handling) and log a clear error message mentioning the invalid token and MLDSA context.base/server/upgrade/11.10.0/02-UpdateKeyParameters.py (2)
114-119: 💤 Low valueMinor: trailing newline is added even when the source file had none.
If
has_trailing_nlis False butoutis non-empty, line 115 still appends'\n'. That changes byte-for-byte content for any profile that lacked a trailing newline, which can show up as an extra noisy line in upgrade diffs. Consider tightening toif has_trailing_nl: body += '\n'to truly preserve the original file's terminator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py` around lines 114 - 119, The code currently appends a trailing '\n' whenever out is non-empty, which adds a newline even if the original file lacked one; change the condition that appends the terminator to check only has_trailing_nl (i.e., replace "if has_trailing_nl or out:" with "if has_trailing_nl:") so that body, out, has_trailing_nl and content logic preserves the original file terminator exactly (locate the snippet that sets body = '\n'.join(out) and the subsequent conditional that appends '\n').
19-31: 💤 Low valueHeuristic classification works for known profiles but is brittle.
classify_key_parameter_tokendecides the family purely from the token shape: any of{44,65,87}→mldsa, all-digit →rsa, else →ec. That happens to match every CSV in the current profile set, but it loses information about the originatingkeyType. If a future/legacy profile ever haskeyType=RSAwith a token literally44(or any other ML-DSA size string is reused as an RSA size in a vendored profile), the upgrade would misclassify it asmldsa.44=true.If you want to harden this, consider passing the sibling
constraint.params.keyTypeintoclassify_key_parameter_tokenas a tiebreaker (e.g. whenkeyType in {"RSA","EC","MLDSA"}route accordingly, fall back to the current heuristic only whenkeyTypeis-or unset). Not a blocker for the profiles being shipped in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py` around lines 19 - 31, classify_key_parameter_token currently infers family purely from token shape; change its signature to accept the sibling constraint.params.keyType and use keyType as the primary tiebreaker (when keyType is one of "RSA","EC","MLDSA" route to 'rsa','ec','mldsa' respectively), and only fall back to the existing heuristic (MLDSA_SIZES / isdigit / else) when keyType is missing, '-' or unrecognized; update all call sites to pass constraint.params.keyType so we preserve original behavior for unset keyType but avoid misclassification when keyType is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 70-74: The fields keyType, keyParams and allowedKeys are declared
static and accumulate mutable per-instance config across setConfig calls; either
make all three non-static instance fields on class KeyConstraint so each profile
instance has its own state, or at minimum ensure allowedKeys is cleared wherever
keyType/keyParams are reset (the same reset path used for keyType/keyParams in
setConfig). Update setConfig and any reset logic to operate on the instance
fields (or include allowedKeys = "" alongside the existing resets) and remove
static usage to eliminate cross-instance races flagged by SonarCloud.
---
Outside diff comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 523-574: setConfig mishandles names starting with
CONFIG_ALLOWED_KEYS_PREFIX: it stores into the static allowedKeys but then falls
through and clobbers keyType/keyParams and never persists the allowedKeys entry.
Fix setConfig by treating allowedKeys.* as a separate branch: when name
startsWith CONFIG_ALLOWED_KEYS_PREFIX and the leaf validates (use the existing
validation logic that extracts algType and keyValue and calls
validateAlgorithmName/validateKeyParams), call super.setConfig(name, value) to
persist that specific leaf, reset allowedKeys = "" afterward, and then return
early so the later keyType/keyParams logic (and the final
super.setConfig(CONFIG_KEY_TYPE, ...) / super.setConfig(CONFIG_KEY_PARAMETERS,
...)) does not run for allowedKeys entries; ensure existing behavior for
CONFIG_KEY_TYPE and CONFIG_KEY_PARAMETERS remains unchanged and continue to call
validateKeyParams only for those specific names.
---
Duplicate comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 134-157: mapAllowedKeys currently catches Exception around the
whole enumeration, which swallows ERejectException and EPropertyException from
validateAlgorithmName and the explicit throw, causing invalid config to be
ignored; modify mapAllowedKeys so that only non-validation errors are caught
(e.g., call getAllowedKeysStore() and store.getProperties() inside a try/catch,
but perform the per-leaf parsing/validation (validateAlgorithmName and the
explicit throw of ERejectException) outside that catch) or rethrow
ERejectException/EPropertyException if caught in the existing catch block;
reference method mapAllowedKeys, the validateAlgorithmName call, and the thrown
ERejectException/EPropertyException so the fix ensures validation exceptions
propagate instead of being logged and swallowed.
---
Nitpick comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 489-494: The MLDSA branch currently only logs tokens; change it to
validate each token in the same way RSA/EC branches do: inside the
keyType.equals("MLDSA") block (in class KeyConstraint, method
validateKeyParams), iterate params, trim and skip null/empty, verify each token
is numeric (parse to int) and that its value is one of 44, 65 or 87; if any
token is non-numeric or not in {44,65,87}, fail validation (throw or return
false consistent with the method's error handling) and log a clear error message
mentioning the invalid token and MLDSA context.
In `@base/server/upgrade/11.10.0/02-UpdateKeyParameters.py`:
- Around line 114-119: The code currently appends a trailing '\n' whenever out
is non-empty, which adds a newline even if the original file lacked one; change
the condition that appends the terminator to check only has_trailing_nl (i.e.,
replace "if has_trailing_nl or out:" with "if has_trailing_nl:") so that body,
out, has_trailing_nl and content logic preserves the original file terminator
exactly (locate the snippet that sets body = '\n'.join(out) and the subsequent
conditional that appends '\n').
- Around line 19-31: classify_key_parameter_token currently infers family purely
from token shape; change its signature to accept the sibling
constraint.params.keyType and use keyType as the primary tiebreaker (when
keyType is one of "RSA","EC","MLDSA" route to 'rsa','ec','mldsa' respectively),
and only fall back to the existing heuristic (MLDSA_SIZES / isdigit / else) when
keyType is missing, '-' or unrecognized; update all call sites to pass
constraint.params.keyType so we preserve original behavior for unset keyType but
avoid misclassification when keyType is explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 782b81a0-7298-454a-8fbf-3a8e4d10a6a0
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (61)
- .gitignore
- base/ca/shared/profiles/ca/AdminCert.cfg
- base/ca/shared/profiles/ca/caAgentFileSigning.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caCACert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caECInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
- base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECUserCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caAdminCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caSubsystemCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caRARouterCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/ca/shared/profiles/ca/caDirBasedDualCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caRAserverCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
🚧 Files skipped from review as they are similar to previous changes (28)
- base/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfg
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- base/ca/shared/profiles/ca/acmeServerCert.cfg
- base/ca/shared/profiles/ca/caECAdminCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/caCMCcaCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caDualCert.cfg
- base/ca/shared/profiles/ca/caECDualCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
- base/ca/shared/profiles/ca/caCMCocspCert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/server/src/main/resources/UserMessages.properties
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template
- base/ca/shared/profiles/ca/caStorageCert.cfg
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
00cd4fc to
f03aa71
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (1)
134-157:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
mapAllowedKeysstill swallows its own validation exceptions.Although a previous review on this region was marked as addressed, the broad
catch (Exception e)at line 153 still surrounds the entire loop, which means:
- The
throw new ERejectException(...)at line 145 (malformed leaf) is caught and demoted to a warning.- The
EPropertyExceptionthrown byvalidateAlgorithmName(algType)at line 148 (e.g.allowedKeys.foo.bar) is also caught and demoted.Net effect: the method declares
throws ERejectException, but in practice it never propagates one — invalid/malformedallowedKeys.*leaves are silently accepted, and validation proceeds with a partial map. This is the same critical issue raised previously; please confirm whether it was meant to be fixed.🐛 Suggested narrowing of the catch
- try { - Map<String, String> properties = store.getProperties(); - for (String leaf : properties.keySet()) { - String[] parts = leaf.split("\\.", 2); - if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { - throw new ERejectException(CMS.getUserMessage("CMS_PROFILE_INVALID_CONFIGURATION_PARAM", leaf)); - } - String algType = parts[0].toUpperCase(Locale.ROOT); - validateAlgorithmName(algType); - String keyValue = parts[1]; - String propValue = properties.get(leaf); - result.computeIfAbsent(algType, k -> new HashMap<>()).put(keyValue, propValue); - } - } catch (Exception e) { - logger.warn("KeyConstraint: failed to enumerate allowedKeys: {}", e.getMessage()); - } + Map<String, String> properties; + try { + properties = store.getProperties(); + } catch (EBaseException e) { + logger.warn("KeyConstraint: failed to enumerate allowedKeys: {}", e.getMessage()); + return result; + } + for (String leaf : properties.keySet()) { + String[] parts = leaf.split("\\.", 2); + if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { + throw new ERejectException(CMS.getUserMessage("CMS_PROFILE_INVALID_CONFIGURATION_PARAM", leaf)); + } + String algType = parts[0].toUpperCase(Locale.ROOT); + try { + validateAlgorithmName(algType); + } catch (EPropertyException e) { + throw new ERejectException(e.getMessage()); + } + String keyValue = parts[1]; + result.computeIfAbsent(algType, k -> new HashMap<>()).put(keyValue, properties.get(leaf)); + } return result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 134 - 157, mapAllowedKeys currently wraps the entire parsing loop in a broad catch(Exception) which swallows ERejectException and EPropertyException from the malformed leaf and validateAlgorithmName calls; change the error handling so malformed/validation exceptions propagate: call getAllowedKeysStore() and then getProperties() inside a try that only catches IO/config-access related exceptions (or catch and rethrow non-validation exceptions), but do not catch Exception around the for-loop; ensure ERejectException and EPropertyException thrown by validateAlgorithmName(String algType) and the "throw new ERejectException(...)" remain uncaught so they propagate from mapAllowedKeys as declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 134-157: mapAllowedKeys currently wraps the entire parsing loop in
a broad catch(Exception) which swallows ERejectException and EPropertyException
from the malformed leaf and validateAlgorithmName calls; change the error
handling so malformed/validation exceptions propagate: call
getAllowedKeysStore() and then getProperties() inside a try that only catches
IO/config-access related exceptions (or catch and rethrow non-validation
exceptions), but do not catch Exception around the for-loop; ensure
ERejectException and EPropertyException thrown by validateAlgorithmName(String
algType) and the "throw new ERejectException(...)" remain uncaught so they
propagate from mapAllowedKeys as declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ae05356-fcd2-470b-9365-6153a36c023f
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (23)
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- .gitignore
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caRAserverCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/AdminCert.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/server/src/main/resources/UserMessages.properties
- base/ca/shared/profiles/ca/caSignedLogCert.cfg
🚧 Files skipped from review as they are similar to previous changes (65)
- base/ca/shared/profiles/ca/caCMCserverCert.cfg
- base/ca/shared/profiles/ca/caECAdminCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
- base/ca/shared/profiles/ca/caCMCocspCert.cfg
- base/ca/shared/profiles/ca/caAgentFileSigning.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caDirBasedDualCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caRARouterCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caDualCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caSubsystemCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caStorageCert.cfg
- base/ca/shared/profiles/ca/acmeServerCert.cfg
- base/ca/shared/profiles/ca/caCACert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECDualCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caAdminCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
f03aa71 to
20248e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/ca/shared/profiles/ca/caECUserCert.cfg`:
- Around line 40-42: The config uses keys like
policyset.userCertSet.3.constraint.params.allowedKeys.ec.nistp256 which don't
follow the parser's required allowedKeys.<KEY-TYPE>_<KEY-PARAM> format; update
those entries (e.g., the keys referenced under
policyset.userCertSet.3.constraint.params.allowedKeys) to use the underscore
naming convention (ec_nistp256, ec_nistp384, ec_nistp521) so the parser
recognizes the EC-curve restrictions now that keyParameters has been removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f103875f-090f-433c-b0d4-3411c4ce2f74
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (20)
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- .gitignore
- base/ca/shared/profiles/ca/caECDualCert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/acmeServerCert.cfg
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caSubsystemCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caSignedLogCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
🚧 Files skipped from review as they are similar to previous changes (71)
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caStorageCert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caAgentFileSigning.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caCACert.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caRARouterCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCert.cfg
- base/ca/shared/profiles/ca/caAdminCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
- base/ca/shared/profiles/ca/caCMCECUserCert.cfg
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/ca/shared/profiles/ca/caCMCcaCert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfg
- base/ca/shared/profiles/ca/AdminCert.cfg
- base/ca/shared/profiles/ca/caDualCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caECAdminCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfg
- base/ca/shared/profiles/ca/caDirBasedDualCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caCMCECserverCert.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/server/upgrade/11.10.0/02-UpdateKeyParameters.py
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/server/src/main/resources/UserMessages.properties
- base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java
edewata
reviewed
May 12, 2026
Comment on lines
+63
to
+66
| with open(path, 'r', encoding='utf-8') as f: | ||
| content = f.read() | ||
| has_trailing_nl = content.endswith('\n') | ||
| lines = content.splitlines() |
Contributor
There was a problem hiding this comment.
We actually have a pki.PropertyFile class that can parse profiles, but this is fine.
| lead = line[:len(line) - len(line.lstrip())] | ||
| base = k[:-len('keyParameters')] | ||
| value = (val or '').strip() | ||
| if value: |
Contributor
There was a problem hiding this comment.
To reduce indentations this line probably can be changed into:
if not value:
logger.info('%s: removed %s', os.path.basename(path), k)
# drop original keyParameters line
continue
for part in value.split(','): # no need to indent this line
...
Comment on lines
+98
to
+101
| policyset.serverCertSet.10.constraint.params.keyType=RSA | ||
| policyset.serverCertSet.10.constraint.params.keyParameters=2048,3072,4096 | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.2048=true | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.3072=true | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.4096=true |
Contributor
There was a problem hiding this comment.
I think the capitalization of the alg names should be consistent since these params are closely related. So if we have keyType=RSA we should have allowedKeys.RSA.2048=true too.
Member
There was a problem hiding this comment.
I am wondering what is the goal to have keyType and allow allowdKeys. They provide redundant information. Can it be dropped or I am missing some hidden features?
Contributor
Author
There was a problem hiding this comment.
The more I think about it the more it doesn't make sense anymore to keep keyType field. I will check the code and open the discussion if I don't find any reason for it to stay. I also don't think this should be done later, it makes more sense to me to deprecate them both in the same upgrade if that's where we are heading anyway.
fmarco76
reviewed
May 15, 2026
Member
fmarco76
left a comment
fmarco76
left a comment
There was a problem hiding this comment.
I have not reviewed the JS part yet. I am planning when the constraints is completed.
Comment on lines
+98
to
+101
| policyset.serverCertSet.10.constraint.params.keyType=RSA | ||
| policyset.serverCertSet.10.constraint.params.keyParameters=2048,3072,4096 | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.2048=true | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.3072=true | ||
| policyset.serverCertSet.10.constraint.params.allowedKeys.rsa.4096=true |
Member
There was a problem hiding this comment.
I am wondering what is the goal to have keyType and allow allowdKeys. They provide redundant information. Can it be dropped or I am missing some hidden features?
| validateAlgorithmName(algType); | ||
| String keyValue = parts[1]; | ||
| String propValue = properties.get(leaf); | ||
| result.computeIfAbsent(algType, k -> new HashMap<>()).put(keyValue, propValue); |
Member
There was a problem hiding this comment.
The propValue can be true or false. If false it should be skipped. Is there a reason to store it creating a multi level map?
Contributor
Author
There was a problem hiding this comment.
Since another value cannot arrive from another source anymore (we dropped mixed keyParameters and allowedKeys) this can definitely be simplified. Will do before next review. Even in case of multiple rows with same key but different value the last line will win, in this new format it isn't possible to specify both true and false for any alg-strength combination
| return allowed; | ||
| } | ||
| for (Map.Entry<String, String> entry : overridesByInnerKey.entrySet()) { | ||
| String keyValue = entry.getKey(); |
Member
There was a problem hiding this comment.
See comment related to the map building
| } | ||
| } | ||
|
|
||
| private void validateAlgorithmName(String algName) throws EPropertyException { |
Member
There was a problem hiding this comment.
This method need a refactory. Anyway, I am wondering if the validity check can be done against the supported algorithms as defined in JSS KeyPairAlgorithm.java.
Contributor
Author
There was a problem hiding this comment.
KeyPairAlgorithm.fromString seems to do exactly what we need. Can we imagine a future scenario where PKI/IPA needs to validate an alg not supported by JSS? Otherwise we definitely should use that directly. @fmarco76 @edewata @frasertweedale
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
20248e1 to
cc7126e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/ca/shared/profiles/ca/caStorageCert.cfg (1)
79-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the double comma in the signing algorithms list.
Line 79 contains a double comma (
,,) betweenSHA512withRSAandSHA256withEC, which creates an empty element in the comma-separated list and may cause parsing issues or unexpected behavior.🔧 Proposed fix
-policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87 +policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caStorageCert.cfg` at line 79, The signing algorithms list in the property policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed contains a double comma (between SHA512withRSA and SHA256withEC) which creates an empty list element; remove the extra comma so the comma-separated values are valid (ensure the sequence becomes ...SHA512withRSA,SHA256withEC,... in the policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed entry).
♻️ Duplicate comments (4)
base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (4)
72-74:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftShared static config state leaks across calls and can trigger false mixed-config failures.
keyType,keyParams, andallowedKeysare static mutable accumulators forsetConfig(). This state is shared across instances/calls, andallowedKeysis not reset with the same lifecycle as the others.Also applies to: 479-484, 617-619
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 72 - 74, keyType, keyParams and allowedKeys are declared as static mutable fields in KeyConstraint which lets state leak across instances/calls and causes mixed-config failures; change them so they are not shared static state—either make them instance fields on the KeyConstraint class or, better, make them local variables inside the setConfig(...) method and ensure allowedKeys is reset/initialized each invocation (also update other occurrences at the referenced locations). Update references to keyType, keyParams and allowedKeys in setConfig and related helpers so they use the new non-static variables (or the method-local variables) to eliminate cross-call state leakage.
566-579:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
allowedKeys.*=falsebypasses leaf format and algorithm validation.Line 567 gates parsing/validation on
"true"only. A malformed or unknown leaf with value"false"is accepted and persisted without validation.🐛 Proposed fix
- if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) - && Optional.ofNullable(value).map(String::trim).filter("true"::equalsIgnoreCase).isPresent() - && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() - ) { + if (name != null && name.startsWith(CONFIG_ALLOWED_KEYS_PREFIX) + && name.length() > CONFIG_ALLOWED_KEYS_PREFIX.length() + ) { String leaf = name.substring(CONFIG_ALLOWED_KEYS_PREFIX.length()); String[] parts = leaf.split("\\.", 2); if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { throw new EPropertyException(CMS.getUserMessage("CMS_INVALID_PROPERTY", name)); } String algType = parts[0].toUpperCase(Locale.ROOT); validateAlgorithmName(algType); String keyValue = parts[1]; validateKeyParams(name, algType, keyValue); + + String normalized = Optional.ofNullable(value).map(String::trim).orElse(""); + if (!normalized.equalsIgnoreCase("true") && !normalized.equalsIgnoreCase("false")) { + throw new EPropertyException(CMS.getUserMessage("CMS_INVALID_PROPERTY", name)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 566 - 579, The current conditional in KeyConstraint that only triggers parsing when the value equals "true" lets entries like allowedKeys.*=false bypass validation; change the guard so any non-null, non-empty value that is not the literal "false" (case-insensitive) causes parsing and validation. Update the if condition around CONFIG_ALLOWED_KEYS_PREFIX to use Optional.ofNullable(value).map(String::trim).filter(v -> !v.equalsIgnoreCase("false")).isPresent() (or equivalent) so that malformed leaves still reach validateAlgorithmName(...) and validateKeyParams(...).
336-341:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMixed-config error is masked by throwing the wrong exception type.
Line 340 throws
EPropertyException, but Line 404-Line 410 converts non-ERejectExceptionerrors into genericCMS_PROFILE_KEY_NOT_FOUND. The mixed-config cause gets hidden.🐛 Proposed fix
- throw new EPropertyException(CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_PARAMETERS)); + throw new ERejectException( + CMS.getUserMessage( + getLocale(request), + "CMS_MIXED_KEY_CONFIGURATION", + CONFIG_ALLOWED_KEYS_PARAM, + CONFIG_KEY_PARAMETERS));Also applies to: 404-410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 336 - 341, The mixed-config validation currently throws EPropertyException in KeyConstraint.validate which is later mapped to a generic CMS_PROFILE_KEY_NOT_FOUND; change the thrown type to ERejectException (with the same CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION", ... ) message) in the branch that logs "Invalid Configuration: can't mix ..." so the original cause is not masked, and similarly update any other places in KeyConstraint (around the non-ERejectException-to-CMS_PROFILE_KEY_NOT_FOUND conversion at the 404-410 area) to ensure mixed-config errors are raised as ERejectException rather than EPropertyException so they bypass the generic mapping.
140-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMalformed
allowedKeysentries are silently downgraded to warnings.
mapAllowedKeys()throwsERejectExceptionon invalid leaves (Line 145), but Line 153 catches allExceptionand returns a partial map. That accepts broken configuration instead of failing fast.🐛 Proposed fix
- try { + try { Map<String, String> properties = store.getProperties(); for (String leaf : properties.keySet()) { String[] parts = leaf.split("\\.", 2); if (parts.length != 2 || parts[0].isEmpty() || parts[1].isEmpty()) { throw new ERejectException(CMS.getUserMessage("CMS_PROFILE_INVALID_CONFIGURATION_PARAM", leaf)); } String algType = parts[0].toUpperCase(Locale.ROOT); validateAlgorithmName(algType); String keyValue = parts[1]; String propValue = properties.get(leaf); result.computeIfAbsent(algType, k -> new HashMap<>()).put(keyValue, propValue); } + } catch (ERejectException e) { + throw e; } catch (Exception e) { logger.warn("KeyConstraint: failed to enumerate allowedKeys: {}", e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 140 - 155, The code in mapAllowedKeys() is swallowing all exceptions and returning a partial result; update the error handling so malformed configuration triggers failure: do not catch and suppress ERejectException (rethrow it) and avoid a broad catch-all that masks parsing/validation errors — either remove the broad catch so exceptions propagate, or change it to explicitly catch ERejectException and rethrow (and only log/handle truly non-fatal exceptions). Reference: KeyConstraint.mapAllowedKeys, validateAlgorithmName, ERejectException, and logger.warn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@base/ca/shared/profiles/ca/caStorageCert.cfg`:
- Line 79: The signing algorithms list in the property
policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed contains a
double comma (between SHA512withRSA and SHA256withEC) which creates an empty
list element; remove the extra comma so the comma-separated values are valid
(ensure the sequence becomes ...SHA512withRSA,SHA256withEC,... in the
policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed entry).
---
Duplicate comments:
In
`@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`:
- Around line 72-74: keyType, keyParams and allowedKeys are declared as static
mutable fields in KeyConstraint which lets state leak across instances/calls and
causes mixed-config failures; change them so they are not shared static
state—either make them instance fields on the KeyConstraint class or, better,
make them local variables inside the setConfig(...) method and ensure
allowedKeys is reset/initialized each invocation (also update other occurrences
at the referenced locations). Update references to keyType, keyParams and
allowedKeys in setConfig and related helpers so they use the new non-static
variables (or the method-local variables) to eliminate cross-call state leakage.
- Around line 566-579: The current conditional in KeyConstraint that only
triggers parsing when the value equals "true" lets entries like
allowedKeys.*=false bypass validation; change the guard so any non-null,
non-empty value that is not the literal "false" (case-insensitive) causes
parsing and validation. Update the if condition around
CONFIG_ALLOWED_KEYS_PREFIX to use
Optional.ofNullable(value).map(String::trim).filter(v ->
!v.equalsIgnoreCase("false")).isPresent() (or equivalent) so that malformed
leaves still reach validateAlgorithmName(...) and validateKeyParams(...).
- Around line 336-341: The mixed-config validation currently throws
EPropertyException in KeyConstraint.validate which is later mapped to a generic
CMS_PROFILE_KEY_NOT_FOUND; change the thrown type to ERejectException (with the
same CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION", ... ) message) in the
branch that logs "Invalid Configuration: can't mix ..." so the original cause is
not masked, and similarly update any other places in KeyConstraint (around the
non-ERejectException-to-CMS_PROFILE_KEY_NOT_FOUND conversion at the 404-410
area) to ensure mixed-config errors are raised as ERejectException rather than
EPropertyException so they bypass the generic mapping.
- Around line 140-155: The code in mapAllowedKeys() is swallowing all exceptions
and returning a partial result; update the error handling so malformed
configuration triggers failure: do not catch and suppress ERejectException
(rethrow it) and avoid a broad catch-all that masks parsing/validation errors —
either remove the broad catch so exceptions propagate, or change it to
explicitly catch ERejectException and rethrow (and only log/handle truly
non-fatal exceptions). Reference: KeyConstraint.mapAllowedKeys,
validateAlgorithmName, ERejectException, and logger.warn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fdd9d90-383b-4cb9-8af2-52909ee64877
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- base/ca/shared/profiles/ca/caRAserverCert.cfg
- base/ca/shared/profiles/ca/caCMCECsubsystemCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caAgentServerCert.cfg
- base/ca/shared/profiles/ca/caMLDSAUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caUserCert.cfg
- base/ca/shared/profiles/ca/caUserSMIMEcapCert.cfg
🚧 Files skipped from review as they are similar to previous changes (72)
- base/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caCrossSignedCACert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/acmeServerCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caECAdminCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/caServerKeygen_UserCert.cfg
- base/ca/shared/profiles/ca/caCMCkraTransportCert.cfg
- base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg
- base/ca/shared/profiles/ca/caCMCserverCert.cfg
- base/ca/shared/profiles/ca/caTPSCert.cfg
- base/ca/shared/profiles/ca/caIPAserviceCert.cfg
- base/ca/shared/profiles/ca/caECAgentServerCert.cfg
- base/ca/shared/profiles/ca/caJarSigningCert.cfg
- base/ca/shared/profiles/ca/caMLDSAServerCert.cfg
- base/ca/shared/profiles/ca/caEncECUserCert.cfg
- base/ca/shared/profiles/ca/caCMCECUserCert.cfg
- base/ca/shared/profiles/ca/caRAagentCert.cfg
- base/ca/shared/profiles/ca/caCMCcaCert.cfg
- base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg
- base/ca/shared/profiles/ca/caSigningECUserCert.cfg
- base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg
- base/ca/shared/profiles/ca/caRARouterCert.cfg
- base/ca/shared/profiles/ca/caTransportCert.cfg
- base/ca/shared/profiles/ca/caDualRAuserCert.cfg
- base/ca/shared/profiles/ca/caSignedLogCert.cfg
- base/ca/shared/profiles/ca/caRACert.cfg
- base/ca/shared/profiles/ca/caFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthServerCert.cfg
- base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caSigningUserCert.cfg
- base/ca/shared/profiles/ca/caDualCert.cfg
- base/ca/shared/profiles/ca/caDirUserCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfg
- base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg
- base/ca/shared/profiles/ca/caServerCertWithSCT.cfg
- base/ca/shared/profiles/ca/caCMCECserverCert.cfg
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caInstallCACert.cfg
- base/ca/shared/profiles/ca/ECAdminCert.cfg
- base/ca/shared/profiles/ca/estServiceCert.cfg
- base/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECServerCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg
- base/ca/shared/profiles/ca/caDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caECFullCMCUserCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caServerCert.cfg
- base/ca/shared/profiles/ca/caCMCUserCert.cfg
- base/ca/shared/profiles/ca/caAgentFileSigning.cfg
- base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caOtherCert.cfg
- base/ca/shared/profiles/ca/caMLDSAAdminCert.cfg
- base/ca/shared/profiles/ca/caAdminCert.cfg
- base/ca/shared/profiles/ca/caRouterCert.cfg
- base/ca/shared/profiles/ca/caDirBasedDualCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg
- base/ca/shared/profiles/ca/caEncUserCert.cfg
- base/ca/shared/profiles/ca/caOCSPCert.cfg
- base/ca/shared/profiles/ca/caAuditSigningCert.cfg
- base/server/src/main/resources/UserMessages.properties
- base/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfg
- base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg
- base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg
- base/ca/shared/profiles/ca/caCMCocspCert.cfg
- base/server/upgrade/11.10.0/02-UpdateKeyParameters.py
- base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template
Enabled keys should now be enabled or disabled using params.allowedKeys.<alg>.<strength>=true/false. params.keyParameters is still considered valid for backword compatibility but is considered deprecated, mixing keyParameters and allowedKeys will result in an error. Includes an upgrade script. Assisted by Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
agaragna77
force-pushed
the
KeyConstraintRefactoring
branch
from
cc7126e to
3cb5c4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg (1)
86-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the double comma in the signing algorithms list.
Line 86 contains a double comma (
SHA512withRSA,,SHA256withEC) which creates an empty element in the allowed signing algorithms list and could cause parsing issues or validation errors.🔧 Proposed fix
-policyset.transportCertSet.8.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87 +policyset.transportCertSet.8.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caInternalAuthTransportCert.cfg` at line 86, The property policyset.transportCertSet.8.constraint.params.signingAlgsAllowed contains a stray double comma creating an empty element; edit that value to remove the extra comma between SHA512withRSA and SHA256withEC so the comma-separated list has no empty entries (ensure the final value is a single valid list of algorithms under signingAlgsAllowed).base/ca/shared/profiles/ca/caStorageCert.cfg (1)
79-79:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix double comma syntax error in signingAlgsAllowed.
Line 79 contains a double comma (
SHA512withRSA,,SHA256withEC) which will cause parsing errors or unexpected behavior.🐛 Proposed fix
-policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87 +policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed=SHA256withRSA,SHA512withRSA,SHA256withEC,SHA384withRSA,SHA384withEC,SHA512withEC,SHA256withRSA/PSS,SHA384withRSA/PSS,SHA512withRSA/PSS,ML-DSA-44,ML-DSA-65,ML-DSA-87🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caStorageCert.cfg` at line 79, The property policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed contains a stray double comma between SHA512withRSA and SHA256withEC causing a syntax/parsing error; edit that value to remove the extra comma so the algorithms list is a single comma-separated sequence (and scan the same property for any other accidental empty entries) — locate the key policyset.drmStorageCertSet.9.constraint.params.signingAlgsAllowed and replace the double-comma with a single comma.
♻️ Duplicate comments (5)
base/ca/shared/profiles/ca/caOtherCert.cfg (1)
33-33:⚠️ Potential issue | 🟠 MajorRSA 1024 remains enabled in this profile.
The past review comment regarding the weak RSA 1024 key size is still applicable. RSA 1024 is cryptographically deprecated and should be disabled in this actively updated profile unless there is a documented compatibility requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caOtherCert.cfg` at line 33, The profile still enables RSA 1024 via the setting policyset.otherCertSet.3.constraint.params.allowedKeys.RSA.1024=true; change this entry to explicitly disable 1024-bit RSA (set to false or remove the key) so the profile no longer permits RSA 1024, and ensure any related acceptance tests or documentation referencing policyset.otherCertSet.3 reflect the removal/false setting to avoid reintroducing weak keys.base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java (2)
526-528:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the staged
allowedKeysstate after eachsetConfig()round.Line 527 stores the new value in a static staging field, but this method still only clears
keyTypeandkeyParamslater. After the firstallowedKeys.*update, a latersetConfig(keyType|keyParameters, ...)on any profile can trip the mixed-config guard with stale state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 526 - 528, The setter for configuration currently stashes allowedKeys into the static staging field when name startsWith CONFIG_ALLOWED_KEYS_PREFIX but does not clear it later, which can cause stale allowedKeys to trigger mixed-config guards on subsequent setConfig(keyType or keyParameters) calls; modify setConfig() to clear the staged allowedKeys field (the static allowedKeys variable) at the same point where keyType and keyParams are cleared so that after each setConfig() round the staged state (allowedKeys, keyType, keyParams) is reset and cannot leak into later operations.
144-164:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't swallow malformed
allowedKeysconfiguration.Lines 148-152 deliberately reject bad leaves, but the broad catch at Line 162 converts those validation failures into a warning and returns a partial/empty allow-map. That lets an invalid profile keep running with silently degraded key checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java` around lines 144 - 164, The current catch-all in KeyConstraint swallows validation failures (ERejectException) when parsing allowedKeys; update the exception handling so malformed entries are not converted into mere warnings: in the method that parses store.getProperties() (inside class KeyConstraint) either remove the broad catch or change it to selectively rethrow ERejectException (e.g., if (e instanceof ERejectException) throw (ERejectException)e; else log the other exceptions), ensuring the method signature permits throwing ERejectException if needed; this preserves the existing logger.warn for unexpected errors but ensures malformed configuration validation failures are propagated.base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template (2)
209-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the profile-defined key-type order.
Sorting here changes which
<OPTION>is first and therefore selected by default. For the commonkeyType="-"path, this turns the oldRSA,ECdefault intoEC,RSA, silently changing the browser-side default key type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 209 - 218, The code is reordering profile-defined key types by sorting keyTypesRet into keyTypesTrimmed, which changes the default selected <OPTION>; remove the sort so the original order is preserved: stop calling keyTypesTrimmed.sort(...) and assign keyTypesRet to the trimmed array without sorting (affecting variables keyTypesRet, keyTypesTrimmed and the trimming loop in ProfileSelect.template).
493-495:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't reintroduce ML-DSA 44 when the effective allowlist is empty.
If filtering or overrides remove every ML-DSA parameter, this fallback still renders
44, so the UI can offer a choice that the backend rejects. Please only fall back when44is actually allowed, or render a disabled placeholder instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/webapps/ca/ee/ca/ProfileSelect.template` around lines 493 - 495, The fallback that sets options = '<OPTION VALUE=44 SELECTED>44' when options.length == 0 and keyType == "MLDSA" must be changed to first check whether 44 is actually present in the effective allowlist (the same filter/override logic that produced options); only insert the 44 option if that check confirms it's allowed, otherwise render a disabled placeholder option (e.g. '<OPTION DISABLED SELECTED>—</OPTION>') so the UI doesn't offer a choice the backend will reject; update the logic in ProfileSelect.template around the options / keyType handling to perform this allowlist check instead of unconditionally injecting VALUE=44.
🟠 Major comments (29)
base/ca/shared/profiles/ca/caAdminCert.cfg-35-38 (1)
35-38:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove keyParameters entirely—this is a breaking change despite PR objectives stating backward compatibility.
The configuration files show
keyParametershas been completely removed from profile definitions, yet the PR description states "Maintains params.keyParameters for backward compatibility; the new params.allowedKeys fields take precedence." Verification of KeyConstraint.java confirms the code cannot maintain both formats simultaneously—it explicitly rejects mixing old and new parameters with an error (lines 282–283 in KeyConstraint.java:"Invalid Configuration: can't mix {} and {}/{}). This forces a hard migration for deployments relying onkeyParameters, contradicting the stated backward compatibility guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caAdminCert.cfg` around lines 35 - 38, The PR removes keyParameters from profile configs (e.g., policyset.adminCertSet.* entries) but KeyConstraint.java currently forbids mixing old `params.keyParameters` with new `params.allowedKeys` and therefore breaks backward compatibility; restore true backward compatibility by updating KeyConstraint.java to accept both formats: change the validation that throws on mixing to allow coexistence (or prefer `params.allowedKeys` when both present) and ensure parsing logic still reads `keyParameters` into the same internal representation used by the new `allowedKeys` path; reference KeyConstraint.java and the config keys `params.keyParameters` and `params.allowedKeys` when making the change.base/ca/shared/profiles/ca/caSignedLogCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsider removing RSA 1024-bit key support.
RSA 1024-bit keys are permitted in this deprecated audit log signing profile. Since 1024-bit RSA has been cryptographically weak since 2013, and the profile description notes it is deprecated in favor of
caAuditSigningCert, either remove the 1024-bit option or document the legacy compatibility requirement.🔒 Suggested fix
-policyset.caLogSigningSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.caLogSigningSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caSignedLogCert.cfg` at line 33, The profile currently enables RSA 1024-bit keys via policyset.caLogSigningSet.3.constraint.params.allowedKeys.RSA.1024=true; remove support by deleting that parameter or set it to false, and update the profile header/description to note the deprecated legacy compatibility requirement and point to caAuditSigningCert as the recommended alternative so reviewers can see why 1024 is disabled.base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsider removing RSA 1024-bit key support.
RSA 1024-bit keys are allowed for CMC audit signing certificates. This key size has been deprecated for over a decade and poses a security risk. Remove this option unless there is a documented requirement for legacy system compatibility.
🔒 Suggested fix
-policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caCMCauditSigningCert.cfg` at line 33, The configuration currently enables RSA 1024-bit keys via the setting policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.1024=true; remove this allowance (delete the setting or change it to false) so 1024-bit RSA is no longer permitted for CMC audit signing certificates, leaving other allowedKeys entries intact and adding a comment if legacy compatibility justification is required.base/ca/shared/profiles/ca/caJarSigningCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsider removing RSA 1024-bit key support.
Line 33 allows RSA 1024-bit keys, which have been deprecated since 2013 and are cryptographically weak by modern standards. Unless legacy compatibility is explicitly required, remove
allowedKeys.RSA.1024=trueto enforce stronger key sizes. If 1024-bit support must remain for backward compatibility, add a comment documenting the rationale and security trade-off.🔒 Suggested fix to remove weak key size
-policyset.caJarSigningSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.caJarSigningSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caJarSigningCert.cfg` at line 33, Remove the weak 1024-bit RSA allowance by deleting the policy entry policyset.caJarSigningSet.3.constraint.params.allowedKeys.RSA.1024=true (which currently enables RSA-1024) so only stronger sizes are permitted; if you must preserve RSA-1024 for legacy reasons, instead leave the entry but add a clear comment next to policyset.caJarSigningSet.3.constraint.params.allowedKeys.RSA.1024=true explaining the required backward-compatibility rationale and the known security trade-offs.base/ca/shared/profiles/ca/caTPSCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsider removing RSA 1024-bit key support.
RSA 1024-bit keys are allowed for TPS server certificates, which have been cryptographically deprecated since 2013. Modern security standards require at least 2048-bit RSA keys. Unless backward compatibility with legacy TPS systems is specifically required, remove this weak key size option.
🔒 Suggested fix
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caTPSCert.cfg` at line 33, The configuration currently enables weak RSA 1024-bit keys via the setting policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; remove or disable this entry and ensure only RSA key sizes >=2048 are allowed (e.g., set policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=false or delete the line and verify policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048 is present and true) so TPS server certificates no longer permit 1024-bit RSA keys.base/ca/shared/profiles/ca/caRAserverCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. RA server certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caRAserverCert.cfg` at line 33, The configuration currently enables RSA 1024-bit keys via policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; change this to disallow 1024-bit keys and enforce a minimum of 2048 bits by setting the 1024 entry to false (or removing it) and ensuring the 2048 entry (policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048) is present and set to true so RA server certificates require ≥2048-bit RSA keys.base/ca/shared/profiles/ca/caIPAserviceCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. IPA service certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caIPAserviceCert.cfg` at line 33, The policy currently enables RSA 1024-bit keys via policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; change this to disallow 1024-bit keys (set the 1024 entry to false or remove it) and ensure the policy allows at least 2048-bit keys by enabling policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true (or the equivalent 4096 entry) so the IPA service certificate enforces a minimum RSA key size of ≥2048 bits.base/ca/shared/profiles/ca/caSubsystemCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. Subsystem certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caSubsystemCert.cfg` at line 33, The policy currently allows RSA 1024-bit keys via policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true which is weak; change the policy to disallow 1024-bit keys (set that flag to false or remove it) and ensure RSA 2048 (and larger) is explicitly allowed (e.g., policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true) so subsystem certificates enforce a minimum of 2048-bit RSA; update any related allowedKeys entries for policyset.serverCertSet.3 to reflect this minimum.base/ca/shared/profiles/ca/caRARouterCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. RA router certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caRARouterCert.cfg` at line 33, The config currently enables RSA 1024 via policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; update the policy to disallow 1024-bit keys and require a stronger minimum (set RSA.1024 to false/remove it and ensure RSA.2048 (and larger) are enabled) so RA router certificates enforce ≥2048-bit RSA; specifically modify the entries related to policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.* to remove/disable the 1024 flag and enable 2048+ keys.base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg-27-27 (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and have been deprecated by NIST since 2013. Device certificates with 1024-bit RSA keys are vulnerable to factorization attacks. Consider removing this option to enforce stronger key sizes (≥2048 bits).
🔒 Suggested change to remove RSA 1024-bit support
-policyset.cmcDeviceCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.cmcDeviceCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/estFullcmcDeviceCert.cfg` at line 27, The policy line policyset.cmcDeviceCertSet.3.constraint.params.allowedKeys.RSA.1024=true should be removed or set to false to disallow weak 1024-bit RSA keys; update the policy entry for allowedKeys.RSA to only permit stronger sizes (e.g., ensure entries like allowedKeys.RSA.2048=true and allowedKeys.RSA.3072=true exist) and remove or disable the RSA.1024 entry so device certificates require ≥2048-bit RSA keys.base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg-31-31 (1)
31-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. User certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caSimpleCMCUserCert.cfg` at line 31, The policy currently enables RSA 1024-bit keys via the setting policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024=true; disable or remove that entry and ensure the policy requires a minimum of 2048-bit RSA keys (e.g., set the 1024 flag to false and enable/confirm policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.2048=true or equivalent) so user certificates cannot be issued with RSA keys smaller than 2048 bits.base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider allowing RSA 1024-bit keys.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013. CMC subsystem certificates should enforce stronger minimum key sizes (≥2048 bits).
🔒 Suggested change
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caCMCsubsystemCert.cfg` at line 33, The config currently enables RSA 1024-bit keys via policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true which is insecure; change the policy to disallow 1024-bit RSA (set that flag to false or remove it) and ensure the minimum allowed RSA key size is ≥2048 (verify policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048 is enabled and any smaller sizes are disabled) so CMC subsystem certificates require at least 2048-bit RSA keys.base/ca/shared/profiles/ca/caServerCertWithSCT.cfg-33-36 (1)
33-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk, especially for server certificates. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caServerCertWithSCT.cfg` around lines 33 - 36, The config permits weak RSA 1024-bit keys via the setting policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; remove or disable that option by deleting the 1024 entry or setting it to false so only secure sizes (e.g., 2048, 3072, 4096) remain allowed; update any related documentation/comments referencing allowed RSA sizes and run any validation or tests that check policyset.serverCertSet.3 to ensure the change is applied.base/ca/shared/profiles/ca/caDirPinUserCert.cfg-38-41 (1)
38-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.userCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.userCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.userCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caDirPinUserCert.cfg` around lines 38 - 41, Remove the weak RSA 1024-bit allowance by deleting or disabling the policy entry policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024 (so only 2048/3072/4096 remain allowed); update the policy definition where policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024 is set to true and ensure it is either removed or set to false to enforce stronger RSA key sizes.base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg-29-32 (1)
29-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfg` around lines 29 - 32, Remove the weak RSA 1024-bit allowance by deleting or disabling the policy key "policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024" (or set it to false) so only strong RSA sizes remain (2048, 3072, 4096) in the allowedKeys list; update any related documentation or tests that reference "policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024" to reflect its removal.base/ca/shared/profiles/ca/caDirBasedDualCert.cfg-119-122 (1)
119-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caDirBasedDualCert.cfg` around lines 119 - 122, Remove the weak RSA 1024 allowance by deleting or disabling the policy entry policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.1024 (or set it to false) so only strong key sizes remain (e.g., 2048, 3072, 4096) in the signingCertSet policy; update any related documentation/comments referencing RSA 1024 to avoid reintroduction.base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg-33-36 (1)
33-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk for server certificates. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caServerCertWithCRLDP.cfg` around lines 33 - 36, Remove the weak RSA 1024-bit allowance by updating the policy key entry policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024 (either delete the line or set it to false) so only strong key sizes remain (2048, 3072, 4096) under policyset.serverCertSet.3.constraint.params.allowedKeys.RSA; ensure no other references enable 1024-bit keys for serverCertSet.3.base/ca/shared/profiles/ca/caDirBasedDualCert.cfg-34-37 (1)
34-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caDirBasedDualCert.cfg` around lines 34 - 37, Remove the weak RSA 1024-bit entry from the allowed keys by deleting the policy key policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.1024 or disabling it (set to false) so only stronger sizes remain (2048, 3072, 4096) in policyset.encryptionCertSet.3; update any tests or documentation that enumerate allowed RSA sizes if present.base/ca/shared/profiles/ca/caServerCert.cfg-33-36 (1)
33-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk for server certificates. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.4096=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caServerCert.cfg` around lines 33 - 36, The config currently allows RSA 1024-bit keys via the setting policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; remove that allowance (delete the RSA.1024 line or set it to false) so only RSA.2048/3072/4096 remain permitted, ensuring server certificates cannot use deprecated 1024-bit RSA keys; update policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024 accordingly and keep the other RSA size entries unchanged.base/ca/shared/profiles/ca/caAuditSigningCert.cfg-34-39 (1)
34-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSA 1024-bit keys are cryptographically weak and deprecated.
Including RSA 1024-bit keys in the allowed key list poses a security risk, particularly for audit signing certificates which are critical for security and compliance. RSA 1024-bit has been considered insecure since 2013 and is not recommended for new certificates. Consider removing this option to enforce stronger key sizes.
🔒 Proposed fix to remove weak key size
-policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.2048=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.3072=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.4096=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.EC.nistp256=true policyset.auditSigningCertSet.3.constraint.params.allowedKeys.EC.nistp521=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caAuditSigningCert.cfg` around lines 34 - 39, Remove the weak RSA 1024 allowance by changing or deleting the policy key policyset.auditSigningCertSet.3.constraint.params.allowedKeys.RSA.1024 (do not leave it set to true); update the config so only secure key sizes remain (e.g., keep RSA.2048/3072/4096 and EC.nistp256/nistp521) and run any configuration validation/tests that reference auditSigningCertSet to ensure no consumers expect RSA.1024.base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg-34-34 (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable RSA 1024 for OCSP signing certificates.
OCSP signing certificates authenticate certificate status responses. Allowing RSA 1024 creates a vulnerability where OCSP responses could be forged. Please disable this weak key size.
🔒 Proposed fix
-policyset.ocspCertSet.3.constraint.params.allowedKeys.RSA.1024=true +policyset.ocspCertSet.3.constraint.params.allowedKeys.RSA.1024=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfg` at line 34, The OCSP signing config currently allows RSA 1024 via the setting policyset.ocspCertSet.3.constraint.params.allowedKeys.RSA.1024=true; update this to disable 1024-bit RSA by changing that setting to false (policyset.ocspCertSet.3.constraint.params.allowedKeys.RSA.1024=false) so RSA 1024 is not permitted for OCSP signing certificates.base/ca/shared/profiles/ca/caRouterCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable RSA 1024 for router certificates.
Router certificates are used for network authentication and TLS. RSA 1024 is insufficient for these security-sensitive use cases and should be disabled.
🔒 Proposed fix
-policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true +policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caRouterCert.cfg` at line 33, The policy enables RSA 1024 for router certificates via the setting policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024=true; update this policy to disable RSA 1024 for router certificates by changing the value to false (or removing the key entirely) so that policyset.serverCertSet.3.constraint.params.allowedKeys.RSA.1024 no longer permits 1024-bit RSA.base/ca/shared/profiles/ca/caTransportCert.cfg-33-33 (1)
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable RSA 1024 for transport certificates.
Transport certificates protect key material for the Data Recovery Manager. RSA 1024 has been deprecated by NIST since 2013 and poses a factorization risk. Please set this to
falseto enforce stronger key requirements.🔒 Proposed fix
-policyset.transportCertSet.3.constraint.params.allowedKeys.RSA.1024=true +policyset.transportCertSet.3.constraint.params.allowedKeys.RSA.1024=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caTransportCert.cfg` at line 33, The transport certificate policy currently allows 1024-bit RSA via the setting policyset.transportCertSet.3.constraint.params.allowedKeys.RSA.1024=true; change this setting to false to disable RSA-1024 for transport certificates (i.e., set policyset.transportCertSet.3.constraint.params.allowedKeys.RSA.1024=false) so stronger key sizes are enforced for the Data Recovery Manager.base/ca/shared/profiles/ca/caCrossSignedCACert.cfg-30-30 (1)
30-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable RSA 1024 for CA certificates.
Even though this profile is currently disabled (enable=false), CA certificates form the root of trust and should never allow RSA 1024. Please disable this weak key size to prevent accidental use if the profile is enabled.
🔒 Proposed fix
-policyset.caCertSet.3.constraint.params.allowedKeys.RSA.1024=true +policyset.caCertSet.3.constraint.params.allowedKeys.RSA.1024=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caCrossSignedCACert.cfg` at line 30, The CA profile currently permits 1024-bit RSA via the setting policyset.caCertSet.3.constraint.params.allowedKeys.RSA.1024=true; change that to disable RSA-1024 (set the value to false or remove the entry) so CA certificates cannot use 1024-bit keys; update the policy key policyset.caCertSet.3.constraint.params.allowedKeys.RSA.1024 accordingly and ensure no other policyset.caCertSet.* entries re-enable 1024-bit RSA.base/ca/shared/profiles/ca/caUserCert.cfg-40-40 (1)
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable RSA 1024 for user certificates.
User certificates with RSA 1024 do not meet current cryptographic standards. Please disable this weak key size to enforce minimum 2048-bit RSA keys.
🔒 Proposed fix
-policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024=true +policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caUserCert.cfg` at line 40, Disable RSA 1024 for user certificates by updating the policy setting policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024 from true to false (or remove the entry) so that 1024-bit RSA keys are not allowed and the system enforces a minimum of 2048-bit RSA keys for user certificates.base/ca/shared/profiles/ca/caDualCert.cfg-34-37 (1)
34-37:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove cryptographically weak RSA 1024-bit key allowance.
RSA 1024-bit keys are cryptographically weak and deprecated by NIST since 2013 (SP 800-131A). Allowing 1024-bit keys in both encryption and signing certificate sets creates security vulnerabilities and compliance risks (PCI DSS, FIPS).
Remove lines 34 and 119 to prevent weak certificates from being issued.
🔒 Proposed fix
policyset.encryptionCertSet.3.constraint.params.keyType=RSA -policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.2048=truepolicyset.signingCertSet.3.constraint.params.keyType=RSA -policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.signingCertSet.3.constraint.params.allowedKeys.RSA.2048=trueAlso applies to: 119-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caDualCert.cfg` around lines 34 - 37, Remove the weak RSA 1024 allowance by deleting any occurrences of the property key ending in ".allowedKeys.RSA.1024=true" from the certificate policy sets (e.g., policyset.encryptionCertSet.* and policyset.signingCertSet.*); keep the stronger RSA entries (2048, 3072, 4096) intact so only RSA.2048 and larger are allowed for signing and encryption (look for keys like policyset.encryptionCertSet.3.constraint.params.allowedKeys.RSA.1024 and the equivalent policyset.signingCertSet.* entry and remove them).base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg-34-34 (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove cryptographically weak RSA 1024-bit key allowance.
RSA 1024-bit keys are deprecated by NIST (SP 800-131A) and create security vulnerabilities. Device certificates with weak keys are susceptible to factorization attacks and compliance failures.
Remove line 34 to enforce a minimum 2048-bit RSA key size.
🔒 Proposed fix
policyset.userCertSet.3.constraint.params.keyType=- -policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.userCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caUUIDdeviceCert.cfg` at line 34, Remove the weak RSA 1024-bit allowance by deleting the policy entry "policyset.userCertSet.3.constraint.params.allowedKeys.RSA.1024=true" so the profile no longer permits 1024-bit RSA keys and only enforces stronger key sizes (ensure other entries like RSA.2048 are present or default to 2048+ as needed); update any related policyset.userCertSet.* constraints to reflect a minimum of 2048-bit RSA if required.base/ca/shared/profiles/ca/AdminCert.cfg-34-34 (1)
34-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove cryptographically weak RSA 1024-bit key allowance.
Administrator certificates require strong cryptographic protection. RSA 1024-bit keys are deprecated by NIST (SP 800-131A) and create security vulnerabilities that could compromise administrative access.
Remove line 34 to enforce a minimum 2048-bit RSA key size for administrator certificates.
🔒 Proposed fix
policyset.adminCertSet.3.constraint.params.keyType=RSA -policyset.adminCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.adminCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/AdminCert.cfg` at line 34, Remove the weak RSA 1024 allowance by deleting the configuration entry "policyset.adminCertSet.3.constraint.params.allowedKeys.RSA.1024=true" so administrator certificate policies no longer permit 1024-bit RSA keys; ensure only 2048+ entries (e.g., RSA.2048 or higher) remain and verify there are no other occurrences of "RSA.1024" in the admin cert policy set or related policy entries.base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg-40-40 (1)
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove cryptographically weak RSA 1024-bit key allowance.
RSA 1024-bit keys are deprecated by NIST (SP 800-131A) and create security vulnerabilities. User certificates with weak keys are susceptible to factorization attacks and compliance failures.
Remove line 40 to enforce a minimum 2048-bit RSA key size.
🔒 Proposed fix
policyset.cmcUserCertSet.3.constraint.name=Key Constraint -policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024=true policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.2048=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfg` at line 40, Remove the allowance of 1024-bit RSA keys by deleting the configuration entry policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.1024=true so the policy no longer permits RSA-1024; ensure only RSA.2048 (or larger) entries remain (e.g., policyset.cmcUserCertSet.3.constraint.params.allowedKeys.RSA.2048) so the profile enforces a minimum 2048-bit RSA key size.
🟡 Minor comments (1)
base/ca/shared/profiles/ca/caECAdminCert.cfg-34-37 (1)
34-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify keyType=- when only EC keys are allowed.
Line 34 sets
keyType=-(wildcard), but only EC curves are enabled in allowedKeys. This creates semantic inconsistency: the wildcard suggests any key type is acceptable, yet the configuration explicitly permits only EC. If the intention is to useallowedKeys.*as the authoritative constraint and deprecatekeyType, consider settingkeyType=ECfor clarity, or document thatkeyType=-means "determined by allowedKeys." Otherwise, this may confuse administrators and complicate validation logic.♻️ Suggested fix for consistency
-policyset.adminCertSet.3.constraint.params.keyType=- +policyset.adminCertSet.3.constraint.params.keyType=EC policyset.adminCertSet.3.constraint.params.allowedKeys.EC.nistp256=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/ca/shared/profiles/ca/caECAdminCert.cfg` around lines 34 - 37, The policy entry policyset.adminCertSet.3.constraint.params.keyType currently uses a wildcard ("-") while only EC curves are enabled via policyset.adminCertSet.3.constraint.params.allowedKeys.EC.nistp256 / nistp384 / nistp521, causing semantic mismatch; update keyType to "EC" (or add a clarifying comment/documentation stating that keyType="-" is intentionally resolved by allowedKeys) so the intent is explicit and validation logic and administrators are not confused; update the policyset.adminCertSet.3.constraint.params.keyType value to EC (or add the explanatory doc adjacent to that key).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8c5cb01-cc8c-40ae-9892-83aeda499f5c
📒 Files selected for processing (94)
.gitignorebase/ca/shared/profiles/ca/AdminCert.cfgbase/ca/shared/profiles/ca/ECAdminCert.cfgbase/ca/shared/profiles/ca/acmeServerCert.cfgbase/ca/shared/profiles/ca/caAdminCert.cfgbase/ca/shared/profiles/ca/caAgentFileSigning.cfgbase/ca/shared/profiles/ca/caAgentServerCert.cfgbase/ca/shared/profiles/ca/caAuditSigningCert.cfgbase/ca/shared/profiles/ca/caCACert.cfgbase/ca/shared/profiles/ca/caCMCECUserCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCert.cfgbase/ca/shared/profiles/ca/caCMCECserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCECsubsystemCert.cfgbase/ca/shared/profiles/ca/caCMCUserCert.cfgbase/ca/shared/profiles/ca/caCMCauditSigningCert.cfgbase/ca/shared/profiles/ca/caCMCcaCert.cfgbase/ca/shared/profiles/ca/caCMCcaIssuanceProtectionCert.cfgbase/ca/shared/profiles/ca/caCMCkraStorageCert.cfgbase/ca/shared/profiles/ca/caCMCkraTransportCert.cfgbase/ca/shared/profiles/ca/caCMCocspCert.cfgbase/ca/shared/profiles/ca/caCMCserverCert.cfgbase/ca/shared/profiles/ca/caCMCserverCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caCMCsubsystemCert.cfgbase/ca/shared/profiles/ca/caCrossSignedCACert.cfgbase/ca/shared/profiles/ca/caDirBasedDualCert.cfgbase/ca/shared/profiles/ca/caDirPinUserCert.cfgbase/ca/shared/profiles/ca/caDirUserCert.cfgbase/ca/shared/profiles/ca/caDualCert.cfgbase/ca/shared/profiles/ca/caDualRAuserCert.cfgbase/ca/shared/profiles/ca/caECAdminCert.cfgbase/ca/shared/profiles/ca/caECAgentServerCert.cfgbase/ca/shared/profiles/ca/caECDirPinUserCert.cfgbase/ca/shared/profiles/ca/caECDirUserCert.cfgbase/ca/shared/profiles/ca/caECDualCert.cfgbase/ca/shared/profiles/ca/caECFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caECFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caECServerCert.cfgbase/ca/shared/profiles/ca/caECServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caECServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caECSubsystemCert.cfgbase/ca/shared/profiles/ca/caECUserCert.cfgbase/ca/shared/profiles/ca/caEncECUserCert.cfgbase/ca/shared/profiles/ca/caEncUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCSharedTokenCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserCert.cfgbase/ca/shared/profiles/ca/caFullCMCUserSignedCert.cfgbase/ca/shared/profiles/ca/caIPAserviceCert.cfgbase/ca/shared/profiles/ca/caInstallCACert.cfgbase/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfgbase/ca/shared/profiles/ca/caInternalAuthDRMstorageCert.cfgbase/ca/shared/profiles/ca/caInternalAuthOCSPCert.cfgbase/ca/shared/profiles/ca/caInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caInternalAuthTransportCert.cfgbase/ca/shared/profiles/ca/caJarSigningCert.cfgbase/ca/shared/profiles/ca/caMLDSAAdminCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthServerCert.cfgbase/ca/shared/profiles/ca/caMLDSAInternalAuthSubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAServerCert.cfgbase/ca/shared/profiles/ca/caMLDSASubsystemCert.cfgbase/ca/shared/profiles/ca/caMLDSAUserCert.cfgbase/ca/shared/profiles/ca/caOCSPCert.cfgbase/ca/shared/profiles/ca/caOtherCert.cfgbase/ca/shared/profiles/ca/caRACert.cfgbase/ca/shared/profiles/ca/caRARouterCert.cfgbase/ca/shared/profiles/ca/caRAagentCert.cfgbase/ca/shared/profiles/ca/caRAserverCert.cfgbase/ca/shared/profiles/ca/caRouterCert.cfgbase/ca/shared/profiles/ca/caServerCert.cfgbase/ca/shared/profiles/ca/caServerCertWithCRLDP.cfgbase/ca/shared/profiles/ca/caServerCertWithSCT.cfgbase/ca/shared/profiles/ca/caServerKeygen_DirUserCert.cfgbase/ca/shared/profiles/ca/caServerKeygen_UserCert.cfgbase/ca/shared/profiles/ca/caSignedLogCert.cfgbase/ca/shared/profiles/ca/caSigningECUserCert.cfgbase/ca/shared/profiles/ca/caSigningUserCert.cfgbase/ca/shared/profiles/ca/caSimpleCMCUserCert.cfgbase/ca/shared/profiles/ca/caStorageCert.cfgbase/ca/shared/profiles/ca/caSubsystemCert.cfgbase/ca/shared/profiles/ca/caTPSCert.cfgbase/ca/shared/profiles/ca/caTransportCert.cfgbase/ca/shared/profiles/ca/caUUIDdeviceCert.cfgbase/ca/shared/profiles/ca/caUserCert.cfgbase/ca/shared/profiles/ca/caUserSMIMEcapCert.cfgbase/ca/shared/profiles/ca/estFullcmcDeviceCert.cfgbase/ca/shared/profiles/ca/estServiceCert.cfgbase/ca/shared/webapps/ca/ee/ca/ProfileSelect.templatebase/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.javabase/server/src/main/resources/UserMessages.propertiesbase/server/upgrade/11.10.0/02-UpdateKeyParameters.py
✅ Files skipped from review due to trivial changes (9)
- base/ca/shared/profiles/ca/caCMCkraStorageCert.cfg
- base/ca/shared/profiles/ca/caECUserCert.cfg
- base/ca/shared/profiles/ca/caMLDSASubsystemCert.cfg
- base/ca/shared/profiles/ca/caECSubsystemCert.cfg
- base/ca/shared/profiles/ca/caECDirPinUserCert.cfg
- base/ca/shared/profiles/ca/caECDirUserCert.cfg
- base/ca/shared/profiles/ca/caECServerCertWithSCT.cfg
- .gitignore
- base/ca/shared/profiles/ca/caECInternalAuthSubsystemCert.cfg
Comment on lines
+279
to
+283
| if (keyParams.length > 0 || (keyType != null && !keyType.isEmpty())) { | ||
| logger.warn("KeyConstraint.validate: use of {}/{} is DEPRECATED, use {} instead", CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS, CONFIG_ALLOWED_KEYS_PARAM); | ||
| if (allowedKeysMap.size() > 0) { | ||
| logger.error("Invalid Configuration: can't mix {} and {}/{}", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS); | ||
| throw new EPropertyException(CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS)); |
There was a problem hiding this comment.
allowedKeys must not be treated as mutually exclusive with keyType.
The migrated profiles in this PR still keep constraint.params.keyType alongside allowedKeys.* — for example, base/ca/shared/profiles/ca/caECSimpleCMCUserCert.cfg Line 33 and base/ca/shared/profiles/ca/caInternalAuthAuditSigningCert.cfg Line 33. This branch will therefore raise CMS_MIXED_KEY_CONFIGURATION before any real key validation runs, rejecting enrollments for the converted profiles. allowedKeys replaces keyParameters, not keyType.
Suggested fix
- if (keyParams.length > 0 || (keyType != null && !keyType.isEmpty())) {
- logger.warn("KeyConstraint.validate: use of {}/{} is DEPRECATED, use {} instead", CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS, CONFIG_ALLOWED_KEYS_PARAM);
- if (allowedKeysMap.size() > 0) {
- logger.error("Invalid Configuration: can't mix {} and {}/{}", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS);
- throw new EPropertyException(CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION", CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_TYPE, CONFIG_KEY_PARAMETERS));
- }
+ if (keyParams.length > 0) {
+ logger.warn("KeyConstraint.validate: use of {} is DEPRECATED, use {} instead",
+ CONFIG_KEY_PARAMETERS, CONFIG_ALLOWED_KEYS_PARAM);
+ if (!allowedKeysMap.isEmpty()) {
+ logger.error("Invalid Configuration: can't mix {} and {}",
+ CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_PARAMETERS);
+ throw new ERejectException(CMS.getUserMessage(
+ getLocale(request),
+ "CMS_MIXED_KEY_CONFIGURATION",
+ CONFIG_ALLOWED_KEYS_PARAM,
+ CONFIG_KEY_PARAMETERS));
+ }
logger.debug("{}: {}", CONFIG_KEY_PARAMETERS, keyParameters);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/ca/src/main/java/com/netscape/cms/profile/constraint/KeyConstraint.java`
around lines 279 - 283, The code in KeyConstraint.validate currently treats
allowedKeysMap as mutually exclusive with keyType and throws
CMS_MIXED_KEY_CONFIGURATION when allowedKeysMap.size()>0 even if only keyType is
present; change the logic so the error is only raised when allowedKeysMap is
mixed with keyParameters (i.e., when keyParams.length>0), not when keyType is
present. Update the if-condition that now reads if (keyParams.length > 0 ||
(keyType != null && !keyType.isEmpty())) to only test for keyParams
(keyParams.length > 0), adjust the logger.warn message to indicate deprecation
of keyParameters (CONFIG_KEY_PARAMETERS) in favor of CONFIG_ALLOWED_KEYS_PARAM
while not treating CONFIG_KEY_TYPE as conflicting, and change the
logger.error/exception to reference only CONFIG_ALLOWED_KEYS_PARAM and
CONFIG_KEY_PARAMETERS (leaving CONFIG_KEY_TYPE out) so throw new
EPropertyException(CMS.getUserMessage("CMS_MIXED_KEY_CONFIGURATION",
CONFIG_ALLOWED_KEYS_PARAM, CONFIG_KEY_PARAMETERS)); remains accurate.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
base/server/src/main/resources/UserMessages.properties (1)
28-28: ⚡ Quick winConsider clarifying the message format.
The
{1}/{2}notation in the message template is potentially ambiguous. It's unclear whether the slash is literal or represents alternatives. For better clarity, consider revising to make the relationship between the three parameters more explicit, such as:
"cannot use both {0} and {1}/{2}"→"cannot use {0} together with {1} and {2}"- or
"cannot use both {0} and the combination of {1} and {2}"This will improve readability for end users and clarity for translators.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/server/src/main/resources/UserMessages.properties` at line 28, The message template CMS_MIXED_KEY_CONFIGURATION is ambiguous because "{1}/{2}" could be read as a literal slash or alternatives; update the template to a clearer phrasing such as "Invalid Configuration: cannot use {0} together with {1} and {2}" or "Invalid Configuration: cannot use {0} together with the combination of {1} and {2}" so translators and users clearly understand the relationship between the three placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@base/server/src/main/resources/UserMessages.properties`:
- Line 28: The message template CMS_MIXED_KEY_CONFIGURATION is ambiguous because
"{1}/{2}" could be read as a literal slash or alternatives; update the template
to a clearer phrasing such as "Invalid Configuration: cannot use {0} together
with {1} and {2}" or "Invalid Configuration: cannot use {0} together with the
combination of {1} and {2}" so translators and users clearly understand the
relationship between the three placeholders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0b4ca69-a3cd-437f-9ecc-81c1da7f906d
📒 Files selected for processing (1)
base/server/src/main/resources/UserMessages.properties
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added alternative KeyConstraint parameters to profile configuration files
Enabled keys can now be enabled or singularly disabled using params.allowedKeys._=true/false
params.keyParameters is still considered valid for backword compatibility but the new fields take precedence
More details here: https://redhat.atlassian.net/browse/IDM-5708?focusedCommentId=16751341
Summary by CodeRabbit
New Features
Chores