fix(cert): handle unencrypted and SEC 1 PEM private keys in readImpl#237
Open
mavisakalyan wants to merge 2 commits intoakash-network:mainfrom
Open
fix(cert): handle unencrypted and SEC 1 PEM private keys in readImpl#237mavisakalyan wants to merge 2 commits intoakash-network:mainfrom
mavisakalyan wants to merge 2 commits intoakash-network:mainfrom
Conversation
readImpl only matched ENCRYPTED PRIVATE KEY and legacy Proc-Type:4,ENCRYPTED. Unencrypted PKCS#8 (PRIVATE KEY) and SEC 1 (EC PRIVATE KEY) fell through to the else branch, returning errUnsupportedEncryptedPEM. Add handling for: - PRIVATE KEY: use block bytes directly (no decryption needed) - EC PRIVATE KEY: parse SEC 1 and re-marshal as PKCS#8 Also improve the error for truly unknown PEM types to include the block type. refs #ISSUE_NUMBER
Contributor
WalkthroughThe changes extend key-pair parsing to accept unencrypted PKCS#8 ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
go/util/tls/key_pair_manager.go (1)
292-302: Same redundant parse→marshal→parse round-trip as ingo/node/cert/v1/utils/key_pair_manager.go.
ecKeyis already available butprivKeyIis leftnil, sox509.ParsePKCS8PrivateKey(privKeyPlaintext)is called again at line 311. AssignprivKeyI = ecKeyafter the marshal to eliminate the redundant decode.♻️ Proposed fix
privKeyPlaintext, err = x509.MarshalPKCS8PrivateKey(ecKey) if err != nil { return nil, nil, nil, fmt.Errorf("%w: failed re-encoding EC key as PKCS#8", err) } + privKeyI = ecKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/util/tls/key_pair_manager.go` around lines 292 - 302, In the EC private key branch (block.Type == "EC PRIVATE KEY") you marshal ecKey into privKeyPlaintext but never set privKeyI, causing a redundant x509.ParsePKCS8PrivateKey(privKeyPlaintext) later; after successfully calling x509.MarshalPKCS8PrivateKey(ecKey) assign privKeyI = ecKey so downstream code can use the already-parsed key and avoid the extra parse round-trip (update variables ecKey and privKeyI in the key_pair_manager.go EC handling).go/node/cert/v1/utils/key_pair_manager.go (1)
287-297: RedundantParsePKCS8PrivateKeyround-trip for theEC PRIVATE KEYpath.
ecKeyis already parsed butprivKeyIis leftnil, causingx509.ParsePKCS8PrivateKey(privKeyPlaintext)to be called again at line 306 — a parse→marshal→parse cycle. AssigningprivKeyI = ecKeyafter the successful marshal short-circuits this.♻️ Proposed fix
privKeyPlaintext, err = x509.MarshalPKCS8PrivateKey(ecKey) if err != nil { return nil, nil, nil, fmt.Errorf("%w: failed re-encoding EC key as PKCS#8", err) } + privKeyI = ecKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/cert/v1/utils/key_pair_manager.go` around lines 287 - 297, The EC private key path currently parses the SEC1 key into ecKey, marshals it to PKCS#8 into privKeyPlaintext, but leaves privKeyI nil so later code calls x509.ParsePKCS8PrivateKey again; after successful x509.MarshalPKCS8PrivateKey set privKeyI = ecKey (the parsed key) to short-circuit the redundant parse→marshal→parse cycle (refer to block.Type == "EC PRIVATE KEY", x509.ParseECPrivateKey, x509.MarshalPKCS8PrivateKey, privKeyPlaintext, and privKeyI).go/util/tls/key_pair_manager_test.go (1)
43-120: LGTM — tests correctly cover the three new paths.Tests for the unencrypted PKCS#8, SEC1 EC, and unknown-type paths are clear and focused. One optional improvement for
TestReadImpl_UnknownPEMType: asserting that the returned error wrapserrUnsupportedEncryptedPEM(viaerrors.Is) or that the error string contains the block type would lock in the improved error-message contract introduced by this PR.🔍 Optional: assert error content in TestReadImpl_UnknownPEMType
_, _, _, err := kpm.readImpl(&buf) if err == nil { t.Fatal("expected error for unknown PEM type, got nil") } +if !strings.Contains(err.Error(), "SOME UNKNOWN KEY") { + t.Errorf("expected error to contain PEM block type, got: %v", err) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/util/tls/key_pair_manager_test.go` around lines 43 - 120, Update TestReadImpl_UnknownPEMType to assert the returned error wraps or identifies the expected sentinel by using errors.Is(err, errUnsupportedEncryptedPEM) or checking the error string contains the PEM block type; locate the test function TestReadImpl_UnknownPEMType and modify its final assertion (after calling kpm.readImpl) to use errors.Is with errUnsupportedEncryptedPEM or a substring match on the unknown block type so the test verifies the specific error contract from keyPairManager.readImpl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/util/tls/key_pair_manager_test.go`:
- Around line 43-120: Add equivalent unit tests for the duplicate implementation
in go/node/cert/v1/utils/key_pair_manager.go mirroring the tests in
go/util/tls/key_pair_manager_test.go (exercise readImpl with unencrypted PKCS#8,
SEC1 EC private key, and unknown PEM type) so the duplicate code has coverage;
also fix the missing symbol AuthVersionOID in key_pair_manager.go by either
importing it from the package that defines it or defining the appropriate
asn1.ObjectIdentifier constant (matching the one used by the other
key_pair_manager.go) and update imports accordingly so the file compiles.
---
Nitpick comments:
In `@go/node/cert/v1/utils/key_pair_manager.go`:
- Around line 287-297: The EC private key path currently parses the SEC1 key
into ecKey, marshals it to PKCS#8 into privKeyPlaintext, but leaves privKeyI nil
so later code calls x509.ParsePKCS8PrivateKey again; after successful
x509.MarshalPKCS8PrivateKey set privKeyI = ecKey (the parsed key) to
short-circuit the redundant parse→marshal→parse cycle (refer to block.Type ==
"EC PRIVATE KEY", x509.ParseECPrivateKey, x509.MarshalPKCS8PrivateKey,
privKeyPlaintext, and privKeyI).
In `@go/util/tls/key_pair_manager_test.go`:
- Around line 43-120: Update TestReadImpl_UnknownPEMType to assert the returned
error wraps or identifies the expected sentinel by using errors.Is(err,
errUnsupportedEncryptedPEM) or checking the error string contains the PEM block
type; locate the test function TestReadImpl_UnknownPEMType and modify its final
assertion (after calling kpm.readImpl) to use errors.Is with
errUnsupportedEncryptedPEM or a substring match on the unknown block type so the
test verifies the specific error contract from keyPairManager.readImpl.
In `@go/util/tls/key_pair_manager.go`:
- Around line 292-302: In the EC private key branch (block.Type == "EC PRIVATE
KEY") you marshal ecKey into privKeyPlaintext but never set privKeyI, causing a
redundant x509.ParsePKCS8PrivateKey(privKeyPlaintext) later; after successfully
calling x509.MarshalPKCS8PrivateKey(ecKey) assign privKeyI = ecKey so downstream
code can use the already-parsed key and avoid the extra parse round-trip (update
variables ecKey and privKeyI in the key_pair_manager.go EC handling).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
go/node/cert/v1/utils/key_pair_manager_test.go (3)
52-53:pem.Encodeerrors silently dropped.While
pem.Encodeinto abytes.Buffernever actually fails today, discarding the error with_ =means any future change to the writer type (e.g., a file or pipe) would silently produce corrupt PEM data and yield confusing test failures. Preferrequire.NoError(testify) or an explicitt.Fatalcheck.♻️ Proposed fix (shown for TestReadImpl_UnencryptedPKCS8; apply same pattern in the other two tests)
- _ = pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: certDer}) - _ = pem.Encode(&buf, &pem.Block{Type: "PRIVATE KEY", Bytes: keyDer}) + if err := pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: certDer}); err != nil { + t.Fatal(err) + } + if err := pem.Encode(&buf, &pem.Block{Type: "PRIVATE KEY", Bytes: keyDer}); err != nil { + t.Fatal(err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/cert/v1/utils/key_pair_manager_test.go` around lines 52 - 53, In the tests (e.g., TestReadImpl_UnencryptedPKCS8) replace the silent discards of pem.Encode (currently written as "_ = pem.Encode(&buf, ... )") with explicit error checks: capture the returned error from pem.Encode and assert success using require.NoError(t, err) (or call t.Fatalf if testify isn't used) so any write failures into the buffer fail the test instead of producing silent corrupt PEM output; apply the same change for all occurrences in key_pair_manager_test.go.
43-120: No regression tests for the pre-existing encrypted key paths.The three new tests cover the newly added
PRIVATE KEY,EC PRIVATE KEY, and unknown-type paths, but there are no tests for the already-supportedENCRYPTED PRIVATE KEY(PKCS#8 encrypted) and legacyProc-Type: 4,ENCRYPTEDcode paths. Adding at least a smoke test for those cases would prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/cert/v1/utils/key_pair_manager_test.go` around lines 43 - 120, Add smoke tests that exercise the existing encrypted-key code paths: create two new tests (e.g. TestReadImpl_EncryptedPKCS8 and TestReadImpl_ProcTypeEncrypted) that mirror the style of TestReadImpl_UnencryptedPKCS8 and TestReadImpl_SEC1ECPrivateKey but produce an "ENCRYPTED PRIVATE KEY" PKCS#8 PEM block and a legacy PEM block with Proc-Type: 4,ENCRYPTED (use x509.MarshalPKCS8PrivateKey or x509.MarshalECPrivateKey for key DER, then wrap the PKCS#8 bytes into an "ENCRYPTED PRIVATE KEY" PEM or use x509.EncryptPEMBlock to produce a PEM with the "Proc-Type" headers), call kpm.readImpl(&buf), and assert that err is nil and cert, privKeyData, and pubKey are non-nil to prevent regressions in readImpl handling of encrypted keys.
63-71: Assertions only check non-nil; key/cert correspondence is not verified.Tests pass as long as
readImplreturns any non-nil values. A regression that returns a stale or mismatched certificate/key would go undetected. Consider asserting that the parsed public key matches the generated private key and that the certificate's raw DER bytes matchcertDer.♻️ Proposed additional assertions (unencrypted PKCS#8 test; apply equivalently to SEC1 test)
if pubKey == nil { t.Fatal("expected non-nil public key") } + + // Verify the returned public key matches the generated key. + parsedECPub, ok := pubKey.(*ecdsa.PublicKey) + if !ok { + t.Fatal("expected *ecdsa.PublicKey") + } + if parsedECPub.X.Cmp(priv.PublicKey.X) != 0 || parsedECPub.Y.Cmp(priv.PublicKey.Y) != 0 { + t.Fatal("returned public key does not match generated key") + } + + // Verify the returned certificate DER bytes match the generated cert. + if !bytes.Equal(cert.Raw, certDer) { + t.Fatal("returned certificate does not match generated certificate") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/cert/v1/utils/key_pair_manager_test.go` around lines 63 - 71, Current assertions only check for non-nil values; update the test that calls readImpl to also validate key/cert correspondence by (1) parsing privKeyData into a private key and deriving its public key and asserting it equals pubKey (use the same parsing logic used elsewhere in tests), and (2) asserting that cert.Raw (or cert.RawTBSCertificate as appropriate) equals the expected certDer bytes; apply the same extra assertions in the SEC1 variation of the test so mismatched or stale key/cert returns are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/node/cert/v1/utils/key_pair_manager_test.go`:
- Around line 26-33: The certificate template in key_pair_manager_test.go uses
RSA-only flags (x509.KeyUsageDataEncipherment | x509.KeyUsageKeyEncipherment)
which are incorrect for ECDSA; update the KeyUsage field on the template
variable (the x509.Certificate literal named "template") to use
x509.KeyUsageDigitalSignature instead (keep ExtKeyUsage as-is) so the ECDSA
tests assert the correct key usage.
---
Nitpick comments:
In `@go/node/cert/v1/utils/key_pair_manager_test.go`:
- Around line 52-53: In the tests (e.g., TestReadImpl_UnencryptedPKCS8) replace
the silent discards of pem.Encode (currently written as "_ = pem.Encode(&buf,
... )") with explicit error checks: capture the returned error from pem.Encode
and assert success using require.NoError(t, err) (or call t.Fatalf if testify
isn't used) so any write failures into the buffer fail the test instead of
producing silent corrupt PEM output; apply the same change for all occurrences
in key_pair_manager_test.go.
- Around line 43-120: Add smoke tests that exercise the existing encrypted-key
code paths: create two new tests (e.g. TestReadImpl_EncryptedPKCS8 and
TestReadImpl_ProcTypeEncrypted) that mirror the style of
TestReadImpl_UnencryptedPKCS8 and TestReadImpl_SEC1ECPrivateKey but produce an
"ENCRYPTED PRIVATE KEY" PKCS#8 PEM block and a legacy PEM block with Proc-Type:
4,ENCRYPTED (use x509.MarshalPKCS8PrivateKey or x509.MarshalECPrivateKey for key
DER, then wrap the PKCS#8 bytes into an "ENCRYPTED PRIVATE KEY" PEM or use
x509.EncryptPEMBlock to produce a PEM with the "Proc-Type" headers), call
kpm.readImpl(&buf), and assert that err is nil and cert, privKeyData, and pubKey
are non-nil to prevent regressions in readImpl handling of encrypted keys.
- Around line 63-71: Current assertions only check for non-nil values; update
the test that calls readImpl to also validate key/cert correspondence by (1)
parsing privKeyData into a private key and deriving its public key and asserting
it equals pubKey (use the same parsing logic used elsewhere in tests), and (2)
asserting that cert.Raw (or cert.RawTBSCertificate as appropriate) equals the
expected certDer bytes; apply the same extra assertions in the SEC1 variation of
the test so mismatched or stale key/cert returns are detected.
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
1 participant
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.
Description
readImplinkey_pair_manager.goonly handledENCRYPTED PRIVATE KEYand legacyProc-Type: 4,ENCRYPTEDPEM blocks. Unencrypted PKCS#8 (PRIVATE KEY) and SEC 1 (EC PRIVATE KEY) fell to the else branch, returningerrUnsupportedEncryptedPEM. This breaks users whose mTLS cert PEM files have unencrypted private keys (e.g. generated externally or on macOS arm64 with Akash CLI v1.1.1).Purpose of the Change
Related Issues
Checklist
Notes for Reviewers
The same bug exists in two copies of
readImpl:go/util/tls/key_pair_manager.goandgo/node/cert/v1/utils/key_pair_manager.go. Both are patched identically. A third copy inakash-network/node(x/cert/utils/) should be updated once this merges and the node bumps itspkg.akt.dev/godependency. Unit tests are added forgo/util/tls/covering unencrypted PKCS#8, SEC 1 EC, and unknown PEM type rejection.