WIP: CNTRLPLANE-2012: Add configurable PKI support for installer-generated signer certificates#10396
WIP: CNTRLPLANE-2012: Add configurable PKI support for installer-generated signer certificates#10396hasbro17 wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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 |
|
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
| When specified, the installer uses signerCertificates to configure the installer-generated | ||
| signer certificates. The defaults field is auto-populated if not provided. |
There was a problem hiding this comment.
Make pki.defaults optional in the schema.
Lines 4898-4899 say defaults is auto-populated when omitted, but Lines 5237-5238 require it. Any schema-based validation will reject the intended defaulting path, including configs that only override signerCertificates.
Also applies to: 5237-5238
🤖 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 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.
| clientCertificates: | ||
| description: |- | ||
| clientCertificates optionally overrides certificate parameters for | ||
| client authentication certificates used to authenticate to servers. | ||
| When set, these parameters take precedence over defaults for all client certificates. | ||
| When omitted, the defaults are used for client certificates. | ||
| minProperties: 1 | ||
| properties: | ||
| 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. | ||
| properties: | ||
| algorithm: | ||
| description: |- | ||
| algorithm specifies the key generation algorithm. | ||
| Valid values are "RSA" and "ECDSA". | ||
|
|
||
| When set to RSA, the rsa field must be specified and the generated key | ||
| will be an RSA key with the configured key size. | ||
|
|
||
| When set to ECDSA, the ecdsa field must be specified and the generated key | ||
| will be an ECDSA key using the configured elliptic curve. | ||
| enum: | ||
| - RSA | ||
| - ECDSA | ||
| type: string | ||
| ecdsa: | ||
| description: |- | ||
| ecdsa specifies ECDSA key parameters. | ||
| Required when algorithm is ECDSA, and forbidden otherwise. | ||
| properties: | ||
| curve: | ||
| description: |- | ||
| curve specifies the NIST elliptic curve for ECDSA keys. | ||
| Valid values are "P256", "P384", and "P521". | ||
|
|
||
| When set to P256, the NIST P-256 curve (also known as secp256r1) is used, | ||
| providing 128-bit security. | ||
|
|
||
| When set to P384, the NIST P-384 curve (also known as secp384r1) is used, | ||
| providing 192-bit security. | ||
|
|
||
| When set to P521, the NIST P-521 curve (also known as secp521r1) is used, | ||
| providing 256-bit security. | ||
| enum: | ||
| - P256 | ||
| - P384 | ||
| - P521 | ||
| type: string | ||
| required: | ||
| - curve | ||
| type: object | ||
| rsa: | ||
| description: |- | ||
| rsa specifies RSA key parameters. | ||
| Required when algorithm is RSA, and forbidden otherwise. | ||
| properties: | ||
| keySize: | ||
| description: |- | ||
| keySize specifies the size of RSA keys in bits. | ||
| Valid values are multiples of 1024 from 2048 to 8192. | ||
| format: int32 | ||
| maximum: 8192 | ||
| minimum: 2048 | ||
| multipleOf: 1024 | ||
| type: integer | ||
| required: | ||
| - keySize | ||
| type: object | ||
| required: | ||
| - algorithm | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: rsa is required when algorithm is RSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''RSA'' ? has(self.rsa) | ||
| : !has(self.rsa)' | ||
| - message: ecdsa is required when algorithm is ECDSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''ECDSA'' ? has(self.ecdsa) | ||
| : !has(self.ecdsa)' | ||
| type: object | ||
| defaults: | ||
| description: |- | ||
| defaults specifies the default certificate configuration that applies | ||
| to all certificates unless overridden by a category override. | ||
| properties: | ||
| key: | ||
| description: |- | ||
| key specifies the cryptographic parameters for the certificate's key pair. | ||
| This field is required in defaults to ensure all certificates have a | ||
| well-defined key configuration. | ||
| properties: | ||
| algorithm: | ||
| description: |- | ||
| algorithm specifies the key generation algorithm. | ||
| Valid values are "RSA" and "ECDSA". | ||
|
|
||
| When set to RSA, the rsa field must be specified and the generated key | ||
| will be an RSA key with the configured key size. | ||
|
|
||
| When set to ECDSA, the ecdsa field must be specified and the generated key | ||
| will be an ECDSA key using the configured elliptic curve. | ||
| enum: | ||
| - RSA | ||
| - ECDSA | ||
| type: string | ||
| ecdsa: | ||
| description: |- | ||
| ecdsa specifies ECDSA key parameters. | ||
| Required when algorithm is ECDSA, and forbidden otherwise. | ||
| properties: | ||
| curve: | ||
| description: |- | ||
| curve specifies the NIST elliptic curve for ECDSA keys. | ||
| Valid values are "P256", "P384", and "P521". | ||
|
|
||
| When set to P256, the NIST P-256 curve (also known as secp256r1) is used, | ||
| providing 128-bit security. | ||
|
|
||
| When set to P384, the NIST P-384 curve (also known as secp384r1) is used, | ||
| providing 192-bit security. | ||
|
|
||
| When set to P521, the NIST P-521 curve (also known as secp521r1) is used, | ||
| providing 256-bit security. | ||
| enum: | ||
| - P256 | ||
| - P384 | ||
| - P521 | ||
| type: string | ||
| required: | ||
| - curve | ||
| type: object | ||
| rsa: | ||
| description: |- | ||
| rsa specifies RSA key parameters. | ||
| Required when algorithm is RSA, and forbidden otherwise. | ||
| properties: | ||
| keySize: | ||
| description: |- | ||
| keySize specifies the size of RSA keys in bits. | ||
| Valid values are multiples of 1024 from 2048 to 8192. | ||
| format: int32 | ||
| maximum: 8192 | ||
| minimum: 2048 | ||
| multipleOf: 1024 | ||
| type: integer | ||
| required: | ||
| - keySize | ||
| type: object | ||
| required: | ||
| - algorithm | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: rsa is required when algorithm is RSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''RSA'' ? has(self.rsa) | ||
| : !has(self.rsa)' | ||
| - message: ecdsa is required when algorithm is ECDSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''ECDSA'' ? has(self.ecdsa) | ||
| : !has(self.ecdsa)' | ||
| required: | ||
| - key | ||
| type: object | ||
| servingCertificates: | ||
| description: |- | ||
| servingCertificates optionally overrides certificate parameters for | ||
| TLS server certificates used to serve HTTPS endpoints. | ||
| When set, these parameters take precedence over defaults for all serving certificates. | ||
| When omitted, the defaults are used for serving certificates. | ||
| minProperties: 1 | ||
| properties: | ||
| 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. | ||
| properties: | ||
| algorithm: | ||
| description: |- | ||
| algorithm specifies the key generation algorithm. | ||
| Valid values are "RSA" and "ECDSA". | ||
|
|
||
| When set to RSA, the rsa field must be specified and the generated key | ||
| will be an RSA key with the configured key size. | ||
|
|
||
| When set to ECDSA, the ecdsa field must be specified and the generated key | ||
| will be an ECDSA key using the configured elliptic curve. | ||
| enum: | ||
| - RSA | ||
| - ECDSA | ||
| type: string | ||
| ecdsa: | ||
| description: |- | ||
| ecdsa specifies ECDSA key parameters. | ||
| Required when algorithm is ECDSA, and forbidden otherwise. | ||
| properties: | ||
| curve: | ||
| description: |- | ||
| curve specifies the NIST elliptic curve for ECDSA keys. | ||
| Valid values are "P256", "P384", and "P521". | ||
|
|
||
| When set to P256, the NIST P-256 curve (also known as secp256r1) is used, | ||
| providing 128-bit security. | ||
|
|
||
| When set to P384, the NIST P-384 curve (also known as secp384r1) is used, | ||
| providing 192-bit security. | ||
|
|
||
| When set to P521, the NIST P-521 curve (also known as secp521r1) is used, | ||
| providing 256-bit security. | ||
| enum: | ||
| - P256 | ||
| - P384 | ||
| - P521 | ||
| type: string | ||
| required: | ||
| - curve | ||
| type: object | ||
| rsa: | ||
| description: |- | ||
| rsa specifies RSA key parameters. | ||
| Required when algorithm is RSA, and forbidden otherwise. | ||
| properties: | ||
| keySize: | ||
| description: |- | ||
| keySize specifies the size of RSA keys in bits. | ||
| Valid values are multiples of 1024 from 2048 to 8192. | ||
| format: int32 | ||
| maximum: 8192 | ||
| minimum: 2048 | ||
| multipleOf: 1024 | ||
| type: integer | ||
| required: | ||
| - keySize | ||
| type: object | ||
| required: | ||
| - algorithm | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: rsa is required when algorithm is RSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''RSA'' ? has(self.rsa) | ||
| : !has(self.rsa)' | ||
| - message: ecdsa is required when algorithm is ECDSA, and forbidden | ||
| otherwise | ||
| rule: 'has(self.algorithm) && self.algorithm == ''ECDSA'' ? has(self.ecdsa) | ||
| : !has(self.ecdsa)' | ||
| type: object |
There was a problem hiding this comment.
Don't expose PKI overrides the installer does not honor.
This feature currently applies PKI settings only to installer-generated signer certs. Exposing clientCertificates, servingCertificates, and defaults as if they affect all certificates makes the InstallConfig API promise knobs that will not change generated leaf certs.
🤖 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 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.
There was a problem hiding this comment.
@sanchezl The enhancement mentions we will only support Day-1 (installer) support for signer certificate configuration.
I've currently embedded the PKIProfile type into the InstallConfig to stay aligned with the official API and keep the door open for supporting other fields.
So right now in the API type PKIProfile: Defaults is +required but we make it effectively optional in the installer by auto-populating it in SetInstallConfigDefaults()
So the happy path from the enhancement works where we don't have to specify Defaults:
pki:
signerCertificates:
key:
algorithm: RSA
rsa:
keySize: 4096But you could also do defaults only:
pki:
defaults:
key:
algorithm: RSA
rsa:
keySize: 4096Since no signerCertificates override is specified, signer certs fall back to defaults and use RSA 4096. Currently only signers consume this, but eventually this could extend to servingCertificates/clientCertificates overrides.
And you could even do Custom defaults + signer override:
pki:
defaults:
key:
algorithm: RSA
rsa:
keySize: 4096
signerCertificates:
key:
algorithm: ECDSA
ecdsa:
curve: P521Here Signer certs use ECDSA P-521 (override takes precedence) so the defaults field is redundant until we support leaf cert customization as well.
So my question is whether we should allow fields other than signerCertificates in the InstallConfig today with the anticipation that we would support them before GA?
Or should we hide everything else and wait until we do decide to support them.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Precedence summary for signers as it is right now
┌────────────────────────────────────┬──────────────────────────────────────────────────────────────────┐
│ Config │ Effective signer algorithm │
├────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
│ No pki field │ RSA 2048 (hardcoded default) │
├────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
│ pki: {} │ RSA 2048 (auto-populated defaults) │
├────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
│ pki.defaults only │ Uses defaults.key │
├────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
│ pki.signerCertificates only │ Uses signerCertificates.key (defaults auto-populated but unused) │
├────────────────────────────────────┼──────────────────────────────────────────────────────────────────┤
│ Both defaults + signerCertificates │ signerCertificates wins for signers │
└────────────────────────────────────┴──────────────────────────────────────────────────────────────────┘
There was a problem hiding this comment.
The installer team will be very weary of adding too much config to the install-config.yaml. As such we limited it to only signerCertificates.
We should define our own PKI type with only signerCertificates. This way:
- The CRD schema only shows what users can actually configure
- The installer team controls when/if new fields are adopted
- No risk of upstream API changes silently exposing unsupported config
For the Day-2 PKI resource creation, we should:
- Start with
library-go/pkg/pki.DefaultPKIProfile()as the base (or a similar local copy until library-go PR merges) - Merge the user's
signerCertificatesconfig on top and create the Day-2 PKI resource from the merged result
There was a problem hiding this comment.
Yeah I was leaning that way as well but wasn't sure. Will strip out the unneccessary config for now and keep it to signerCertificates
| * `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.
Document defaults.key or stop referring to it.
Line 304 says defaults is auto-populated, but the field list here only describes signerCertificates. As written, users can’t tell whether defaults.key is supported input or just internal defaulting behavior.
Also applies to: 304-304
🤖 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, 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.
| // Set KeyUsage based on algorithm — ECDSA keys cannot perform key encipherment | ||
| adjustedCfg := *cfg | ||
| baseUsage := keyUsageForAlgorithm(params.Algorithm) | ||
| if cfg.IsCA { | ||
| adjustedCfg.KeyUsages = baseUsage | x509.KeyUsageCertSign | ||
| } else { | ||
| adjustedCfg.KeyUsages = baseUsage | ||
| } |
There was a problem hiding this comment.
Preserve caller-specified KeyUsages here.
These lines rebuild adjustedCfg.KeyUsages from scratch, so any explicit bits on cfg.KeyUsages disappear. That’s broader than the ECDSA KeyEncipherment fix and changes the semantics of CertCfg.KeyUsages.
Suggested fix
- // Set KeyUsage based on algorithm — ECDSA keys cannot perform key encipherment
- adjustedCfg := *cfg
- baseUsage := keyUsageForAlgorithm(params.Algorithm)
- if cfg.IsCA {
- adjustedCfg.KeyUsages = baseUsage | x509.KeyUsageCertSign
- } else {
- adjustedCfg.KeyUsages = baseUsage
- }
+ // Default KeyUsage when unset, but otherwise preserve caller intent and
+ // only remove bits that are incompatible with the selected algorithm.
+ adjustedCfg := *cfg
+ if adjustedCfg.KeyUsages == 0 {
+ adjustedCfg.KeyUsages = keyUsageForAlgorithm(params.Algorithm)
+ }
+ if params.Algorithm == configv1alpha1.KeyAlgorithmECDSA {
+ adjustedCfg.KeyUsages &^= x509.KeyUsageKeyEncipherment
+ }
+ if cfg.IsCA {
+ adjustedCfg.KeyUsages |= x509.KeyUsageCertSign
+ }📝 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.
| // Set KeyUsage based on algorithm — ECDSA keys cannot perform key encipherment | |
| adjustedCfg := *cfg | |
| baseUsage := keyUsageForAlgorithm(params.Algorithm) | |
| if cfg.IsCA { | |
| adjustedCfg.KeyUsages = baseUsage | x509.KeyUsageCertSign | |
| } else { | |
| adjustedCfg.KeyUsages = baseUsage | |
| } | |
| // Default KeyUsage when unset, but otherwise preserve caller intent and | |
| // only remove bits that are incompatible with the selected algorithm. | |
| adjustedCfg := *cfg | |
| if adjustedCfg.KeyUsages == 0 { | |
| adjustedCfg.KeyUsages = keyUsageForAlgorithm(params.Algorithm) | |
| } | |
| if params.Algorithm == configv1alpha1.KeyAlgorithmECDSA { | |
| adjustedCfg.KeyUsages &^= x509.KeyUsageKeyEncipherment | |
| } | |
| if cfg.IsCA { | |
| adjustedCfg.KeyUsages |= x509.KeyUsageCertSign | |
| } |
🤖 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 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.
There was a problem hiding this comment.
The caller-set KeyUsages are wrong for ECDSA anyway.
OR-ing caller bits would be incorrect because the existing callers all hardcoded the RSA pattern (KeyEncipherment | DigitalSignature | CertSign), which would re-introduce KeyEncipherment on ECDSA certs.
That's exactly what we need to prevent.
However I'll remove KeyUsages from all 10 signer asset CertCfgs entirely. Callers will no longer set a value that gets unconditionally discarded.
GenerateSelfSignedCertificate is the sole owner of KeyUsages for self-signed certs, setting it based on the key algorithm.
There was a problem hiding this comment.
@hasbro17, that makes sense — centralising ownership of KeyUsages in GenerateSelfSignedCertificate is cleaner than trying to mask individual incompatible bits from callers that already carry RSA-centric defaults. Removing the field from all 10 CertCfg sites is the right call here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| switch block.Type { | ||
| case "RSA PRIVATE KEY": | ||
| return x509.ParsePKCS1PrivateKey(block.Bytes) | ||
| case "EC PRIVATE KEY": | ||
| return x509.ParseECPrivateKey(block.Bytes) | ||
| case "PRIVATE KEY": | ||
| return x509.ParsePKCS8PrivateKey(block.Bytes) | ||
| default: | ||
| return nil, fmt.Errorf("unsupported PEM block type: %s", block.Type) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PemToPrivateKey call sites:\n'
rg -n -C2 '\bPemToPrivateKey\s*\('
printf '\nPrivateKeyToPem supported key types:\n'
sed -n '15,39p' pkg/asset/tls/utils.go
printf '\ngenerateSubjectKeyID supported key types:\n'
sed -n '204,219p' pkg/asset/tls/tls.goRepository: openshift/installer
Length of output: 5134
Reject unsupported PKCS#8 key types at decode time.
x509.ParsePKCS8PrivateKey accepts key types beyond RSA/ECDSA. The rest of this package (PrivateKeyToPem and generateSubjectKeyID) only supports RSA/ECDSA keys, so this branch can admit unsupported keys that will fail later when used with certificate generation paths.
Suggested fix
case "PRIVATE KEY":
- return x509.ParsePKCS8PrivateKey(block.Bytes)
+ key, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+ if err != nil {
+ return nil, err
+ }
+ switch key.(type) {
+ case *rsa.PrivateKey, *ecdsa.PrivateKey:
+ return key, nil
+ default:
+ return nil, fmt.Errorf("unsupported PKCS#8 private key type: %T", key)
+ }🤖 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 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).
pkg/types/pki/validation.go
Outdated
| // 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.
| // 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.
| if c.PKI != nil { | ||
| allErrs = append(allErrs, pkivalidation.ValidatePKIProfile(c.PKI, field.NewPath("pki"), c.FIPS)...) | ||
| } |
There was a problem hiding this comment.
Validate pki.defaults.key as part of install-config validation.
At Line 285, ValidatePKIProfile is called, but current validator logic (in pkg/types/pki/validation.go) only checks profile.SignerCertificates.Key. Invalid pki.defaults.key can slip through and fail later in certificate generation instead of returning a clean config error.
Proposed fix
--- a/pkg/types/pki/validation.go
+++ b/pkg/types/pki/validation.go
@@
func ValidatePKIProfile(profile *configv1alpha1.PKIProfile, fldPath *field.Path, fips bool) field.ErrorList {
allErrs := field.ErrorList{}
@@
+ // Validate defaults key config if Algorithm is set (non-zero value)
+ if profile.Defaults.Key.Algorithm != "" {
+ allErrs = append(allErrs, ValidateKeyConfig(
+ profile.Defaults.Key,
+ fldPath.Child("defaults", "key"),
+ fips,
+ )...)
+ }
+
// 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)...)
}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 `@pkg/types/validation/installconfig.go` around lines 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.
|
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
|
|
/test ? |
|
/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
dfa87a6 to
71dcced
Compare
|
/test e2e-aws-ovn-techpreview |
|
@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. |
3 participants
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