feat(sdk): Encapsulate all crypto in cryptoservice, make it pluggable#835
feat(sdk): Encapsulate all crypto in cryptoservice, make it pluggable#835elizabethhealy merged 13 commits intomainfrom
Conversation
* 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>
* 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>
* 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>
Summary of ChangesHello @elizabethhealy, 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 introduces a significant architectural improvement by refactoring the cryptography layer into a pluggable service. This change enhances modularity, testability, and extensibility, allowing for flexible integration of different cryptographic implementations. The core library now centralizes all crypto operations, providing a consistent and secure foundation for future development. 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
Ignored Files
Activity
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly refactors cryptographic operations into a pluggable CryptoService, enhancing architectural flexibility and test coverage. However, a medium-severity vulnerability was identified in the RSA public key import logic, which incorrectly validates and accepts weak RSA keys (less than 2048 bits). Additionally, there is a high-severity finding concerning the extractPublicKeyPem implementation that could impact EC-based certificates.
* 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>
* 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>
* 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>
* 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>
X-Test Failure Report |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by encapsulating cryptographic operations within a pluggable CryptoService. This greatly improves the architecture by abstracting away the underlying crypto implementation. The changes are extensive, touching many parts of the SDK, and have been applied consistently for the most part.
I've identified a few areas for improvement:
- In the CLI, a validation for PEM-formatted keys is missing.
- A comment in the OIDC logic has become outdated due to the refactoring.
- The
web-app's custom OIDC client contains complex workarounds to deal with the new opaque key types, which should be refactored to align with the new crypto architecture of the SDK.
Overall, this is a great step forward for the SDK's design. Addressing these points will further improve the quality and maintainability of the code.
Note: Security Review did not run due to the size of the PR.
|
/gemini summary |
Summary of ChangesThis pull request introduces a pluggable cryptography service to the SDK, improving modularity, testability, and security. It encapsulates all crypto operations within a consistent interface, allowing for flexible integration of different cryptographic implementations and custom crypto backends. 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 review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the cryptography layer, moving from direct jose and WebCrypto API usage to a new CryptoService abstraction. This change allows for dependency injection of cryptographic implementations, enhancing flexibility and testability. Key changes include replacing CryptoKey with opaque KeyPair and SymmetricKey types, vendoring jose JWT helper functions and the dpop library to use the new CryptoService interface, and updating all related modules (CLI, access, authentication providers, TDF3 client, assertions, ciphers, and key access models) to adopt the new crypto abstraction. The KasPublicKeyInfo structure is simplified by removing the key property, and the OpenTDF constructor now accepts an optional cryptoService parameter, defaulting to a native browser implementation. Additionally, a test file contains duplicated test cases for OpenTDF crypto service initialization that should be removed for maintainability.
Note: Security Review did not run due to the size of the PR.
…cert compatibility (#854) * codex suggestions on review * copilot and gemini comments
eugenioenko
left a comment
There was a problem hiding this comment.
This looks fantastic! 🔥
Thank you @elizabethhealy for putting the stacked PR for review this mega feature, extremely helpful.
FEATURE BRANCH
This pull request introduces a pluggable cryptography service to the SDK, improving modularity, testability, and security. It encapsulates all crypto operations within a consistent interface, allowing for flexible integration of different cryptographic implementations and custom crypto backends.
Highlights
CryptoServiceinterface for pluggable cryptography implementations, enhancing modularity and extensibility.CryptoServiceusing the Web Crypto API, supporting a wide range of cryptographic operations.CryptoService, centralizing cryptographic primitives.joseutility functions for JWT claims and critical header validation to maintain consistent behavior.CryptoServicemethods and refactored JWT utility functions.