Skip to content

Add KMS signer support to run-enclave.sh#433

Open
jjeangal wants to merge 4 commits into
celo-integration-rebase-17from
jg/kms-signer-support
Open

Add KMS signer support to run-enclave.sh#433
jjeangal wants to merge 4 commits into
celo-integration-rebase-17from
jg/kms-signer-support

Conversation

@jjeangal
Copy link
Copy Markdown
Member

@jjeangal jjeangal commented May 8, 2026

This PR

  • Add KMS signer path to run-enclave.sh: when SIGNER_ENDPOINT and SIGNER_ADDRESS env vars are set, pass --signer.endpoint and --signer.address to the enclave batcher instead of --private-key.
  • Fully backward-compatible: if neither env var is set, existing OP_BATCHER_PRIVATE_KEY or test mnemonic paths are unchanged.
  • Log the signer endpoint and address in the startup config summary.

Key Places to Review

  • espresso/docker/op-batcher-tee/run-enclave.sh — authentication section (lines ~96-107): KMS signer branch added above the existing private key branch.

How to Test This PR

  1. Deploy with enable_kms_signer = true in tee-op-deploy — the ECS task will set SIGNER_ENDPOINT and SIGNER_ADDRESS env vars on the container.
  2. Confirm the enclave batcher starts with --signer.endpoint and --signer.address in its args (visible in logs).
  3. Confirm existing deployments with enable_kms_signer = false are unaffected — the private key path runs as before.

@claude
Copy link
Copy Markdown

claude Bot commented May 8, 2026

Claude finished @jjeangal's task in 1m 8s —— View job


Review of PR #433: Add KMS signer support to run-enclave.sh

  • Gather context and read changed files
  • Check existing review comments
  • Review code quality, security, and correctness
  • Post review feedback

Summary

Clean, well-scoped change. The three-way auth branch (KMS signer → private key → test mnemonic) is well-structured and fully backward-compatible. The ${VAR:-} defensive defaults on the new env vars are good practice with set -e.

One suggestion posted as an inline comment: If only one of SIGNER_ENDPOINT / SIGNER_ADDRESS is set, the script silently falls through to the private key path. This could mask a misconfiguration. I suggested adding an explicit error for partial KMS configuration — see the inline comment.

Everything else looks good — the logging additions are clear, the flag names (--signer.endpoint, --signer.address) are passed correctly, and the priority order makes sense.

Comment thread espresso/docker/op-batcher-tee/run-enclave.sh
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for KMS signing in the run-enclave.sh script by adding SIGNER_ENDPOINT and SIGNER_ADDRESS configuration options. The review feedback suggests simplifying the log messages for missing KMS variables to avoid confusion regarding fallback behavior. Furthermore, it is recommended to implement explicit error handling for partial KMS configurations to prevent accidental silent fallbacks to private keys or mnemonics.

Comment thread espresso/docker/op-batcher-tee/run-enclave.sh Outdated
Comment thread espresso/docker/op-batcher-tee/run-enclave.sh Outdated
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.

3 participants