feat: Add IAM role authentication support for AWS KMS wallets #920
Conversation
- Make credentials optional during wallet import/creation - Use AWS SDK default credential chain (IAM roles, etc.) when credentials are not specified
WalkthroughMakes AWS KMS credentials optional across server route, utils, and DB layers. Removes runtime enforcement of AWS credentials, conditionally passes credentials when both keys are present, updates schemas/types to allow undefined/null, and fixes a credentials property typo. Control flow now defers to AWS default providers when credentials are absent. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as Server Route<br/>(import.ts)
participant Params as fetchAwsKmsWalletParams
participant Import as importAwsKmsWallet
participant KMS as AWS SDK / KMSClient
User->>API: POST /backend-wallet/import
API->>Params: Read AWS params (optional)
Params-->>API: { accessKeyId?, secretAccessKey?, region? }
API->>Import: importAwsKmsWallet({credentials?})
alt Credentials provided
Import->>KMS: new KMSClient({ credentials, region })
note right of KMS: Uses supplied access/secret
else Credentials absent or partial
Import->>KMS: new KMSClient({ credentials: undefined, region })
note right of KMS: Falls back to default provider chain
end
KMS-->>Import: Key metadata
Import-->>API: Wallet details (creds possibly null/undefined stored)
API-->>User: Import result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/utils/wallets/import-aws-kms-wallet.ts (1)
9-13: Fix parameter name typo:crendentials→credentials.The parameter name is misspelled. This is the source of the typo that propagates to callers in
create-aws-kms-wallet.tsandimport.ts.Apply this diff:
interface ImportAwsKmsWalletParams { awsKmsArn: string; - crendentials?: { + credentials?: { accessKeyId: string; secretAccessKey: string; }; label?: string; }Then update all usages of this parameter in the function:
export const importAwsKmsWallet = async ({ - crendentials, + credentials, awsKmsArn, label, }: ImportAwsKmsWalletParams) => { const { keyId, region } = splitAwsKmsArn(awsKmsArn); const account = await getAwsKmsAccount({ client: thirdwebClient, keyId, config: { region, - credentials: crendentials ? { - accessKeyId: crendentials.accessKeyId, - secretAccessKey: crendentials.secretAccessKey, + credentials: credentials ? { + accessKeyId: credentials.accessKeyId, + secretAccessKey: credentials.secretAccessKey, } : undefined, }, }); const walletAddress = account.address; await createWalletDetails({ type: WalletType.awsKms, address: walletAddress, awsKmsArn, label, - awsKmsAccessKeyId: crendentials?.accessKeyId, - awsKmsSecretAccessKey: crendentials?.secretAccessKey, + awsKmsAccessKeyId: credentials?.accessKeyId, + awsKmsSecretAccessKey: credentials?.secretAccessKey, }); return walletAddress; };Note: Callers in
create-aws-kms-wallet.ts(line 30) andimport.ts(line 171) must also be updated.
♻️ Duplicate comments (3)
src/server/routes/backend-wallet/import.ts (2)
171-171: Same typo issue: "crendentials" should be "credentials".This line uses the same typo flagged in
create-aws-kms-wallet.ts. Please fix this across all affected files for consistency and readability.
171-171: Fix parameter name typo:crendentials→credentials.The parameter name is misspelled (see related comment in
create-aws-kms-wallet.ts).Apply this diff:
- crendentials: walletCredentials, + credentials: walletCredentials,Note: This change must be coordinated with fixing the parameter definition in
src/server/utils/wallets/import-aws-kms-wallet.ts.src/server/utils/wallets/import-aws-kms-wallet.ts (1)
7-14: Fix parameter name typo: "crendentials" should be "credentials".The parameter name
crendentialsis a typo. This should be corrected tocredentialsfor better code readability. This typo appears in multiple places:
- Line 9: Interface definition
- Line 20: Function parameter
- Line 30: Conditional check
- Lines 45-46: Optional chaining
🧹 Nitpick comments (3)
src/server/utils/wallets/fetch-aws-kms-wallet-params.ts (2)
14-15: Documentation could be more precise about region handling.The comment states "Only AWS region is required" but the code doesn't enforce this. The
awsRegionfield is also optional in the type definition (line 7). If region is truly required by the AWS SDK, consider either:
- Enforcing it at this level, or
- Updating the comment to reflect that region can also fall back to SDK defaults
14-15: Align documentation withawsRegionbehavior. The docs claim “Only AWS region is required,” butawsRegionis optional inAwsKmsWalletParams. Either makeawsRegionrequired and add runtime validation, or update the docs to note that region is optional and will fall back to the AWS SDK’s default region provider chain.src/shared/db/wallets/create-wallet-details.ts (1)
102-104: Consider explicitly handlingawsKmsAccessKeyIdfor consistency.The
awsKmsSecretAccessKeyfield is explicitly set tonullwhen absent, butawsKmsAccessKeyIdrelies on the spread operator. While Prisma should handle undefined values correctly in create operations, explicitly setting both fields would improve consistency and code clarity.Apply this diff for the
aws-kmstype:if (walletDetails.type === "aws-kms") { return prisma.walletDetails.create({ data: { ...walletDetails, address: walletDetails.address.toLowerCase(), - awsKmsSecretAccessKey: walletDetails.awsKmsSecretAccessKey ? encrypt(walletDetails.awsKmsSecretAccessKey) : null, + awsKmsAccessKeyId: walletDetails.awsKmsAccessKeyId ?? null, }, }); }Apply this diff for the
smart:aws-kmstype:if (walletDetails.type === "smart:aws-kms") { return prisma.walletDetails.create({ data: { ...walletDetails, - address: walletDetails.address.toLowerCase(), awsKmsSecretAccessKey: walletDetails.awsKmsSecretAccessKey ? encrypt(walletDetails.awsKmsSecretAccessKey) : null, + awsKmsAccessKeyId: walletDetails.awsKmsAccessKeyId ?? null, accountSignerAddress: walletDetails.accountSignerAddress.toLowerCase(), - accountFactoryAddress: walletDetails.accountFactoryAddress?.toLowerCase(), entrypointAddress: walletDetails.entrypointAddress?.toLowerCase(), }, }); }Also applies to: 128-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
src/server/routes/backend-wallet/import.ts(1 hunks)src/server/utils/wallets/create-aws-kms-wallet.ts(2 hunks)src/server/utils/wallets/create-smart-wallet.ts(1 hunks)src/server/utils/wallets/fetch-aws-kms-wallet-params.ts(1 hunks)src/server/utils/wallets/import-aws-kms-wallet.ts(3 hunks)src/shared/db/wallets/create-wallet-details.ts(4 hunks)src/shared/db/wallets/get-wallet-details.ts(1 hunks)src/shared/utils/account.ts(2 hunks)src/shared/utils/cache/get-wallet.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/shared/db/wallets/create-wallet-details.ts (1)
src/shared/utils/crypto.ts (1)
encrypt(5-7)
src/server/routes/backend-wallet/import.ts (1)
src/server/utils/wallets/import-aws-kms-wallet.ts (1)
importAwsKmsWallet(19-50)
🔇 Additional comments (22)
src/server/utils/wallets/fetch-aws-kms-wallet-params.ts (1)
3-8: LGTM! Credentials made optional to support IAM role authentication.The type changes correctly reflect that AWS credentials are now optional, allowing the AWS SDK to use its default credential provider chain (including IAM roles) when credentials are not explicitly provided.
src/server/utils/wallets/create-smart-wallet.ts (2)
65-75: LGTM! Conditional credential handling correctly implemented.The credentials object is correctly created only when both
awsAccessKeyIdandawsSecretAccessKeyare present, otherwiseundefinedis passed. This allows the AWS SDK to fall back to its default credential provider chain (IAM roles, environment variables, etc.) when explicit credentials are not provided.
70-73: LGTM! Correct conditional credential handling.The logic correctly provides credentials only when both
awsAccessKeyIdandawsSecretAccessKeyare defined, allowing the AWS SDK to fall back to IAM roles when credentials are absent.src/shared/utils/account.ts (3)
65-75: LGTM! Consistent credential handling for AWS KMS wallets.The conditional credential logic correctly mirrors the pattern used across the codebase, ensuring credentials are only passed when both
awsKmsAccessKeyIdandawsKmsSecretAccessKeyare present.
70-73: LGTM! Consistent credential handling.The conditional credential provisioning correctly ensures credentials are only passed when both values are present, enabling IAM role authentication when credentials are absent.
106-109: LGTM! Consistent pattern for smart wallets.The same conditional logic is correctly applied for smart AWS KMS wallet admin accounts.
src/shared/utils/cache/get-wallet.ts (3)
62-68: LGTM! Nullish coalescing ensures undefined instead of null.The use of
?? undefinedcorrectly converts null values to undefined, which is consistent with the credential handling pattern throughout the codebase. This ensures theAwsKmsWalletconstructor receives either a valid string or undefined, never null.
65-67: LGTM! Correct null-to-undefined conversion.The nullish coalescing operator correctly converts
nulldatabase values toundefined, which is appropriate for the AWS SDK's optional credential handling.
106-108: LGTM! Consistent pattern for smart wallets.The same null-to-undefined conversion is correctly applied for smart AWS KMS wallets.
src/server/utils/wallets/create-aws-kms-wallet.ts (2)
55-61: LGTM! Conditional credential configuration for KMSClient.The credentials object is correctly configured for the AWS KMS client, following the same pattern used throughout the PR. The property name
credentialsis correct here (unlike line 30).
57-61: LGTM! Correct conditional credentials for KMS client.The credentials are correctly provided to the KMS client only when both access key ID and secret access key are present.
src/server/routes/backend-wallet/import.ts (3)
29-42: LGTM! Schema correctly reflects optional credentials.The schema update properly documents that AWS credentials are optional, with clear documentation explaining that default AWS credentials will be used when not provided. This aligns with the PR objective of supporting IAM role authentication.
150-173: Verify AWS KMS IAM-based authentication and error handlingEnsure your EC2/ECS deployments have IAM roles attached with
kms:GetPublicKey,kms:Sign, etc., and confirm AWS SDK surfaces clear errors on authentication or permission failures. Add integration tests, for example:# Test AWS KMS wallet import using IAM role (no credentials provided) # Test error handling when IAM role is missing or insufficient
161-168: LGTM! Correct optional credential handling.The removal of mandatory credential validation aligns with the PR objective to support IAM role authentication. The conditional
walletCredentialsconstruction correctly provides credentials only when both values are present, enabling fallback to AWS SDK default credential providers.src/shared/db/wallets/get-wallet-details.ts (3)
62-69: LGTM! Schema correctly updated to support nullable credentials.The schema changes properly reflect that AWS KMS credentials can be null, which is consistent with the PR's objective of making credentials optional. The
z.string().nullable()validation ensures proper type safety while allowing null values.
202-209: Decryption and fallback logic correctly handles all credential scenarios. Verified that all wallet‐creation/import flows always supply both AWS KMS credentials together (or neither), and downstream code already requires both.
66-67: LGTM! Schema updated to support optional credentials.Making these fields nullable correctly supports the IAM role authentication feature where credentials may not be stored in the database.
The runtime handling at lines 202-209 correctly falls back to config values or null when database values are absent.
src/server/utils/wallets/import-aws-kms-wallet.ts (4)
25-35: LGTM! Correct conditional credential handling.The function correctly passes credentials only when
crendentialsis provided, otherwise passesundefined. This allows the AWS SDK to fall back to its default credential provider chain (IAM roles, environment variables, etc.).
39-47: LGTM! Optional chaining correctly handles undefined credentials.The use of optional chaining (
crendentials?.accessKeyId,crendentials?.secretAccessKey) correctly propagates undefined values when credentials are not provided. This aligns with the nullable schema definition inget-wallet-details.ts.
30-34: LGTM! Correct conditional credential handling.The conditional credential construction correctly supports IAM role authentication by passing
undefinedwhen credentials are not provided.
45-46: LGTM! Appropriate use of optional chaining.Optional chaining correctly handles the case where
credentialsmay beundefined.src/shared/db/wallets/create-wallet-details.ts (1)
21-22: LGTM! Credentials correctly made optional for IAM role authentication.The type definitions appropriately make both
awsKmsSecretAccessKeyandawsKmsAccessKeyIdoptional for bothaws-kmsandsmart:aws-kmswallet types, enabling IAM role-based authentication when credentials are not provided.Also applies to: 38-39
|
This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this PR will be closed in 3 days. |
|
@d4mr @joaquim-verges |
1 participant
Changes
Main changes
import.ts: Remove credential validation and make them optionalcreate-aws-kms-wallet.ts: Set credentials object only when credentials existfetch-aws-kms-wallet-params.ts: Remove required checks for credentials (delegated to AWS SDK default chain)create-wallet-details.ts,get-wallet-details.ts: Make credentials nullable in DB schemaaccount.ts,get-wallet.ts: Set credentials only when they existImportant notes
API setup example:
POST /backend-wallet/import Content-Type: application/json { "awsKmsArn": "arn:aws:kms:us-east-1:123456789012:key/12345678-1234-1234-1234-123456789012" }How this PR will be tested
Output
To set up from the Dashboard, specify the KMS ARN from the following screen:
PR-Codex overview
This PR focuses on making AWS KMS credentials optional by allowing them to be nullable or undefined. It modifies various schemas and functions to accommodate this change, enhancing flexibility in handling AWS credentials while maintaining functionality.
Detailed summary
awsKmsSecretAccessKeyandawsKmsAccessKeyIdto nullable inget-wallet-details.ts.create-smart-wallet.ts,create-aws-kms-wallet.ts, andimport-aws-kms-wallet.tsto use optional chaining.import.ts.fetch-aws-kms-wallet-params.tsto makeawsAccessKeyId,awsSecretAccessKey, andawsRegionoptional.Summary by CodeRabbit
New Features
Bug Fixes