pki: use library-go crypto for cert generation#1
Open
sanchezl wants to merge 19 commits intohasbro17:install-config-pkifrom
Open
pki: use library-go crypto for cert generation#1sanchezl wants to merge 19 commits intohasbro17:install-config-pkifrom
sanchezl wants to merge 19 commits intohasbro17:install-config-pkifrom
Conversation
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)
…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: ECDSA P-256, signerCertificates: ECDSA P-384). 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 ECDSA P-384 (matching DefaultPKIProfile) instead of the legacy RSA-2048 default. Assisted-by: Claude Code (Opus 4.6)
1 task
e9f8ba0 to
a41ad19
Compare
Replace configv1alpha1.CertificateConfig embedding in types.PKIConfig with installer-local types (CertificateConfig, KeyConfig, RSAKeyConfig, ECDSAKeyConfig, KeyAlgorithm, ECDSACurve). This prevents accidentally picking up new fields from the upstream API type. The JSON serialization shape is unchanged, so install-config YAML parsing and the CRD remain compatible. Conversion to upstream API types happens at the boundary when building the PKI CR manifest.
Add SignerPKIConfig asset to the dependency graph that resolves the effective PKI profile for certificate generation. The profile is never nil: feature gate off returns explicit RSA 2048, feature gate on returns library-go's DefaultPKIProfile with user overrides. EffectiveSignerPKIProfile now returns configv1alpha1.PKIProfile (never nil) instead of *types.PKIConfig (nullable). The local DefaultPKIProfile copy is replaced by an import from library-go/pkg/pki. The deprecated EffectiveSignerPKIConfig shim is retained temporarily for callers not yet migrated.
SelfSignedCertKey.Generate() now accepts a libcrypto.KeyPairGenerator and delegates to library-go's NewSigningCertificate for CA certs. Non-CA self-signed certs (IronicTLSCert) fall back to the legacy path. All 11 signer assets now depend on SignerPKIConfig and resolve their KeyPairGenerator via libpki.ResolveCertificateConfig with the <component>.<purpose> certificate naming convention.
a41ad19 to
f5e4533
Compare
SignedCertKey.Generate() now uses library-go's GetCAFromBytes to reconstruct the CA, then dispatches to NewServerCertificate, NewClientCertificate, or NewPeerCertificate based on an explicit CertificateType field on CertCfg. Each leaf cert asset now depends on SignerPKIConfig and resolves its KeyPairGenerator via libpki.ResolveCertificateConfig. When ConfigurablePKI is enabled, leaves follow the profile defaults (ECDSA P-256). When disabled, they use RSA 2048. AppendParent is handled by selectively encoding only the leaf cert (DoNotAppendParent) or the full chain (AppendParent) from the library-go TLSCertificateConfig.
PrivateKeyToPem now delegates to libcrypto.EncodeKey and CertToPem delegates to libcrypto.EncodeCertificates. Both are marked as deprecated in favor of using library-go directly. PemToPrivateKey and PemToCertificate retain their implementations since library-go doesn't expose standalone PEM parsing functions, but are also marked deprecated.
PKIConfiguration now depends on tls.SignerPKIConfig instead of InstallConfig directly. It uses the pre-resolved Profile to build the PKI CR, and gates manifest emission on ConfigurablePKIEnabled. This eliminates duplicate config resolution logic and the local toAPICertificateConfig conversion function.
Replace the forked selfSignedCertificate, generateSelfSignedCertificate, generateSubjectKeyID, and rsaPublicKey with a single call to libcrypto.NewSigningCertificate. The forked code existed to allow empty OU in the Subject, which library-go handles natively. This removes ~80 lines of duplicated crypto code including a SHA-1 SubjectKeyId implementation.
fb0bf42 to
b4808ea
Compare
…review # Conflicts: # go.mod # go.sum # vendor/modules.txt
Files like .idea/, .envrc, dev-retest*.sh, CLAUDE-luis.md, and docs/design/ drafts were untracked local files that got swept up by git add -A during the merge conflict resolution.
The tls_assets testscript test runs "openshift-install agent create certificates" which now requires InstallConfig in the dependency chain (via SignerPKIConfig). Previously signer assets like KubeAPIServerLBSignerCertKey had no dependencies, but now they depend on SignerPKIConfig which depends on InstallConfig. Add a minimal install-config.yaml and agent-config.yaml to the test, matching the pattern used by other agent image asset tests.
The "agent create certificates" command generates signer certs at runtime on the bootstrap node (for the agent UI flow) without an install-config.yaml present. Before the library-go migration, these signers had zero dependencies and always generated RSA 2048 keys. The SignerPKIConfig asset now implements WritableAsset with a Load() method that checks for install-config on disk. When install-config is absent, Load() returns the default RSA 2048 profile, bypassing the Generate() dependency chain entirely. When install-config is present, Load() returns false and the normal Generate() path resolves the profile from InstallConfig. This also reverts the install-config addition to the tls_assets integration test, since the test should work without it.
eba946f to
88daf8f
Compare
9fb84fa to
c7403e2
Compare
0dbe7e4 to
200625a
Compare
200625a to
e873c80
Compare
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
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.
Context
This is a review/refactoring of openshift#10396 that migrates the installer's cert generation to use
library-go/pkg/cryptoandlibrary-go/pkg/pkiinstead of hand-rolling crypto primitives.@hasbro17 — I went through your PR in detail and wanted to suggest an alternative approach that aligns the installer with the same cert generation APIs that other operators are adopting (CKAO via openshift/cluster-kube-apiserver-operator#2087, service-ca-operator via openshift/service-ca-operator#327). Here's why I made each change:
Why library-go instead of hand-rolled crypto
Your PR adds
GeneratePrivateKeyWithParams,GenerateECDSAPrivateKey,GenerateRSAPrivateKey,keyUsageForAlgorithm,PKIConfigToKeyParams, and changesPrivateKeyToPem/PemToPrivateKeyto handle both RSA and ECDSA. All of this already exists inlibrary-go/pkg/crypto(after openshift/library-go#2127):GeneratePrivateKeyWithParams(params)KeyPairGenerator.GenerateKeyPair()viaRSAKeyPairGenerator{Bits}orECDSAKeyPairGenerator{Curve}keyUsageForAlgorithm(algo)KeyUsageForPublicKey(pub)GenerateSelfSignedCertificate(cfg, params)NewSigningCertificate(name, keyGen, opts...)PrivateKeyToPem(key)/CertToPem(cert)TLSCertificateConfig.GetPEMBytes()/EncodeKey()generateSubjectKeyID(pub)(SHA-1)SubjectKeyIDFromPublicKey(pub)(SHA-256, FIPS-compatible)Additionally,
library-go/pkg/pkiprovides:DefaultPKIProfile()— replaces the local copyKeyPairGeneratorFromAPI(keyConfig)— convertsconfigv1alpha1.KeyConfig→KeyPairGeneratorResolveCertificateConfig(provider, certType, name)— resolves effective key config per certificate type and name, matching how CKAO and service-ca-operator resolve their configsWhy local PKI types instead of embedding configv1alpha1
Your PR embeds
configv1alpha1.CertificateConfigdirectly intypes.PKIConfig. This means ifCertificateConfiggains new fields upstream (e.g.,SignatureAlgorithm,Extensions), they'd silently appear in the install-config YAML API without the installer explicitly opting in or validating them.I replaced this with installer-local types (
types.CertificateConfig,types.KeyConfig,types.RSAKeyConfig,types.ECDSAKeyConfig) that mirror only the fields we support. Conversion to upstream API types happens at the boundary when building the PKI CR manifest.Why SignerPKIConfig as an asset
Your PR has each signer call
pkidefaults.EffectiveSignerPKIConfig(installConfig.Config)independently. This means every signer resolves the same config from InstallConfig, and the PKI manifest does the same resolution separately.I introduced a
SignerPKIConfigasset in the dependency graph that resolves the effectivePKIProfileonce. All signers and the PKI manifest depend on it. The profile is never nil — feature gate off returns explicit RSA 2048, feature gate on returnslibpki.DefaultPKIProfile()with user overrides. This eliminates nil-check branching everywhere.Why EffectiveSignerPKIProfile returns PKIProfile (not *PKIConfig)
Your
EffectiveSignerPKIConfigreturns*types.PKIConfig(nullable). The newEffectiveSignerPKIProfilereturnsconfigv1alpha1.PKIProfile(never nil). This:Defaults(for leaf certs) andSignerCertificates, enabling leaf certs to also follow the profileWhy leaf certs also migrate
Your PR deliberately left leaf certs using RSA 2048. This refactoring migrates them too — when ConfigurablePKI is enabled, leaves follow the profile's
Defaults(ECDSA P-256). This is consistent with how the library-go PKI profile system works and what the cluster operators expect.Each leaf cert now has an explicit
CertificateTypefield (serving/client/peer) and resolves its key algorithm vialibpki.ResolveCertificateConfig.Why certificate names
Each cert gets a name string (e.g.,
"kube-apiserver.kubelet-client-signer") passed toResolveCertificateConfig. These match theCertificateNamestrings being adopted in CKAO (openshift/cluster-kube-apiserver-operator#2087) and service-ca-operator (openshift/service-ca-operator#327), and are cross-referenced against the TLS artifact registry in openshift/origin.What's preserved from your PR
pkifield on InstallConfig and its feature gatingWhat's deleted
tls.go(GeneratePrivateKeyWithParams,GenerateRSAPrivateKey,GenerateECDSAPrivateKey,keyUsageForAlgorithm,PKIConfigToKeyParams,PrivateKeyParams)configimage/ingressoperatorsigner.goDefaultPKIProfile()copy (replaced bylibrary-go/pkg/pkiimport)Test plan
go test ./pkg/asset/tls/...— all passgo test ./pkg/types/pki/...— all passgo test ./pkg/types/validation/...— all passgo test ./pkg/asset/manifests/...— all passgo test ./pkg/asset/imagebased/configimage/...— all passgo build ./pkg/... ./cmd/...— clean build