CNTRLPLANE-2012: Refactor TLS cert generation to support configurable key algorithms#10594
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR generalizes the TLS PKI layer from RSA-only to support both RSA and ECDSA key generation. It introduces ChangesMulti-algorithm PKI support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
|
/hold Depends on #10593 |
openshift-ci
Bot
added
the
do-not-merge/hold
cb0bd95 to
96bfc2d
Compare
96bfc2d to
194d9f5
Compare
|
Rebased onto #10593 which is not yet merged. |
194d9f5 to
7e5b690
Compare
|
/retest-required |
|
@hasbro17 Let's rebase this PR now that the part-1 PR is merged? |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
Verification AnalysisSigner Certificate Verification — RSA-2048 Behavior PreservedFrom
No PKI CR manifest in rendered assets (correct — non-TechPreview job, ConfigurablePKI gate disabled). CI Results
Unit Test CoverageNew tests verify configurable key algorithm support:
|
|
/verified by "TestSelfSignedCertKeyGenerateWithPKIConfig,TestCrossAlgorithmCertificateSigning,TestKeyUsageForAlgorithm,TestSignatureAlgorithmAutoDetection,TestPrivateKeyToPemRoundtrip",pull-ci-openshift-installer-main-e2e-aws-ovn |
openshift-ci-robot
added
the
verified
|
@sanchezl: This PR has been marked as verified by 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. |
7e5b690 to
6102421
Compare
openshift-ci-robot
removed
the
verified
|
/retest |
|
/test e2e-agent-compact-ipv4 |
|
/retest |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-fips |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-single-node-techpreview |
|
@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/05ad4210-793e-11f1-9c73-909afdf30fe2-0 |
|
@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/064f0050-793e-11f1-9c55-90fce9d7e935-0 |
|
@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/06e1a360-793e-11f1-9b68-4fbe5e833d61-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-fips |
|
@tthvo: trigger 2 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/f0a7f110-7962-11f1-8a28-ee7e4521dd30-0 |
|
/retest |
|
/test e2e-metal-ipi-ovn e2e-metal-single-node-live-iso |
|
/retest ci/prow/e2e-aws-ovn-techpreview ci/prow/e2e-azure-ovn ci/prow/e2e-metal-ipi-ovn ci/prow/e2e-metal-single-node-live-iso |
|
/test e2e-aws-ovn-techpreview e2e-azure-ovn e2e-metal-ipi-ovn e2e-metal-single-node-live-iso |
|
/retest |
|
/retest-required |
|
@coderabbitai Following #10594 (comment), let's do the same analysis again on the latest e2e jobs. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr checks 10594 --repo openshift/installer --required=false 2>&1 | head -150
echo "---detailed statuses---"
gh api repos/openshift/installer/commits/pki-2-tls-refactor/status --paginate 2>&1 | python3 -c "
import json,sys
d=json.load(sys.stdin)
for s in d.get('statuses', []):
print(s.get('context'), s.get('state'), s.get('target_url'))
" 2>&1 | sort | head -300Length of output: 10418 🏁 Script executed: #!/bin/bash
set -euo pipefail
pip install pyyaml cryptography -q 2>/dev/null || true
declare -A JOBS=(
[e2e-aws-ovn-fips]="pull-ci-openshift-installer-main-e2e-aws-ovn-fips:2072717973004488704"
[e2e-aws-ovn-single-node]="pull-ci-openshift-installer-main-e2e-aws-ovn-single-node:2072717973042237440"
[e2e-aws-ovn-upgrade]="pull-ci-openshift-installer-main-e2e-aws-ovn-upgrade:2072717973163872256"
[e2e-aws-ovn-techpreview]="pull-ci-openshift-installer-main-e2e-aws-ovn-techpreview:2074511197255766016"
[e2e-metal-assisted]="pull-ci-openshift-installer-main-e2e-metal-assisted:2072540769855475712"
[e2e-metal-ipi-ovn]="pull-ci-openshift-installer-main-e2e-metal-ipi-ovn:2074511203127791616"
[e2e-metal-ovn-two-node-arbiter]="pull-ci-openshift-installer-main-e2e-metal-ovn-two-node-arbiter:2072540784107720704"
)
for shortname in "${!JOBS[@]}"; do
IFS=":" read -r job build <<< "${JOBS[$shortname]}"
echo "=== $shortname (build=$build) ==="
PREFIX="pr-logs/pull/openshift_installer/10594/${job}/${build}/artifacts/${shortname}/"
LIST=$(curl -s "https://storage.googleapis.com/storage/v1/b/test-platform-results/o?prefix=${PREFIX}&delimiter=/")
echo "$LIST" | python3 -c "import json,sys; d=json.load(sys.stdin); print('\n'.join(d.get('prefixes',[])))" | grep -E "gather-extra|gather-must-gather|common-gather" || echo " (no gather step found)"
doneLength of output: 3759 🏁 Script executed: #!/bin/bash
set -euo pipefail
pip install pyyaml cryptography -q 2>/dev/null || true
declare -A JOBS=(
[e2e-aws-ovn-fips]="pull-ci-openshift-installer-main-e2e-aws-ovn-fips:2072717973004488704"
[e2e-aws-ovn-single-node]="pull-ci-openshift-installer-main-e2e-aws-ovn-single-node:2072717973042237440"
[e2e-aws-ovn-upgrade]="pull-ci-openshift-installer-main-e2e-aws-ovn-upgrade:2072717973163872256"
[e2e-aws-ovn-techpreview]="pull-ci-openshift-installer-main-e2e-aws-ovn-techpreview:2074511197255766016"
[e2e-metal-ovn-two-node-arbiter]="pull-ci-openshift-installer-main-e2e-metal-ovn-two-node-arbiter:2072540784107720704"
)
mkdir -p /tmp/certs2
for shortname in "${!JOBS[@]}"; do
IFS=":" read -r job build <<< "${JOBS[$shortname]}"
URL="https://storage.googleapis.com/test-platform-results/pr-logs/pull/openshift_installer/10594/${job}/${build}/artifacts/${shortname}/gather-extra/artifacts/inspect/namespaces/openshift-kube-apiserver-operator/core/secrets.yaml"
OUT="/tmp/certs2/${shortname}.yaml"
status=$(curl -s -o "$OUT" -w "%{http_code}" "$URL")
echo "$shortname: HTTP $status size=$(wc -c < "$OUT" 2>/dev/null || echo 0)"
doneLength of output: 1512 🏁 Script executed: #!/bin/bash
set -euo pipefail
pip install pyyaml cryptography -q 2>/dev/null || true
python3 << 'EOF'
import yaml, base64, glob, os
from cryptography import x509
from cryptography.hazmat.backends import default_backend
from cryptography.hazmat.primitives.asymmetric import rsa, ec
for path in sorted(glob.glob('/tmp/certs2/*.yaml')):
shortname = os.path.basename(path).replace('.yaml','')
with open(path) as f:
content = f.read()
if not content.strip() or content.strip().startswith('<'):
print(f"=== {shortname}: not valid yaml/empty ===")
continue
print(f"\n=== {shortname} ===")
docs = yaml.safe_load(content)
items = docs.get('items', docs if isinstance(docs, list) else [])
found = False
for item in items:
name = item.get('metadata', {}).get('name', '')
if 'signer' not in name:
continue
data = item.get('data', {})
crt_b64 = data.get('tls.crt')
if not crt_b64:
continue
found = True
crt_der = base64.b64decode(crt_b64)
cert = x509.load_pem_x509_certificate(crt_der, default_backend())
pub = cert.public_key()
if isinstance(pub, rsa.RSAPublicKey):
alg = f"RSA-{pub.key_size}"
elif isinstance(pub, ec.EllipticCurvePublicKey):
alg = f"ECDSA-{pub.curve.name}"
else:
alg = str(type(pub))
ku = cert.extensions.get_extension_for_class(x509.KeyUsage).value
usages = [a for a in ['digital_signature','key_encipherment','key_cert_sign','crl_sign'] if getattr(ku, a, False)]
print(f" {name}: {alg} KeyUsage={usages}")
if not found:
print(" No signer secrets found!")
EOF
echo "---checking techpreview gather-extra structure---"
JOB="pull-ci-openshift-installer-main-e2e-aws-ovn-techpreview"
BUILD="2074511197255766016"
PREFIX="pr-logs/pull/openshift_installer/10594/${JOB}/${BUILD}/artifacts/e2e-aws-ovn-techpreview/gather-extra/artifacts/inspect/namespaces/"
curl -s "https://storage.googleapis.com/storage/v1/b/test-platform-results/o?prefix=${PREFIX}&delimiter=/" | python3 -c "
import json,sys
d=json.load(sys.stdin)
prefixes = d.get('prefixes',[])
matches = [p for p in prefixes if 'kube-apiserver' in p]
print('\n'.join(matches) if matches else 'no kube-apiserver namespace dirs found')
print('total namespace dirs:', len(prefixes))
"Length of output: 6291 🏁 Script executed: #!/bin/bash
set -euo pipefail
JOB="pull-ci-openshift-installer-main-e2e-aws-ovn-techpreview"
BUILD="2074511197255766016"
curl -s "https://storage.googleapis.com/test-platform-results/pr-logs/pull/openshift_installer/10594/${JOB}/${BUILD}/finished.json" | python3 -m json.tool 2>/dev/null | head -20
echo "---metal-ipi-ovn---"
JOB2="pull-ci-openshift-installer-main-e2e-metal-ipi-ovn"
BUILD2="2074511203127791616"
curl -s "https://storage.googleapis.com/test-platform-results/pr-logs/pull/openshift_installer/10594/${JOB2}/${BUILD2}/finished.json" | python3 -m json.tool 2>/dev/null | head -20Length of output: 1473 Enough evidence gathered to compose a concise final analysis. Latest e2e job verification (signer CA cert algorithm/KeyUsage)
So for the jobs with usable artifacts, RSA-2048 preservation continues to hold as expected (no regression from the previous check). No ECDSA signers showed up in this batch — the prior ECDSA P-384 finding on TechPreview came from an earlier successful TechPreview run; this latest TechPreview attempt failed before reaching a gatherable state, so I can't re-confirm the operator-side ECDSA rotation behavior on this specific build. I'd suggest re-running this check once |
|
/verified by CI I'm happy with the e2e results so far. Let's get this in early so if something breaks, we will notice it during the week. |
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by 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. |
|
/skip |
|
/tide refresh |
|
/retest-required |
|
@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. |
5 participants
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