Skip to content

fix: use existing pattern to support SEC1 format EC keys for nanoTDF unwraps#3756

Open
aescalera-defenseunicorns wants to merge 1 commit into
opentdf:mainfrom
aescalera-defenseunicorns:fix/sec1-nanotdf-rewraps
Open

fix: use existing pattern to support SEC1 format EC keys for nanoTDF unwraps#3756
aescalera-defenseunicorns wants to merge 1 commit into
opentdf:mainfrom
aescalera-defenseunicorns:fix/sec1-nanotdf-rewraps

Conversation

@aescalera-defenseunicorns

@aescalera-defenseunicorns aescalera-defenseunicorns commented Jul 17, 2026

Copy link
Copy Markdown

With a SEC1 EC key, the server starts cleanly because StandardCrypto parses EC keys lazily, then fails at request time in two places:

  • FindKeyByID's key probe (service/internal/security/in_process_provider.go), which makes GET /kas/v2/kas_public_key?algorithm=ec:secp256r1 return a misleading 404.
  • StandardCrypto.Decrypt, which breaks EC/nanoTDF rewraps.

Proposed Changes

  • use existing pattern to support SEC1 format keys
    case strings.Contains(err.Error(), "use ParseECPrivateKey instead"):
    priv, err = x509.ParseECPrivateKey(block.Bytes)

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

Summary by CodeRabbit

  • Bug Fixes
    • Improved EC private key import compatibility by supporting both PKCS#8 and SEC1/RFC5915 PEM formats.
    • Preserved clear error handling for invalid PEM data and unsupported key types.

Signed-off-by: Antonio Escalera <antonio@defenseunicorns.com>
@aescalera-defenseunicorns
aescalera-defenseunicorns requested a review from a team as a code owner July 17, 2026 21:39
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ECPrivateKeyFromPem now supports SEC1/RFC5915 EC private keys after PKCS#8 parsing fails. Tests cover PKCS#8 and SEC1 encodings across P-256, P-384, and P-521, plus invalid PEM and RSA inputs.

Changes

EC key decoding

Layer / File(s) Summary
PEM parsing fallback and validation
lib/ocrypto/ec_key_pair.go, lib/ocrypto/ec_key_pair_test.go
ECPrivateKeyFromPem retries SEC1 parsing for the corresponding PKCS#8 error, while tests validate multiple curves, encodings, malformed PEM, and non-EC keys.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: sujankota

Poem

A rabbit found keys in a PEM-lined maze,
PKCS#8 and SEC1 earned praise.
P-256 hopped, P-384 flew,
P-521 joined the queue.
RSA stayed out of the burrowed craze.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding SEC1 EC key support for nanoTDF unwraps using the existing parsing pattern.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@aescalera-defenseunicorns aescalera-defenseunicorns changed the title fix: use existing pattern to support SEC1 format EC keys fix: use existing pattern to support SEC1 format EC keys for nanoTDF unwraps Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant