Skip to content

feat: KMS admin keys#323

Merged
xDarksome merged 10 commits into
mainfrom
feat/admin-kms-keys
Feb 5, 2026
Merged

feat: KMS admin keys#323
xDarksome merged 10 commits into
mainfrom
feat/admin-kms-keys

Conversation

@xDarksome

Copy link
Copy Markdown
Member

Description

  • Adds support for KMS keys to wcn_admin CLI
  • Creates admin KMS keys for each WCN cluster
  • Changes required SSO identity for SOPS key access from "Read-Only" to "Administrator"

How Has This Been Tested?

Manually

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@claude

claude Bot commented Feb 4, 2026

Copy link
Copy Markdown

Claude finished @xDarksome's task in 1m 24s —— View job


Code Review Complete

Issue 1: Terraform Policy Typo - Incorrect Action Key
File: infra/modules/admin-key/main.tf:33
Severity: Critical - Terraform will fail validation/apply

Line 33 uses actions (lowercase) but should be Action (capitalized) to match AWS IAM policy syntax. This inconsistency with line 25 will cause Terraform plan/apply to fail.

# Line 33 - INCORRECT
actions = [

# Should be
Action = [

Issue 2: Unreachable Code Can Be Reached
File: crates/admin_cli/src/main.rs:99
Severity: Low - Logic error in error handling

The unreachable!() at line 99 is actually reachable if both private_key and kms_key_id are None. While clap's ArgGroup validation should prevent this, using unreachable!() is semantically incorrect. Better to use anyhow::bail!() or return proper error.

} else {
    // `clap` validates that exactly one required argument is present
    unreachable!()
};

Consider:

} else {
    anyhow::bail!("Either private_key or kms_key_id must be provided")
};

Issue 3: Missing KMS Feature Guard in RpcProvider
File: crates/cluster/src/smart_contract/evm.rs:50
Severity: Medium - Compilation error when KMS feature disabled

Line 50 handles SignerKind::Kms without #[cfg(feature = "kms")] guard, but SignerKind::Kms variant only exists when KMS feature enabled. This will fail compilation when kms feature is not enabled.

let wallet: EthereumWallet = match &signer.kind {
    SignerKind::PrivateKey(key) => key.clone().into(),
    SignerKind::Kms(key) => key.clone().into(),  // Line 50 - needs feature guard
};

Add feature guard:

#[cfg(feature = "kms")]
SignerKind::Kms(key) => key.clone().into(),

Issue 4: Inconsistent SSO Role Naming
File: infra/modules/sops-encryption-key/main.tf:42
Consideration: Verify intended SSO role name

Changed from *AWSReservedSSO_Read-Only_* to *AWSReservedSSO_Administrator_*. While described in PR description, verify this matches actual SSO role name in AWS account - pattern matching with wildcards can silently fail if role name differs (e.g., "Admin" vs "Administrator").


Issue 5: KMS Key Multi-Region Without Replica Configuration
File: infra/modules/admin-key/main.tf:7
Consideration: Incomplete multi-region setup

Line 7 sets multi_region = true but no replica keys configured. Multi-region KMS keys require explicit replica configuration in other regions. Current setup creates primary key only - consider documenting expected replica regions or adding replica key resources.


✅ No external domain URLs found outside approved domains.


@xDarksome xDarksome marked this pull request as ready for review February 5, 2026 11:35
Copilot AI review requested due to automatic review settings February 5, 2026 11:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds support for AWS KMS (Key Management Service) keys as an alternative to raw private keys for WCN cluster admin operations. The PR implements KMS signing capabilities in the admin CLI tool and provisions KMS keys in the infrastructure for each WCN cluster region.

Changes:

  • Added AWS KMS key infrastructure modules and instances for testnet and mainnet clusters
  • Implemented KMS-based transaction signing in the Rust codebase as an alternative to private key signing
  • Updated SOPS encryption key access requirements from "Read-Only" to "Administrator" SSO role

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
infra/modules/admin-key/main.tf New Terraform module for creating multi-region KMS keys for signing smart contract transactions
infra/testnet/main.tf Instantiates admin KMS key module for testnet EU region
infra/mainnet/main.tf Instantiates admin KMS key modules for all mainnet regions (EU, US, AP, SA)
infra/modules/sops-encryption-key/main.tf Updates KMS policy to require Administrator role instead of Read-Only for SOPS operations
infra/testnet/sops/admin.json Adds KMS key ARN and updates SOPS metadata (encrypted file)
env.sh Adds conditional environment variable exports for both private key and KMS key modes
crates/cluster/src/smart_contract/evm.rs Implements KMS-based signer support alongside existing private key signer
crates/cluster/Cargo.toml Adds kms feature flag with aws-sdk-kms dependency
crates/admin_cli/src/main.rs Updates CLI to accept either private key or KMS key ARN via mutually exclusive arguments
crates/admin_cli/Cargo.toml Enables kms feature and adds AWS SDK dependencies
Cargo.toml Adds aws-sdk-kms workspace dependency
Cargo.lock Adds all transitive dependencies for AWS SDK integration
VERSION Bumps version to 260205.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread infra/modules/admin-key/main.tf Outdated
Comment thread env.sh
@xDarksome xDarksome requested a review from mario-reown February 5, 2026 12:02
@xDarksome xDarksome merged commit 3e49332 into main Feb 5, 2026
15 checks passed
@xDarksome xDarksome deleted the feat/admin-kms-keys branch February 5, 2026 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants