CNF-26737: fix: use correct PRIVATE KEY PEM tag for ECDSA PKCS#8 keys - #1827
sebrandon1 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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 |
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
WalkthroughEC private-key handling now converts SEC1 inputs through PKCS#8 parsing. EC serialization and persistence use the ChangesEC PEM label normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ECPrivateKeyPEM
participant OpenSSL
participant process_pem_ec_private_key
participant process_pem_private_key
ECPrivateKeyPEM->>OpenSSL: convert SEC1 PEM to PKCS#8 PEM
OpenSSL->>process_pem_ec_private_key: return converted PEM
process_pem_ec_private_key->>process_pem_private_key: parse PKCS#8 private key
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
f097fff to
ffffc89
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cluster_crypto/distributed_private_key.rs (1)
107-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
PrivateKey::pem()to eliminate code duplication.The
PrivateKeyenum already implements apem()method that encapsulates this exact serialization logic (as seen insrc/cluster_crypto/keys.rs). Refactor this block to use.pem()directly to keep the domain logic DRY, ensuring consistency with how it is done incommit_k8s_private_key.♻️ Proposed refactor
- let private_key_pem = match &self.key_regenerated.clone().context("key was no regenerated")? { - PrivateKey::Rsa(rsa_private_key) => pem::Pem::new("RSA PRIVATE KEY", rsa_private_key.to_pkcs1_der()?.as_bytes()), - PrivateKey::Ec(ec_bytes) => pem::Pem::new("PRIVATE KEY", ec_bytes.as_ref()), - }; + let private_key_pem = self.key_regenerated.clone().context("key was no regenerated")?.pem()?;🤖 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 `@src/cluster_crypto/distributed_private_key.rs` around lines 107 - 110, Replace the duplicated RSA/EC serialization match in the key regeneration flow with the existing `PrivateKey::pem()` method, while preserving the current `key_regenerated` context/error handling. Follow the usage pattern in `commit_k8s_private_key` and keep the resulting PEM value behavior unchanged.
🤖 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.
Nitpick comments:
In `@src/cluster_crypto/distributed_private_key.rs`:
- Around line 107-110: Replace the duplicated RSA/EC serialization match in the
key regeneration flow with the existing `PrivateKey::pem()` method, while
preserving the current `key_regenerated` context/error handling. Follow the
usage pattern in `commit_k8s_private_key` and keep the resulting PEM value
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3acd642-6826-43fd-bc6c-fcebf093b16a
📒 Files selected for processing (4)
src/cluster_crypto/crypto_objects.rssrc/cluster_crypto/distributed_private_key.rssrc/cluster_crypto/keys.rssrc/ocp_postprocess/cluster_domain_rename/etcd_rename.rs
💤 Files with no reviewable changes (1)
- src/ocp_postprocess/cluster_domain_rename/etcd_rename.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cluster_crypto/keys.rs
eca1e18 to
ceda4a9
Compare
ceda4a9 to
c2e118b
Compare
|
Hey @omertuc, could you mark these ok-to-test when you get a chance? All builds are passing on GitHub Actions. Thanks! |
|
/ok-to-test |
|
/retest |
1 similar comment
|
/retest |
02a6daf to
74fc42c
Compare
|
The |
|
/retest |
65bdd57 to
4c24167
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster_crypto/keys.rs (1)
36-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the EC private key serialization contract in sync with RSA.
PrivateKey::Rsaserializes as Base64, whilePrivateKey::Ecserializes as raw PEM text (BEGIN PRIVATE KEY). Consumers that handlePrivateKeyas one JSON string type will decode RSA correctly but get invalid key material for EC. Align the EC branch with the existing RSA/Base64 contract and adjust only the serialization-focused test assertion if needed.🔧 Proposed fix to align encodings
- Self::Ec(_) => serializer.serialize_str(&self.pem().map_err(serde::ser::Error::custom)?.to_string()), + Self::Ec(_) => serializer.serialize_str(&base64_standard.encode(self.pem().map_err(serde::ser::Error::custom)?.to_string())),🤖 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 `@src/cluster_crypto/keys.rs` around lines 36 - 46, Update the EC branch in PrivateKey’s Serialize implementation to Base64-encode the PEM bytes using the same base64_standard encoder as the Rsa branch, while preserving the existing PEM generation and error propagation. Adjust only the serialization-focused test assertion if it expects raw EC PEM text.
🤖 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.
Outside diff comments:
In `@src/cluster_crypto/keys.rs`:
- Around line 36-46: Update the EC branch in PrivateKey’s Serialize
implementation to Base64-encode the PEM bytes using the same base64_standard
encoder as the Rsa branch, while preserving the existing PEM generation and
error propagation. Adjust only the serialization-focused test assertion if it
expects raw EC PEM text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e62f6b71-5000-4ce0-b9e1-c579c16508e5
📒 Files selected for processing (3)
src/cluster_crypto/crypto_objects.rssrc/cluster_crypto/distributed_private_key.rssrc/cluster_crypto/keys.rs
fe2ccd0 to
8d3a434
Compare
|
openshift/release#82217 is now merged — issuing retests to see if it passes. /retest |
8d3a434 to
e069ba6
Compare
|
/retest |
1 similar comment
|
/retest |
c6bfa31 to
22cd35b
Compare
sebrandon1
changed the title
|
@sebrandon1: This pull request references CNF-26737 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 the "5.1.0" version, but no target version was set. 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. |
ef8fee6 to
57aac7f
Compare
d1e882e to
bb816cb
Compare
bb816cb to
55e7cc4
Compare
|
@sebrandon1: This pull request references CNF-26737 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 the "5.1.0" version, but no target version was set. 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. |
|
@sebrandon1: This pull request references CNF-26737 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 the "5.1.0" version, but no target version was set. 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. |
|
@sebrandon1: 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
Summary
PRIVATE KEY(RFC 5958) instead ofEC PRIVATE KEYwhen serializing ECDSA PKCS#8 keys stored inPrivateKey::EcSerializeand filesystem commits throughPrivateKey::pem()so output tags stay consistentEC PRIVATE KEY) input still converts via openssl to PKCS#8, then reusesprocess_pem_private_keyRelated PRs
Jira
Upstream References
Test Plan
PrivateKey::Ecpem()/ Serialize emitBEGIN PRIVATE KEY, notBEGIN EC PRIVATE KEYtest_ec_serialize_uses_correct_pem_tag,test_ec_pkcs8_full_round_trip_der_equalitytest_crypto_ec_pkcs8_input.shpasses