fix: EC fallback to PEM #2758
fix: EC fallback to PEM #2758
Conversation
Summary of ChangesHello @pflynn-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Key in PEM's embrace, PKCS8 first, then EC, Robust parse, secure. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances EC private key parsing by adding a fallback to the traditional EC format if PKCS8 parsing fails. This is a good improvement for robustness. My review focuses on two main areas: error handling and testing. The error returned when both parsing methods fail can be improved to make both underlying errors programmatically inspectable. More critically, this change lacks unit tests to verify the new fallback logic and its failure modes. Adding comprehensive tests is essential to ensure the correctness of this cryptographic functionality and prevent future regressions.
| if err != nil { | ||
| return nil, fmt.Errorf("ec x509.ParsePKCS8PrivateKey failed: %w", 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) | ||
| } | ||
| return ConvertToECDHPrivateKey(ecKey) | ||
| } |
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
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.
| 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)) |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
| 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 |
There was a problem hiding this comment.
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.
| // 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 |
|
The bug persists. Reproduce with my codespace on this branch: https://symmetrical-funicular-gv6xqxx76r3v5rx.github.dev/ Update: Codespaces might be unshareable. Use this script in a codespace based on this branch to reproduce on a standardized environment: cp opentdf-dev.yaml opentdf.yaml
./.github/scripts/init-temp-keys.sh
sudo cp ./keys/localhost.crt /usr/local/share/ca-certificates/ && sudo update-ca-certificates
# Kill existing containers and start fresh
docker ps -aq | xargs -r docker rm -f
docker compose up -d
sleep 5
read -p "Are the docker containers ready? Press Enter to continue..."
go run ./service provision keycloak
go run ./service provision fixtures
go run ./service start &
sleep 5
read -p "Is the go service ready? Press Enter to continue..."
git clone https://github.com/nibsbin/opentdf-basekey-bug.git
cd opentdf-basekey-bug
npm run dev |
Proposed Changes
This pull request improves the robustness of the
ECPrivateKeyFromPemfunction inec_key_pair.goby adding support for parsing EC private keys in both PKCS8 and traditional EC formats. If parsing as PKCS8 fails, the function now attempts to parse the key as an EC private key before returning an error.Key improvement to private key parsing:
ECPrivateKeyFromPemto try parsing the input as a traditional EC private key if PKCS8 parsing fails, improving compatibility with different PEM-encoded EC key formats.Checklist
Testing Instructions