Skip to content

fix: EC fallback to PEM #2758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: EC fallback to PEM #2758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/ocrypto/ec_key_pair.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,12 @@

priv, err := x509.ParsePKCS8PrivateKey(block.Bytes)
if err != nil {
return nil, fmt.Errorf("ec x509.ParsePKCS8PrivateKey failed: %w", err)
// If PKCS8 fails, try EC private key format

Check failure on line 354 in lib/ocrypto/ec_key_pair.go

View workflow job for this annotation

GitHub Actions / go (lib/ocrypto) 

File is not properly formatted (gofumpt)
Copy link
Contributor

@b-long b-long Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat surprised there's no logger in this file, but maybe that was a performance choice?

In any case, I was thinking a warning message might be useful just above the comment on line 354.

Printing to STDOUT here just as an example... I'll defer to others about the logger usage.

Suggested change
// If PKCS8 fails, try EC private key format
// Log warning about PKCS8 parsing failure
fmt.Printf("WARNING: ec x509.ParsePKCS8PrivateKey failed: %v\n", err)
// If PKCS8 fails, try EC private key format

ecKey, ecErr := x509.ParseECPrivateKey(block.Bytes)
if ecErr != nil {
return nil, fmt.Errorf("failed to parse private key as PKCS8 or EC format: PKCS8 error: %w, EC error: %v", err, ecErr)

Check failure on line 357 in lib/ocrypto/ec_key_pair.go

View workflow job for this annotation

GitHub Actions / go (lib/ocrypto) 

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current error message wraps the PKCS8 error (err) but only includes the EC error (ecErr) as a string. This prevents callers from programmatically inspecting ecErr using errors.Is or errors.As.

Since the project appears to use Go 1.20+ (indicated by the crypto/ecdh package), you can use errors.Join to wrap both errors. This makes the error handling more robust and idiomatic, allowing callers to inspect both underlying causes of failure.

Suggested change
return nil, fmt.Errorf("failed to parse private key as PKCS8 or EC format: PKCS8 error: %w, EC error: %v", err, ecErr)
return nil, errors.Join(fmt.Errorf("PKCS8: %w", err), fmt.Errorf("EC: %w", ecErr))

}
return ConvertToECDHPrivateKey(ecKey)
}
Comment on lines 353 to 360
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This change introduces new logic for parsing EC keys, but there are no corresponding unit tests as indicated by the unchecked item in the PR checklist. It's crucial to add unit tests to cover the new fallback behavior.

Please add tests for at least the following scenarios:

  • A key that is successfully parsed as a PKCS8 key.
  • A key that fails PKCS8 parsing but is successfully parsed as a traditional EC key (the new code path).
  • A key that fails both parsing methods.
  • Invalid input that fails pem.Decode.

Without tests, it's difficult to be confident in the correctness of this new logic and protect against future regressions.


switch privateKey := priv.(type) {
Expand Down
Loading