Convert certificates without using openssl#3665
Convert certificates without using openssl#3665skitt wants to merge 1 commit intosubmariner-io:develfrom
Conversation
|
🤖 Created branch: z_pr3665/skitt/internal-certificate-parsing |
skitt
force-pushed
the
internal-certificate-parsing
branch
6 times, most recently
from
c19c49c to
f7ed239
Compare
submariner-bot
force-pushed
the
z_pr3664/tpantelis/adj_cert_handler
branch
from
d1f90b3 to
c83f08e
Compare
submariner-bot
changed the base branch from
z_pr3664/tpantelis/adj_cert_handler
to
devel
|
🤖 The base of this PR has been updated to devel |
skitt
force-pushed
the
internal-certificate-parsing
branch
from
f7ed239 to
140173a
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
🤖 Closed branches: [z_pr3665/skitt/internal-certificate-parsing] |
|
🤖 Created branch: z_pr3665/skitt/internal-certificate-parsing |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
|
🤖 Closed branches: [z_pr3665/skitt/internal-certificate-parsing] |
|
🤖 Created branch: z_pr3665/skitt/internal-certificate-parsing |
skitt
force-pushed
the
internal-certificate-parsing
branch
from
140173a to
fe288b9
Compare
WalkthroughParses PEM certificates and private keys in-memory, encodes PKCS#12 blobs via github.com/cert-manager/go-pkcs12, writes the .p12 blob to a temp file and imports it into NSS (pk12util); tests now generate a CA and per-name client certificates programmatically; go.mod dependencies updated. Changes
Sequence DiagramsequenceDiagram
participant Handler as Certificate Handler
participant PEM as PEM Parser (encoding/pem, x509)
participant PKCS12 as PKCS#12 Encoder (go-pkcs12)
participant File as File System (temp .p12)
participant NSS as NSS (pk12util)
Handler->>PEM: Decode certificate PEM -> x509.Certificate(s)
PEM-->>Handler: Certificates
Handler->>PEM: Decode private key PEM -> PKCS#1/PKCS#8 key
PEM-->>Handler: Private Key
Handler->>PKCS12: Encode cert(s)+key to PKCS#12 bytes
PKCS12-->>Handler: PKCS#12 bytes
Handler->>File: Write temp .p12 file (blob)
File-->>Handler: Temp file path
Handler->>NSS: Import .p12 via pk12util (file path)
NSS-->>Handler: Import result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skitt
force-pushed
the
internal-certificate-parsing
branch
from
fe288b9 to
c96688d
Compare
|
This PR/issue depends on:
|
skitt
force-pushed
the
internal-certificate-parsing
branch
from
c96688d to
fdd817c
Compare
skitt
requested review from
Oats87,
aswinsuryan,
dfarrell07,
maayanf24,
sridhargaddam,
tpantelis,
vthapar and
yboaron
as code owners
skitt
force-pushed
the
internal-certificate-parsing
branch
from
fdd817c to
a7b2520
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: 5
🤖 Fix all issues with AI agents
In `@pkg/cable/libreswan/certificate_handler_test.go`:
- Around line 171-173: The test's "unchanged cert data" case is wrong: it clones
testCertData and reassigns certificate.CADataKey to caPEM (which is already
present), so it doesn't recreate the prior newCertData used in the previous
assertion; this makes the hash differ and will run certutil -N. Change the test
to use the actual unchanged data from the previous run (i.e., reuse newCertData
from the earlier assertion) or otherwise construct an identical map to the one
previously passed to handler.OnSignedCallback so the hash comparison in
handler.OnSignedCallback detects no change and no certutil -N commands are
expected; update references to newCertData, testCertData, certificate.CADataKey,
and the call to handler.OnSignedCallback accordingly.
- Around line 45-85: The test currently invokes
certificate.CreateCAKeyAndCertificate, x509.CreateCertificate and defines/uses
the createSignedCertificate closure (which itself calls rsa.GenerateKey and uses
Expect) at top-level during spec tree construction; move this setup into a
Ginkgo setup node (BeforeEach or BeforeSuite) and convert the top-level
variables (caKey, caCert, caDER, caPEM, createSignedCertificate, testCertData,
newCertData) to package/test-scope declarations that are assigned inside that
BeforeEach/BeforeSuite, keeping all Expect(...) assertions only inside that
setup block (so use Expect within BeforeEach/BeforeSuite when calling
CreateCAKeyAndCertificate, x509.CreateCertificate and inside
createSignedCertificate) to avoid running Gomega matchers at tree-construction
time.
In `@pkg/cable/libreswan/certificate_handler.go`:
- Around line 99-113: The PEM parsing loop using pem.Decode and
x509.ParseCertificate may leave parsedCert nil if no "CERTIFICATE" block is
found; after the loop that sets parsedCert, add a nil check for parsedCert and
return a clear error (e.g., fmt.Errorf or errors.Wrap) indicating no certificate
was found/parsed before calling pkcs12.Modern.EncodeWithFriendlyName, so you
avoid a panic when parsedCert is nil.
- Around line 115-133: After the PEM decode loop in certificate_handler.go,
ensure you validate that a private key was actually parsed: check if parsedKey
== nil and return a descriptive error (e.g., "no private key found in keyData")
before calling pkcs12.Modern.EncodeWithFriendlyName; this prevents passing a nil
parsedKey into EncodeWithFriendlyName and mirrors the certificate parsing
validation.
- Line 155: The p12File.Close() call is ignoring its error which can hide
write/flush failures; update the code around p12File.Close() (the variable
p12File in certificate_handler.go) to capture its returned error and handle or
propagate it (e.g., err := p12File.Close(); if err != nil { return
fmt.Errorf("closing p12 file: %w", err) } or log and return) before proceeding
to remove the file, ensuring any write/close failures surface instead of being
swallowed.
f4e718e to
f8f1739
Compare
This implements PEM-encoded X.509 certificate parsing using Go crypto, and exports the result using SSLMate's go-pkcs12 package (or rather, cert-manager's fork of go-pkcs12 which adds support for encoding with a friendly name; see SSLMate/go-pkcs12#67 for details). Signed-off-by: Stephen Kitt <skitt@redhat.com>
skitt
force-pushed
the
internal-certificate-parsing
branch
from
f8f1739 to
753bbce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cable/libreswan/certificate_handler.go`:
- Around line 99-165: The certificate parsing currently overwrites parsedCert
for each "CERTIFICATE" block, so change the loop that processes certData to
treat the first parsed CERTIFICATE as the leaf (set parsedCert only if nil) and
append subsequent parsed certificates to a certChain slice
([]*x509.Certificate); then pass certChain (not an empty slice) as the
intermediate chain argument to pkcs12.Modern.EncodeWithFriendlyName along with
parsedKey and parsedCert. Ensure parsedCert is checked for nil after the loop as
before and keep existing error handling around pem.Block types and pkcs12/p12
file creation (references: parsedCert, certData, certChain,
pkcs12.Modern.EncodeWithFriendlyName, parsedKey, p12File).
| // Parse certificate data | ||
| var parsedCert *x509.Certificate | ||
| var err error | ||
|
|
||
| for block, rest := pem.Decode(certData); block != nil; block, rest = pem.Decode(rest) { | ||
| switch block.Type { | ||
| case "CERTIFICATE": | ||
| parsedCert, err = x509.ParseCertificate(block.Bytes) | ||
| if err != nil { | ||
| return errors.Wrap(err, "error parsing certificate data") | ||
| } | ||
| default: | ||
| return fmt.Errorf("unexpected block type %q in certificate data", block.Type) | ||
| } | ||
| } | ||
| defer os.Remove(certFile.Name()) | ||
|
|
||
| if _, err := certFile.Write(certData); err != nil { | ||
| return errors.Wrap(err, "failed to write certificate to temporary file") | ||
| if parsedCert == nil { | ||
| return errors.New("no certificate found in certificate data") | ||
| } | ||
|
|
||
| certFile.Close() | ||
|
|
||
| keyFile, err := os.CreateTemp(RootDir, "submariner-key-*.key") | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to create temporary key file") | ||
| // Parse key data | ||
| var parsedKey any | ||
|
|
||
| for block, rest := pem.Decode(keyData); block != nil; block, rest = pem.Decode(rest) { | ||
| switch block.Type { | ||
| case "PRIVATE KEY": | ||
| parsedKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) | ||
| if err != nil { | ||
| return errors.Wrap(err, "error parsing key data") | ||
| } | ||
| case "RSA PRIVATE KEY": | ||
| parsedKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) | ||
| if err != nil { | ||
| return errors.Wrap(err, "error parsing key data") | ||
| } | ||
| default: | ||
| return fmt.Errorf("unexpected block type %q in key data", block.Type) | ||
| } | ||
| } | ||
| defer os.Remove(keyFile.Name()) | ||
|
|
||
| if _, err := keyFile.Write(keyData); err != nil { | ||
| return errors.Wrap(err, "failed to write key to temporary file") | ||
| if parsedKey == nil { | ||
| return errors.New("no private key found in key data") | ||
| } | ||
|
|
||
| keyFile.Close() | ||
|
|
||
| // Create PKCS#12 file with openssl | ||
| // Export PKCS#12 file | ||
| p12File, err := os.CreateTemp(RootDir, "submariner-client-*.p12") | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to create temporary pkcs12 file") | ||
| } | ||
|
|
||
| defer os.Remove(p12File.Name()) | ||
| p12File.Close() | ||
|
|
||
| // Use empty password for PKCS#12 | ||
| pkcs12Password := "" | ||
|
|
||
| opensslCmd := exec.CommandContext(ctx, "openssl", "pkcs12", "-export", | ||
| "-in", certFile.Name(), | ||
| "-inkey", keyFile.Name(), | ||
| "-out", p12File.Name(), | ||
| "-name", nickname, | ||
| "-passout", "pass:"+pkcs12Password) | ||
| if err := execWithOutput(command.New(opensslCmd)); err != nil { | ||
| return errors.Wrap(err, "failed to create PKCS#12 file") | ||
| pkcsData, err := pkcs12.Modern.EncodeWithFriendlyName(nickname, parsedKey, parsedCert, []*x509.Certificate{}, pkcs12Password) | ||
| if err != nil { | ||
| return errors.Wrap(err, "error encoding to PKCS#12") | ||
| } | ||
|
|
||
| if _, err := p12File.Write(pkcsData); err != nil { | ||
| return errors.Wrap(err, "error writing PKCS#12 file") | ||
| } | ||
|
|
||
| if err := p12File.Close(); err != nil { | ||
| return errors.Wrap(err, "error closing PKCS#12 file") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/cable/libreswan/certificate_handler.go | sed -n '99,165p'Repository: submariner-io/submariner
Length of output: 2413
🌐 Web query:
go-pkcs12 EncodeWithFriendlyName additionalCerts parameter chain order
💡 Result:
In github.com/cert-manager/go-pkcs12, EncodeWithFriendlyName(..., caCerts, ...) (your “additionalCerts”) does not build or sort a chain for you—it just encodes the leaf plus whatever you pass in. The expected/compatible ordering is:
- Leaf (end-entity) cert (
certificate) - Then
caCertsin the slice order you provide
This matches the library’s own decode expectation: DecodeChain treats the first certificate as the leaf and subsequent certificates as the CA chain. Therefore, when encoding, pass caCerts in proper chain order (typically: intermediate closest to the leaf → up toward the root, optionally including the root). [1]
Source: pkg.go.dev docs for EncodeWithFriendlyName / Encode and DecodeChain. [1]
[1] (pkg.go.dev)
Citations:
🌐 Web query:
X.509 certificate chain PEM encoding order leaf intermediate root
💡 Result:
For a PEM-encoded certificate chain (multiple -----BEGIN CERTIFICATE----- blocks concatenated), the conventional and standards-aligned order is:
- Leaf / end-entity certificate (the server/client cert)
- Intermediate CA certificate(s), in sequence so that each cert signs the one immediately before it
- Root CA certificate (typically omitted because clients are expected to already have trust anchors)
This matches TLS requirements: the sender’s certificate must be first, and each following certificate should/must certify the preceding one, with the root allowed to be omitted. [1], [2]
In OpenSSL terms: intermediates go in the “untrusted”/chain bundle (concatenated PEMs), while roots belong in the “trusted” store/CAfile. [3]
So the usual PEM bundle is: leaf -> intermediate(s) (no root). [1], [2], [3]
Sources
[1] RFC 8446 (TLS 1.3), Certificate message ordering and root omission: https://www.ietf.org/rfc/rfc8446.html
[2] IETF Datatracker (TLS 1.2 bis draft) quoting TLS 1.2 Certificate message rules (sender first; each following cert certifies previous; root may be omitted): https://datatracker.ietf.org/doc/html/draft-ietf-tls-rfc5246-bis
[3] OpenSSL verify documentation (separate untrusted intermediates vs trusted roots; multiple PEM certs concatenated): https://docs.openssl.org/master/man1/openssl-verify/
Handle PEM chains so the leaf certificate is used and intermediates preserved.
The certificate parsing loop (lines 103–113) overwrites parsedCert on each CERTIFICATE block. In a multi-certificate PEM file, this means the last certificate becomes the leaf cert passed to EncodeWithFriendlyName, while earlier certificates (including the actual leaf) are discarded. According to go-pkcs12 and standard PEM chain ordering, the first certificate is the leaf and subsequent ones form the chain.
The fix captures the first certificate as the leaf, collects remaining certificates into a chain slice, and passes that chain to EncodeWithFriendlyName:
Proposed fix (leaf + chain handling)
- // Parse certificate data
- var parsedCert *x509.Certificate
- var err error
+ // Parse certificate data
+ var (
+ parsedCert *x509.Certificate
+ certChain []*x509.Certificate
+ err error
+ )
for block, rest := pem.Decode(certData); block != nil; block, rest = pem.Decode(rest) {
- switch block.Type {
- case "CERTIFICATE":
- parsedCert, err = x509.ParseCertificate(block.Bytes)
- if err != nil {
- return errors.Wrap(err, "error parsing certificate data")
- }
- default:
- return fmt.Errorf("unexpected block type %q in certificate data", block.Type)
- }
+ if block.Type != "CERTIFICATE" {
+ return fmt.Errorf("unexpected block type %q in certificate data", block.Type)
+ }
+
+ cert, err := x509.ParseCertificate(block.Bytes)
+ if err != nil {
+ return errors.Wrap(err, "error parsing certificate data")
+ }
+
+ if parsedCert == nil {
+ parsedCert = cert
+ } else {
+ certChain = append(certChain, cert)
+ }
}
if parsedCert == nil {
return errors.New("no certificate found in certificate data")
}
@@
- pkcsData, err := pkcs12.Modern.EncodeWithFriendlyName(nickname, parsedKey, parsedCert, []*x509.Certificate{}, pkcs12Password)
+ pkcsData, err := pkcs12.Modern.EncodeWithFriendlyName(nickname, parsedKey, parsedCert, certChain, pkcs12Password)
if err != nil {
return errors.Wrap(err, "error encoding to PKCS#12")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Parse certificate data | |
| var parsedCert *x509.Certificate | |
| var err error | |
| for block, rest := pem.Decode(certData); block != nil; block, rest = pem.Decode(rest) { | |
| switch block.Type { | |
| case "CERTIFICATE": | |
| parsedCert, err = x509.ParseCertificate(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing certificate data") | |
| } | |
| default: | |
| return fmt.Errorf("unexpected block type %q in certificate data", block.Type) | |
| } | |
| } | |
| defer os.Remove(certFile.Name()) | |
| if _, err := certFile.Write(certData); err != nil { | |
| return errors.Wrap(err, "failed to write certificate to temporary file") | |
| if parsedCert == nil { | |
| return errors.New("no certificate found in certificate data") | |
| } | |
| certFile.Close() | |
| keyFile, err := os.CreateTemp(RootDir, "submariner-key-*.key") | |
| if err != nil { | |
| return errors.Wrap(err, "failed to create temporary key file") | |
| // Parse key data | |
| var parsedKey any | |
| for block, rest := pem.Decode(keyData); block != nil; block, rest = pem.Decode(rest) { | |
| switch block.Type { | |
| case "PRIVATE KEY": | |
| parsedKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing key data") | |
| } | |
| case "RSA PRIVATE KEY": | |
| parsedKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing key data") | |
| } | |
| default: | |
| return fmt.Errorf("unexpected block type %q in key data", block.Type) | |
| } | |
| } | |
| defer os.Remove(keyFile.Name()) | |
| if _, err := keyFile.Write(keyData); err != nil { | |
| return errors.Wrap(err, "failed to write key to temporary file") | |
| if parsedKey == nil { | |
| return errors.New("no private key found in key data") | |
| } | |
| keyFile.Close() | |
| // Create PKCS#12 file with openssl | |
| // Export PKCS#12 file | |
| p12File, err := os.CreateTemp(RootDir, "submariner-client-*.p12") | |
| if err != nil { | |
| return errors.Wrap(err, "failed to create temporary pkcs12 file") | |
| } | |
| defer os.Remove(p12File.Name()) | |
| p12File.Close() | |
| // Use empty password for PKCS#12 | |
| pkcs12Password := "" | |
| opensslCmd := exec.CommandContext(ctx, "openssl", "pkcs12", "-export", | |
| "-in", certFile.Name(), | |
| "-inkey", keyFile.Name(), | |
| "-out", p12File.Name(), | |
| "-name", nickname, | |
| "-passout", "pass:"+pkcs12Password) | |
| if err := execWithOutput(command.New(opensslCmd)); err != nil { | |
| return errors.Wrap(err, "failed to create PKCS#12 file") | |
| pkcsData, err := pkcs12.Modern.EncodeWithFriendlyName(nickname, parsedKey, parsedCert, []*x509.Certificate{}, pkcs12Password) | |
| if err != nil { | |
| return errors.Wrap(err, "error encoding to PKCS#12") | |
| } | |
| if _, err := p12File.Write(pkcsData); err != nil { | |
| return errors.Wrap(err, "error writing PKCS#12 file") | |
| } | |
| if err := p12File.Close(); err != nil { | |
| return errors.Wrap(err, "error closing PKCS#12 file") | |
| } | |
| // Parse certificate data | |
| var ( | |
| parsedCert *x509.Certificate | |
| certChain []*x509.Certificate | |
| err error | |
| ) | |
| for block, rest := pem.Decode(certData); block != nil; block, rest = pem.Decode(rest) { | |
| if block.Type != "CERTIFICATE" { | |
| return fmt.Errorf("unexpected block type %q in certificate data", block.Type) | |
| } | |
| cert, err := x509.ParseCertificate(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing certificate data") | |
| } | |
| if parsedCert == nil { | |
| parsedCert = cert | |
| } else { | |
| certChain = append(certChain, cert) | |
| } | |
| } | |
| if parsedCert == nil { | |
| return errors.New("no certificate found in certificate data") | |
| } | |
| // Parse key data | |
| var parsedKey any | |
| for block, rest := pem.Decode(keyData); block != nil; block, rest = pem.Decode(rest) { | |
| switch block.Type { | |
| case "PRIVATE KEY": | |
| parsedKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing key data") | |
| } | |
| case "RSA PRIVATE KEY": | |
| parsedKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) | |
| if err != nil { | |
| return errors.Wrap(err, "error parsing key data") | |
| } | |
| default: | |
| return fmt.Errorf("unexpected block type %q in key data", block.Type) | |
| } | |
| } | |
| if parsedKey == nil { | |
| return errors.New("no private key found in key data") | |
| } | |
| // Export PKCS#12 file | |
| p12File, err := os.CreateTemp(RootDir, "submariner-client-*.p12") | |
| if err != nil { | |
| return errors.Wrap(err, "failed to create temporary pkcs12 file") | |
| } | |
| defer os.Remove(p12File.Name()) | |
| // Use empty password for PKCS#12 | |
| pkcs12Password := "" | |
| pkcsData, err := pkcs12.Modern.EncodeWithFriendlyName(nickname, parsedKey, parsedCert, certChain, pkcs12Password) | |
| if err != nil { | |
| return errors.Wrap(err, "error encoding to PKCS#12") | |
| } | |
| if _, err := p12File.Write(pkcsData); err != nil { | |
| return errors.Wrap(err, "error writing PKCS#12 file") | |
| } | |
| if err := p12File.Close(); err != nil { | |
| return errors.Wrap(err, "error closing PKCS#12 file") | |
| } |
🤖 Prompt for AI Agents
In `@pkg/cable/libreswan/certificate_handler.go` around lines 99 - 165, The
certificate parsing currently overwrites parsedCert for each "CERTIFICATE"
block, so change the loop that processes certData to treat the first parsed
CERTIFICATE as the leaf (set parsedCert only if nil) and append subsequent
parsed certificates to a certChain slice ([]*x509.Certificate); then pass
certChain (not an empty slice) as the intermediate chain argument to
pkcs12.Modern.EncodeWithFriendlyName along with parsedKey and parsedCert. Ensure
parsedCert is checked for nil after the loop as before and keep existing error
handling around pem.Block types and pkcs12/p12 file creation (references:
parsedCert, certData, certChain, pkcs12.Modern.EncodeWithFriendlyName,
parsedKey, p12File).
4 participants
This implements PEM-encoded X.509 certificate parsing using Go crypto, and exports the result using SSLMate's go-pkcs12 package (or rather, cert-manager's fork of go-pkcs12 which adds support for encoding with a friendly name; see SSLMate/go-pkcs12#67 for details).
Depends on submariner-io/admiral#1258
Summary by CodeRabbit
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.