Skip to content

chore(sdk): consolidate key-algorithm types + typed accessors (DSPX-4040) - #971

Merged
dmihalcik-virtru merged 2 commits into
mainfrom
DSPX-4040-keyalg-refactor
Jul 17, 2026
Merged

chore(sdk): consolidate key-algorithm types + typed accessors (DSPX-4040)#971
dmihalcik-virtru merged 2 commits into
mainfrom
DSPX-4040-keyalg-refactor

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Jul 16, 2026

Copy link
Copy Markdown
Member

What

Extracts the EC/RSA key-algorithm type refactor that had accreted onto the ML-KEM feature PR #933 (DSPX-3229) into its own reviewable/rollback-able change off main. No ML-KEM here — that stays on #933, which will rebase on top of this.

  • crypto/declarations.ts is now the single source of truth for key algorithms: per-family as const arrays (EC_KEY_ALGORITHMS, RSA_KEY_ALGORITHMS, KEY_ALGORITHMS), derived subtypes (EcKeyAlgorithm/RsaKeyAlgorithm), runtime guards (isEcKeyAlgorithm/isRsaKeyAlgorithm/isKeyAlgorithm), and exhaustive typed accessors (ecAlgorithmToCurve/rsaAlgorithmToModulusBits).
  • Call sites (dpop.ts, crypto/core/keys.ts, crypto/core/key-format.ts, client/index.ts) drop startsWith('ec:'|'rsa:') / split(':') / curve ternaries in favor of the guards and accessors.
  • access.ts re-exports the historic surface as thin aliases over the source of truth: PUBLIC_KEY_ALGORITHMS = KEY_ALGORITHMS, KasPublicKeyAlgorithm = KeyAlgorithm, isPublicKeyAlgorithm = isKeyAlgorithm. PUBLIC_KEY_ALGORITHMS is exported from the public entrypoint.
  • CLI: encapKeyType / rewrapKeyType gain choices: PUBLIC_KEY_ALGORITHMS, so unsupported values are rejected up front with the valid list.

Why

Mixing a broad type refactor with the ML-KEM feature made #933 hard to review and hard to roll back. Landing the feature-independent slice on its own keeps each change reviewable and independently revertable.

Bug fix (intentional behavior change)

isPublicKeyAlgorithm previously accepted only ec:secp256r1 / rsa:2048 and wrongly rejected ec:secp384r1, ec:secp521r1, rsa:4096. It now delegates to isKeyAlgorithm and accepts all supported EC/RSA algorithms.

Consequence: the CLI now offers all 5 as choices, but client-side wrapping still supports only a subset (ec:secp256r1, rsa:2048); unsupported picks now fail at wrap time rather than at arg-parse time.

How to test

  • cd lib && npm run build && npm run lint && npm test — green (334 tests, coverage thresholds met); .d.ts gains PUBLIC_KEY_ALGORITHMS / subtypes / accessors, KasPublicKeyAlgorithm + isPublicKeyAlgorithm still exported.
  • make i then cd cli && npm run build && npm test — green.
  • isPublicKeyAlgorithm('ec:secp384r1') and ('rsa:4096') now return true.
  • opentdf encrypt --encapsulation-algorithm rsa:9999 … is rejected up front with the choices list; ec:secp384r1 is accepted.

Scope / risk

Out of scope: all ML-KEM types/guards/accessors/KAO and edits to tdf.ts / key-access.ts / tests (those are ML-KEM-only on #933). Touches crypto type plumbing — see the bug-fix note above.

DSPX-4040

Summary by CodeRabbit

  • New Features

    • Added a shared list of supported public-key algorithms for SDK and CLI consumers.
    • Exposed algorithm lists and validation helpers for RSA and elliptic-curve keys.
    • CLI encryption and rewrapping options now provide validation for supported algorithms.
  • Bug Fixes

    • Improved key import, wrapping, and DPoP handling across supported RSA and elliptic-curve algorithms.
    • Added clearer validation for unsupported or invalid key algorithms.

…X-4040)

