feat(sdk): use opaque keys for sym and asym#836
feat(sdk): use opaque keys for sym and asym#836elizabethhealy merged 70 commits intofeat/pluggable-crypto-servicefrom
Conversation
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
…ithub.com/opentdf/web-sdk into dspx-1969-extend-crypto-service-interface
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
|
/gemini summary |
There was a problem hiding this comment.
Code Review
This pull request introduces opaque key types for both symmetric and asymmetric cryptography throughout the SDK, significantly improving security and code clarity by abstracting raw key material. However, a high-severity vulnerability was identified in the assertion verification logic, where untrusted keys from JWT headers are used without validation. Additionally, a medium-severity information leak was found in the CLI tool's logging. For further improvements, the use of SHA-1 in RSA-OAEP should be upgraded to SHA-256 for better security, and a missing method in the CryptoService interface for generating EC signing keys needs to be implemented.
| /** | ||
| * Generate an RSA key pair for signing/verification. | ||
| * @returns PEM-encoded key pair | ||
| * @returns Opaque key pair | ||
| */ | ||
| generateSigningKeyPair: () => Promise<PemKeyPair>; | ||
|
|
||
| /** | ||
| * Compute HMAC-SHA256 of content using key. | ||
| * @param key - Hex-encoded key bytes (e.g., "0a1b2c...") | ||
| * @param content - Hex-encoded content bytes to authenticate | ||
| * @returns Hex-encoded HMAC result | ||
| * | ||
| * Note: Callers should treat inputs and outputs as hex-encoded byte strings. | ||
| * Implementations may normalize case, but callers MUST NOT rely on a specific case. | ||
| */ | ||
| hmac: (key: string, content: string) => Promise<string>; | ||
| generateSigningKeyPair: () => Promise<KeyPair>; |
There was a problem hiding this comment.
The generateSigningKeyPair method currently only supports generating RSA key pairs, while generateECKeyPair is specifically for ECDH. This leaves a gap for generating EC key pairs for signing (e.g., for ES256/ES384/ES512 assertions).
Consider adding a generateECSigningKeyPair method to the CryptoService interface to provide a complete set of key generation capabilities. This would be consistent with the support for EC-based signing algorithms in other parts of the service.
There was a problem hiding this comment.
we dont currently have need to generate ec signging keys within the code, any ec signing and verification handling is from external keys coming from assertions or jwts.
should the need arise we can add this logic
Summary of ChangesThis pull request introduces a fundamental refactoring of the SDK's cryptographic key management, transitioning from direct manipulation of PEM-encoded strings and raw byte arrays to a more secure and abstract opaque key object model. This change significantly improves the security posture by encapsulating key material, enabling more robust cryptographic service implementations, and streamlining key handling across the library's authentication, DPoP, and TDF components. It also lays the groundwork for future enhancements like hardware security module (HSM) integration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
|
/gemini summary |
|
/gemini review |
Summary of ChangesThis pull request fundamentally refactors the SDK's cryptographic key management by transitioning from direct manipulation of PEM-encoded strings and raw byte arrays to a more secure and abstract opaque key object model. This change significantly improves the security posture by encapsulating key material, enabling more robust cryptographic service implementations, and streamlining key handling across the library's authentication, DPoP, and TDF components. It also lays the groundwork for future enhancements like hardware security module (HSM) integration, ensuring a more secure and extensible cryptographic foundation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
There was a problem hiding this comment.
Code Review
This is a major and important refactoring to introduce opaque keys for cryptographic operations, enhancing security and abstraction. The changes are extensive, touching many parts of the SDK, from the core crypto service to authentication providers and the CLI. The new CryptoService interface is well-defined, and the implementation correctly uses opaque types to encapsulate key material. The correction of the elliptic curve identifier from P-512 to P-521 is also a welcome fix. Overall, this is a high-quality contribution that significantly improves the SDK's architecture. My review includes one minor suggestion for improving type consistency in the web-app session handling.
Note: Security Review is unavailable for this PR.
| // TODO: This validation function might be too restrictive and also allow for some weird edge cases. | ||
| // Like a PEM that begins with BEGIN PUBLIC KEY but ends with END PRIVATE KEY | ||
| // Use the isPemKeyPair from the lib/src/crypto/crypto-utils.ts file for a more robust solution if this becomes an issue. | ||
| function isPemFormatted(key: string): boolean { |
There was a problem hiding this comment.
removing now unnecessary pem comversion code
| export enum NamedCurve { | ||
| P256 = 'P-256', | ||
| P384 = 'P-384', | ||
| P512 = 'P-512', |
There was a problem hiding this comment.
this was fixed on main as well i believe
| removePemFormatting, | ||
| isPemKeyPair, | ||
| isCryptoKeyPair, | ||
| } from '../../tdf3/src/crypto/crypto-utils.js'; |
There was a problem hiding this comment.
exporting some helpful crypto types and helpers for downstream use to avoid code duplication
| encryptWithPublicKey: (payload: Binary, publicKey: string) => Promise<Binary>; | ||
|
|
||
| /** Get length random bytes as a hex-encoded string. */ | ||
| generateInitializationVector: (length?: number) => Promise<string>; |
There was a problem hiding this comment.
removed in favor of randomBytes, seemed like unnecessary duplication
| * Note: Callers should treat inputs and outputs as hex-encoded byte strings. | ||
| * Implementations may normalize case, but callers MUST NOT rely on a specific case. | ||
| */ | ||
| hmac: (key: string, content: string) => Promise<string>; |
There was a problem hiding this comment.
this remains, it just does not handle the hex encoding anymore, the core hmac functionality replaces the hmac+signSymmetric functions to avoid unnecessary duplication
| * | ||
| * Note: Multi-KAS may not be available in all secure environments (single KAS only) | ||
| */ | ||
| splitSymmetricKey: (key: SymmetricKey, numShares: number) => Promise<SymmetricKey[]>; |
There was a problem hiding this comment.
needed to move these into the crypto service as well since technically keys shouldnt leave a hsm for these ops
| } else if (keySize === 4096) { | ||
| algorithm = 'rsa:4096'; | ||
| } else { | ||
| throw new ConfigurationError( |
There was a problem hiding this comment.
being more specific on supported key sizes
X-Test Failure Reportopentdf-sdk-lib |
| * @returns Validated PEM and detected algorithm | ||
| * @throws ConfigurationError if key format invalid or algorithm not supported | ||
| */ | ||
| importPublicKeyPem: (pem: string) => Promise<PublicKeyInfo>; |
There was a problem hiding this comment.
rename for clarity, not really an import anymore, just getting info about the public key
| } | ||
|
|
||
| if (typeof key.key === 'string') { | ||
| if (!cryptoService.importPrivateKey) { |
There was a problem hiding this comment.
to maintain compatibility with some of the existing flow -- this allows importing private keys only if the crypto service supports it, if a users crypto service impl does not then they will have to provide opaque keys generated by their crypto service
| mimeType = 'unknown', | ||
| windowSize = DEFAULT_SEGMENT_SIZE, | ||
| keyMiddleware = defaultKeyMiddleware, | ||
| keyMiddleware: keyMiddlewareOpt, |
There was a problem hiding this comment.
needed to support the ability to pass the cryoto service into the key middle ware on construction
| "require": "./dist/cjs/src/crypto/index.js", | ||
| "import": "./dist/web/src/crypto/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
exposing for use as helpers downstream
|
xtest failure is due to some added tests, they are failing on main as well so im ignoring for now and filing as separate bug |
| return 'ES384'; | ||
| case 'P-512': | ||
| case 'P-521': | ||
| return 'ES512'; |
There was a problem hiding this comment.
Does this (ES512) need to be updated?
There was a problem hiding this comment.
ya this is expected ES512 = ECDSA using P-521 + SHA-512
| * @internal | ||
| */ | ||
| function unwrapKey(key: PublicKey | PrivateKey): CryptoKey { | ||
| return (key as any)._internal; |
There was a problem hiding this comment.
Nit: Lots of anys in this file that I'd love to see typed, but that could be a follow-up PR.
There was a problem hiding this comment.
ill make a note to clean this up
70312e3
into
feat/pluggable-crypto-service
…#835) * feat(sdk): Extend the crypto service interface (#803) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * not implemented error --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat: Add jwt utils (#804) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * vendor jose lib functions to avoid maintenance * gemini suggestions, updates to better match jose behavior * remove issue with merge * gemini suggestions * strict jose version since were vendoring * forgot to update package lock --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat: Expose cryptoservice (#805) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * expose cryptoservice in opentdf obj * expose cryptoservice in opentdf obj * fix tests * fix issue with merge * use not implemented var --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat: Use crypto service in assertions (#806) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * expose cryptoservice in opentdf obj * expose cryptoservice in opentdf obj * fix tests * assertions use crypto service * address comments * fix cli * remove dupe test * add comment * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat: update everything to use crypto service (#809) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * expose cryptoservice in opentdf obj * expose cryptoservice in opentdf obj * fix tests * assertions use crypto service * address comments * fix cli * tdf using crypto service * format * use cs for ec ops and remove crypto key from kas * format * suggestions * moving off of cryptokeypair * try to fix cli and web app builds * salt generation * resolve conflicts * re add comment --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat: custom dpop with cryptoservice (#811) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * expose cryptoservice in opentdf obj * expose cryptoservice in opentdf obj * fix tests * assertions use crypto service * address comments * fix cli * tdf using crypto service * format * use cs for ec ops and remove crypto key from kas * format * suggestions * moving off of cryptokeypair * try to fix cli and web app builds * salt generation * dpop changes * fix cli * fix linting, remove dpop dep from package and lock * update webapp package and lock * suggestions * dont change api of auth providers * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * resolve conflicts * fix tests * resolve some issues with the merge --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * feat(sdk): use opaque keys for sym and asym (#836) * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * extend the crypto service interface * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * enable ci for this branch, gemini suggestions * add ec methods to crypto service * format * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * fixes to ec implementation Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions * jwt utils * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * suggestions, format * add support for rs256 signing and verification * add suggestion * expose cryptoservice in opentdf obj * expose cryptoservice in opentdf obj * fix tests * assertions use crypto service * address comments * fix cli * tdf using crypto service * format * use cs for ec ops and remove crypto key from kas * format * suggestions * moving off of cryptokeypair * try to fix cli and web app builds * salt generation * dpop changes * fix cli * fix linting, remove dpop dep from package and lock * update webapp package and lock * suggestions * use opaque keys for sym and asym Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * dont change api of auth providers * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * use opaque keys for sym and asym Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * web app fixes * remove some functions from the interface that arent needed * remove sign symmetric in favor of hmac * linting and comments * format * suggestions, web app fix * make import private key optional * cli and web app updates * try with correctVerificationKey * still allow assertions string input if supported by crypto service * format * try to fix test * resolve some merge issues * undo some of the things i removed * fix web app build, gemini suggestions * 🤖 🎨 Autoformat Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * expose some more types and helper functions * missed file, default to provided crypto service in key middleware --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com> * remove CurveName in favor of NamedCurve * remove feature branch from workflow yaml * remove outdated comments * remove dupe test * fix(sdk): harden pluggable crypto JWT/assertion key handling and DER/cert compatibility (#854) * codex suggestions on review * copilot and gemini comments --------- Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
TLDR
KeyPair,PublicKey,PrivateKey,SymmetricKey)splitSymmetricKey) and merging (mergeSymmetricKeys) operations within the crypto service (private symmetric keys shouldnt have to leave the hsm for this)/cryptoutilswhich has some helper functions and types that can be reused in other crypto service implementations to avoid code duplicationSummary of Changes
This pull request introduces a fundamental refactoring of the SDK's cryptographic key management, transitioning from direct manipulation of PEM-encoded strings and raw byte arrays to a more secure and abstract opaque key object model. This change significantly improves the security posture by encapsulating key material, enabling more robust cryptographic service implementations, and streamlining key handling across the library's authentication, DPoP, and TDF components. It also lays the groundwork for future enhancements like hardware security module (HSM) integration.
Highlights
KeyPair,PublicKey,PrivateKey,SymmetricKey) for all cryptographic operations, abstracting key material from direct string or byte array representations. This enhances security by encapsulating key details and enables more flexible cryptographic service implementations, such as hardware security module (HSM) integration.CryptoServiceinterface has been significantly expanded to include new methods for importing and exporting opaque keys to/from PEM and JWK formats. It also introduces dedicated methods for symmetric key splitting (splitSymmetricKey) and merging (mergeSymmetricKeys), and updates existing methods to accept and return the new opaque key types.P512elliptic curve identifier has been corrected toP521across the codebase, ensuring alignment with standard cryptographic specifications and Web Crypto API usage for improved compatibility and correctness.CryptoServiceimplementation, reducing scattered PEM string handling and promoting consistent, secure key management throughout the library's authentication, DPoP, and TDF components.