feat: create recoverable keys from dashboard #2783
Conversation
harshsbhat
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces functionality to allow setting recoverable keys when creating keys in the dashboard. The changes span multiple files in the dashboard application, adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested Reviewers
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/dashboard/lib/trpc/routers/key/create.ts (2)
3-7: Add relevant error logging for new imports
You have introduced new imports (env,EncryptRequest,RequestContext, andVault). While everything looks fine, consider logging or re-throwing with error details when these modules fail to load to aid in debugging unexpected runtime issues.
116-145: Encryption logic and database insert
The addition of the encryption logic is well-structured:
- Checking
recoverEnabledandstoreEncryptedKeysbefore using Vault.- Handling errors with TRPCError.
A couple of suggestions:
- Logging: Consider logging the
_errdetails to a secure or masked target so you have more insight into the failure root cause instead of discarding the caught error.- Atomicity: The current transaction ensures atomic writes. If the Vault call times out or fails, the transaction is rolled back, which is good. Confirm that any partial failures are handled properly in logs for debugging.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (2)
186-215: Conditional messaging on recovery
Displaying context-dependent messaging ("This key can be recovered" vs. "cannot be recovered") provides clarity to the user. Great approach.
794-844: UI block for Recoverable Keys
The UI block is well-organized, with a switch forrecoverEnabledand explanatory text about encrypted vault storage. A minor enhancement could be to clarify the data retention policy or accountability if there are multiple vault encryption keys across partitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(8 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/page.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(3 hunks)
🔇 Additional comments (7)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
38-38: New optional field in input schema
The new recoverEnabled: z.boolean().optional() field is clear and well-typed. Ensure consumers of the createKey procedure are aware that omitting the field defaults it to undefined. This might require additional checks on the front-end if usage patterns vary.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/page.tsx (1)
65-65: Propagating storeEncryptedKeys
Passing storeEncryptedKeys={keyAuth.storeEncryptedKeys} is a neat integration point, ensuring the UI reacts accordingly. Verify that any edge cases—like missing or false values—are gracefully handled in the CreateKey component.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts (1)
90-90: Default value for recoverEnabled
Setting recoverEnabled to false by default is a reasonable approach to ensuring users explicitly opt in to recoverability. This helps with best-practice security defaults.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (4)
49-60: New props in CreateKey
Introducing storeEncryptedKeys to the Props and handling them in the CreateKey component is clear. Great job ensuring default values are assigned for optional fields like prefix and bytes.
77-77: recoverEnabled: false
Defaulting recoverEnabled to false aligns with security best practices. Good choice to prevent accidental creation of recoverable keys.
116-118: Resetting recoverable state
This logic ensures recoverable toggles off on form submission if the user did not explicitly check recoverEnabled. Verify that partial form submissions or validations do not leave the component in an inconsistent state.
252-252: Resetting recoverEnabled after creation
Resetting the field ensures a fresh state for each subsequent key creation. Double-check that user preferences related to recoverability are appropriately re-initialized if they create multiple keys in succession.
|
third try 😔 hopefully, I get it this time |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/lib/trpc/routers/key/create.ts(3 hunks)
🔇 Additional comments (2)
apps/dashboard/lib/trpc/routers/key/create.ts (2)
3-3: LGTM! Clean schema extension and imports.
The new imports and schema changes are well-structured and properly typed.
Also applies to: 7-7, 38-38
116-147: Verify security measures for recoverable keys
Please ensure the following security aspects are properly addressed:
- Verify that key material is properly cleaned up if the transaction fails
- Confirm that the vault's encryption meets security requirements
- Ensure audit logs capture the recovery-enabled status
Run the following script to check audit logging implementation:
✅ Verification successful
Security measures for recoverable keys are properly implemented
The implementation has appropriate security measures in place:
- Key material cleanup is handled through database transactions which ensure atomicity - if any part fails, the entire transaction is rolled back
- The vault encryption service:
- Uses secure HTTPS communication
- Requires authentication via bearer token
- Includes request tracing via request IDs
- Properly handles and reports encryption errors
- Errors during encryption or storage are properly caught and logged, with appropriate error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if audit logs capture recovery-enabled status
# Check audit log schema and implementations
rg -A 5 "insertAuditLogs"
# Check for any existing audit log patterns
ast-grep --pattern 'insertAuditLogs($_, {
$$$
description: $_,
$$$
})'
Length of output: 66102
Script:
#!/bin/bash
# Check if audit logs capture recovery-enabled status in the create key router
rg -A 10 "key.create" apps/dashboard/lib/trpc/routers/key/create.ts
# Check if there are any audit logs related to key recovery
rg -A 10 "recovery" apps/dashboard/lib/trpc/routers/key/create.ts
Length of output: 431
Script:
#!/bin/bash
# Check the key creation implementation for cleanup on failure
rg -A 10 "catch\(" apps/dashboard/lib/trpc/routers/key/create.ts
# Check vault encryption implementation
rg -A 10 "class Vault" -g "*.ts"
Length of output: 3151
Script:
#!/bin/bash
# Check vault encryption implementation details
cat internal/vault/src/index.ts
# Check for transaction rollback handling
rg -A 10 "transaction" apps/dashboard/lib/trpc/routers/key/create.ts
Length of output: 2797
| await tx | ||
| .insert(schema.encryptedKeys) | ||
| .values({ | ||
| keyId: keyId, | ||
| workspaceId: workspace?.id, | ||
| encrypted: vaultRes.encrypted, | ||
| encryptionKeyId: vaultRes.keyId, | ||
| }) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: | ||
| "We are unable to store encrypted the key. Please contact support using [email protected]", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve database operation safety
The database operations need similar improvements:
- Remove optional chaining for workspace.id
- Consider cleanup on failure
- Use consistent error messaging
Apply this diff:
await tx
.insert(schema.encryptedKeys)
.values({
keyId: keyId,
- workspaceId: workspace?.id,
+ workspaceId: workspace.id,
encrypted: vaultRes.encrypted,
encryptionKeyId: vaultRes.keyId,
})
.catch((err) => {
console.error(err);
+ // Consider cleanup of the key if encryption storage fails
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
- message:
- "We are unable to store encrypted the key. Please contact support using [email protected]",
+ message: "Unable to process request. Please contact support.",
});
});Committable suggestion skipped: line range outside the PR's diff.
| if (input.recoverEnabled && keyAuth?.storeEncryptedKeys) { | ||
| const vault = new Vault(env().AGENT_URL, env().AGENT_TOKEN); | ||
| const encryptReq: EncryptRequest = { | ||
| keyring: workspace?.id, | ||
| data: key, | ||
| }; | ||
| const requestId = crypto.randomUUID(); | ||
| const context: RequestContext = { requestId }; | ||
| const vaultRes = await vault.encrypt(context, encryptReq).catch((err) => { | ||
| console.error(err); | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Encryption Failed. Please contact support using [email protected]", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen error handling and null checks
A few suggestions to improve the robustness of the encryption implementation:
- Validate environment variables before vault initialization
- Replace optional chaining with explicit null check for workspace
- Consider using more user-friendly error messages
Apply this diff:
if (input.recoverEnabled && keyAuth?.storeEncryptedKeys) {
+ if (!env().AGENT_URL || !env().AGENT_TOKEN) {
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: "Unable to process request. Please contact support.",
+ });
+ }
+ if (!workspace) {
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: "Unable to process request. Please contact support.",
+ });
+ }
const vault = new Vault(env().AGENT_URL, env().AGENT_TOKEN);
const encryptReq: EncryptRequest = {
- keyring: workspace?.id,
+ keyring: workspace.id,
data: key,
};
const requestId = crypto.randomUUID();
const context: RequestContext = { requestId };
const vaultRes = await vault.encrypt(context, encryptReq).catch((err) => {
console.error(err);
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
- message: "Encryption Failed. Please contact support using [email protected]",
+ message: "Unable to process request. Please contact support.",
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (input.recoverEnabled && keyAuth?.storeEncryptedKeys) { | |
| const vault = new Vault(env().AGENT_URL, env().AGENT_TOKEN); | |
| const encryptReq: EncryptRequest = { | |
| keyring: workspace?.id, | |
| data: key, | |
| }; | |
| const requestId = crypto.randomUUID(); | |
| const context: RequestContext = { requestId }; | |
| const vaultRes = await vault.encrypt(context, encryptReq).catch((err) => { | |
| console.error(err); | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Encryption Failed. Please contact support using [email protected]", | |
| }); | |
| }); | |
| if (input.recoverEnabled && keyAuth?.storeEncryptedKeys) { | |
| if (!env().AGENT_URL || !env().AGENT_TOKEN) { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Unable to process request. Please contact support.", | |
| }); | |
| } | |
| if (!workspace) { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Unable to process request. Please contact support.", | |
| }); | |
| } | |
| const vault = new Vault(env().AGENT_URL, env().AGENT_TOKEN); | |
| const encryptReq: EncryptRequest = { | |
| keyring: workspace.id, | |
| data: key, | |
| }; | |
| const requestId = crypto.randomUUID(); | |
| const context: RequestContext = { requestId }; | |
| const vaultRes = await vault.encrypt(context, encryptReq).catch((err) => { | |
| console.error(err); | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Unable to process request. Please contact support.", | |
| }); | |
| }); |
There was a problem hiding this comment.
This is fine:
- keyring: workspace?.id,
- keyring: workspace.id,
I doubt the other two change requests tho. Let's wait for some maintainer
There was a problem hiding this comment.
@harshsbhat, understood! We'll defer to the maintainer's verdict on those additional changes. Let me know if you need anything else in the meantime.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
114-145:⚠️ Potential issueStrengthen error handling and encryption implementation.
The encryption logic needs improvements in several areas:
- The optional chaining on
workspace?.idshould be removed as workspace existence is already verified.- Error messages expose internal details that should be abstracted.
- Consider cleanup if encryption succeeds but database insertion fails.
Apply this diff to address these issues:
- if (input.recoverEnabled && keyAuth?.storeEncryptedKeys) { + if (input.recoverEnabled && keyAuth.storeEncryptedKeys) { const vault = new Vault(env().AGENT_URL, env().AGENT_TOKEN); const encryptReq: EncryptRequest = { - keyring: workspace?.id, + keyring: workspace.id, data: key, }; const requestId = crypto.randomUUID(); const context: RequestContext = { requestId }; const vaultRes = await vault.encrypt(context, encryptReq).catch((err) => { console.error(err); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", - message: "Encryption Failed. Please contact support using [email protected]", + message: "Unable to process request. Please contact support.", }); }); await tx .insert(schema.encryptedKeys) .values({ keyId: keyId, workspaceId: workspace.id, encrypted: vaultRes.encrypted, encryptionKeyId: vaultRes.keyId, }) .catch((err) => { console.error(err); + // Consider cleanup of the encrypted key from vault if db insertion fails throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", - message: - "We are unable to store encrypted the key. Please contact support using [email protected]", + message: "Unable to process request. Please contact support.", }); }); }
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (1)
803-853: Fix the documentation link.The documentation link appears to be incorrect:
- The URL is missing the protocol (https://)
- The link text includes a period that should be outside the link
Apply this diff:
- href={"unkey.com/docs/security/recovering-keys"} + href={"https://unkey.com/docs/security/recovering-keys"} > - unkey.com/docs/security/recovering-keys. + unkey.com/docs/security/recovering-keys + </Link> + .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx(8 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/validation.ts
🔇 Additional comments (4)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
37-37: LGTM!The input schema change correctly adds the optional
recoverEnabledfield.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/new/client.tsx (3)
49-49: LGTM!The Props type and component signature are correctly updated to include the
storeEncryptedKeysproperty.Also applies to: 54-60
77-77: LGTM!The form state management for
recoverEnabledis implemented correctly, including default values and submission handling.Also applies to: 116-118, 134-134
192-221: LGTM!The alert messages are well-crafted and provide clear information about key recoverability status.
|
It is building the dashboard correctly on the local setup. Not sure why it is failing here. Can you send me the fail message if it is on my side? |
|
Yup it's failing for the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/dashboard/package.json (1)
59-59: Consider removing the caret from the workspace dependency version.The
workspace:^version specifier includes a caret which is unusual for workspace dependencies. Consider using justworkspace:*orworkspace:to avoid any potential versioning issues.- "@unkey/vault": "workspace:^", + "@unkey/vault": "workspace:*",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/dashboard/package.json(1 hunks)
| @@ -56,6 +56,7 @@ | |||
| "@unkey/ui": "workspace:^", | |||
| "@unkey/validation": "workspace:^", | |||
| "@unkey/vercel": "workspace:^", | |||
| "@unkey/vault": "workspace:^", | |||
There was a problem hiding this comment.
💡 Codebase verification
Workspace pattern doesn't include the vault package location
The build failure is occurring because the vault package is located at internal/vault/ but the workspace configuration in the root package.json only includes "apps/*" and "packages/*" patterns. The vault package needs to be moved to the packages/ directory or the workspace pattern needs to be updated to include the internal/* pattern.
- Move
internal/vault/topackages/vault/
OR - Add
"internal/*"to the workspaces array in the root package.json
🔗 Analysis chain
Verify the workspace dependency setup.
The build failure mentioned in the PR comments suggests that the @unkey/vault module is not being found. Please ensure that:
- The vault package exists in the workspace
- The vault package is built before the dashboard
- Type declarations are properly generated and exported
Let's verify the workspace setup with the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workspace setup for @unkey/vault
# Check if the vault package exists in the workspace
echo "Checking for vault package..."
fd -t f "package.json" -x grep -l "@unkey/vault"
# Check for type declarations
echo "Checking for type declarations..."
fd -t f "index.d.ts" -x grep -l "vault"
# Check package.json files for build order dependencies
echo "Checking build dependencies..."
fd -t f "package.json" -x grep -l "\"build\".*@unkey/vault"
Length of output: 419
Script:
#!/bin/bash
# Description: Deep dive into vault package setup
# Check vault package.json contents
echo "=== Vault package.json contents ==="
cat ./internal/vault/package.json
# Check for TypeScript configuration
echo -e "\n=== TypeScript configuration files ==="
fd -t f "tsconfig.json|.d.ts$" -p internal/vault
# Check for workspace-level configuration
echo -e "\n=== Workspace configuration ==="
cat package.json
Length of output: 2340
|
Hey @perkinsjr, I added it in the package.json and also pushed the pnpm-lock. Can you check it now? |
What does this PR do?
Fixes #2097
This PR allows users to create recoverable keys through the dashboard, which is only possible through API as of now.
Type of change
How should this be tested?
Make sure you have store_encrypted_keys enabled. Head over to the dashboard and create a new key http://localhost:3000/apis/api_id/keys/ks_id/new with
recoverable: true. Get the key_id of the newly created key and use it withgetKeyAPI like this ( make sure you have appropriate permissions to get the decrypted key ):The decrypted key should be logged in the response
store_encrypted_keysthe recoverable card is not visible.Checklist
https://www.loom.com/share/713ebadf169b43eb9055dda724dd7008?sid=82985e82-072f-4a10-9595-78c010d7484f
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation