CNTRLPLANE-2012: Add configurable PKI support for installer-generated signer certificates#10396
Open
hasbro17 wants to merge 5 commits intoopenshift:mainfrom
Open
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
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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)
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
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)
hasbro17
force-pushed
the
install-config-pki
branch
from
71dcced to
d792394
Compare
Contributor
Author
|
/retest-required |
Contributor
Author
|
/test e2e-aws-ovn-techpreview |
Contributor
Author
|
/retest-required |
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
Contributor
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-techpreview |
Contributor
|
@sanchezl: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1e48cc80-27b4-11f1-9131-75ee6cd16cdf-0 |
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)
hasbro17
force-pushed
the
install-config-pki
branch
from
d7b8103 to
8596880
Compare
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)
…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)
hasbro17
force-pushed
the
install-config-pki
branch
from
8596880 to
057f2a7
Compare
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. |
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
commented
Mar 30, 2026
Comment on lines
+67
to
+68
| // TODO(htariq): Should this be Default if PKI is unset in the install config? | ||
| Mode: configv1alpha1.PKICertificateManagementModeCustom, |
Contributor
Author
There was a problem hiding this comment.
@sanchezl What should the mode be for the PKI CR in the default case where no PKI is specified in the InstallConfig? I have it set to custom here but we have Unmanaged and Default as well.
My read on that is if an admin specifies signer overrides via the installconfig then it for sure is Custom but if there's no override then it's Default. Unmanaged doesn't really enter the picture if we're always creating the PKI CR when the featuregate is on.
hasbro17
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
Waiting on the presubmits in openshift/release#77043 to merge so we can run them here. Not sure if I can do a multi pr test though to validate before merging that. |
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.
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 11 installer-generated signer certificates via a new
pkifield in theInstallConfig. Thefeature is gated behind
ConfigurablePKI(TechPreviewNoUpgrade/DevPreviewNoUpgrade).When
ConfigurablePKIis enabled andpkiis omitted, signer certificates default to ECDSA P-384 (matching the DefaultPKIProfile fromlibrary-go).
When the feature gate is disabled, behavior is unchanged (RSA 2048).
Example usage in install-config.yaml:
Key changes:
PKI *types.PKIConfigfield on InstallConfig exposing onlysignerCertificates, reusing vendored sub-types from openshift/api (PR CNTRLPLANE-1752: Add PKI API to config.openshift.io/v1alpha1 api#2645)ConfigurablePKIfeature gate check inGatedFeatures()ValidatePKIConfig()pkg/asset/tls/to support both RSA and ECDSA key generation with algorithm-appropriate KeyUsage flagsPrivateKeyToPem()to return([]byte, error)and handle both RSA and ECDSA key formatsEffectiveSignerPKIConfig()to determine key parametersconfig.openshift.io/v1alpha1 PKICR manifest (cluster-pki-02-config.yaml) when ConfigurablePKI is enabled, with DefaultPKIProfile defaults and user overridesTODO:
ConfigurablePKIfeature gate enabled and verifies the signer certs and PKI resource