Introduce a single source of truth for key algorithms in
crypto/declarations.ts: per-family arrays (EC_KEY_ALGORITHMS,
RSA_KEY_ALGORITHMS, KEY_ALGORITHMS), derived subtypes
(EcKeyAlgorithm/RsaKeyAlgorithm), guards (isEcKeyAlgorithm/
isRsaKeyAlgorithm/isKeyAlgorithm), and typed accessors
(ecAlgorithmToCurve/rsaAlgorithmToModulusBits). Replace
startsWith()/split(':')/ternary parsing at call sites and export
PUBLIC_KEY_ALGORITHMS from the public entrypoint.

Also fixes a latent bug: isPublicKeyAlgorithm previously accepted
only ec:secp256r1/rsa:2048 and rejected the other supported EC/RSA
algorithms; it now delegates to isKeyAlgorithm.
@dmihalcik-virtru
dmihalcik-virtru requested a review from a team as a code owner July 16, 2026 17:24
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The SDK centralizes RSA and EC key-algorithm definitions, guards, and conversions. Crypto import, wrapping, encryption, access, and DPoP logic consume the shared helpers, while the public SDK export and CLI options use the centralized supported-algorithm list.

Changes

Key algorithm centralization

Layer / File(s) Summary
Shared algorithm contracts
lib/tdf3/src/crypto/declarations.ts
Supported algorithms, type guards, conversion helpers, and PublicKeyInfo.algorithm now use centralized declarations.
Crypto algorithm dispatch
lib/tdf3/src/crypto/core/*, lib/tdf3/src/client/index.ts
Key import, wrapping, and encryption dispatch through RSA/EC helper predicates and conversion functions.
SDK and CLI algorithm integration
lib/src/access.ts, lib/src/auth/dpop.ts, lib/src/opentdf.ts, cli/src/cli.ts
The shared algorithm list is exposed publicly, used by access and DPoP logic, and applied to CLI option validation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • opentdf/web-sdk#888: Introduced the crypto-core module split used by the updated key-format and key-wrapping logic.
  • opentdf/web-sdk#933: Overlaps on centralized public-key algorithms and CLI wrapping-key validation.

Suggested reviewers: eugenioenko

Poem

I’m a rabbit with keys in a row,
RSA and EC now know where to go.
Guards guide the crypto lane,
CLI choices stay in frame.
Shared algorithms hop along—
A tidy burrow, safe and strong!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 summarizes the main refactor: consolidating key-algorithm types and typed accessors in the SDK.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-4040-keyalg-refactor

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request centralizes the management of supported key algorithms by defining them in a single source of truth within crypto/declarations.ts. It introduces type guards and helper functions to replace error-prone string parsing and manual type casting throughout the codebase. The reviewer suggested an improvement to the isKeyAlgorithm implementation to enhance code reuse by leveraging existing type guards, which has been noted.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lib/tdf3/src/crypto/declarations.ts
…HMS (DSPX-4040)

Add choices: PUBLIC_KEY_ALGORITHMS to the encapKeyType and
rewrapKeyType options so yargs rejects unsupported algorithms up
front with the valid list, and narrows argv to the literal union.
@dmihalcik-virtru
dmihalcik-virtru force-pushed the DSPX-4040-keyalg-refactor branch from 2a7bd62 to 644cda0 Compare July 16, 2026 17:35
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

X-Test Failure Report

✅ go@main-v0.9.0
opentdf-ctl
opentdf-sdk-lib

@dmihalcik-virtru dmihalcik-virtru changed the title refactor(sdk): consolidate key-algorithm types + typed accessors (DSPX-4040) chore(sdk): consolidate key-algorithm types + typed accessors (DSPX-4040) Jul 16, 2026
@dmihalcik-virtru
dmihalcik-virtru merged commit c25e2cd into main Jul 17, 2026
47 of 48 checks passed
@dmihalcik-virtru
dmihalcik-virtru deleted the DSPX-4040-keyalg-refactor branch July 17, 2026 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants