feat: encrypt metadata feature #2059
Conversation
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughWalkthroughThe changes introduce a feature for managing encrypted metadata across the application. This includes the addition of an Changes
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
harshsbhat
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
internal/vault/src/index.ts (1)
1-124: Add import statements and JSDoc comments.The file is missing import statements and JSDoc comments.
Consider adding:
- Import statements for the types and modules used, such as
RequestInitandperformance.- JSDoc comments for the class and methods to describe their purpose, parameters, and return values. For example:
/** * Client for the Vault service. */ export class Vault { // ... /** * Encrypts the given data using the specified keyring. * @param c The request context. * @param req The encrypt request. * @returns The encrypt response. * @throws Error if the encrypt request fails. */ public async encrypt(c: RequestContext, req: EncryptRequest): Promise<EncryptResponse> { // ... } // ... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (10)
- apps/api/src/routes/v1_keys_createKey.ts (3 hunks)
- apps/api/src/routes/v1_keys_verifyKey.ts (4 hunks)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (6 hunks)
- apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permissions.ts (2 hunks)
- apps/dashboard/lib/trpc/routers/key/create.ts (5 hunks)
- apps/dashboard/package.json (1 hunks)
- internal/db/src/schema/keys.ts (1 hunks)
- internal/vault/package.json (1 hunks)
- internal/vault/src/index.ts (1 hunks)
- packages/rbac/src/permissions.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- internal/vault/package.json
Additional comments not posted (24)
packages/rbac/src/permissions.ts (1)
41-42: LGTM!The addition of the
"encrypt_meta"and"decrypt_meta"actions to theapiActionsenum is consistent with the PR objectives of introducing a new feature for handling encrypted metadata. The naming of the actions follows the existing convention in the enum, and the changes are limited to the enum without affecting the logic or control flow in the file.internal/vault/src/index.ts (2)
1-28: LGTM!The type definitions for encryption and decryption requests and responses are correctly defined.
33-40: LGTM!The
Vaultclass constructor is correctly implemented.apps/dashboard/package.json (1)
56-56: LGTM!The addition of the
@unkey/vaultdependency with version specifierworkspace:^is approved. This change suggests that the dashboard project will now utilize functionality related to vault management, which aligns with the objectives outlined in the PR summary and linked issue.apps/dashboard/lib/trpc/routers/key/create.ts (5)
7-7: LGTM!The import statement is necessary to use the
Vaultclass and related types for encrypting metadata.
20-20: LGTM!The addition of the
encryptedMetaparameter to the input schema allows users to provide metadata in an encrypted format, enhancing the functionality to handle sensitive information securely.
72-84: LGTM!The code segment correctly implements the logic for encrypting the
encryptedMetausing theVaultclass. It follows the necessary steps to construct the encryption request, generate a request context, and call thevault.encryptmethod. The encrypted metadata is then stored in themetaSecretvariable for further use.
95-95: LGTM!Including the
metaSecretin the database insertion ensures that the encrypted metadata is saved alongside other key details.
115-115: LGTM!The change in the error message is minor and maintains consistency with other error messages in the codebase.
internal/db/src/schema/keys.ts (1)
51-51: LGTM!The addition of the
encryptedMetafield aligns with the PR objective of introducing a new feature for handling encrypted metadata. The field is of typetext, which is suitable for storing encrypted data, and the field name clearly conveys its purpose. The field is seamlessly integrated into the existing schema without disrupting the overall structure.apps/dashboard/app/(app)/settings/root-keys/[keyId]/permissions/permissions.ts (2)
54-61: LGTM!The code changes are approved for the following reasons:
- The new entries are consistent with the existing pattern of defining permissions.
- The permission strings follow the established pattern for workspace permissions.
- The changes align with the PR objective of introducing metadata encryption functionality.
189-196: LGTM!The code changes are approved for the following reasons:
- The new entries are consistent with the existing pattern of defining permissions.
- The permission strings follow the established pattern for API permissions.
- The changes align with the PR objective of introducing metadata encryption functionality.
apps/api/src/routes/v1_keys_verifyKey.ts (4)
181-190: LGTM!The addition of the optional
encryptedMetafield to the response schema enhances the API by allowing clients to access additional encrypted metadata associated with the key. This change aligns with the PR objective of introducing support for encrypted metadata.
Line range hint
301-365: LGTM!The added logic for decrypting the
encryptedMetafield is a crucial part of the encrypted metadata feature. It ensures that only authorized requests with the necessary permissions can decrypt the metadata, enhancing security. The use ofrootKeyAuthand specific permission checks is a good approach. The retry mechanism improves the robustness of the decryption process, and assigning a default empty JSON object tometaSecretif decryption fails is a sensible fallback.
373-373: LGTM!Adding the decrypted
encryptedMetato the response body is necessary to provide clients with access to the additional metadata associated with the key. Parsing themetaSecretvariable ensures that the metadata is returned in the correct format.
1-5: LGTM!The remaining code changes are minor and necessary to support the encrypted metadata feature. They include adding the
vaultservice to the destructured services object and updating therequestBodyandresponseBodyfields in the analytics ingestion for proper logging.Also applies to: 340-340, 349-349, 354-359
apps/api/src/routes/v1_keys_createKey.ts (4)
77-87: LGTM!The code changes are approved.
350-351: LGTM!The code changes are approved.
352-390: LGTM!The code changes are approved.
403-403: LGTM!The code changes are approved.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (4)
81-97: LGTM!The code changes for adding the
encryptMetaEnabledandencryptMetaform fields look good. The refinement forencryptMetais a nice touch to ensure only valid JSON can be entered.
210-212: LGTM!The code changes for deleting the
encryptMetafield from the form values ifencryptMetaEnabledis set tofalselook good. This ensures that theencryptMetafield is not sent to the server unnecessarily.
224-224: LGTM!The code changes for parsing the
encryptMetafield as JSON before sending it to the server look good.
832-925: LGTM!The code changes for adding the UI components for handling encrypted metadata look good. The card component, switch, textarea, and format button all work together nicely to provide a good user experience for entering and managing encrypted metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- internal/vault/src/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- internal/vault/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/api/src/routes/v1_keys_createKey.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/v1_keys_createKey.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/api/src/routes/v1_keys_createKey.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/v1_keys_createKey.ts
| .optional() | ||
| .openapi({ | ||
| description: | ||
| "This is a place for dynamic meta data, anything that feels useful for you should go here", |
There was a problem hiding this comment.
This needs to explain that it's encrypted too, as this string is used in sdks and documentation.
I know the field is named encryptedMeta but it doesn't hurt to be obvious.
| }), | ||
| ); | ||
|
|
||
| metaSecret = encryptedMeta.encrypted; |
There was a problem hiding this comment.
you're ignoring the returned encryption key id, that needs to be stored as well, otherwise we can't reroll the encryption keys
| ), | ||
| ); | ||
| const encryptedMeta = | ||
| typeof val.key.encryptedMeta === "string" |
There was a problem hiding this comment.
what else could it be?
This should always be a string or a falsy value, right?
| @@ -326,13 +337,40 @@ export const registerV1KeysVerifyKey = (app: App) => | |||
| code: val.code, | |||
| }); | |||
| } | |||
| let metaSecret = ""; | |||
There was a problem hiding this comment.
This is way too expensive to do uncached. Making a network request to the agent in the critical path is too slow.
We need to move this into the cache's swr callback
| let metaSecret = ""; | ||
| if (val.key.encryptedMeta) { | ||
| try { | ||
| await rootKeyAuth( |
There was a problem hiding this comment.
what rootKey are you using here?
verifying keys does not require a root key. I'm very surprised this didn't cause issues when you tested it.
|
|
||
| metaSecret = decryptRes.plaintext; | ||
| } catch { | ||
| metaSecret = "{}"; |
There was a problem hiding this comment.
I think undefined and therefore not returning the field in the api response is preferrable
| <> | ||
| <p className="text-xs text-content-subtle"> | ||
| Encrypt sensitive data before associating it with this key to store it | ||
| securely. Whenever you verify this key with the decrypt metadata |
There was a problem hiding this comment.
As mentioned above, verifying keys does not require a root key (yet), therefore you can't do permission checks
| @@ -48,6 +48,7 @@ export const keys = mysqlTable( | |||
| ownerId: varchar("owner_id", { length: 256 }), | |||
| identityId: varchar("identity_id", { length: 256 }), | |||
| meta: text("meta"), | |||
| encryptedMeta: text("encryptedMeta"), | |||
There was a problem hiding this comment.
This is not sufficient, we need to store both the ciphertext and the encryptionKeyId, you can check out the encrypted_keys table for a reference.
We probably want to put this in a different table as well, or reuse the secrets table, wdyt?
internal/vault/src/index.ts
Outdated
| this.token = token; | ||
| } | ||
|
|
||
| private async fetchWithMetrics( |
There was a problem hiding this comment.
this doesn't seem to do anything, I assume you just copied it.
Let me give you some context on why we had stuff like the requestId here. We were running into some latency issues between the API and the agent and needed to log latency times for each requestId, to correlate them with what the customer saw.
The dashboard talking to the agent doesn't really need that and we can remove it entirely
|
Hey @harshsbhat are you waiting on anything from the team? Looking at Andreas comments there are still things to resolve in your changes. |
Yeah, I was a little busy with my college work. I have a few unpushed changes. Also, few small questions that I will put on discord in a bit if that's fine as I belive that to be a faster way to communicate. But should be able to get this done in this week. |
|
No rush, Just making sure I didn't miss something, please ping me in Discord when you ask the questions. Andreas is on PTO and I might miss them |
Pushed! I have added the requested changes.
|
What does this PR do?
Fixes: #1678
This PR introduces a new feature called encrypted metadata so that users can share sensitive data associated with the key.
It also introduces an internal package named
@unkey/vaultso that other applications likedashboarduse theagent/vaultfairly easily.Type of change
How should this be tested?
https://www.loom.com/share/99b4d4f9f9124b6b87ebaa5544216d86?sid=612b293b-4e23-49d3-8aeb-99fe44e113bc
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Summary by CodeRabbit
New Features
encryptedMetafield for secure metadata handling during key creation and verification.Vaultclass for efficient encryption and decryption operations.Bug Fixes
Chores