Repository navigation
Conversation
omertuc
commented
Oct 19, 2026
omertuc
commented
Member
- Stop using ring directly, find cert signers by key with openssl
- Re-vendor after dropping the direct ring dependency
- Replace x509-certificate with x509-cert, dropping ring entirely
- Re-vendor after dropping x509-certificate
- Refuse to break AKIDs that name the issuer's issuer
recert no longer calls into ring anywhere. The direct ring dependency is dropped; ring 0.16 is still pulled in by x509-certificate, but only its ASN.1 types are used. Keys: signing keys no longer go through ring's InMemorySigningKeyPair. Public key derivation is done with `openssl pkey -pubout`, and the key algorithm comes from parsing the PKCS#8 structure. Cert signer discovery: previously x509-certificate (ring) verified signatures, falling back to a name-bound `openssl verify` when it didn't support the algorithm. x509-certificate picks the verification algorithm from the signee's own key type rather than the signer's, so every cert whose key type differs from its CA's went down the fallback path, which also didn't allow SHA-1 signatures under RHEL/Fedora crypto policies. Such signers were silently missed. Signers are now found purely by public key, with no name matching. To avoid spawning openssl for every (signer, signee) pair, one `openssl verify` runs per distinct public key, checking all signees at once against synthetic trust anchors carrying that key, crafted so that the signature check is the only thing that can fail. Anything other than "OK" or a signature mismatch is a hard error. If SHA-1 signed certs are present and openssl can't verify SHA-1 signatures (FIPS mode), we error out instead of missing their signers. FIPS: tests that need Ed25519 or SHA-1 signatures (both unavailable in the RHEL 9 FIPS provider) are skipped in FIPS mode, and the SHA-1 error path is tested instead. `make fips-test` runs the unit and e2e tests in a UBI9 container with the FIPS provider forced on, and a new workflow runs it in CI. CONTAINER_ENGINE selects docker/podman for the test container targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regenerated with cargo-vendor-filterer. ring 0.17 (and its spin 0.9 and untrusted 0.9 dependencies) are gone, so the remaining ring 0.16, spin 0.5 and untrusted 0.7 are now vendored without a version suffix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
x509-certificate (and through it, ring and bcder) was only used for parsing and re-encoding certificates and keys. Those are now done with RustCrypto's format crates (x509-cert, der, spki, pkcs8), which contain no cryptography, so ring is no longer part of the build at all. recert now has its own small KeyAlgorithm/EcdsaCurve types (algorithms.rs) instead of x509-certificate's, and Certificate holds an x509_cert::Certificate plus the original DER bytes (what the signature covers, and what gets hashed for external cert matching). Output is unchanged except for these fixes: - ECDSA signature AlgorithmIdentifiers no longer carry NULL parameters, which x509-certificate always added but RFC 5758 says must be absent. - The fix_ed25519_alg_id post-processing hack is gone; Ed25519 AlgorithmIdentifiers are encoded correctly (no parameters) directly. - The JWT "kid" for Ed25519 keys is now computed from the correct SubjectPublicKeyInfo. It used to be computed from x509-certificate's, which has the X25519 OID instead of the Ed25519 one. - Signing keys keep the SubjectPublicKeyInfo exactly as openssl derived it, rather than rebuilding it from the key algorithm and key bits. - Expiration mutations support certs using GeneralizedTime (they used to bail), and use GeneralizedTime for dates after 2049 as RFC 5280 requires (they used to write a UTCTime with the wrong century). - CN replacement only rewrites common names that actually change, so untouched CNs of string types we can't write no longer cause errors. Verified by diffing the structure of certs regenerated with CN/SAN replacement and expiration extension (RSA, P-256, P-384, Ed25519, multi attribute and UTF8String names, AKID with issuer and serial) between the old and new code; the only difference is the ECDSA NULL parameters. The cert signer relationships and summary metadata of a 210 cert set are identical too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regenerated with cargo-vendor-filterer. Removes ring, untrusted, bcder, x509-certificate, web-sys and pem 2.0.1, and stubs out cc, which is no longer built for any supported platform now that ring is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An AKID can identify the issuing cert by its issuer name and serial number (authorityCertIssuer + authorityCertSerialNumber). We update the serial number when re-signing, but CN replacement doesn't touch the name in the AKID, so renaming such a cert's CA's issuer produced a cert whose AKID points at a name that no longer exists, which openssl rejects when building the chain. Go never writes this AKID form, so OCP certs don't have it. Rather than supporting it, error out when CN replacement would change the name in the AKID. Certs using that form are still fine as long as the name isn't being replaced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 855 files, which is 555 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. ⚙️ Run configuration
⛔ Files ignored due to path filters: 45
📒 Files selected for processing: 855
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/test e2e-aws-ovn-single-node-recert-serial |
Contributor
|
PR needs rebase. 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. |
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.