WIP: CNTRLPLANE-2012: Add configurable PKI support for installer-generated signer certificates#10396
Open
hasbro17 wants to merge 5 commits intoopenshift:mainfrom
Open
WIP: CNTRLPLANE-2012: Add configurable PKI support for installer-generated signer certificates#10396hasbro17 wants to merge 5 commits intoopenshift:mainfrom
hasbro17 wants to merge 5 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@hasbro17: This pull request references CNTRLPLANE-2012 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
hasbro17
changed the title
WalkthroughThis pull request introduces configurable PKI support to the OpenShift installer, enabling customization of cryptographic parameters (RSA vs ECDSA algorithms and key sizes/curves) for installer-generated signer certificates. Changes include CRD schema updates, PKI configuration types with validation, refactored TLS certificate generation to support multiple algorithms, and comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
The unit tests cover the following areas but add quite a bit of volume to this PR:
I've split most of the unit tests into their own commit, and looking at the actual PKI integration and TLS changes commit-wise would make it easier to review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
pkg/asset/tls/utils_test.go (1)
57-89: Add a PKCS#8 case for the new parser branch.
PemToPrivateKeynow has a"PRIVATE KEY"path inpkg/asset/tls/utils.go, but this suite still only exercises PKCS#1 and EC blocks. One PKCS#8 RSA/ECDSA case would keep that branch from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/utils_test.go` around lines 57 - 89, Test suite is missing a PKCS#8 ("PRIVATE KEY") case for the new PemToPrivateKey branch; add a subtest that takes an RSA or ECDSA private key (use GenerateRSAPrivateKey or GenerateECDSAPrivateKey), marshal it with x509.MarshalPKCS8PrivateKey to produce PKCS#8 bytes, wrap those bytes in a PEM block labeled "PRIVATE KEY" (similar to PrivateKeyToPem), feed that PEM to PemToPrivateKey and assert no error and that the returned value is the expected *rsa.PrivateKey or *ecdsa.PrivateKey type so the PKCS#8 branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 4898-4899: The schema currently requires pki.defaults but the spec
states it is auto-populated; update the YAML schema so pki.defaults is optional
(remove it from any required[] lists or mark it nullable/optional in the pki
object definition) and ensure the pki object still allows a signerCertificates
override without defaults present; apply the same change where pki.defaults is
similarly enforced (the other occurrence referenced) and update the pki.defaults
description to note it is auto-populated when omitted.
- Around line 4902-5152: The schema exposes clientCertificates,
servingCertificates, and defaults as general PKI overrides but the installer
only applies them to installer-generated signer certs—remove or hide these
fields (clientCertificates, servingCertificates, defaults) from the
InstallConfig schema in install.openshift.io_installconfigs.yaml so the API does
not promise unsupported overrides; alternatively, if you must keep them for
compatibility, update their descriptions to explicitly state they only affect
installer-generated signer certificates and add a schema marker (e.g.,
x-kubernetes-hidden or a clear deprecation note) so callers are not misled.
Ensure you update the definitions and any x-kubernetes-validations that
reference these symbols (clientCertificates, servingCertificates, defaults) so
the schema and documentation consistently reflect the limited scope.
In `@docs/user/customization.md`:
- Around line 57-66: The docs currently mention that `defaults` is
auto-populated but never documents `defaults.key`, causing confusion about
whether `defaults.key` is a user-supplied field or internal-only; update the
`pki` section to either (A) document `defaults.key` under `signerCertificates`
(describe its shape and allowed subfields `key.algorithm`, `key.rsa.keySize`,
`key.ecdsa.curve`, valid values and whether it may be provided by users), or (B)
remove/clarify the statement that `defaults` is auto-populated and stop
referring to `defaults.key` as if it were user-configurable; reference `pki`,
`signerCertificates`, and `defaults.key` when making the change.
In `@pkg/asset/tls/tls.go`:
- Around line 266-273: The current code clobbers any caller-set cfg.KeyUsages by
replacing adjustedCfg.KeyUsages with baseUsage; instead preserve caller bits:
compute baseUsage := keyUsageForAlgorithm(params.Algorithm) and then set
adjustedCfg.KeyUsages = cfg.KeyUsages | baseUsage (and if cfg.IsCA also OR
x509.KeyUsageCertSign) so existing explicit bits on cfg.KeyUsages remain honored
while still applying the algorithm-derived and CA bits.
In `@pkg/asset/tls/utils.go`:
- Around line 86-95: The PKCS#8 branch currently calls x509.ParsePKCS8PrivateKey
which can return key types other than RSA/ECDSA and those will later break
PrivateKeyToPem and generateSubjectKeyID; change the "PRIVATE KEY" case
(x509.ParsePKCS8PrivateKey) to assert the returned key is either *rsa.PrivateKey
or *ecdsa.PrivateKey and return an explicit error for any other types so
unsupported PKCS#8 keys are rejected at decode time (refer to the "PRIVATE KEY"
case, x509.ParsePKCS8PrivateKey, and the downstream uses PrivateKeyToPem and
generateSubjectKeyID).
In `@pkg/types/pki/validation.go`:
- Around line 19-23: The current guard in the signer key validation (using
profile.SignerCertificates.Key.Algorithm != "") lets partial key configs (e.g.,
rsa.keySize or ecdsa.curve) skip validation and be silently ignored; change the
check so ValidateKeyConfig is invoked whenever the Key block is non-empty (i.e.,
any of Key.Algorithm, Key.KeySize, Key.Curve or other key fields are set) rather
than only when Algorithm is set, and make the same non-empty-key detection
change in GetEffectiveSignerKeyConfig so fallback/default merging is validated
correctly; add regression tests covering partial defaults and partial
signerCertificates.key inputs to ensure these are validated.
In `@pkg/types/validation/installconfig.go`:
- Around line 284-286: The PKI profile validation call in ValidatePKIProfile
(invoked from install-config validation when c.PKI != nil) currently only checks
profile.SignerCertificates.Key and therefore misses validating pki.defaults.key;
update pkivalidation.ValidatePKIProfile to explicitly validate the defaults.key
field (and any related key fields under profile.Defaults) using the same key
validation logic as for SignerCertificates.Key so invalid default keys fail fast
during install-config validation rather than later during certificate
generation.
---
Nitpick comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 57-89: Test suite is missing a PKCS#8 ("PRIVATE KEY") case for the
new PemToPrivateKey branch; add a subtest that takes an RSA or ECDSA private key
(use GenerateRSAPrivateKey or GenerateECDSAPrivateKey), marshal it with
x509.MarshalPKCS8PrivateKey to produce PKCS#8 bytes, wrap those bytes in a PEM
block labeled "PRIVATE KEY" (similar to PrivateKeyToPem), feed that PEM to
PemToPrivateKey and assert no error and that the returned value is the expected
*rsa.PrivateKey or *ecdsa.PrivateKey type so the PKCS#8 branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0fc8704-7ecb-42db-a25a-ef8fbe0e10af
📒 Files selected for processing (34)
data/data/install.openshift.io_installconfigs.yamldocs/user/customization.mdpkg/asset/ignition/machine/arbiter_ignition_customizations_test.gopkg/asset/ignition/machine/arbiter_test.gopkg/asset/ignition/machine/master_ignition_customizations_test.gopkg/asset/ignition/machine/master_test.gopkg/asset/ignition/machine/worker_ignition_customizations_test.gopkg/asset/ignition/machine/worker_test.gopkg/asset/tls/adminkubeconfig.gopkg/asset/tls/aggregator.gopkg/asset/tls/apiserver.gopkg/asset/tls/boundsasigningkey.gopkg/asset/tls/certkey.gopkg/asset/tls/certkey_test.gopkg/asset/tls/ironictls.gopkg/asset/tls/kubecontrolplane.gopkg/asset/tls/kubelet.gopkg/asset/tls/root.gopkg/asset/tls/tls.gopkg/asset/tls/tls_test.gopkg/asset/tls/utils.gopkg/asset/tls/utils_test.gopkg/types/defaults/installconfig.gopkg/types/defaults/installconfig_test.gopkg/types/defaults/validation/featuregates.gopkg/types/installconfig.gopkg/types/pki/helpers.gopkg/types/pki/helpers_test.gopkg/types/pki/validation.gopkg/types/pki/validation_test.gopkg/types/validation/featuregate_test.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.gopkg/types/zz_generated.deepcopy.go
pkg/types/pki/validation.go
Outdated
Comment on lines
+19
to
+23
| // Validate signerCertificates override if Algorithm is set (non-zero value) | ||
| if profile.SignerCertificates.Key.Algorithm != "" { | ||
| allErrs = append(allErrs, ValidateKeyConfig(profile.SignerCertificates.Key, | ||
| fldPath.Child("signerCertificates", "key"), fips)...) | ||
| } |
There was a problem hiding this comment.
Validate populated key blocks even when algorithm is omitted.
The guard on Line 20 lets partial configs like rsa.keySize: 4096 or ecdsa.curve: P384 bypass validation entirely. GetEffectiveSignerKeyConfig() in pkg/types/pki/helpers.go then uses the same Algorithm != "" check, so those inputs are silently ignored, and profile.Defaults.Key is never validated before being used as the fallback. That means broken PKI config can pass install-config validation and only fail or change behavior later during cert generation.
Suggested fix
func ValidatePKIProfile(profile *configv1alpha1.PKIProfile, fldPath *field.Path, fips bool) field.ErrorList {
allErrs := field.ErrorList{}
if profile == nil {
return allErrs
}
- // Validate signerCertificates override if Algorithm is set (non-zero value)
- if profile.SignerCertificates.Key.Algorithm != "" {
+ if profile.Defaults.Key.Algorithm != "" ||
+ profile.Defaults.Key.RSA.KeySize != 0 ||
+ profile.Defaults.Key.ECDSA.Curve != "" {
+ allErrs = append(allErrs, ValidateKeyConfig(profile.Defaults.Key,
+ fldPath.Child("defaults", "key"), fips)...)
+ }
+
+ if profile.SignerCertificates.Key.Algorithm != "" ||
+ profile.SignerCertificates.Key.RSA.KeySize != 0 ||
+ profile.SignerCertificates.Key.ECDSA.Curve != "" {
allErrs = append(allErrs, ValidateKeyConfig(profile.SignerCertificates.Key,
fldPath.Child("signerCertificates", "key"), fips)...)
}
return allErrs
}Please add regression cases for partial defaults.key / signerCertificates.key inputs with this change.
📝 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
| // Validate signerCertificates override if Algorithm is set (non-zero value) | |
| if profile.SignerCertificates.Key.Algorithm != "" { | |
| allErrs = append(allErrs, ValidateKeyConfig(profile.SignerCertificates.Key, | |
| fldPath.Child("signerCertificates", "key"), fips)...) | |
| } | |
| func ValidatePKIProfile(profile *configv1alpha1.PKIProfile, fldPath *field.Path, fips bool) field.ErrorList { | |
| allErrs := field.ErrorList{} | |
| if profile == nil { | |
| return allErrs | |
| } | |
| if profile.Defaults.Key.Algorithm != "" || | |
| profile.Defaults.Key.RSA.KeySize != 0 || | |
| profile.Defaults.Key.ECDSA.Curve != "" { | |
| allErrs = append(allErrs, ValidateKeyConfig(profile.Defaults.Key, | |
| fldPath.Child("defaults", "key"), fips)...) | |
| } | |
| if profile.SignerCertificates.Key.Algorithm != "" || | |
| profile.SignerCertificates.Key.RSA.KeySize != 0 || | |
| profile.SignerCertificates.Key.ECDSA.Curve != "" { | |
| allErrs = append(allErrs, ValidateKeyConfig(profile.SignerCertificates.Key, | |
| fldPath.Child("signerCertificates", "key"), fips)...) | |
| } | |
| return allErrs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/types/pki/validation.go` around lines 19 - 23, The current guard in the
signer key validation (using profile.SignerCertificates.Key.Algorithm != "")
lets partial key configs (e.g., rsa.keySize or ecdsa.curve) skip validation and
be silently ignored; change the check so ValidateKeyConfig is invoked whenever
the Key block is non-empty (i.e., any of Key.Algorithm, Key.KeySize, Key.Curve
or other key fields are set) rather than only when Algorithm is set, and make
the same non-empty-key detection change in GetEffectiveSignerKeyConfig so
fallback/default merging is validated correctly; add regression tests covering
partial defaults and partial signerCertificates.key inputs to ensure these are
validated.
Contributor
Author
There was a problem hiding this comment.
We have partial config tests in TestValidateKeyConfig. Specifically:
- Line 161: missing algorithm — has RSA keySize but no algorithm
- Line 206: RSA missing key size — has algorithm=RSA but no keySize (zero value)
- Line 224: ECDSA missing curve — has algorithm=ECDSA but no curve (empty string)
- Line 233: RSA with ECDSA config — mismatched: RSA algorithm but ECDSA params also set
- Line 243: ECDSA with RSA config — mismatched: ECDSA algorithm but RSA params also set
And in TestValidatePKIConfig:
- Line 53: empty PKI config — pki: {} with no signerCertificates specified
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at the test files for PKI validation
fd -e go 'validation_test' pkg/types/pki/
fd -e go 'pki' pkg/types/pki/
Length of output: 32
🏁 Script executed:
#!/bin/bash
# Find all test files in pki package
fd -e go . pkg/types/pki/ --exec cat {}
Length of output: 14892
@hasbro17, the referenced tests don't cover the concern — they exercise two different functions:
-
TestValidateKeyConfigcallsValidateKeyConfigdirectly. Yes, it proves that function can catch a missing algorithm. But the issue is thatValidatePKIProfile's guard (Algorithm != "") preventsValidateKeyConfigfrom ever being called when a user supplies onlyrsa.keySize: 4096withoutalgorithm. The validation is never reached. -
TestValidatePKIConfigtestsValidatePKIConfig(which validates*types.PKIConfig). That function correctly returns aRequirederror whenAlgorithm == "". ButValidatePKIProfilevalidates*configv1alpha1.PKIProfile— a different type with different logic.
There is no TestValidatePKIProfile in the test file at all. The concrete gap is:
// This config passes ValidatePKIProfile without any error today:
profile := &configv1alpha1.PKIProfile{
SignerCertificates: configv1alpha1.CertificateConfig{
Key: configv1alpha1.KeyConfig{
// Algorithm is "", so the guard fires and ValidateKeyConfig is never called
RSA: configv1alpha1.RSAKeyConfig{KeySize: 4096},
},
},
}A test for ValidatePKIProfile with that input would fail to produce an error under the current code, confirming the bypass. The fix and regression tests are still needed in the context of ValidatePKIProfile specifically.
Contributor
Author
There was a problem hiding this comment.
I think you're referencing an outdated commit.
ValidatePKIProfile doesn't exist in our codebase anymore after the refactor.
Here's the actual validation flow:
- ValidatePKIConfig(pkiConfig *types.PKIConfig, ...) in validation.go:13
- If pkiConfig != nil and Algorithm == "" → returns Required error immediately (line 21-24)
- If Algorithm != "" → calls ValidateKeyConfig() which validates the full config
So when a user supplies pki: { signerCertificates: { key: { rsa: { keySize: 4096 } } } } (no algorithm):
- ValidatePKIConfig catches it at line 21 with "signerCertificates.key is required when pki is specified"
- ValidateKeyConfig is never called — but that's correct, because the config is already invalid
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of validation.go in the pki package
fd -e go 'validation' pkg/types/pki/ --exec cat {}
Length of output: 10610
@hasbro17, you're right — I was commenting on stale code from an earlier commit. The current ValidatePKIConfig implementation is sound:
- A partial config like
{ rsa: { keySize: 4096 } }(noalgorithm) hits theAlgorithm == ""guard at line 20 and returns aRequirederror immediately —ValidateKeyConfigis never needed in that path. - Only fully-specified configs (with
Algorithmset) proceed toValidateKeyConfig, which then validates the rest.
The concern doesn't apply to the refactored code. Sorry for the noise.
Contributor
Author
|
Since the flow from specifying the PKI config to signer cert/key generation is a bit long the following flow helps me visualize all the relevant pieces that this PR is modifying. flowchart LR
A["install-config.yaml<br/><code>pki.signerCertificates</code>"] --> B["SetInstallConfigDefaults()<br/>auto-populate defaults"]
B --> C["ValidateFeatureSet()<br/>ConfigurablePKI gate"]
B --> D["ValidatePKIProfile()<br/>algorithm, keySize, curve"]
C --> E["10 Signer Assets"]
D --> E
style A fill:#e1f5fe
style E fill:#c8e6c9
Key generation flowchart LR
A["SelfSignedCertKey.Generate(pkiProfile)"] --> B["GetEffectiveSignerKeyConfig()<br/>signer override → defaults → RSA 2048"]
B --> C{"Algorithm?"}
C -->|RSA| D["GenerateRSAPrivateKey(keySize)<br/>KeyUsage: Sign+Encipher+CertSign"]
C -->|ECDSA| E["GenerateECDSAPrivateKey(curve)<br/>KeyUsage: Sign+CertSign"]
D --> F["SelfSignedCertificate(crypto.Signer)<br/>SignatureAlg auto-detected"]
E --> F
F --> G["Signer CA cert+key (PEM)"]
G --> H["SignedCertKey.Generate()<br/>Leaf certs: always RSA 2048"]
style A fill:#e1f5fe
style G fill:#c8e6c9
style H fill:#fff9c4
|
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-aws-ovn-techpreview |
hasbro17
force-pushed
the
install-config-pki
branch
from
dfa87a6 to
71dcced
Compare
Contributor
Author
|
/test e2e-aws-ovn-techpreview |
Add configurable PKI support to InstallConfig behind the ConfigurablePKI
feature gate, allowing users to specify cryptographic parameters for
installer-generated signer certificates.
Key changes:
- Define custom PKIConfig type with only signerCertificates field
- Add PKI *PKIConfig field to InstallConfig
- Add ConfigurablePKI feature gate entry in GatedFeatures()
- Create pkg/types/pki/ with validation functions
- Wire PKI validation into ValidateInstallConfig()
- Require signerCertificates.key when pki is present (pki: {} is invalid)
Assisted-by: Claude Code (Opus 4.6)
Update pkg/asset/tls/ to generate signer certificates with either RSA or ECDSA keys based on the PKI config from InstallConfig. Leaf certificates continue to use RSA 2048. Key changes: - Add DefaultRSAKeySize/DefaultKeyAlgorithm constants - Add RSA/ECDSA key generation functions and PEM encode/decode support - Change SelfSignedCertificate to accept crypto.Signer - Change SignedCertificate/GenerateSignedCertificate caKey to crypto.PrivateKey - Add pkiConfig parameter to SelfSignedCertKey.Generate() - Update PKIConfigToKeyParams to accept *types.PKIConfig - Set algorithm-appropriate KeyUsage (ECDSA omits KeyEncipherment) - All signer assets now depend on InstallConfig and pass PKI config - Add type assertion in BoundSASigningKey.Load() for RSA requirement Assisted-by: Claude Code (Opus 4.6)
Add tests covering PKI validation, feature gate enforcement, certificate generation with PKI configs, and cross-algorithm certificate signing. Key additions: - Test ValidatePKIConfig catches invalid configs and empty PKI - Test ConfigurablePKI feature gate with TechPreview and CustomNoUpgrade - Test ValidateInstallConfig catches invalid PKI with field paths - Test SelfSignedCertKey.Generate() with non-nil PKI configs - Test ECDSA CA signing RSA leaf certificate with chain verification - Test RSA/ECDSA key generation, KeyUsage flags, signature algorithm detection - Test PEM encode/decode roundtrip for RSA and ECDSA keys Assisted-by: Claude Code (Opus 4.6)
Document the configurable PKI feature in docs/user/customization.md, following the existing inline documentation pattern for install-config properties. Key additions: - Add pki field entry with nested signerCertificates structure - Add RSA 4096 and ECDSA P-384 install-config example fragments - Document ConfigurablePKI feature gate requirement - Note scope: signer certificates only, leaf certs unaffected Assisted-by: Claude Code (Opus 4.6)
hasbro17
force-pushed
the
install-config-pki
branch
from
71dcced to
d792394
Compare
…abled When the ConfigurablePKI feature gate is active, the installer now generates a config.openshift.io/v1alpha1 PKI custom resource manifest (manifests/cluster-pki-02-config.yaml) that is applied to the cluster during bootstrap. This CR provides day-2 operators with the certificate parameters to use when rotating certificates. The PKI CR uses mode: Custom with DefaultPKIProfile as the base (defaults: RSA-2048, signerCertificates: RSA-4096). User overrides from install-config pki.signerCertificates are layered on top. When the feature gate is enabled but pki is not specified in install-config, the installer also aligns its own signer cert generation to RSA-4096 (matching DefaultPKIProfile) instead of the legacy RSA-2048 default. Assisted-by: Claude Code (Opus 4.6)
Contributor
Author
|
/retest-required |
Contributor
Author
|
/test e2e-aws-ovn-techpreview |
Contributor
Author
|
/retest-required |
Contributor
|
@hasbro17: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
hasbro17
commented
Mar 23, 2026
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
data/data/install.openshift.io_installconfigs.yaml (1)
4946-4949:⚠️ Potential issue | 🟠 MajorRemove stale
defaults/overrideswording from the schema description.Line 4949 says key settings can come from “defaults” in an “overrides entry”, but this InstallConfig schema only exposes
pki.signerCertificates. That description now documents behavior users cannot configure and can mislead API consumers.Suggested doc fix
key: description: |- key specifies the cryptographic parameters for the certificate's key pair. - Currently this is the only configurable parameter. When omitted in an - overrides entry, the key configuration from defaults is used. + Currently this is the only configurable parameter for signer certificates.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/data/install.openshift.io_installconfigs.yaml` around lines 4946 - 4949, Update the schema description for the "key" entry under pki.signerCertificates to remove the stale references to "defaults" and "overrides" and instead describe the actual configurable surface (i.e., that only the key cryptographic parameters for the certificate's key pair are configurable via pki.signerCertificates). Locate the block containing the description text that mentions "defaults" and "overrides" and replace that wording with a concise sentence stating that the schema exposes configuration of the certificate key parameters via pki.signerCertificates and that if omitted, the signer certificate uses its built-in/default key configuration (do not mention non-existent overrides mechanism).
🧹 Nitpick comments (3)
pkg/asset/tls/utils_test.go (1)
57-89: Add PKCS#8 coverage forPemToPrivateKey.
pkg/asset/tls/utils.gonow has a"PRIVATE KEY"branch, andpkg/asset/tls/boundsasigningkey.goroutes user-provided keys through it. These tests only exercise PKCS#1 and SEC1 PEM, so that compatibility path can regress unnoticed.🧪 Suggested test addition
import ( "crypto/ecdsa" "crypto/rsa" + "crypto/x509" + "encoding/pem" "testing" @@ func TestPemToPrivateKeyFormats(t *testing.T) { @@ t.Run("EC PEM block", func(t *testing.T) { key, err := GenerateECDSAPrivateKey(configv1alpha1.ECDSACurveP256) assert.NoError(t, err) pemBytes := PrivateKeyToPem(key) decoded, err := PemToPrivateKey(pemBytes) assert.NoError(t, err) _, ok := decoded.(*ecdsa.PrivateKey) assert.True(t, ok, "expected *ecdsa.PrivateKey") }) + + t.Run("PKCS#8 RSA PEM block", func(t *testing.T) { + key, err := GenerateRSAPrivateKey(2048) + assert.NoError(t, err) + der, err := x509.MarshalPKCS8PrivateKey(key) + assert.NoError(t, err) + pemBytes := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + + decoded, err := PemToPrivateKey(pemBytes) + assert.NoError(t, err) + assert.IsType(t, &rsa.PrivateKey{}, decoded) + }) + + t.Run("PKCS#8 EC PEM block", func(t *testing.T) { + key, err := GenerateECDSAPrivateKey(configv1alpha1.ECDSACurveP256) + assert.NoError(t, err) + der, err := x509.MarshalPKCS8PrivateKey(key) + assert.NoError(t, err) + pemBytes := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + + decoded, err := PemToPrivateKey(pemBytes) + assert.NoError(t, err) + assert.IsType(t, &ecdsa.PrivateKey{}, decoded) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/utils_test.go` around lines 57 - 89, Tests for PemToPrivateKey only cover PKCS#1/SEC1 formats; add a test that encodes both RSA and ECDSA private keys as PKCS#8 ("PRIVATE KEY") PEM blocks and asserts PemToPrivateKey correctly decodes them to *rsa.PrivateKey and *ecdsa.PrivateKey respectively (reuse GenerateRSAPrivateKey, GenerateECDSAPrivateKey and PrivateKeyToPem helpers or produce PKCS#8 PEM for those keys), so the "PRIVATE KEY" branch exercised by boundsasigningkey.go is covered. Ensure the test cases mirror existing patterns (error cases + decoding assertions) and use the same function names PemToPrivateKey, GenerateRSAPrivateKey, GenerateECDSAPrivateKey, PrivateKeyToPem to locate insertion points.pkg/asset/tls/certkey_test.go (2)
199-202: Also pin the leaf RSA size here.The PR keeps leaf certificates at RSA-2048, but this only checks
*rsa.PrivateKey. A different RSA size would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/certkey_test.go` around lines 199 - 202, The test currently only asserts the leaf key type from PemToPrivateKey(certKey.Key()) is *rsa.PrivateKey; update it to also type-assert the returned leafKey to *rsa.PrivateKey (after the existing assert) and assert its modulus bit-length equals 2048 (e.g., check rsaKey.N.BitLen() == 2048) so the test pins the RSA size used by SignedCertKey.
105-159: Assert the configured signer parameters, not just the algorithm family.This still passes if RSA-4096 regresses to RSA-2048 or if P-384 regresses to a different ECDSA curve. Since this test is validating
PKIConfigpropagation, please assert the generated key size / curve too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/certkey_test.go` around lines 105 - 159, Update TestSelfSignedCertKeyGenerateWithPKIConfig to assert the concrete key parameters from the generated private key in addition to algorithm: after calling PemToPrivateKey(key) and the existing IsType/assertions, for the RSA case assert the rsa.PrivateKey.Size()*8 or rsa.N.BitLen() equals 4096, and for the ECDSA case assert the ecdsa.PrivateKey.Curve matches the expected curve (e.g., elliptic.P384()). These checks should be added directly in the subtest after key is decoded (inside the t.Run callback) and reference the existing test helpers and types (TestSelfSignedCertKeyGenerateWithPKIConfig, CertCfg, SelfSignedCertKey.Generate, PemToPrivateKey) so the test fails if PKIConfig key-size/curve isn’t propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/customization.md`:
- Around line 57-66: Update the pki docs to match the implemented defaults:
change the default signer key size from RSA-2048 to RSA-4096 when the
ConfigurablePKI feature gate is enabled and pki is omitted, and clarify that
when a top-level pki block is present the signerCertificates and its nested key
object are required (with algorithm required and rsa.keySize valid multiples of
1024 between 2048–8192); reference the ConfigurablePKI feature gate,
signerCertificates, key, algorithm and rsa.keySize symbols so the maintainer can
find the corresponding logic in defaults and tests.
In `@pkg/asset/tls/tls.go`:
- Around line 28-65: PKIConfigToKeyParams currently applies hardcoded defaults
(DefaultKeyAlgorithm/DefaultRSAKeySize) when pkiConfig is nil, which
duplicates/defaults signer key params here instead of in the PKI layer; remove
that nil-path defaulting so this helper only converts an already-resolved
config. Change PKIConfigToKeyParams to not return RSA-2048 for a nil
*types.PKIConfig — either require callers to pass a non-nil/resolved pkiConfig
or return the zero-value PrivateKeyParams when pkiConfig is nil — and update
callers to perform resolution using the PKI defaulting logic in the PKI layer
(pkg/types/pki/defaults.go / pkg/asset/manifests/pki.go). Ensure the conversion
still uses keyConfig := pkiConfig.SignerCertificates.Key and preserves the
switch on keyConfig.Algorithm (configv1alpha1.KeyAlgorithmRSA /
KeyAlgorithmECDSA) to populate RSAKeySize or ECDSACurve.
In `@pkg/asset/tls/utils.go`:
- Around line 15-38: The helper PrivateKeyToPem currently calls logrus.Fatalf on
marshal/unsupported-type errors which terminates the process; change its
signature to return ([]byte, error) instead of []byte, replace logrus.Fatalf
calls with returning wrapped errors (e.g., fmt.Errorf or errors.Wrap) for the
ECDSA marshal failure and unsupported key type, and return the PEM bytes with a
nil error on success; update all call sites that use PrivateKeyToPem (such as
pkg/asset/tls/certkey.go and pkg/asset/tls/keypair.go) to handle the returned
error and propagate it upward instead of relying on process exit.
---
Duplicate comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 4946-4949: Update the schema description for the "key" entry under
pki.signerCertificates to remove the stale references to "defaults" and
"overrides" and instead describe the actual configurable surface (i.e., that
only the key cryptographic parameters for the certificate's key pair are
configurable via pki.signerCertificates). Locate the block containing the
description text that mentions "defaults" and "overrides" and replace that
wording with a concise sentence stating that the schema exposes configuration of
the certificate key parameters via pki.signerCertificates and that if omitted,
the signer certificate uses its built-in/default key configuration (do not
mention non-existent overrides mechanism).
---
Nitpick comments:
In `@pkg/asset/tls/certkey_test.go`:
- Around line 199-202: The test currently only asserts the leaf key type from
PemToPrivateKey(certKey.Key()) is *rsa.PrivateKey; update it to also type-assert
the returned leafKey to *rsa.PrivateKey (after the existing assert) and assert
its modulus bit-length equals 2048 (e.g., check rsaKey.N.BitLen() == 2048) so
the test pins the RSA size used by SignedCertKey.
- Around line 105-159: Update TestSelfSignedCertKeyGenerateWithPKIConfig to
assert the concrete key parameters from the generated private key in addition to
algorithm: after calling PemToPrivateKey(key) and the existing
IsType/assertions, for the RSA case assert the rsa.PrivateKey.Size()*8 or
rsa.N.BitLen() equals 4096, and for the ECDSA case assert the
ecdsa.PrivateKey.Curve matches the expected curve (e.g., elliptic.P384()). These
checks should be added directly in the subtest after key is decoded (inside the
t.Run callback) and reference the existing test helpers and types
(TestSelfSignedCertKeyGenerateWithPKIConfig, CertCfg,
SelfSignedCertKey.Generate, PemToPrivateKey) so the test fails if PKIConfig
key-size/curve isn’t propagated.
In `@pkg/asset/tls/utils_test.go`:
- Around line 57-89: Tests for PemToPrivateKey only cover PKCS#1/SEC1 formats;
add a test that encodes both RSA and ECDSA private keys as PKCS#8 ("PRIVATE
KEY") PEM blocks and asserts PemToPrivateKey correctly decodes them to
*rsa.PrivateKey and *ecdsa.PrivateKey respectively (reuse GenerateRSAPrivateKey,
GenerateECDSAPrivateKey and PrivateKeyToPem helpers or produce PKCS#8 PEM for
those keys), so the "PRIVATE KEY" branch exercised by boundsasigningkey.go is
covered. Ensure the test cases mirror existing patterns (error cases + decoding
assertions) and use the same function names PemToPrivateKey,
GenerateRSAPrivateKey, GenerateECDSAPrivateKey, PrivateKeyToPem to locate
insertion points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68544947-cf52-4b23-8a47-7d56e6024c29
📒 Files selected for processing (37)
data/data/install.openshift.io_installconfigs.yamldocs/user/customization.mdpkg/asset/ignition/machine/arbiter_ignition_customizations_test.gopkg/asset/ignition/machine/arbiter_test.gopkg/asset/ignition/machine/master_ignition_customizations_test.gopkg/asset/ignition/machine/master_test.gopkg/asset/ignition/machine/worker_ignition_customizations_test.gopkg/asset/ignition/machine/worker_test.gopkg/asset/manifests/operators.gopkg/asset/manifests/pki.gopkg/asset/manifests/pki_test.gopkg/asset/tls/adminkubeconfig.gopkg/asset/tls/aggregator.gopkg/asset/tls/apiserver.gopkg/asset/tls/boundsasigningkey.gopkg/asset/tls/certkey.gopkg/asset/tls/certkey_test.gopkg/asset/tls/ironictls.gopkg/asset/tls/kubecontrolplane.gopkg/asset/tls/kubelet.gopkg/asset/tls/root.gopkg/asset/tls/tls.gopkg/asset/tls/tls_test.gopkg/asset/tls/utils.gopkg/asset/tls/utils_test.gopkg/explain/printer_test.gopkg/types/defaults/installconfig.gopkg/types/installconfig.gopkg/types/pki/defaults.gopkg/types/pki/defaults_test.gopkg/types/pki/validation.gopkg/types/pki/validation_test.gopkg/types/validation/featuregate_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.gopkg/types/zz_generated.deepcopy.go
Comment on lines
+57
to
+66
| * `pki` (optional object): Configures cryptographic parameters for installer-generated signer certificates. | ||
| Requires the `ConfigurablePKI` feature gate (set `featureSet: TechPreviewNoUpgrade` or use `CustomNoUpgrade` with `featureGates`). | ||
| When omitted, all signer certificates use RSA 2048 (existing default behavior). | ||
| * `signerCertificates` (optional object): Overrides key parameters for all signer certificates. | ||
| * `key` (optional object): Key generation parameters. | ||
| * `algorithm` (required string): The key algorithm. Valid values are `RSA` and `ECDSA`. | ||
| * `rsa` (optional object): RSA-specific parameters. Required when `algorithm` is `RSA`. | ||
| * `keySize` (required integer): Key size in bits. Valid values are multiples of 1024 from 2048 to 8192. | ||
| * `ecdsa` (optional object): ECDSA-specific parameters. Required when `algorithm` is `ECDSA`. | ||
| * `curve` (required string): The elliptic curve. Valid values are `P256`, `P384`, and `P521`. |
There was a problem hiding this comment.
The pki docs are out of sync with the implemented defaults.
With ConfigurablePKI enabled and pki omitted, pkg/types/pki/defaults.go synthesizes signer RSA-4096 and pkg/asset/manifests/pki_test.go asserts that behavior. The current text says RSA-2048 and also marks signerCertificates / key optional even though pki requires them when present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user/customization.md` around lines 57 - 66, Update the pki docs to
match the implemented defaults: change the default signer key size from RSA-2048
to RSA-4096 when the ConfigurablePKI feature gate is enabled and pki is omitted,
and clarify that when a top-level pki block is present the signerCertificates
and its nested key object are required (with algorithm required and rsa.keySize
valid multiples of 1024 between 2048–8192); reference the ConfigurablePKI
feature gate, signerCertificates, key, algorithm and rsa.keySize symbols so the
maintainer can find the corresponding logic in defaults and tests.
Comment on lines
28
to
+65
| const ( | ||
| keySize = 2048 | ||
| // DefaultRSAKeySize is the default RSA key size used when PKI config is not specified. | ||
| DefaultRSAKeySize int32 = 2048 | ||
| // DefaultKeyAlgorithm is the default key algorithm used when PKI config is not specified. | ||
| DefaultKeyAlgorithm = configv1alpha1.KeyAlgorithmRSA | ||
| ) | ||
|
|
||
| // PrivateKeyParams specifies parameters for private key generation. | ||
| type PrivateKeyParams struct { | ||
| Algorithm configv1alpha1.KeyAlgorithm | ||
| RSAKeySize int32 | ||
| ECDSACurve configv1alpha1.ECDSACurve | ||
| } | ||
|
|
||
| // PKIConfigToKeyParams converts PKI config to key generation parameters. | ||
| // If pkiConfig is nil, returns default RSA 2048 parameters. | ||
| func PKIConfigToKeyParams(pkiConfig *types.PKIConfig) PrivateKeyParams { | ||
| if pkiConfig == nil { | ||
| return PrivateKeyParams{ | ||
| Algorithm: DefaultKeyAlgorithm, | ||
| RSAKeySize: DefaultRSAKeySize, | ||
| } | ||
| } | ||
|
|
||
| keyConfig := pkiConfig.SignerCertificates.Key | ||
| params := PrivateKeyParams{ | ||
| Algorithm: keyConfig.Algorithm, | ||
| } | ||
|
|
||
| switch keyConfig.Algorithm { | ||
| case configv1alpha1.KeyAlgorithmRSA: | ||
| params.RSAKeySize = keyConfig.RSA.KeySize | ||
| case configv1alpha1.KeyAlgorithmECDSA: | ||
| params.ECDSACurve = keyConfig.ECDSA.Curve | ||
| } | ||
|
|
||
| return params | ||
| } |
There was a problem hiding this comment.
Don't default signer key params from *types.PKIConfig in tls.go.
This helper can't distinguish "ConfigurablePKI disabled" from "gate enabled, pki omitted". Those states now have different effective signer defaults: pkg/types/pki/defaults.go defines RSA-4096 for signer certs, and pkg/asset/manifests/pki.go persists that profile, but the nil path here hardcodes RSA-2048. That split source of truth can make the installer-generated signer CAs diverge from the emitted cluster-pki-02-config.yaml. Please move default resolution up to the PKI layer and have this package convert only an already-resolved key config.
Suggested direction
-// PKIConfigToKeyParams converts PKI config to key generation parameters.
-// If pkiConfig is nil, returns default RSA 2048 parameters.
-func PKIConfigToKeyParams(pkiConfig *types.PKIConfig) PrivateKeyParams {
- if pkiConfig == nil {
- return PrivateKeyParams{
- Algorithm: DefaultKeyAlgorithm,
- RSAKeySize: DefaultRSAKeySize,
- }
- }
-
- keyConfig := pkiConfig.SignerCertificates.Key
+// KeyConfigToKeyParams converts an already-resolved signer key config to
+// key generation parameters. Defaulting should happen in the PKI layer.
+func KeyConfigToKeyParams(keyConfig configv1alpha1.KeyConfig) PrivateKeyParams {
params := PrivateKeyParams{
Algorithm: keyConfig.Algorithm,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/tls/tls.go` around lines 28 - 65, PKIConfigToKeyParams currently
applies hardcoded defaults (DefaultKeyAlgorithm/DefaultRSAKeySize) when
pkiConfig is nil, which duplicates/defaults signer key params here instead of in
the PKI layer; remove that nil-path defaulting so this helper only converts an
already-resolved config. Change PKIConfigToKeyParams to not return RSA-2048 for
a nil *types.PKIConfig — either require callers to pass a non-nil/resolved
pkiConfig or return the zero-value PrivateKeyParams when pkiConfig is nil — and
update callers to perform resolution using the PKI defaulting logic in the PKI
layer (pkg/types/pki/defaults.go / pkg/asset/manifests/pki.go). Ensure the
conversion still uses keyConfig := pkiConfig.SignerCertificates.Key and
preserves the switch on keyConfig.Algorithm (configv1alpha1.KeyAlgorithmRSA /
KeyAlgorithmECDSA) to populate RSAKeySize or ECDSACurve.
Comment on lines
+15
to
+38
| // PrivateKeyToPem converts a private key (RSA or ECDSA) to PEM format. | ||
| func PrivateKeyToPem(key crypto.PrivateKey) []byte { | ||
| var block *pem.Block | ||
|
|
||
| switch k := key.(type) { | ||
| case *rsa.PrivateKey: | ||
| block = &pem.Block{ | ||
| Type: "RSA PRIVATE KEY", | ||
| Bytes: keyInBytes, | ||
| }, | ||
| ) | ||
| return keyinPem | ||
| Bytes: x509.MarshalPKCS1PrivateKey(k), | ||
| } | ||
| case *ecdsa.PrivateKey: | ||
| bytes, err := x509.MarshalECPrivateKey(k) | ||
| if err != nil { | ||
| logrus.Fatalf("failed to marshal ECDSA private key: %v", err) | ||
| } | ||
| block = &pem.Block{ | ||
| Type: "EC PRIVATE KEY", | ||
| Bytes: bytes, | ||
| } | ||
| default: | ||
| logrus.Fatalf("unsupported private key type: %T", key) | ||
| } | ||
|
|
||
| return pem.EncodeToMemory(block) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PrivateKeyToPem call sites:\n'
rg -nC2 '\bPrivateKeyToPem\s*\(' --type=go
printf '\nFatal exits in pkg/asset/tls:\n'
rg -nC2 'logrus\.Fatalf' pkg/asset/tlsRepository: openshift/installer
Length of output: 3002
Avoid Fatalf in this shared PEM helper.
PrivateKeyToPem is called throughout the cert-generation pipeline (in pkg/asset/tls/certkey.go, pkg/asset/tls/keypair.go, and elsewhere). Using logrus.Fatalf for marshal failures or unsupported key types terminates the entire process instead of returning errors that call sites can handle—this breaks the error propagation chain and makes testing impossible.
Return errors from this function instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/tls/utils.go` around lines 15 - 38, The helper PrivateKeyToPem
currently calls logrus.Fatalf on marshal/unsupported-type errors which
terminates the process; change its signature to return ([]byte, error) instead
of []byte, replace logrus.Fatalf calls with returning wrapped errors (e.g.,
fmt.Errorf or errors.Wrap) for the ECDSA marshal failure and unsupported key
type, and return the PEM bytes with a nil error on success; update all call
sites that use PrivateKeyToPem (such as pkg/asset/tls/certkey.go and
pkg/asset/tls/keypair.go) to handle the returned error and propagate it upward
instead of relying on process exit.
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.
See PKI config enhancement openshift/enhancements#1882 for background
Overview:
Currently, the OpenShift installer hard-codes all signer certificates to use RSA 2048-bit keys.
This PR adds configurable PKI support to the installer, allowing users to specify cryptographic parameters for the 10 installer-generated signer certificates via a new pki field in the
InstallConfig. Thefeature is gated behind
ConfigurablePKI(TechPreviewNoUpgrade/DevPreviewNoUpgrade).Example usage in install-config.yaml:
Key changes:
PKI *configv1alpha1.PKIProfilefield to InstallConfig, using the official type from openshift/api (PR CNTRLPLANE-1752: Add PKI API to config.openshift.io/v1alpha1 api#2645)ConfigurablePKIfeature gate check inGatedFeatures()ValidatePKIProfile()pkg/asset/tls/to support both RSA and ECDSA key generation with algorithm-appropriate KeyUsage flagsSelfSignedCertKey.Generate()to accept a pkiProfile parameterPemToPrivateKey()/PrivateKeyToPem()to handle both RSA and ECDSA key formatsInstallConfigand pass PKI config to key generationTODO:
ConfigurablePKIfeaturegate enabled and verifies the signer certs and PKI resource