CNTRLPLANE-2012: Refactor TLS cert generation to support configurable key algorithms#10594
Open
hasbro17 wants to merge 2 commits into
Open
CNTRLPLANE-2012: Refactor TLS cert generation to support configurable key algorithms#10594hasbro17 wants to merge 2 commits into
hasbro17 wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@hasbro17: This pull request references CNTRLPLANE-2012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (27)
WalkthroughAdds feature-gated configurable PKI: CRD schema and InstallConfig types, PKI defaults/conversion, validation and feature-gate wiring, TLS key-parameter APIs and PEM helpers for RSA/ECDSA, updates TLS assets to use the new APIs, and a manifests asset that emits a PKI CR when the feature is enabled. ChangesConfigurable PKI Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/asset/tls/utils.go (1)
15-39: 💤 Low value
pem.EncodeToMemorycan return nil on encoding failure.While rare in practice for valid blocks,
pem.EncodeToMemoryreturnsnilif the block cannot be encoded. The function should check for this case to avoid returningnil, nilwhich could cause subtle bugs downstream.♻️ Proposed fix to check for nil result
- return pem.EncodeToMemory(block), nil + encoded := pem.EncodeToMemory(block) + if encoded == nil { + return nil, fmt.Errorf("failed to encode PEM block") + } + return encoded, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/tls/utils.go` around lines 15 - 39, In PrivateKeyToPem, after calling pem.EncodeToMemory(block) ensure the returned []byte is not nil before returning; if pem.EncodeToMemory(block) returns nil, return an explicit error (e.g., "failed to encode PEM") instead of returning nil, nil. Update the function to call pem.EncodeToMemory(block), check for nil, and return the encoded bytes on success or a descriptive error when encoding fails; reference the PrivateKeyToPem function and the pem.EncodeToMemory call to locate the change.pkg/types/installconfig.go (1)
262-268: 💤 Low valueDocumentation inconsistency:
Keyfield marked+optionalbut effectively required.
CertificateConfighasMinProperties=1validation, andKeyis the only property. This meansKeymust be present, contradicting the+optionalannotation. Per thePKIConfigdocstring stating "signerCertificates must be fully specified with algorithm and key parameters," consider changing to+required.Proposed fix
type CertificateConfig struct { // key specifies the cryptographic parameters for the certificate's key pair. - // +optional + // +required Key KeyConfig `json:"key,omitzero"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/installconfig.go` around lines 262 - 268, CertificateConfig's Key is documented as optional but Package-level validation (+kubebuilder:validation:MinProperties=1) makes it required; update the field annotation for Key in the CertificateConfig struct (the Key field of type KeyConfig) to reflect that it is required (replace `+optional` with `+required`) so the docstring, kubebuilder validation and the json tag (`json:"key,omitzero"`) are consistent; ensure the change is applied to the CertificateConfig definition and any related comments mentioning signerCertificates if present.pkg/types/pki/validation_test.go (1)
12-263: 💤 Low valueNo case exercises
fips: true.Both tables declare a
fipsfield but every case leaves itfalse, mirroring the unusedfipsparameter invalidation.go. When FIPS-specific validation is implemented (see thevalidation.gocomment), add cases coveringfips: trueso the FIPS constraints are actually verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/pki/validation_test.go` around lines 12 - 263, Tests don't exercise FIPS mode: both TestValidatePKIConfig and TestValidateKeyConfig declare a fips field but never set it true, so FIPS-specific validation logic in ValidatePKIConfig and ValidateKeyConfig is untested; add new table entries with fips: true in both test tables (use the existing fldPath variables) that cover expected FIPS constraints (e.g., disallow RSA sizes/curves not permitted under FIPS and require FIPS-approved algorithms), and set expectError/errorCount accordingly so FIPS-specific branches are actually validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 5087-5092: The field description for spec.pki currently asserts
that installer-generated signer certificates use signerCertificates (and implies
the feature is active under ConfigurablePKI) which is untrue until signer wiring
is implemented; update the description or withhold publishing spec.pki: either
(a) soften the text to state that signerCertificates will be used once the
signer wiring is implemented and that current behavior defaults to RSA-2048 when
PKI is nil, referencing spec.pki and signerCertificates and the ConfigurablePKI
feature gate, or (b) remove/hold the spec.pki entry from the published schema
until the follow-up that wires signer generation lands so oc explain does not
advertise unimplemented behavior.
In `@pkg/types/pki/validation.go`:
- Around line 12-116: The fips bool is never used; either implement
FIPS-specific checks or remove it — remove it here: drop the fips parameter from
ValidatePKIConfig, ValidateKeyConfig, validateRSAKeyConfig, and
validateECDSAKeyConfig (and from any callers/tests), update their signatures to
not accept fips, and remove the fips forwarding in ValidatePKIConfig →
ValidateKeyConfig and ValidateKeyConfig →
validateRSAKeyConfig/validateECDSAKeyConfig so the validators remain consistent
and compile.
In `@pkg/types/validation/installconfig_test.go`:
- Around line 3120-3149: These two negative test cases ("invalid PKI signer with
unsupported algorithm" and "invalid PKI signer with bad RSA key size") are
tripping the global pki feature-gate instead of exercising PKI validation;
update their setup to opt into ConfigurablePKI by enabling the "pki" feature
gate around the test (e.g., set the feature gate to true before constructing the
InstallConfig and restore it after), so the assertion targets types.PKIConfig
validation (SignerCertificates/Key) rather than the gate check; reference the
test case names and types.PKIConfig/ConfigurablePKI when making the change.
---
Nitpick comments:
In `@pkg/asset/tls/utils.go`:
- Around line 15-39: In PrivateKeyToPem, after calling pem.EncodeToMemory(block)
ensure the returned []byte is not nil before returning; if
pem.EncodeToMemory(block) returns nil, return an explicit error (e.g., "failed
to encode PEM") instead of returning nil, nil. Update the function to call
pem.EncodeToMemory(block), check for nil, and return the encoded bytes on
success or a descriptive error when encoding fails; reference the
PrivateKeyToPem function and the pem.EncodeToMemory call to locate the change.
In `@pkg/types/installconfig.go`:
- Around line 262-268: CertificateConfig's Key is documented as optional but
Package-level validation (+kubebuilder:validation:MinProperties=1) makes it
required; update the field annotation for Key in the CertificateConfig struct
(the Key field of type KeyConfig) to reflect that it is required (replace
`+optional` with `+required`) so the docstring, kubebuilder validation and the
json tag (`json:"key,omitzero"`) are consistent; ensure the change is applied to
the CertificateConfig definition and any related comments mentioning
signerCertificates if present.
In `@pkg/types/pki/validation_test.go`:
- Around line 12-263: Tests don't exercise FIPS mode: both TestValidatePKIConfig
and TestValidateKeyConfig declare a fips field but never set it true, so
FIPS-specific validation logic in ValidatePKIConfig and ValidateKeyConfig is
untested; add new table entries with fips: true in both test tables (use the
existing fldPath variables) that cover expected FIPS constraints (e.g., disallow
RSA sizes/curves not permitted under FIPS and require FIPS-approved algorithms),
and set expectError/errorCount accordingly so FIPS-specific branches are
actually validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79bf9351-dd9e-4d6e-a89c-4378addd4242
⛔ Files ignored due to path filters (1)
pkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (32)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/imagebased/configimage/ingressoperatorsigner.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/keypair.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/conversion.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.go
Comment on lines
+5087
to
+5092
| pki: | ||
| description: |- | ||
| PKI configures cryptographic parameters for installer-generated | ||
| signer certificates. When specified, all signer certificates use the | ||
| algorithm and parameters from signerCertificates. | ||
| Feature gated by ConfigurablePKI. |
There was a problem hiding this comment.
Don't publish spec.pki as active before signer generation consumes it.
This schema says all installer-generated signer certificates use signerCertificates, but the supplied PR context says signer wiring is still deferred and current behavior remains RSA-2048 via nil PKI config. That makes the CRD/oc explain contract misleading for anyone enabling ConfigurablePKI. Either hold this field until the follow-up lands, or soften the description so it does not claim behavior that is not implemented yet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/data/install.openshift.io_installconfigs.yaml` around lines 5087 - 5092,
The field description for spec.pki currently asserts that installer-generated
signer certificates use signerCertificates (and implies the feature is active
under ConfigurablePKI) which is untrue until signer wiring is implemented;
update the description or withhold publishing spec.pki: either (a) soften the
text to state that signerCertificates will be used once the signer wiring is
implemented and that current behavior defaults to RSA-2048 when PKI is nil,
referencing spec.pki and signerCertificates and the ConfigurablePKI feature
gate, or (b) remove/hold the spec.pki entry from the published schema until the
follow-up that wires signer generation lands so oc explain does not advertise
unimplemented behavior.
Comment on lines
+12
to
+116
| func ValidatePKIConfig(pkiConfig *types.PKIConfig, fldPath *field.Path, fips bool) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| if pkiConfig == nil { | ||
| return allErrs | ||
| } | ||
|
|
||
| // signerCertificates.key must be fully specified when pki is present | ||
| if pkiConfig.SignerCertificates.Key.Algorithm == "" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("signerCertificates", "key"), | ||
| "signerCertificates.key is required when pki is specified")) | ||
| return allErrs | ||
| } | ||
|
|
||
| allErrs = append(allErrs, ValidateKeyConfig(pkiConfig.SignerCertificates.Key, | ||
| fldPath.Child("signerCertificates", "key"), fips)...) | ||
|
|
||
| return allErrs | ||
| } | ||
|
|
||
| // ValidateKeyConfig validates the KeyConfig structure. | ||
| // KeyConfig fields are value types: RSA is RSAKeyConfig, ECDSA is ECDSAKeyConfig. | ||
| // Use zero-value checks (KeySize == 0, Curve == "") instead of nil checks. | ||
| func ValidateKeyConfig(config types.KeyConfig, fldPath *field.Path, fips bool) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| if config.Algorithm == "" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("algorithm"), | ||
| "algorithm must be specified")) | ||
| return allErrs | ||
| } | ||
|
|
||
| if config.Algorithm != types.KeyAlgorithmRSA && config.Algorithm != types.KeyAlgorithmECDSA { | ||
| allErrs = append(allErrs, field.NotSupported(fldPath.Child("algorithm"), | ||
| config.Algorithm, []string{string(types.KeyAlgorithmRSA), string(types.KeyAlgorithmECDSA)})) | ||
| return allErrs | ||
| } | ||
|
|
||
| if config.Algorithm == types.KeyAlgorithmRSA { | ||
| if config.RSA.KeySize == 0 { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("rsa", "keySize"), | ||
| "keySize must be specified when algorithm is RSA")) | ||
| } else { | ||
| allErrs = append(allErrs, validateRSAKeyConfig(config.RSA, fldPath.Child("rsa"), fips)...) | ||
| } | ||
|
|
||
| if config.ECDSA.Curve != "" { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("ecdsa"), | ||
| "ecdsa must not be set when algorithm is RSA")) | ||
| } | ||
| } | ||
|
|
||
| if config.Algorithm == types.KeyAlgorithmECDSA { | ||
| if config.ECDSA.Curve == "" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("ecdsa", "curve"), | ||
| "curve must be specified when algorithm is ECDSA")) | ||
| } else { | ||
| allErrs = append(allErrs, validateECDSAKeyConfig(config.ECDSA, fldPath.Child("ecdsa"), fips)...) | ||
| } | ||
|
|
||
| if config.RSA.KeySize != 0 { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("rsa"), | ||
| "rsa must not be set when algorithm is ECDSA")) | ||
| } | ||
| } | ||
|
|
||
| return allErrs | ||
| } | ||
|
|
||
| func validateRSAKeyConfig(config types.RSAKeyConfig, fldPath *field.Path, fips bool) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| // Validate key size — aligned with API kubebuilder validation: | ||
| // multiples of 1024 from 2048 to 8192 | ||
| if config.KeySize < 2048 || config.KeySize > 8192 || config.KeySize%1024 != 0 { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("keySize"), config.KeySize, | ||
| "must be a multiple of 1024 from 2048 to 8192")) | ||
| } | ||
|
|
||
| return allErrs | ||
| } | ||
|
|
||
| func validateECDSAKeyConfig(config types.ECDSAKeyConfig, fldPath *field.Path, fips bool) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| validCurves := []types.ECDSACurve{ | ||
| types.ECDSACurveP256, | ||
| types.ECDSACurveP384, | ||
| types.ECDSACurveP521, | ||
| } | ||
| valid := false | ||
| for _, curve := range validCurves { | ||
| if config.Curve == curve { | ||
| valid = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !valid { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("curve"), config.Curve, | ||
| "must be P256, P384, or P521")) | ||
| } | ||
|
|
||
| return allErrs | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm fips is never read inside the validators and check callers' intent
rg -nP --type=go -C2 '\bfips\b' pkg/types/pki/validation.go
rg -nP --type=go -C3 'Validate(PKIConfig|KeyConfig)\s*\(' --glob '!**/validation.go'Repository: openshift/installer
Length of output: 3756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show every occurrence of `fips` in the PKI validator file
rg -n --type=go '\bfips\b' pkg/types/pki/validation.go
# 2) Check whether tests assert any FIPS-specific behavior
rg -n --type=go '\bfips\b' pkg/types/pki/validation_test.go
# 3) Inspect where `c.FIPS` is sourced/used in installconfig validation
sed -n '250,330p' pkg/types/validation/installconfig.goRepository: openshift/installer
Length of output: 4483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search all PKI validation code for any actual usage of `fips`
rg -n --type=go '\bfips\b' pkg/types/pki -g'*.go'
# In this specific file, check whether `fips` is referenced in any expressions (if/for/return/etc.)
rg -n --type=go 'fips\s*($|[)\],.;:])|if\s*\(.*\bfips\b|switch\s*\(.*\bfips\b|\bfips\b\s*(==|!=|<=|>=|<|>|\{|\!)' pkg/types/pki/validation.goRepository: openshift/installer
Length of output: 1513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Simple checks: does `validation.go` contain any comparison/if/switch using `fips`?
rg -n --type=go 'if\s*\(.*\bfips\b|switch\s*\(.*\bfips\b|^\s*case\s+.*\bfips\b|\bfips\b\s*(==|!=|<|<=|>|>=)\s*' pkg/types/pki/validation.go || true
rg -n --type=go '\bfips\b' pkg/types/pki/validation.go
# 2) Check whether lint script enables revive/unparam/unuedparam-style checks
sed -n '1,200p' hack/go-lint.sh 2>/dev/null || true
rg -n 'revive|unparam|unusedparam|parameter' hack -S || trueRepository: openshift/installer
Length of output: 1113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for revive/unparam/unused parameter linters in the golangci-lint config
rg -n --hidden --no-ignore-vcs '(revive|unparam|unusedparam|unuedparam|gocritic.*(unparam|unused)|stylecheck|U1000|unused|dupl|ineffassign)' .golangci-lint-v2.yaml
# Show the relevant enabled-linters section(s) if present
rg -n --hidden --no-ignore-vcs 'enabled-linters|disabled-linters|linters:' .golangci-lint-v2.yamlRepository: openshift/installer
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,130p' .golangci-lint-v2.yamlRepository: openshift/installer
Length of output: 2658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on revive + any parameter/unused-related linters in the config
sed -n '1,220p' .golangci-lint-v2.yamlRepository: openshift/installer
Length of output: 2872
🏁 Script executed:
rg -n 'unparam|unusedparam|U1000' .golangci-lint-v2.yamlRepository: openshift/installer
Length of output: 45
🏁 Script executed:
rg -n --type=go '\bfips\b' pkg/types | head -n 50Repository: openshift/installer
Length of output: 1533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=go '\bfips\b' pkg/types | head -n 50Repository: openshift/installer
Length of output: 1533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1500,1560p' pkg/types/validation/installconfig.goRepository: openshift/installer
Length of output: 2745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,160p' pkg/types/pki/validation_test.goRepository: openshift/installer
Length of output: 3660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,340p' pkg/types/pki/validation_test.goRepository: openshift/installer
Length of output: 2512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1510,1555p' pkg/types/validation/installconfig.goRepository: openshift/installer
Length of output: 2036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,160p' pkg/types/pki/validation_test.goRepository: openshift/installer
Length of output: 3660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,340p' pkg/types/pki/validation_test.goRepository: openshift/installer
Length of output: 2512
Apply fips to PKI signer-key validation (or remove it)
pkg/types/pki/validation.go threads the fips flag through ValidatePKIConfig → ValidateKeyConfig → validateRSAKeyConfig/validateECDSAKeyConfig, but never uses it to change any validation behavior. pkg/types/pki/validation_test.go also exercises tc.fips without asserting any FIPS-specific differences.
- Implement the intended FIPS-mode restrictions for PKI signer key parameters, or drop the
fipsparameter from these validators until it’s needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/types/pki/validation.go` around lines 12 - 116, The fips bool is never
used; either implement FIPS-specific checks or remove it — remove it here: drop
the fips parameter from ValidatePKIConfig, ValidateKeyConfig,
validateRSAKeyConfig, and validateECDSAKeyConfig (and from any callers/tests),
update their signatures to not accept fips, and remove the fips forwarding in
ValidatePKIConfig → ValidateKeyConfig and ValidateKeyConfig →
validateRSAKeyConfig/validateECDSAKeyConfig so the validators remain consistent
and compile.
Contributor
Author
|
/hold Depends on #10593 |
openshift-ci
Bot
added
the
do-not-merge/hold
Add the configurable PKI API surface to InstallConfig behind the ConfigurablePKI feature gate. When the gate is active, the installer generates a config.openshift.io/v1alpha1 PKI custom resource manifest that day-2 operators use for certificate rotation parameters. The default PKI profile uses RSA-4096 until all day-2 operators (CKAO, CKMO, etc.) support ECDSA rotation. When pki is not specified in install-config the PKI CR uses mode: Default. When pki is specified the PKI CR uses mode: Custom with DefaultPKIProfile as the base and user signerCertificates overrides layered on top. No certificate generation changes are included — all certs remain RSA-2048. Non-TechPreview clusters are completely unaffected. Assisted-by: Claude Code (Opus 4.6)
Refactor pkg/asset/tls/ to support generating signer certificates with configurable key algorithms (RSA or ECDSA). PrivateKeyToPem now returns ([]byte, error) instead of calling logrus.Fatalf, and GenerateSelfSignedCertificate accepts PrivateKeyParams to control key generation. KeyUsage flags are set based on the algorithm since ECDSA keys cannot perform key encipherment. All signer certs pass nil for pkiConfig in this commit, preserving the existing RSA-2048 behavior. Wiring signers to read PKI config is deferred to a follow-up to avoid breaking codepaths that generate signer certs without an install-config on disk (e.g. agent create certificates, node-joiner add-nodes). Assisted-by: Claude Code (Opus 4.6)
hasbro17
force-pushed
the
pki-2-tls-refactor
branch
from
ed2ed81 to
cb0bd95
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. |
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
2 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.
Summary
Part 2 of splitting #10396 into smaller PRs. Depends on #10593.
Refactors
pkg/asset/tls/to support generating signer certificates with configurable key algorithms (RSA or ECDSA):PrivateKeyToPemnow returns([]byte, error)instead of callinglogrus.FatalfPemToPrivateKeysupports both RSA and ECDSA private keysGenerateSelfSignedCertificateaccepts*PrivateKeyParamsto control key algorithm, size, and curveSelfSignedCertKey.Generateaccepts*types.PKIConfigto pass through PKI configurationKeyUsageflags are set based on the key algorithm (ECDSA keys cannot perform key encipherment)GenerateRSAPrivateKey,GenerateECDSAPrivateKey,PKIConfigToKeyParamsAll signer certs pass
nilforpkiConfigin this commit, preserving the existing RSA-2048 behavior. Wiring signers to read PKI config is deferred to a follow-up to avoid breaking codepaths that generatesigner certs without an install-config on disk (e.g.
agent create certificates,node-joiner add-nodes).PR chain
Summary by CodeRabbit
New Features
Bug Fixes