CNF-25515: feat: add Ed25519 key and certificate support - #1817
sebrandon1 wants to merge 1 commit into
Conversation
|
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. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: WalkthroughThis change adds Ed25519 private and public key support across parsing, serialization, generation, signing, certificate conversion, renewal, distributed-key persistence, and JWT handling. RSA and EC paths remain supported. ChangesEd25519 key and certificate support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Certificate
participant crypto_objects
participant crypto_utils
participant OpenSSL
Certificate->>crypto_objects: process Ed25519 PEM certificate
crypto_objects->>OpenSSL: extract certificate public key
OpenSSL-->>crypto_objects: Ed25519 public-key PEM
crypto_objects->>crypto_utils: parse Ed25519 PKCS#8 key
crypto_utils->>OpenSSL: sign with pkeyutl -rawin
OpenSSL-->>crypto_utils: Ed25519 signature
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3804cee to
3b2f948
Compare
![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
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)
154-176: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
.stderr(Stdio::piped())makes error messages from this helper always empty.
commandonly pipesstdin/stdout;stderrdefaults toinherit()forspawn(). Whenopenssl x509 -pubkeyfails,output.stderrwill be empty, so thebail!at line 172 always reports"openssl failed: "with no detail — while the sibling helperpubkey_pem_from_pkcs8_der(crypto_utils.rs) correctly pipes stderr for the same kind of call. This is now reached by bothfrom_ec_cert_bytesand the newfrom_ed25519_cert_bytes.🩹 Proposed fix
let mut command = Command::new("openssl") .arg("x509") .arg("-pubkey") .arg("-noout") .stdin(Stdio::piped()) .stdout(Stdio::piped()) + .stderr(Stdio::piped()) .spawn() .context("running openssl")?;🤖 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 154 - 176, Update pubkey_pem_from_cert_pem to configure the openssl Command with stderr piped before spawning, matching the sibling helper’s subprocess setup. Preserve the existing wait_with_output and bail! handling so openssl failure details remain included in output.stderr.
🤖 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 `@identified-work.md`:
- Around line 281-283: Reconcile the opportunity count in the summary with the
listed identifiers A1-A4, B1-B5, C1-C3, and D1-D2: either update “Total
Identified” to 14 and adjust the corresponding effort estimate, or add the
missing opportunity identifier and its details.
- Around line 53-59: Correct the A2 public-API inventory by removing
dataurl_encode from the listed public functions, or explicitly identifying it as
an internal helper because its visibility is pub(crate). Keep the documentation
scope focused on the genuinely public functions such as commit_file, globvec,
and read_file_to_string.
- Around line 88-97: Update the `LOG_RECORDS` call sites in `logging.rs`,
including the usages around lines 62 and 94, to initialize/access the
`OnceLock<Arc<Mutex<Vec<String>>>>` via `get_or_init(...).lock()` or a shared
helper instead of calling `LOG_RECORDS.lock()` directly; preserve the existing
record access behavior and complete the `lazy_static` removal.
---
Outside diff comments:
In `@src/cluster_crypto/keys.rs`:
- Around line 154-176: Update pubkey_pem_from_cert_pem to configure the openssl
Command with stderr piped before spawning, matching the sibling helper’s
subprocess setup. Preserve the existing wait_with_output and bail! handling so
openssl failure details remain included in output.stderr.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d98a0a06-cd62-4a6b-828e-0933d01363f9
📒 Files selected for processing (10)
identified-work.mdsrc/cluster_crypto/cert_key_pair.rssrc/cluster_crypto/certificate.rssrc/cluster_crypto/crypto_objects.rssrc/cluster_crypto/crypto_utils.rssrc/cluster_crypto/crypto_utils/jwt.rssrc/cluster_crypto/distributed_private_key.rssrc/cluster_crypto/distributed_public_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
0cea5cb to
aa28854
Compare
327e153 to
c4714ff
Compare
c4714ff to
dad652b
Compare
|
[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 |
6f1af0c to
3021ebc
Compare
|
The |
|
/retest |
36d4a95 to
fda730f
Compare
bbf1c61 to
9c65b32
Compare
|
openshift/release#82217 is now merged — issuing retests to see if it passes. /retest |
9c65b32 to
5d98f69
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/cluster_crypto/crypto_utils.rs`:
- Around line 21-62: Update ed25519_pkcs8_to_v2 to parse and validate the PKCS#8
ASN.1 structure before extracting the private-key seed, instead of assuming it
is always at der[16..48]. Reject malformed or unexpected layouts, while allowing
valid optional attributes, then construct the v2 DER from the parsed 32-byte
seed and public key. Add fixtures covering non-default layouts and
attribute-bearing keys.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dfe433f-3a54-4626-a821-149cd9ce619e
📒 Files selected for processing (7)
src/cluster_crypto/cert_key_pair.rssrc/cluster_crypto/crypto_objects.rssrc/cluster_crypto/crypto_utils.rssrc/cluster_crypto/crypto_utils/jwt.rssrc/cluster_crypto/distributed_private_key.rssrc/cluster_crypto/distributed_public_key.rssrc/cluster_crypto/keys.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/cluster_crypto/crypto_utils/jwt.rs
- src/cluster_crypto/distributed_public_key.rs
- src/cluster_crypto/distributed_private_key.rs
- src/cluster_crypto/cert_key_pair.rs
- src/cluster_crypto/crypto_objects.rs
- src/cluster_crypto/keys.rs
|
/retest |
1 similar comment
|
/retest |
5eeed04 to
9cc9cb9
Compare
sebrandon1
changed the title
|
@sebrandon1: This pull request references CNF-25515 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. |
456389f to
7be2b2d
Compare
6b00d17 to
ef90f08
Compare
Port remaining compile and Ed25519 crypto fixes from PR 1941 so recert builds: bring DecodeRsaPublicKey into scope, convert OpenSSL PKCS#8 v0 keys to ring-compatible v2, and accept Ed25519 SPKI PUBLIC KEY PEMs. Leave e2e tests on 1941 and P-384 SHA-384 signing on 1829. Co-authored-by: Cursor <cursoragent@cursor.com>
ef90f08 to
3deb8a4
Compare
|
@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. |
|
Superseded by #1941 (recert-e2e), which implements Ed25519 support differently (DER-level workaround instead of vendored code changes) and also adds Ed25519 JWT signing. |
Includes unit tests salvaged from rh-ecosystem-edge#1817 and rh-ecosystem-edge#1829 (now closed as superseded by this PR). Co-Authored-By: Claude <noreply@anthropic.com>
Bring remaining Ed25519 test cleanup and SEC1 PKCS#8 helper refactor that were not included when rh-ecosystem-edge#1941 merged the crypto product code from rh-ecosystem-edge#1817 and rh-ecosystem-edge#1829. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Summary
Ed25519(Bytes)variants toPrivateKeyandPublicKeyenums, unblocking all code paths that previously rejected Ed25519 keys/certificates withbail!ortodo!()openssl genpkey -algorithm Ed25519; signing branches on key type — RSA/ECDSA useopenssl dgst -sha256, Ed25519 usesopenssl pkeyutl -sign -rawin(EdDSA has its own internal hash)1.3.101.112; SKID calculation forLibraryGoSha1/LibraryGoSha256still bails on Ed25519 (RSA-specific Go methods; Ed25519 certs use RFC5280/RFC7093 which are algorithm-agnostic)Test plan
cargo buildcompilescargo test— all existing + 17 new tests passcargo clippy/cargo fmt --check— cleanuse_keyrule pointing to the Ed25519 keyCloses #1739
Ref: CNF-25515
Summary by CodeRabbit
New Features
Bug Fixes