feat(sdk): use opaque keys for sym and asym#836
feat(sdk): use opaque keys for sym and asym#836elizabethhealy wants to merge 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>
Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
8b2121f to
1bbc54e
Compare
X-Test Failure Report |
X-Test Failure Reportopentdf-sdk-lib |
|
/gemini review |
|
/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 and 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 |
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.