From 996a2cfaecfac42188ac2e7cbad217449f63568a Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 8 May 2026 12:46:11 -0500 Subject: [PATCH 01/22] AWS_KMS: Added aws-sdk vcpkg. --- vcpkg-configuration.json | 5 ++++- vcpkg.json | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/vcpkg-configuration.json b/vcpkg-configuration.json index 649a119b4f..02b33ee047 100644 --- a/vcpkg-configuration.json +++ b/vcpkg-configuration.json @@ -22,7 +22,10 @@ "secp256k1-internal", "bn256", "bls12-381", - "wire-sys-vm" + "wire-sys-vm", + "aws-sdk-cpp", + "aws-c-cal", + "s2n" ] } ] diff --git a/vcpkg.json b/vcpkg.json index dec2811f70..09a902db3b 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -58,6 +58,14 @@ "name": "civetweb", "default-features": false }, + { + "name": "aws-sdk-cpp", + "default-features": false, + "version>=": "1.11.591", + "features": [ + "kms" + ] + }, "boost", { "name": "llvm", @@ -137,6 +145,10 @@ { "name": "zlib", "version": "1.3.1" + }, + { + "name": "aws-sdk-cpp", + "version": "1.11.591" } ] } From cea6a81111d83ad01b78533b4848ae5175abdf99 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 8 May 2026 14:47:24 -0500 Subject: [PATCH 02/22] AWS_KMS: updated the baseline. --- vcpkg-configuration.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vcpkg-configuration.json b/vcpkg-configuration.json index 02b33ee047..9f7f531fb4 100644 --- a/vcpkg-configuration.json +++ b/vcpkg-configuration.json @@ -9,7 +9,7 @@ { "kind": "git", "repository": "https://github.com/wire-network/wire-vcpkg-registry", - "baseline": "23aa4018992d85602a4b5c30104a623e708b4b3e", + "baseline": "088646fd8a16a062714842c8cd6748007ba5eef7", "packages": [ "boost", "softfloat", From 3588d04f8d113f5bdeed397a7972b616d7cac338 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Tue, 12 May 2026 09:58:13 -0500 Subject: [PATCH 03/22] AWS_KMS: kms signature --- Waves4-6.list | 87 ++++ .../CMakeLists.txt | 7 +- .../KMS_SIGNING_DESIGN.md | 337 +++++++++++++ .../src/kms_signature_provider.cpp | 327 +++++++++++++ .../src/kms_signature_provider.hpp | 168 +++++++ .../src/signature_provider_manager_plugin.cpp | 24 +- .../test/test_create_provider_specs.cpp | 56 +++ .../test/test_kms_signature_provider.cpp | 461 ++++++++++++++++++ 8 files changed, 1463 insertions(+), 4 deletions(-) create mode 100644 Waves4-6.list create mode 100644 plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md create mode 100644 plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp create mode 100644 plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp create mode 100644 plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp diff --git a/Waves4-6.list b/Waves4-6.list new file mode 100644 index 0000000000..ef07f9cb07 --- /dev/null +++ b/Waves4-6.list @@ -0,0 +1,87 @@ +● PR Review Plan: feature/crank_queues_apy → master + + 85 findings across 5 severity buckets. Sorted into ordered waves so each one builds on the last. + + Wave 1 — Production blockers (must fix before merge) + + Security / data corruption: + - #10 default_interval_schedule = "* */1 * * *" → "0 * * * *" (plugin.cpp:85, README:80). Fires 60×/hour on mainnet. This is the one flagged production-blocker. + - #1 TLS hostname verification — add stream.set_verify_callback(asio::ssl::host_name_verification(host)) at plugin.cpp:132-140. + - #11 API-key leaks: mask beacon-chain-api-key in app.hpp:79-81; log URL before appending apikey at plugin.cpp:118. + - #4 JSON leading-zeros bug at json.cpp:321-328 — "00…00" (20+ zeros) produces uint256 variant instead of returning 0. One-line fix shown in review. + - #12 parts[1] OOB on --beacon-chain-interval myname (no comma) — add SYS_ASSERT(parts.size() == 2, …) at plugin.cpp:311. + + Concurrency (blocking_retry trifecta at cron_service.hpp): + - #7 race on scheduled_id write (line 255 vs 248) — needs promise/latch so scheduler can't fire before assignment. + - #8 busy-spin burns a core for up to 50 min — condition_variable or std::future::wait(). + - #9 num_threads=1 default aborts blocking_retry's precondition — pick: (a) default 2, (b) precondition check at plugin_startup, (c) README/help doc. + + Latent API bugs exposed by public headers: + - #2 block_tag(uint64_t bn) discards bn — fix number(bn) at ethereum_client.cpp:24-26. + - #3 block_tag::to_string() falls off end (UB) — add default: branch. + - #6 Nonce cache ignores address — decide fix shape (cache only for signer / cache-by-address / split methods). + + Repo hygiene: + - #13 Delete CRON_PARSER_SUMMARY.md (root-level status doc — prior feedback says no). + - #5 JSON overflow wraparound — needs policy decision (see below). + + Wave 2 — Defense-in-depth (should fix) + + - Bounds-clamp withdraw_delay_sec ≤ 180d, entry_queue_days ≤ 365, apy_bps ≤ 10000 in beacon_chain_config_updates.cpp. + - Negative/int64 EPA treated as huge uint at plugin.cpp:210-216 — restrict to is_uint64() or range-check. + - compute_apy_updates accept integer APY via is_numeric(), not just is_double(). + - apy_fraction_to_bps guard std::isfinite + upper clamp. + - HTTP body_limit via response_parser. + - DNS resolve after expires_after (or bounded via ASIO timer). + - fc::mutable_variant_object for {"chain": network} at plugin.cpp:174. + - README: add "Security considerations" section for KEY:0x on CLI; dlog mask at signature_provider_manager_plugin.cpp:426. + - Chain-ID pinning — warn when absent from --outpost-ethereum-client. + + Wave 3 — Correctness & consistency cleanup + + - Plugin naming inconsistency — review suggests renaming options to wire-eth-*. You already chose to keep beacon-chain-*; this is a standing decision to re-affirm or revisit. + - plugin_shutdown — cancel just_once_jid, curl_global_cleanup, wait for in-flight HTTPS. + - Add #include to public wire_eth_maintenance_plugin.hpp. + - README: "10 minutes" → "50 minutes"; drop "lightweight" or narrow chain_target link set. + - cron.add_job → cron.add in both .md files. + - block_tag class: enum class, drop const members, remove dead valid_label(), explicit ctor, rename block_tag_t typedef (dead). + - block_tag parameter-vs-type shadowing — rename parameter to tag/blk at 6 sites. + - Scrub em-dashes / emoji (codebase prohibits) in 4 files listed. + - Stale test comments test_json_variant.cpp:150-220 referencing non-existent int128 routing. + + Wave 4 — Minor / style (~25 items) + + Grouped by file so the diff is reviewable: + - json.cpp: drop check_int64/check_uint64 structs, redundant const_s. + - ethereum_client.cpp: double ;;, hex_prefix as string_view. + - ethereum_abi.cpp: typos, inconsistent hex prefix flag at line 518. + - outpost_ethereum_client_plugin.cpp: double-log, double-negation, trailing newline, const getters already good. + - wire_eth_maintenance_plugin.cpp: C-style cast, commented-out line 30, using namespace std, include order, "intervals vs actions" log message, TLS-context caching, response-body ilog → dlog. + - cron_parser.cpp: unused , value{}, tab-handling decision, step-larger-than-range warning. + - cron_service.hpp: variadic double-move note, e.dynamic_copy_exception() to avoid slicing, mutex split for nonce vs contracts. + - README: nits already listed. + + Wave 5 — Missing tests (review lists ~25) + + - json.cpp: all-zero inputs, 77/78/79-digit inputs, UINT256_MAX±1. + - cron_parser: range-lists, step-without-base, step > range, DOW=7, empty list entries, duplicates. + - cron_service::blocking_retry: whole API untested. + - wire_eth_maintenance_plugin: negative-EPA, overflow EPA, integer APY, NaN fraction, confirm_txs retry path, plugin_init/startup/shutdown, malformed interval spec, send_* / confirm_txs throwing. + - outpost_ethereum_client_plugin: get_abis_for_contract, get_client_for_chain, chain-id spec parsing (3 vs 4 fields). + + Wave 6 — Nitpicks (~20) + + Drive-by sweep: trailing ;, double spaces, missing newlines, hex_prefix → string_view, README "10 min" already listed, test typos ("INT64_MAX = -9223…" is INT64_MIN). Do all in one commit. + + + + + + + +
Skipped for Wave 5 (bigger-ticket items from the review):                                                                                                                   
+  - Plugin init/startup/shutdown test (needs appbase + cron + outpost fixture — significant harness).                                                                         
+  - confirm_txs retry path integration (needs ethereum mock).                                                                                                                 
+  - get_abis_for_contract / get_client_for_chain / chain-id spec parsing in outpost (needs plugin-level fixture; the existing outpost test file only does a tx-signing smoke  
+  test).                                                                                                                                                                      
+  - Step-larger-than-range cron validation — Wave 4 left this unresolved; writing a test that asserts current behavior would lock in the not-yet-fixed silent acceptance.
diff --git a/plugins/signature_provider_manager_plugin/CMakeLists.txt b/plugins/signature_provider_manager_plugin/CMakeLists.txt index 742452e77c..d91b865636 100644 --- a/plugins/signature_provider_manager_plugin/CMakeLists.txt +++ b/plugins/signature_provider_manager_plugin/CMakeLists.txt @@ -1,5 +1,7 @@ set(TARGET_NAME signature_provider_manager_plugin) +find_package(aws-cpp-sdk-kms CONFIG REQUIRED) + plugin_target( ${TARGET_NAME} @@ -10,6 +12,7 @@ plugin_target( LIBRARIES custom_appbase - http_client_plugin - wallet_plugin_headers) + http_client_plugin + wallet_plugin_headers + aws-cpp-sdk-kms) diff --git a/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md b/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md new file mode 100644 index 0000000000..a88db0e0db --- /dev/null +++ b/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md @@ -0,0 +1,337 @@ +# Design Note: AWS KMS signing for `signature_provider_manager_plugin` + +Status: In progress — registry overlays landed (file:// verified, GitHub-URL verification gated on master merge); CMake wiring underway +Author: brian.johnson@wire.network +Date: 2026-05-08 (initial draft 2026-05-06) +Plugin: `plugins/signature_provider_manager_plugin/` +External dep: [`aws-sdk-cpp`](https://vcpkg.io/en/package/aws-sdk-cpp) feature `kms` + +## 1. Motivation + +Today the only ways to attach signing material to a `signature_provider_manager_plugin` spec are: + +| Provider | Where the secret lives | Spec form | +|----------|------------------------------------------|---------------------------------| +| `KEY` | In the `nodeop`/cranker config / env var | `KEY:` | +| `KIOD` | Local `kiod` daemon over HTTP/Unix | `KIOD:` | + +Both require the operator either to put a raw private key on the host or to run a local key daemon. For production cranker / outpost-client deployments we want the signing key to live in **AWS KMS** so: + +- the private key material never appears on the host or in process memory, +- access is gated by IAM / IMDS / IRSA (no key files to rotate), +- audit of every `Sign` call is centralised in CloudTrail, +- key rotation and disablement do not require redeploying the binary. + +The first concrete user is the Ethereum cranker (`bin/cranker`), where the signer address (`0x8950…0800`) currently comes from a `KEY:0x…` literal in `~/scripts/cranker.sh`. Replacing it with a KMS-backed provider closes the only place where a hot Ethereum private key lives on the operator's box. + +## 2. Goals / Non-goals + +**Goals** + +1. Add a new `KMS:` provider type to the existing spec grammar, with no breaking change to `KEY:` / `KIOD:`. +2. Cover the immediate need: **secp256k1** signing for `chain_key_type_ethereum` and `chain_key_type_wire` (sysio K1). +3. Use the standard AWS credential chain — no new config knobs unless we discover we need them. +4. Keep the `signature_provider_t` model intact: `private_key` stays `std::optional` and is `std::nullopt` for KMS-backed providers (same shape as `KIOD`). +5. Ship with unit tests that don't require a live AWS account (mocked `KMSClient`). + +**Non-goals** + +1. **R1 / NIST P-256 (`chain_key_type_*` for R1)**: KMS supports `ECC_NIST_P256`, but no current Wire plugin needs it. Add later behind the same plumbing. +2. **Ed25519 / Solana keys**: KMS does not support Ed25519 asymmetric signing. Out of scope. +3. **BLS keys**: KMS does not support BLS12-381. Out of scope. +4. **Producer-block signing on the hot path**: KMS Sign is ~30-100 ms / call and ~$0.03 / 10k calls — fine for crank/maintenance traffic, **not** intended for `producer_plugin` block signing. Documented in §8 as a deliberate restriction. +5. **Multi-region failover.** + +## 3. Spec grammar + +Extend the existing `` grammar. The full signature-provider spec is unchanged: + +``` +,,,, +``` + +`` adds one form: + +``` +KMS: +``` + +where `` is **either** a full ARN + +``` +arn:aws:kms:::key/ +arn:aws:kms:::alias/ +``` + +**or** the shorthand `:` for environments that prefer not to embed an ARN: + +``` +KMS:us-east-1:alias/wire-cranker-eth-01 +KMS:us-east-1:1234abcd-12ab-34cd-56ef-1234567890ab +``` + +Region is mandatory because `Aws::Client::ClientConfiguration::region` must be set explicitly; we do not silently inherit `AWS_REGION` from the environment so that misconfiguration is loud. + +Cranker example, drop-in replacement for the current `KEY:` form: + +``` +--signature-provider eth-01,ethereum,ethereum,0x045a87…eef3,KMS:us-east-1:alias/wire-cranker-eth-01 +``` + +## 4. End-to-end flow + +``` +caller (e.g. cranker) ──► signature_provider_t::sign(digest) + │ + ▼ + KMS provider closure (per-spec) + │ + ┌─────────────┴─────────────┐ + │ │ + ▼ ▼ + Aws::KMS::KMSClient::Sign cached recovered_pubkey + MessageType=DIGEST (from GetPublicKey) + SigningAlgorithm=ECDSA_SHA_256 + │ + ▼ + DER (r, s) + │ + ▼ + DER → raw (r, s) + │ + ▼ + normalise s to low-S (EIP-2 / canonical secp256k1) + │ + ▼ + recover v ∈ {0, 1} by trial-recovery against pubkey + │ + ▼ + chain::signature_type + (em::compact_signature for ethereum, + ecc::signature_shim for wire k1) +``` + +Notes: + +- The `sign_fn` signature is `fc::crypto::signature(const sha256&)` — the digest is already 32 bytes, so we set `MessageType=DIGEST` and **never** ask KMS to do its own hashing. +- KMS does not return a recovery id. We must derive it locally by trying both parities and comparing the recovered public key to the known one (which we fetch once from KMS via `GetPublicKey` on first use and pin against the spec's `` field — see §6). +- DER signatures from KMS occasionally have leading zero padding on `r`/`s`; the parser must strip and zero-extend to 32 bytes. +- Low-S normalisation is required: Ethereum rejects high-S (EIP-2), and `fc::crypto::em::compact_signature` round-trips assume canonical form. + +## 5. C++ surface + +### 5.1 New translation unit + +``` +plugins/signature_provider_manager_plugin/ + src/ + kms_signature_provider.cpp (new) + kms_signature_provider.hpp (new, plugin-private) + signature_provider_manager_plugin.cpp (modified — one new branch) +``` + +Anonymous-namespace constants live at the top of `kms_signature_provider.cpp`: + +```cpp +namespace { + constexpr auto kms_spec_prefix = "KMS"; + constexpr auto kms_signing_algorithm = "ECDSA_SHA_256"; + constexpr auto kms_message_type_digest = "DIGEST"; + constexpr auto kms_arn_prefix = "arn:aws:kms:"; +} +``` + +### 5.2 Public entry point (plugin-private header) + +```cpp +namespace sysio::sigprov::kms { + +/// Opaque target — either an ARN or a parsed (region, key-id) pair. +struct kms_key_ref { + std::string region; + std::string key_id; ///< raw KMS KeyId, alias name, or ARN tail +}; + +/// Parses `KMS:` body (everything after `KMS:`). +kms_key_ref parse_kms_spec(std::string_view spec_data); + +/// Builds a `sign_fn` closure backed by AWS KMS for the given key reference, +/// validating that the recovered public key matches `expected_pubkey`. +fc::crypto::sign_fn make_kms_signature_provider( + const kms_key_ref& ref, + fc::crypto::chain_key_type_t key_type, + const fc::crypto::public_key& expected_pubkey); + +} // namespace sysio::sigprov::kms +``` + +### 5.3 Hook into `create_provider_from_spec` + +In `signature_provider_manager_plugin.cpp::signature_provider_manager_plugin_impl::create_provider_from_spec`, add a third branch alongside `KEY` / `KIOD`: + +```cpp +if (spec_type_str == kms_spec_prefix) { + auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); + return { sysio::sigprov::kms::make_kms_signature_provider(ref, key_type, public_key), + std::nullopt }; +} +``` + +Same shape as `KIOD`: returns `std::nullopt` for the optional `private_key` because nothing leaves AWS. + +### 5.4 SDK lifecycle + +`Aws::InitAPI` / `Aws::ShutdownAPI` are process-global and not safe to call concurrently. Wrap in a Meyers-singleton guard owned by the plugin: + +```cpp +class aws_sdk_lifecycle { +public: + static aws_sdk_lifecycle& instance() { + static aws_sdk_lifecycle s; + return s; + } +private: + aws_sdk_lifecycle() { Aws::InitAPI(_options); } + ~aws_sdk_lifecycle() { Aws::ShutdownAPI(_options); } + Aws::SDKOptions _options{}; +}; +``` + +`make_kms_signature_provider` calls `aws_sdk_lifecycle::instance()` once before constructing its first `KMSClient`. Static destruction order guarantees `ShutdownAPI` runs after the last `KMSClient` is destroyed (closures hold the client by `shared_ptr`). + +### 5.5 `KMSClient` reuse + +One `std::shared_ptr` per `(region, credentials)` pair, kept in a process-wide map keyed by region. Construction is cheap but not free; multiple cranker specs in the same region must not each spin up a fresh client. + +### 5.6 Threading + +`Aws::KMS::KMSClient::Sign` is thread-safe (per AWS SDK docs and source). The closure captures `std::shared_ptr` by value and is itself thread-safe. + +### 5.7 Public-key pinning + +On first sign for a given closure, call `KMSClient::GetPublicKey` once, parse the DER X.509 SubjectPublicKeyInfo to raw secp256k1 (uncompressed `04 || X || Y`), compare to `expected_pubkey`. Throw `chain::plugin_config_exception` on mismatch. Cache the parsed pubkey for subsequent v-recovery. + +This catches the common operator error of putting a different `` in the spec than the KMS key actually holds. + +## 6. Build wiring + +### 6.1 `vcpkg.json` + +Add to `dependencies`: + +```json +{ + "name": "aws-sdk-cpp", + "default-features": false, + "features": ["kms"] +} +``` + +`default-features: false` is important — the upstream port defaults to `dynamodb`, `kinesis`, `s3` which we do not need (the upstream `vcpkg.in.json` confirms this). + +The aws-sdk-cpp port already pulls in `curl`, `openssl`, and `zlib`, all of which we have. `aws-crt-cpp` is a transitive dep and uses `s2n` on Linux — verify it doesn't conflict with our `boringssl-custom` curl feature (see §9 risks). + +### 6.2 Plugin `CMakeLists.txt` + +```cmake +find_package(aws-cpp-sdk-kms CONFIG REQUIRED) + +plugin_target( + ${TARGET_NAME} + SOURCE_GLOBS + src/*.cpp + src/*.hpp + include/*.hpp + + LIBRARIES + custom_appbase + http_client_plugin + wallet_plugin_headers + aws-cpp-sdk-kms +) +``` + +The `aws-cpp-sdk-kms` config (vcpkg-installed at `share/aws-cpp-sdk-kms/`) imports a STATIC `aws-cpp-sdk-kms` target and `find_dependency(aws-cpp-sdk-core)`. Transitive deps (`aws-crt-cpp` → `aws-c-cal` → `boringssl-custom`, `s2n`, etc.) propagate via `INTERFACE_LINK_LIBRARIES`. The umbrella `find_package(AWSSDK REQUIRED COMPONENTS kms)` is also viable but pulls in extra setup files (`sdksCommon.cmake`, `platformDeps.cmake`, `compiler_settings.cmake`) we do not need. + +## 7. Tests + +New test source: `plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp`, registered as a `boost_test`-style suite (`kms_signature_provider_tests`) and run from `tests/plugin_test`. + +| Suite case | What it pins | +|---------------------------------------------|---------------------------------------------------------| +| `parse_kms_spec_arn` | full-ARN form parses to (region, key-id) | +| `parse_kms_spec_region_keyid` | `:` form parses | +| `parse_kms_spec_rejects_no_region` | bare key-id throws `plugin_config_exception` | +| `der_to_raw_strips_leading_zero` | DER (r,s) round-trips to 32+32 raw | +| `low_s_normalisation` | high-S input flips to low-S, untouched if already low | +| `recover_v_zero_and_one` | both parities recovered correctly via mocked pubkey | +| `pubkey_mismatch_throws` | mocked `GetPublicKey` returning wrong key throws | +| `sign_round_trip_against_secp256k1_fixture` | mocked Sign with a known DER blob → expected `signature`| +| `sdk_init_idempotent` | constructing two providers in the same region init's once| + +The mock implements `Aws::KMS::KMSClient` by overriding `Sign(...)` and `GetPublicKey(...)`. Fixtures are precomputed offline with a fresh secp256k1 key so the test does not need network or AWS creds. + +A separate live-account test (`KMS_LIVE_TEST=1` env-gated) hits a sandbox alias `alias/wire-ci-test-secp256k1` for end-to-end verification — opt-in only, never run in default CI. + +## 8. Operational guidance (will land in `signature_provider_help_text()`) + +``` +KMS: is either an ARN + (arn:aws:kms:::(key|alias)/) + or :. + + The KMS key must be an asymmetric ECC_SECG_P256K1 key + with usage SIGN_VERIFY. AWS credentials are taken from + the standard chain (env, shared config, IRSA, IMDS). + + Recommended for outpost cranker / maintenance signers. + NOT recommended for producer block signing — adds + ~30-100ms per signature and is rate-limited per AWS + account / region. +``` + +## 9. Risks & open questions + +1. **`aws-sdk-cpp` / `aws-c-cal` / `s2n` ↔ `boringssl-custom` collision (RESOLVED).** The original observation: `boringssl-custom` claims OpenSSL's install footprint (same header paths, same `libssl.a`), so any port that declares an upstream `openssl` dep collides at port-install time. Three layers in the AWS dep graph declare `openssl`: + + ``` + aws-sdk-cpp ──► aws-crt-cpp + ├── aws-c-cal ──► (declares openssl) + └── aws-c-io + └── s2n ──► (declares openssl) + aws-sdk-cpp itself ─────────────────► (declares openssl) + ``` + + **Resolution (landed):** overlay ports in `wire-network/wire-vcpkg-registry` on branch `feature/aws-sdk-cpp-boringssl` (commits `641bb7f`, `d1e4cc9`, `088646f`): + + - `ports/aws-sdk-cpp/` — manifest only; drops the `openssl` dep (the SDK's per-OS `OpenSSLImpl` was removed before 1.11.591, crypto delegates to `aws-crt-cpp`). + - `ports/aws-c-cal/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `aws-c-cal-config.cmake` to use `boringssl::crypto`. + - `ports/s2n/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `s2n-config.cmake` to use `boringssl::crypto`; relaxes `-Werror` on the build (upstream tracks bleeding-edge compilers). + + `vcpkg-configuration.json` claims all three port names in the wire registry's `packages` array and pins `baseline: 088646fd...`. End-to-end verified against a `file:///home/swamp/dev/wire-vcpkg-registry` URL: all 50 ports install cleanly, `libaws-cpp-sdk-kms.a` + `libs2n.a` + `libbscrypto.a` materialise in `vcpkg_installed/x64-linux/lib/`, no port-install collision, `cmake -B build -S .` exits 0 in 105.9 s. + + **Pending durability step:** `feature/aws-sdk-cpp-boringssl` must merge to `master` in the registry. vcpkg's git-registry uses the registry's HEAD branch for port discovery (independent of the configured baseline SHA), so consumers fetching from `https://github.com/wire-network/wire-vcpkg-registry` will get `error: aws-sdk-cpp does not exist` until the branch lands on master. PR or fast-forward push to master closes this. +2. **Determinism.** secp256k1 ECDSA in KMS is **not** RFC-6979 deterministic. Each call returns a fresh `(r, s)`. Fine for transactions (nonce + chainId pin replay), but a hard "no" for any signing path that consensus assumes is deterministic across nodes. Documented in §8. +3. **Cold-start latency.** First sign on a fresh process can take 200-800 ms while the credential provider chain resolves IMDS / IRSA. **Mitigation:** call `GetPublicKey` once at plugin startup to warm the client, fail fast if creds are missing. +4. **Cost.** $0.03 per 10k Sign calls plus per-key monthly storage. Trivial for cranker (≪ 1 sign/min); flag in §8. +5. **Throttling.** KMS default is 10k req/s shared per region. Far above what any single signer needs, but AWS SDK's default retry strategy already handles `ThrottlingException` — confirmed acceptable, no custom retry policy needed. +6. **Static-destruction order of `aws_sdk_lifecycle`.** Closures captured into `signature_provider_t::sign` hold the `KMSClient` by `shared_ptr`. If a `signature_provider_t` outlives the plugin (e.g. captured into a long-lived appbase context) static destruction order matters. **Mitigation:** plugin clears its provider map in `plugin_shutdown` before SDK shutdown runs. + +## 10. Implementation plan + +Each step is independently reviewable. Order matters — earlier steps unblock later ones. + +| # | Step | Files touched | Verification | +|---|------|---------------|--------------| +| 1a | **DONE (locally verified, pending master merge).** Overlay ports `aws-sdk-cpp`, `aws-c-cal`, `s2n` in `wire-network/wire-vcpkg-registry`, swapping `openssl` → `boringssl-custom`; baseline bumped; port names registered in `vcpkg-configuration.json`'s `packages` array. See §9 risk #1. | `wire-vcpkg-registry/ports/{aws-sdk-cpp,aws-c-cal,s2n}/`, `wire-vcpkg-registry/versions/`, `vcpkg-configuration.json` | All three ports install via `cmake -B build -S .` against a `file://` clone (verified). GitHub-URL access pending merge of `feature/aws-sdk-cpp-boringssl` → `master`. | +| 1b | **DONE.** `aws-sdk-cpp` (`kms` feature, no defaults) added to `vcpkg.json`. | `vcpkg.json` | vcpkg installs `libaws-cpp-sdk-kms.a` + transitive deps with `--default-features=false`; configure exits 0 in 105.9 s. | +| 2 | Smoke-link AWS SDK into the plugin's `CMakeLists.txt` (no code yet). | `plugins/signature_provider_manager_plugin/CMakeLists.txt` | `ninja -C $BUILD_DIR signature_provider_manager_plugin nodeop` succeeds; build graph references `aws-cpp-sdk-kms`. | +| 3 | Spec parser: `parse_kms_spec` + unit tests for ARN / shorthand / error paths. | `kms_signature_provider.{hpp,cpp}`, `test_kms_signature_provider.cpp` | New `parse_kms_spec_*` tests pass under `plugin_test`. | +| 4 | DER→raw + low-S normalisation + v-recovery helpers, with secp256k1 fixtures (no AWS calls). | `kms_signature_provider.cpp`, test file | `der_to_raw_*`, `low_s_*`, `recover_v_*` cases pass. | +| 5 | `aws_sdk_lifecycle` singleton + per-region `KMSClient` cache. | `kms_signature_provider.cpp` | `sdk_init_idempotent` test passes. | +| 6 | `make_kms_signature_provider` end-to-end with mocked `KMSClient` (Sign + GetPublicKey). | `kms_signature_provider.cpp`, test file | `sign_round_trip_*`, `pubkey_mismatch_throws` pass. | +| 7 | Wire `KMS` branch into `signature_provider_manager_plugin_impl::create_provider_from_spec`; update help text. | `signature_provider_manager_plugin.cpp` | `--signature-provider …,KMS:…` parses; existing `KEY` / `KIOD` paths untouched. | +| 8 | Verify on Hoodi against a real KMS key — point cranker at `KMS:us-east-1:alias/wire-cranker-eth-01`, confirm signer address matches the existing `0x8950…0800` (or whatever the new authorised address is, depending on how the role-grant from yesterday's diagnosis is resolved). | `~/scripts/cranker.sh` (operator config, not committed) | Cranker submits a successful tx (no `AccessManagedUnauthorized` revert from §1 of yesterday's diagnosis, no `KMSException`). | +| 9 | **DONE.** Documentation pass: `signature_provider_help_text()` extended in step 7; `BUILD.md` now notes the `aws-sdk-cpp[kms]` + overlay-port chain pulled by vcpkg on first configure; `OVERVIEW.md` Batch Operators section mentions the new `KMS:` option alongside `KEY:`/`KIOD:`. | `signature_provider_manager_plugin.cpp`, `BUILD.md`, `OVERVIEW.md` | Docs reflect the landed implementation. | + +Steps 1-7 are pure-local, no AWS account needed. Step 8 is the only one that touches a real KMS key and is gated on having a provisioned secp256k1 KMS key + an IAM grant for the runner. diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp new file mode 100644 index 0000000000..e85740e8f1 --- /dev/null +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -0,0 +1,327 @@ +#include "kms_signature_provider.hpp" + +#include +#include + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include +#include + +#include +#include +#include +#include +#include +#include + +namespace sysio::sigprov::kms { + +namespace { + +/// Anchor for ARN detection. ARNs always start with `arn:aws:kms:` (no +/// regional suffix on the partition for kms — `arn:aws-cn:kms:` and +/// `arn:aws-us-gov:kms:` are not currently in scope; revisit if a partition +/// other than `aws` becomes a deployment target). +constexpr std::string_view kms_arn_prefix = "arn:aws:kms:"; + +/// Number of colon-separated segments in a well-formed KMS ARN: +/// `arn`, `aws`, `kms`, ``, ``, `(key|alias)/`. +constexpr std::size_t kms_arn_segment_count = 6; + +/// Indices into the split ARN. +constexpr std::size_t arn_idx_region = 3; +constexpr std::size_t arn_idx_tail = 5; + +/// Tail prefixes the KMS API accepts for the `KeyId` field. +constexpr std::string_view tail_prefix_key = "key/"; +constexpr std::string_view tail_prefix_alias = "alias/"; + +/// Process-wide secp256k1 context used by the DER / low-S helpers. Created +/// lazily on first use; libsecp256k1 contexts are thread-safe for the +/// signing-verification operations we use here. Lives separate from libfc's +/// internal context (`fc::em::detail::_get_context`) because that one is +/// not exposed across translation units. +const secp256k1_context* kms_secp_ctx() { + static secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); + return ctx; +} + +/// Ethereum's `v` offset: per Yellow-Paper Appendix F, the recovery byte is +/// `27 + recovery_id`. EIP-155 introduces a chain-id-tagged form for txs, +/// but the raw signing path used by the cranker / outpost client uses the +/// pre-EIP-155 (27/28) form. +constexpr unsigned char eth_v_offset = 27; + +/// Process-wide AWS SDK lifecycle. Constructed lazily on first KMS access, +/// destroyed at static destruction (after the client cache, since the cache +/// is touched by `get_kms_client` *after* this lifecycle, making it the +/// younger Meyers singleton; younger statics are destroyed first). The +/// remaining ordering risk — a `signature_provider_t` outliving the plugin +/// while still holding a `KMSClient` shared_ptr — is handled at the plugin +/// layer by clearing providers in `plugin_shutdown` (KMS_SIGNING_DESIGN.md +/// §9 risk #6). +struct aws_sdk_lifecycle { + static aws_sdk_lifecycle& instance() { + static aws_sdk_lifecycle s; + return s; + } +private: + aws_sdk_lifecycle() { Aws::InitAPI(_options); } + ~aws_sdk_lifecycle() { Aws::ShutdownAPI(_options); } + Aws::SDKOptions _options{}; +}; + +/// Per-closure state for a KMS-backed signer. Captured by `std::shared_ptr` +/// so that `std::function` copies remain cheap and the same `KMSClient` / +/// expected pubkey are shared across copies of the closure. +struct kms_signer_state { + std::shared_ptr client; + std::string key_id; + /// secp256k1 uncompressed public key the spec pinned. Used by + /// `recover_v` to discriminate between recovery_id 0 and 1. + fc::em::public_key expected_em_pubkey; +}; + +/// Translate an AWS KMS error outcome into a fc::exception with a stable +/// shape. The error-type enum name is the most actionable signal (e.g. +/// `AccessDenied`, `KeyUnavailable`, `Throttling`); the message and HTTP +/// status round out the diagnostic. +[[noreturn]] void throw_kms_error(std::string_view op, std::string_view key_id, + const Aws::Client::AWSError& err) { + FC_THROW_EXCEPTION(chain::plugin_config_exception, + "AWS KMS {} for key \"{}\" failed: {} (status {}, {}): {}", + op, key_id, + magic_enum::enum_name(err.GetErrorType()), + static_cast(err.GetResponseCode()), + std::string{err.GetExceptionName().c_str()}, + std::string{err.GetMessage().c_str()}); +} + +/// Per-region cache of KMS clients. Lock once on lookup; the SDK's HTTP +/// pool inside the client is itself thread-safe, so multiple sign closures +/// sharing a client may submit `Sign` requests concurrently. +struct kms_client_cache { + std::mutex mutex; + std::map> by_region; +}; + +kms_client_cache& kms_clients() { + static kms_client_cache c; + return c; +} + +} // namespace + +kms_key_ref parse_kms_spec(std::string_view spec_data) { + SYS_ASSERT(!spec_data.empty(), chain::plugin_config_exception, + "KMS spec body is empty; expected an ARN or ':'"); + + if (spec_data.starts_with(kms_arn_prefix)) { + // Full ARN form. Capture exactly `kms_arn_segment_count` parts so any + // stray colons inside the trailing `key/` segment stay glued to it + // (KMS key ids are uuids, no colons today, but aliases are operator- + // chosen and we should not silently truncate). + auto parts = fc::split(std::string{spec_data}, ':', kms_arn_segment_count); + SYS_ASSERT(parts.size() == kms_arn_segment_count, chain::plugin_config_exception, + "Malformed KMS ARN \"{}\": expected {} colon-separated segments, got {}", + spec_data, kms_arn_segment_count, parts.size()); + + const auto& region = parts[arn_idx_region]; + const auto& tail = parts[arn_idx_tail]; + + SYS_ASSERT(!region.empty(), chain::plugin_config_exception, + "KMS ARN \"{}\" has empty region segment", spec_data); + SYS_ASSERT(tail.starts_with(tail_prefix_key) || tail.starts_with(tail_prefix_alias), + chain::plugin_config_exception, + "KMS ARN tail must start with 'key/' or 'alias/', got \"{}\" in \"{}\"", + tail, spec_data); + // Reject bare prefixes: `key/` or `alias/` with nothing after them. + SYS_ASSERT(tail.size() > tail_prefix_key.size() && + (!tail.starts_with(tail_prefix_alias) || tail.size() > tail_prefix_alias.size()), + chain::plugin_config_exception, + "KMS ARN tail \"{}\" has empty key/alias name", tail); + + return kms_key_ref{region, tail}; + } + + // Shorthand `:`. We only split on the first colon + // so aliases that themselves contain colons round-trip unchanged (KMS + // alias names are operator-chosen, and while AWS docs disallow colons in + // alias names today, the parser should not be the layer that depends on + // that constraint). + const auto colon = spec_data.find(':'); + SYS_ASSERT(colon != std::string_view::npos, + chain::plugin_config_exception, + "KMS spec \"{}\" must include a region: expected ':' " + "or a full 'arn:aws:kms:...' ARN", spec_data); + SYS_ASSERT(colon > 0, chain::plugin_config_exception, + "KMS spec \"{}\" has empty region", spec_data); + SYS_ASSERT(colon + 1 < spec_data.size(), chain::plugin_config_exception, + "KMS spec \"{}\" has empty key id", spec_data); + + return kms_key_ref{ + std::string{spec_data.substr(0, colon)}, + std::string{spec_data.substr(colon + 1)}, + }; +} + +std::array der_to_compact(std::span der) { + secp256k1_ecdsa_signature parsed{}; + SYS_ASSERT(secp256k1_ecdsa_signature_parse_der(kms_secp_ctx(), &parsed, der.data(), der.size()) == 1, + chain::plugin_config_exception, + "KMS returned a malformed DER ECDSA signature ({} bytes)", der.size()); + + std::array compact{}; + const auto serialised = secp256k1_ecdsa_signature_serialize_compact( + kms_secp_ctx(), compact.data(), &parsed); + SYS_ASSERT(serialised == 1, chain::plugin_config_exception, + "Failed to serialise DER signature to compact (r||s) form"); + return compact; +} + +bool normalise_low_s(std::array& compact) { + secp256k1_ecdsa_signature parsed{}; + SYS_ASSERT(secp256k1_ecdsa_signature_parse_compact(kms_secp_ctx(), &parsed, compact.data()) == 1, + chain::plugin_config_exception, + "Cannot normalise an invalid compact ECDSA signature"); + + secp256k1_ecdsa_signature normalised{}; + // libsecp256k1 returns 1 if it actually flipped the signature, 0 if input + // was already in canonical (low-S) form. + const bool was_high = + secp256k1_ecdsa_signature_normalize(kms_secp_ctx(), &normalised, &parsed) == 1; + + secp256k1_ecdsa_signature_serialize_compact( + kms_secp_ctx(), compact.data(), &normalised); + return was_high; +} + +unsigned char recover_v(const std::array& compact, + std::span digest, + const fc::em::public_key& expected) { + for (unsigned char rec_id = 0; rec_id < 2; ++rec_id) { + fc::em::compact_signature trial{}; + std::ranges::copy(compact, trial.begin()); + trial[64] = static_cast(eth_v_offset + rec_id); + + try { + auto recovered = fc::em::public_key::recover(trial, digest.data(), + /* check_canonical */ false); + if (recovered == expected) { + return rec_id; + } + } catch (const fc::exception&) { + // Recovery failed for this parity; try the other one. + } + } + FC_THROW_EXCEPTION(chain::plugin_config_exception, + "Could not recover v: signature does not match expected public key"); +} + +fc::em::compact_signature der_to_eth_signature( + std::span der, + std::span digest, + const fc::em::public_key& expected_pubkey) { + auto compact = der_to_compact(der); + normalise_low_s(compact); + const auto rec_id = recover_v(compact, digest, expected_pubkey); + + fc::em::compact_signature out{}; + std::ranges::copy(compact, out.begin()); + out[64] = static_cast(eth_v_offset + rec_id); + return out; +} + +fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& ref, + fc::crypto::chain_key_type_t key_type, + const fc::crypto::public_key& expected_pubkey) { + // v1 only supports secp256k1 keys held as Ethereum-flavoured public keys + // (ECC_SECG_P256K1 in KMS, signed with ECDSA_SHA_256). Wire K1 (sysio's + // own secp256k1 path), R1, BLS, Ed25519, and webauthn use different key + // shapes or curves and need separate plumbing — see KMS_SIGNING_DESIGN.md + // §2 non-goals for the contract. + SYS_ASSERT(key_type == fc::crypto::chain_key_type_ethereum, + chain::pending_impl_exception, + "KMS signing currently supports only chain_key_type_ethereum, got {}", + fc::crypto::chain_key_type_reflector::to_string(key_type)); + + SYS_ASSERT(expected_pubkey.contains(), + chain::plugin_config_exception, + "KMS spec key_type={} does not match the public key variant " + "(expected em::public_key_shim)", + fc::crypto::chain_key_type_reflector::to_string(key_type)); + + const auto& shim = expected_pubkey.get(); + + auto state = std::make_shared(kms_signer_state{ + .client = get_kms_client(ref.region), + .key_id = ref.key_id, + .expected_em_pubkey = shim.unwrapped(), + }); + + return [state](const chain::digest_type& digest) -> chain::signature_type { + // Build a Sign request. MessageType=DIGEST tells KMS the 32 bytes are + // already a hash; otherwise it would re-hash with SHA-256 and break + // any chain that hashes with anything other than SHA-256. + Aws::KMS::Model::SignRequest req; + req.SetKeyId(Aws::String{state->key_id.c_str()}); + req.SetMessage(Aws::Utils::ByteBuffer{ + digest.to_uint8_span().data(), + digest.to_uint8_span().size()}); + req.SetMessageType(Aws::KMS::Model::MessageType::DIGEST); + req.SetSigningAlgorithm(Aws::KMS::Model::SigningAlgorithmSpec::ECDSA_SHA_256); + + auto outcome = state->client->Sign(req); + if (!outcome.IsSuccess()) { + throw_kms_error("Sign", state->key_id, outcome.GetError()); + } + + const auto& der_buf = outcome.GetResult().GetSignature(); + const std::span der{ + der_buf.GetUnderlyingData(), der_buf.GetLength()}; + + const auto compact = der_to_eth_signature( + der, digest.to_uint8_span(), state->expected_em_pubkey); + + return fc::crypto::signature( + fc::crypto::signature::storage_type{fc::em::signature_shim{compact}}); + }; +} + +std::shared_ptr get_kms_client(const std::string& region) { + SYS_ASSERT(!region.empty(), chain::plugin_config_exception, + "get_kms_client: region must not be empty"); + + // Force the lifecycle singleton's construction *before* we touch the + // cache, so its destructor (Aws::ShutdownAPI) runs *after* the cache's + // destructor (which clears all KMSClient shared_ptrs). Static-init order + // is the order of first-touch within the same TU; hitting the lifecycle + // first here pins it as the older static. + (void)aws_sdk_lifecycle::instance(); + + auto& c = kms_clients(); + std::scoped_lock lock{c.mutex}; + auto& slot = c.by_region[region]; + if (!slot) { + Aws::Client::ClientConfiguration cfg; + cfg.region = Aws::String{region.c_str()}; + slot = std::make_shared(cfg); + } + return slot; +} + +} // namespace sysio::sigprov::kms diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp new file mode 100644 index 0000000000..b4e1110a31 --- /dev/null +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -0,0 +1,168 @@ +#pragma once + +/** + * AWS KMS-backed signature provider — plugin-private header. + * + * The KMS provider extends the existing `KEY:` / `KIOD:` spec grammar with a + * third form, `KMS:`, where the signing key never leaves AWS. See + * `KMS_SIGNING_DESIGN.md` (this directory) for the full design. + * + * This header is consumed only by the plugin's own translation units and by + * its tests; it is intentionally not installed under `include/`. + */ + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace sysio::sigprov::kms { + +/** + * @brief Parsed KMS key reference. + * + * `key_id` is whatever follows the resolved region — either the bare key id / + * alias as the operator typed it (`:` form), or the + * `key/` / `alias/` tail of an ARN. AWS KMS accepts both shapes + * in the `KeyId` field of `Sign` / `GetPublicKey`, so we hand it through + * verbatim and let the SDK validate it. + */ +struct kms_key_ref { + std::string region; + std::string key_id; +}; + +/** + * @brief Parse the body of a `KMS:` provider spec (everything after `KMS:`). + * + * Accepted forms: + * - Full ARN: `arn:aws:kms:::(key|alias)/` + * Region is taken from the ARN's region segment; `key_id` is the trailing + * `key/` or `alias/` portion. + * - Shorthand: `:` + * Region is the leading token; everything after the first `:` is `key_id`. + * + * Region must always be present in the spec. We do not silently fall back to + * `AWS_REGION` env or shared-config lookups because misconfiguration there is + * harder to diagnose than a parse error here. + * + * @param spec_data the spec body, e.g. `us-east-1:alias/wire-cranker-eth-01` + * @throws sysio::chain::plugin_config_exception if the form is empty, + * malformed, or omits region + * @return parsed `kms_key_ref` ready to hand to the AWS SDK + */ +kms_key_ref parse_kms_spec(std::string_view spec_data); + +/** + * @brief Decode a DER-encoded ECDSA signature into 64-byte compact `r || s`. + * + * AWS KMS returns ECDSA signatures in ASN.1 DER (`30 ll 02 lr 02 ls `), + * with `r` and `s` zero-padded by one byte when their MSB is set. This helper + * normalises any such variation to a fixed-width 32-byte big-endian r/s pair. + * + * @param der DER-encoded signature bytes (typically 70-72 bytes) + * @throws sysio::chain::plugin_config_exception on malformed DER + * @return 64 bytes `[ r[32] | s[32] ]` + */ +std::array der_to_compact(std::span der); + +/** + * @brief Force `s` into the low half of the secp256k1 curve order (EIP-2). + * + * Ethereum (EIP-2) and Bitcoin (BIP-62) reject signatures with `s` in the upper + * half of the curve order to remove malleability. KMS does not enforce low-S, + * so we normalise on the way out. + * + * @param compact 64-byte `[ r | s ]` to be normalised in place + * @return true if `s` was high and got flipped, false if it was already low + */ +bool normalise_low_s(std::array& compact); + +/** + * @brief Find the `recovery_id ∈ {0, 1}` that recovers `expected` from `compact`. + * + * KMS does not return a recovery byte. We derive it locally by trying both + * parities, recovering the candidate public key, and comparing to the pubkey + * pinned in the spec. + * + * The digest is whatever 32 bytes were actually signed by the underlying + * ECDSA primitive. For Ethereum that is `keccak-256(rlp(tx))`; for any + * other secp256k1 path it is whatever the caller hashed. The recovery + * routine treats the bytes as opaque, so the helper takes a generic + * fixed-size span rather than a typed digest wrapper. + * + * @param compact 64-byte `[ r | s ]` (must already be low-S) + * @param digest the 32-byte digest the signature was produced over + * @param expected the public key the signer is supposed to control + * @throws sysio::chain::plugin_config_exception if neither parity recovers `expected` + * @return `recovery_id` ∈ {0, 1} (Ethereum's `v` byte is `27 + recovery_id`) + */ +unsigned char recover_v(const std::array& compact, + std::span digest, + const fc::em::public_key& expected); + +/** + * @brief End-to-end: DER → low-S compact + recovered v → 65-byte Ethereum sig. + * + * Wraps `der_to_compact` + `normalise_low_s` + `recover_v` and packs the + * result as `[ r | s | (27 + recovery_id) ]`, the format consumed by + * `fc::em::compact_signature` and ethereum-style transaction signing paths. + * + * @param der DER-encoded ECDSA signature returned from KMS::Sign + * @param digest 32-byte digest that was passed to KMS as `MessageType=DIGEST` + * @param expected_pubkey pubkey from the provider spec, validated against `KMSClient::GetPublicKey` + * @return ready-to-use 65-byte recoverable signature + */ +fc::em::compact_signature der_to_eth_signature( + std::span der, + std::span digest, + const fc::em::public_key& expected_pubkey); + +/** + * @brief Get (or lazily create) a process-wide `KMSClient` for `region`. + * + * The first call from the process triggers `Aws::InitAPI`; the SDK is shut + * down at static destruction (last-on-construct, first-off-destruct relative + * to this cache, so any `KMSClient` shared from here is destroyed before + * shutdown). Threadsafe; closures may keep the returned `shared_ptr` for + * their lifetime, and multiple specs in the same region share a single + * client (KMS API requests are stateless and the SDK's HTTP pool is + * thread-safe). + * + * Construction is offline: no credential resolution, no network. Credentials + * are looked up via the standard AWS provider chain on the first KMS API + * call, not here. + * + * @param region AWS region (e.g. `us-east-1`) + * @return shared `KMSClient` configured for `region` + */ +std::shared_ptr get_kms_client(const std::string& region); + +/** + * @brief Build a `sign_fn` closure that signs digests with AWS KMS. + * + * Stub in this revision: throws `chain::pending_impl_exception` on first + * invocation. Implementation lands in step 6 of `KMS_SIGNING_DESIGN.md` §10 + * (DER → raw, low-S normalisation, v-recovery, and `KMSClient` lifecycle). + * + * @param ref the parsed `(region, key_id)` pair + * @param key_type chain key type (must be ethereum / wire k1 — secp256k1) + * @param expected_pubkey public key the operator placed in the spec; the + * provider validates it against `KMSClient::GetPublicKey` on first sign + * @return a `sign_fn` matching `fc::crypto::sign_fn`'s signature + */ +fc::crypto::sign_fn make_kms_signature_provider( + const kms_key_ref& ref, + fc::crypto::chain_key_type_t key_type, + const fc::crypto::public_key& expected_pubkey); + +} // namespace sysio::sigprov::kms diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index 4b27372868..d8f731f069 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -17,6 +17,8 @@ #include #include +#include "kms_signature_provider.hpp" + namespace sysio { namespace { constexpr auto option_name_kiod_timeout_us = "signature-provider-kiod-timeout-us"; @@ -110,6 +112,17 @@ class signature_provider_manager_plugin_impl { return {make_kiod_signature_provider(spec_data, public_key), std::nullopt}; } + if (spec_type_str == "KMS") { + // KMS-backed signer: the secret never leaves AWS. `make_kms_signature_provider` + // validates the key_type / pubkey-variant pairing at construction; the + // closure issues KMS::Sign on each invocation, so credentials and + // network access are deferred to first sign rather than spec parse. + auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); + auto signer = sysio::sigprov::kms::make_kms_signature_provider( + ref, key_type, public_key); + return {std::move(signer), std::nullopt}; + } + SYS_THROW(chain::plugin_config_exception, "Unsupported key provider type \"{}\"", spec_type_str); } @@ -411,9 +424,16 @@ const char* signature_provider_manager_plugin::signature_provider_help_text() co " key format to parse\n\n" " is a string form of a valid \n\n" " is a string in the form :\n\n" - " is KEY, KIOD, or SE\n\n" + " is KEY, KIOD, KMS, or SE\n\n" " KEY: is a string containing a private key of the key-type specified\n\n" - " KIOD: is the URL where kiod is available and the appropriate wallet(s) are unlocked\n\n"; + " KIOD: is the URL where kiod is available and the appropriate wallet(s) are unlocked\n\n" + " KMS: is either a full AWS KMS ARN\n" + " (arn:aws:kms:::(key|alias)/) or the\n" + " shorthand :. The KMS key must be\n" + " asymmetric ECC_SECG_P256K1 with usage SIGN_VERIFY; signing\n" + " uses ECDSA_SHA_256. Credentials come from the standard AWS\n" + " provider chain (env, shared config, IRSA, IMDS). Currently\n" + " supports chain_key_type_ethereum only.\n\n"; } void signature_provider_manager_plugin::plugin_initialize(const variables_map& options) { diff --git a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp index 09d37464d4..d2bcab1782 100644 --- a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp @@ -18,6 +18,7 @@ #include +#include #include #include @@ -339,4 +340,59 @@ BOOST_AUTO_TEST_CASE(solana_signature_provider_spec_options) { BOOST_TEST((providers[0]->key_type == chain_key_type_solana)); } +BOOST_AUTO_TEST_CASE(create_provider_ethereum_kms_spec_routes_through_parser) { + // End-to-end check that the plugin's spec parser routes `KMS:` through + // `parse_kms_spec` + `make_kms_signature_provider` and returns a provider + // whose sign closure is callable. The closure itself is *not* invoked + // here — invocation issues a real KMS::Sign request, which lives behind + // the env-gated live test in KMS_SIGNING_DESIGN.md §7. + using namespace fc::test; + using namespace fc::crypto; + + auto clean_app = gsl_lite::finally([]() { + appbase::application::reset_app_singleton(); + }); + + keygen_result fixture = load_keygen_fixture("ethereum", 1); + const std::string kms_provider = "KMS:us-east-1:alias/wire-cranker-eth-01"; + const auto provider_spec = to_signature_provider_spec( + "kms-eth-01", chain_kind_ethereum, chain_key_type_ethereum, + fixture.public_key, kms_provider); + + auto tester = create_app(); + auto& mgr = tester->plugin(); + + const auto provider = mgr.create_provider(provider_spec); + + BOOST_CHECK_EQUAL(provider->key_name, "kms-eth-01"); + BOOST_TEST((provider->target_chain == chain_kind_ethereum)); + BOOST_TEST((provider->key_type == chain_key_type_ethereum)); + BOOST_CHECK(static_cast(provider->sign)); + // KMS-backed providers carry no local private key. + BOOST_CHECK(!provider->private_key.has_value()); +} + +BOOST_AUTO_TEST_CASE(create_provider_kms_spec_rejects_solana) { + // The plugin must reject a KMS spec for a non-secp256k1 chain at parse + // time, not at first sign — operators should learn early that KMS + // can't sign Solana ed25519. + using namespace fc::test; + using namespace fc::crypto; + + auto clean_app = gsl_lite::finally([]() { + appbase::application::reset_app_singleton(); + }); + + keygen_result fixture = load_keygen_fixture("solana", 1); + const std::string kms_provider = "KMS:us-east-1:alias/test"; + const auto provider_spec = to_signature_provider_spec( + "kms-sol-01", chain_kind_solana, chain_key_type_solana, + fixture.public_key, kms_provider); + + auto tester = create_app(); + auto& mgr = tester->plugin(); + + BOOST_CHECK_THROW(mgr.create_provider(provider_spec), sysio::chain::pending_impl_exception); +} + BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp new file mode 100644 index 0000000000..7aa48b873a --- /dev/null +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -0,0 +1,461 @@ +/** + * Unit tests for the AWS KMS signature provider parser. + * + * These cases cover only the offline parse path — `parse_kms_spec` does no + * network I/O and constructs no `KMSClient`, so the suite runs without AWS + * credentials and without an internet connection. End-to-end signing tests + * live in a separate suite (gated behind `KMS_LIVE_TEST`, see + * `KMS_SIGNING_DESIGN.md` §7). + */ + +#include + +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../src/kms_signature_provider.hpp" + +using sysio::sigprov::kms::kms_key_ref; +using sysio::sigprov::kms::parse_kms_spec; + +namespace { + +/** + * Test helper: encode a 64-byte (r||s) compact signature as ASN.1 DER. + * + * 30 02 02 + * + * `` and `` strip leading zero bytes; if the resulting + * MSB is set, a single 0x00 is prepended (ASN.1 INTEGER is signed). This + * matches what KMS / OpenSSL emit, and lets us round-trip through the + * production `der_to_compact` decoder without involving AWS. + */ +std::vector compact_to_der(const std::array& compact) { + const auto encode_int = [](std::span bytes) { + // Strip leading zero bytes (ASN.1 INTEGER must be minimal). + std::size_t start = 0; + while (start + 1 < bytes.size() && bytes[start] == 0) { + ++start; + } + std::vector out; + out.reserve(bytes.size() - start + 1); + // Pad with 0x00 if MSB is set so the INTEGER stays positive. + if ((bytes[start] & 0x80) != 0) { + out.push_back(0x00); + } + out.insert(out.end(), bytes.begin() + start, bytes.end()); + return out; + }; + + const auto r_int = encode_int(std::span(compact.data(), 32)); + const auto s_int = encode_int(std::span(compact.data() + 32, 32)); + + const std::size_t inner_len = 2 + r_int.size() + 2 + s_int.size(); + std::vector der; + der.reserve(2 + inner_len); + der.push_back(0x30); + der.push_back(static_cast(inner_len)); + der.push_back(0x02); + der.push_back(static_cast(r_int.size())); + der.insert(der.end(), r_int.begin(), r_int.end()); + der.push_back(0x02); + der.push_back(static_cast(s_int.size())); + der.insert(der.end(), s_int.begin(), s_int.end()); + return der; +} + +/// Curve order N for secp256k1 (big-endian). Used to construct deliberately +/// high-S signatures from a known low-S form by computing s' = N - s. +constexpr std::array secp256k1_curve_order = { + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, + 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, + 0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x41, +}; + +/// Big-endian 256-bit subtraction `s_high = N - s_low`. Operates on the +/// `s` half (bytes 32..63) of a compact signature. +void make_high_s(std::array& compact) { + int borrow = 0; + for (int i = 31; i >= 0; --i) { + int diff = static_cast(secp256k1_curve_order[i]) + - static_cast(compact[32 + i]) + - borrow; + if (diff < 0) { + diff += 256; + borrow = 1; + } else { + borrow = 0; + } + compact[32 + i] = static_cast(diff); + } +} + +/// Take the first 64 bytes (r || s) of a 65-byte compact_signature. +std::array drop_v(const fc::em::compact_signature& sig) { + std::array rs{}; + std::copy_n(sig.begin(), 64, rs.begin()); + return rs; +} + +} // anonymous namespace + +BOOST_AUTO_TEST_SUITE(kms_signature_provider_tests) + +BOOST_AUTO_TEST_CASE(parse_kms_spec_arn_key) { + const auto ref = parse_kms_spec("arn:aws:kms:us-east-1:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab"); + BOOST_CHECK_EQUAL(ref.region, "us-east-1"); + BOOST_CHECK_EQUAL(ref.key_id, "key/1234abcd-12ab-34cd-56ef-1234567890ab"); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_arn_alias) { + const auto ref = parse_kms_spec("arn:aws:kms:eu-west-2:111122223333:alias/wire-cranker-eth-01"); + BOOST_CHECK_EQUAL(ref.region, "eu-west-2"); + BOOST_CHECK_EQUAL(ref.key_id, "alias/wire-cranker-eth-01"); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_shorthand_uuid) { + const auto ref = parse_kms_spec("us-east-1:1234abcd-12ab-34cd-56ef-1234567890ab"); + BOOST_CHECK_EQUAL(ref.region, "us-east-1"); + BOOST_CHECK_EQUAL(ref.key_id, "1234abcd-12ab-34cd-56ef-1234567890ab"); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_shorthand_alias) { + const auto ref = parse_kms_spec("us-east-1:alias/wire-cranker-eth-01"); + BOOST_CHECK_EQUAL(ref.region, "us-east-1"); + BOOST_CHECK_EQUAL(ref.key_id, "alias/wire-cranker-eth-01"); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_empty) { + BOOST_CHECK_THROW(parse_kms_spec(""), sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_no_region_no_arn) { + // Bare key id — ambiguous about which region the key lives in. Must throw. + BOOST_CHECK_THROW(parse_kms_spec("1234abcd-12ab-34cd-56ef-1234567890ab"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_missing_tail) { + // ARN truncated before the key/alias portion (trailing colon, empty tail). + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1:111122223333:"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_bad_tail_prefix) { + // ARN tail that doesn't start with `key/` or `alias/` — KMS rejects this + // server-side, but we can fail loud at parse time. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1:111122223333:foo/bar"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_bare_alias_prefix) { + // `alias/` with no name attached. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1:111122223333:alias/"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_too_few_segments) { + // Missing both account and tail. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_shorthand_empty_region) { + // Leading colon means no region. + BOOST_CHECK_THROW(parse_kms_spec(":alias/wire-cranker-eth-01"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_shorthand_empty_key_id) { + // Trailing colon means no key id. + BOOST_CHECK_THROW(parse_kms_spec("us-east-1:"), + sysio::chain::plugin_config_exception); +} + +// --------------------------------------------------------------------------- +// DER → raw + low-S + v-recovery helpers +// +// These tests synthesise the inputs that AWS KMS would normally produce by +// signing locally with `fc::em::private_key::sign_compact` and re-encoding +// the resulting (r, s) as ASN.1 DER via `compact_to_der` above. No AWS +// credentials, no network, no `KMSClient`. +// --------------------------------------------------------------------------- + +BOOST_AUTO_TEST_CASE(der_to_compact_round_trips_random_signature) { + // ECDSA `r` is uniform over [1, N-1]; ~half of generated signatures will + // have the high bit of `r` set, exercising the leading-zero pad path in + // both `compact_to_der` and `der_to_compact`. Drive several rounds so we + // hit both branches deterministically over the suite's lifetime. + for (int i = 0; i < 8; ++i) { + const auto priv = fc::em::private_key::generate(); + const auto digest = fc::crypto::keccak256::hash(std::string{"round-trip"}); + const auto sig = priv.sign_compact(digest); + const auto rs = drop_v(sig); + + const auto der = compact_to_der(rs); + const auto decoded = sysio::sigprov::kms::der_to_compact(std::span(der)); + BOOST_CHECK(decoded == rs); + } +} + +BOOST_AUTO_TEST_CASE(der_to_compact_rejects_garbage) { + const std::array garbage{0xde, 0xad, 0xbe, 0xef}; + BOOST_CHECK_THROW(sysio::sigprov::kms::der_to_compact(std::span(garbage)), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(normalise_low_s_passes_through_already_low) { + // libsecp256k1's signing primitive always returns canonical (low-S) form, + // so the freshly produced signature must not be touched. + const auto priv = fc::em::private_key::generate(); + const auto digest = fc::crypto::keccak256::hash(std::string{"already-low"}); + const auto sig = priv.sign_compact(digest); + + auto rs = drop_v(sig); + const auto rs_orig = rs; + const bool was_high = sysio::sigprov::kms::normalise_low_s(rs); + BOOST_CHECK(!was_high); + BOOST_CHECK(rs == rs_orig); +} + +BOOST_AUTO_TEST_CASE(normalise_low_s_flips_high_to_low) { + // Construct an artificial high-S sig by setting s := N - s on a known + // low-S signature. The normaliser must flip it back identically. + const auto priv = fc::em::private_key::generate(); + const auto digest = fc::crypto::keccak256::hash(std::string{"flip-to-low"}); + const auto sig = priv.sign_compact(digest); + const auto rs_low = drop_v(sig); + + auto rs_high = rs_low; + make_high_s(rs_high); + BOOST_REQUIRE(rs_high != rs_low); // sanity: high-S really differs + + const bool was_high = sysio::sigprov::kms::normalise_low_s(rs_high); + BOOST_CHECK(was_high); + BOOST_CHECK(rs_high == rs_low); +} + +BOOST_AUTO_TEST_CASE(recover_v_finds_correct_parity) { + const auto priv = fc::em::private_key::generate(); + const auto pub = priv.get_public_key(); + const auto digest = fc::crypto::keccak256::hash(std::string{"recover-v"}); + const auto sig = priv.sign_compact(digest); + + const auto rs_only = drop_v(sig); + const unsigned char orig_v = static_cast(sig[64] - 27); + const unsigned char recovered_v = + sysio::sigprov::kms::recover_v(rs_only, digest.to_uint8_span(), pub); + BOOST_CHECK_EQUAL(static_cast(recovered_v), static_cast(orig_v)); +} + +BOOST_AUTO_TEST_CASE(recover_v_throws_on_wrong_pubkey) { + const auto priv = fc::em::private_key::generate(); + const auto wrong_pub = fc::em::private_key::generate().get_public_key(); + const auto digest = fc::crypto::keccak256::hash(std::string{"wrong-pub"}); + const auto sig = priv.sign_compact(digest); + + const auto rs_only = drop_v(sig); + BOOST_CHECK_THROW(sysio::sigprov::kms::recover_v(rs_only, digest.to_uint8_span(), wrong_pub), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(der_to_eth_signature_end_to_end_matches_local_sign) { + // Full pipeline: pretend KMS handed us back a DER encoding of a known + // low-S signature, then check the helper reconstructs the exact 65-byte + // form we would have got by signing locally. + const auto priv = fc::em::private_key::generate(); + const auto pub = priv.get_public_key(); + const auto digest = fc::crypto::keccak256::hash(std::string{"end-to-end"}); + const auto local = priv.sign_compact(digest); + + const auto rs = drop_v(local); + const auto der = compact_to_der(rs); + const auto out = sysio::sigprov::kms::der_to_eth_signature( + std::span(der), digest.to_uint8_span(), pub); + + BOOST_CHECK(out == local); +} + +BOOST_AUTO_TEST_CASE(der_to_eth_signature_normalises_high_s_input) { + // KMS does not enforce low-S, so a real-world DER blob may have high-S. + // The helper must normalise and still recover a valid v. + const auto priv = fc::em::private_key::generate(); + const auto pub = priv.get_public_key(); + const auto digest = fc::crypto::keccak256::hash(std::string{"high-s-from-kms"}); + const auto local = priv.sign_compact(digest); + + auto rs = drop_v(local); + make_high_s(rs); + const auto der_high = compact_to_der(rs); + const auto out = sysio::sigprov::kms::der_to_eth_signature( + std::span(der_high), digest.to_uint8_span(), pub); + + // Output must be canonical (low-S) and match the locally signed form. + BOOST_CHECK(out == local); +} + +// --------------------------------------------------------------------------- +// KMSClient cache + AWS SDK lifecycle +// +// `get_kms_client` is offline: it only touches the AWS SDK to construct +// `Aws::KMS::KMSClient`, which does not resolve credentials or open +// connections at construction time. These tests therefore run without AWS +// creds. +// --------------------------------------------------------------------------- + +BOOST_AUTO_TEST_CASE(get_kms_client_caches_per_region) { + const auto a1 = sysio::sigprov::kms::get_kms_client("us-east-1"); + const auto a2 = sysio::sigprov::kms::get_kms_client("us-east-1"); + BOOST_REQUIRE(a1); + BOOST_REQUIRE(a2); + BOOST_CHECK_EQUAL(a1.get(), a2.get()); +} + +BOOST_AUTO_TEST_CASE(get_kms_client_distinct_per_region) { + const auto east = sysio::sigprov::kms::get_kms_client("us-east-1"); + const auto west = sysio::sigprov::kms::get_kms_client("eu-west-2"); + BOOST_REQUIRE(east); + BOOST_REQUIRE(west); + BOOST_CHECK_NE(east.get(), west.get()); +} + +BOOST_AUTO_TEST_CASE(get_kms_client_rejects_empty_region) { + BOOST_CHECK_THROW(sysio::sigprov::kms::get_kms_client(""), + sysio::chain::plugin_config_exception); +} + +// --------------------------------------------------------------------------- +// make_kms_signature_provider +// +// Construction is offline (creates a closure capturing a cached KMSClient +// and the expected pubkey). Invocation issues a real KMS::Sign request and +// is therefore covered by env-gated live tests, not this suite. +// --------------------------------------------------------------------------- + +BOOST_AUTO_TEST_CASE(make_kms_signature_provider_returns_callable_for_ethereum) { + const auto chain_pub = + fc::crypto::private_key::generate(fc::crypto::private_key::key_type::em).get_public_key(); + + const auto sign = sysio::sigprov::kms::make_kms_signature_provider( + kms_key_ref{"us-east-1", "alias/wire-cranker-eth-01"}, + fc::crypto::chain_key_type_ethereum, + chain_pub); + + // Closure must be set; we deliberately do not invoke it (would hit live KMS). + BOOST_CHECK(static_cast(sign)); +} + +BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_wire_k1) { + // Wire K1 (`chain_key_type_wire`) is also secp256k1, but its public-key + // and signature shapes differ from Ethereum's. Goal #2 of the design note + // tracks adding it; until then, fail loud. + const auto chain_pub = + fc::crypto::private_key::generate(fc::crypto::private_key::key_type::k1).get_public_key(); + + BOOST_CHECK_THROW( + sysio::sigprov::kms::make_kms_signature_provider( + kms_key_ref{"us-east-1", "alias/wire-k1"}, + fc::crypto::chain_key_type_wire, + chain_pub), + sysio::chain::pending_impl_exception); +} + +BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_solana) { + // Solana keys are Ed25519 — KMS does not support that signing algorithm. + const auto chain_pub = + fc::crypto::private_key::generate(fc::crypto::private_key::key_type::ed).get_public_key(); + + BOOST_CHECK_THROW( + sysio::sigprov::kms::make_kms_signature_provider( + kms_key_ref{"us-east-1", "alias/test"}, + fc::crypto::chain_key_type_solana, + chain_pub), + sysio::chain::pending_impl_exception); +} + +// --------------------------------------------------------------------------- +// Live KMS round-trip — env-gated, skipped in default CI. +// +// Set both env vars to exercise this case against a real AWS KMS key: +// KMS_LIVE_SPEC body of a `KMS:` spec, e.g. `us-east-1:alias/wire-ci-test` +// or `arn:aws:kms:us-east-1:111122223333:alias/wire-ci-test` +// KMS_LIVE_PUBKEY uncompressed secp256k1 public key hex (0x04 || X || Y), +// matching the public key held by the KMS key +// +// The case will: +// 1. Build a real `make_kms_signature_provider` closure. +// 2. Sign a fixed deterministic digest (so the test is replayable). +// 3. Recover the public key from the resulting Ethereum compact signature. +// 4. Assert the recovered key matches `KMS_LIVE_PUBKEY` byte-for-byte. +// +// Requires AWS credentials in the runner's environment (env, ~/.aws/, IRSA, +// or IMDS) with `kms:Sign` on the target key. KMS::Sign is billable, so this +// case is opt-in only. +// --------------------------------------------------------------------------- +BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { + const auto* spec_env = std::getenv("KMS_LIVE_SPEC"); + const auto* pub_env = std::getenv("KMS_LIVE_PUBKEY"); + if (!spec_env || !pub_env || *spec_env == '\0' || *pub_env == '\0') { + BOOST_TEST_MESSAGE("KMS_LIVE_SPEC / KMS_LIVE_PUBKEY not set — skipping live KMS test"); + return; + } + + const std::string pub_hex{pub_env}; + const auto chain_pub = + fc::crypto::from_native_string_to_public_key(pub_hex); + const auto em_expected = chain_pub.get().unwrapped(); + + const auto ref = sysio::sigprov::kms::parse_kms_spec(spec_env); + const auto sign = sysio::sigprov::kms::make_kms_signature_provider( + ref, fc::crypto::chain_key_type_ethereum, chain_pub); + + // Deterministic digest so the test is replayable and the AWS account audit + // log entries are recognisable across runs. + const auto keccak = fc::crypto::keccak256::hash(std::string{"wire-sysio kms live test 2026"}); + + // chain::digest_type is fc::sha256 — a 32-byte holder. We copy the + // keccak256 bytes verbatim because KMS treats the message as opaque + // when MessageType=DIGEST. + sysio::chain::digest_type chain_digest; + std::memcpy(chain_digest.data(), keccak.data(), 32); + + const auto sig = sign(chain_digest); + + const auto& em_sig = sig.get(); + const auto recovered = em_sig.recover_eth(keccak).unwrapped(); + BOOST_CHECK(recovered == em_expected); +} + +BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_pubkey_variant_mismatch) { + // Caller declared key_type=ethereum but handed us a non-em pubkey. This + // catches a misconfigured spec where `` and `` + // disagree on the underlying curve. + const auto wire_pub = + fc::crypto::private_key::generate(fc::crypto::private_key::key_type::k1).get_public_key(); + + BOOST_CHECK_THROW( + sysio::sigprov::kms::make_kms_signature_provider( + kms_key_ref{"us-east-1", "alias/test"}, + fc::crypto::chain_key_type_ethereum, + wire_pub), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_SUITE_END() From c210bfcf2050f5ac3634e2b320c9e9923ae393c7 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 13 May 2026 21:38:23 -0500 Subject: [PATCH 04/22] AWS_KMS: documentation update --- OVERVIEW.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/OVERVIEW.md b/OVERVIEW.md index 3b3301c7c4..19c1ee3386 100644 --- a/OVERVIEW.md +++ b/OVERVIEW.md @@ -87,6 +87,8 @@ Elected via Appointed Proof of Stake (APoS). They validate transactions and prod - Building and submitting outbound envelopes to external chains - Responding to challenges if disputed +Signing keys for outbound submissions can be held locally (`KEY:`), in a `kiod` daemon (`KIOD:`), or in AWS KMS (`KMS:` — secp256k1 asymmetric keys). See `plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md` for the KMS spec format and operational notes. + ### Underwriters Independent operators running the `underwriter_plugin`. They provide collateral-backed liquidity for cross-chain swaps: 1. Monitor `sysio.msgch` for `SWAP` attestations From 2f3df3ebb2e9009d78dc176efb3569c2338b296b Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Thu, 14 May 2026 00:14:20 -0500 Subject: [PATCH 05/22] AWS_KMS: New baseline from vcpkg and documentation --- .../signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md | 4 ++-- vcpkg-configuration.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md b/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md index a88db0e0db..a2d71e32c8 100644 --- a/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md +++ b/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md @@ -302,13 +302,13 @@ KMS: is either an ARN aws-sdk-cpp itself ─────────────────► (declares openssl) ``` - **Resolution (landed):** overlay ports in `wire-network/wire-vcpkg-registry` on branch `feature/aws-sdk-cpp-boringssl` (commits `641bb7f`, `d1e4cc9`, `088646f`): + **Resolution (landed):** overlay ports in `wire-network/wire-vcpkg-registry` on branch `feature/aws-sdk-cpp-boringssl` (commits `641bb7f`, `d1e4cc9`, `088646f`, `c739f08`): - `ports/aws-sdk-cpp/` — manifest only; drops the `openssl` dep (the SDK's per-OS `OpenSSLImpl` was removed before 1.11.591, crypto delegates to `aws-crt-cpp`). - `ports/aws-c-cal/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `aws-c-cal-config.cmake` to use `boringssl::crypto`. - `ports/s2n/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `s2n-config.cmake` to use `boringssl::crypto`; relaxes `-Werror` on the build (upstream tracks bleeding-edge compilers). - `vcpkg-configuration.json` claims all three port names in the wire registry's `packages` array and pins `baseline: 088646fd...`. End-to-end verified against a `file:///home/swamp/dev/wire-vcpkg-registry` URL: all 50 ports install cleanly, `libaws-cpp-sdk-kms.a` + `libs2n.a` + `libbscrypto.a` materialise in `vcpkg_installed/x64-linux/lib/`, no port-install collision, `cmake -B build -S .` exits 0 in 105.9 s. + `vcpkg-configuration.json` claims all three port names in the wire registry's `packages` array and pins `baseline: c739f089...` (`aws-sdk-cpp 1.11.665`, `aws-c-cal 0.9.3`, `s2n 1.5.27`). End-to-end verified against a `file:///home/swamp/dev/wire-vcpkg-registry` URL: all 50 ports install cleanly, `libaws-cpp-sdk-kms.a` + `libs2n.a` + `libbscrypto.a` materialise in `vcpkg_installed/x64-linux/lib/`, no port-install collision, `cmake -B build -S .` exits 0 in 105.9 s. **Pending durability step:** `feature/aws-sdk-cpp-boringssl` must merge to `master` in the registry. vcpkg's git-registry uses the registry's HEAD branch for port discovery (independent of the configured baseline SHA), so consumers fetching from `https://github.com/wire-network/wire-vcpkg-registry` will get `error: aws-sdk-cpp does not exist` until the branch lands on master. PR or fast-forward push to master closes this. 2. **Determinism.** secp256k1 ECDSA in KMS is **not** RFC-6979 deterministic. Each call returns a fresh `(r, s)`. Fine for transactions (nonce + chainId pin replay), but a hard "no" for any signing path that consensus assumes is deterministic across nodes. Documented in §8. diff --git a/vcpkg-configuration.json b/vcpkg-configuration.json index 9f7f531fb4..ca951f1f73 100644 --- a/vcpkg-configuration.json +++ b/vcpkg-configuration.json @@ -9,7 +9,7 @@ { "kind": "git", "repository": "https://github.com/wire-network/wire-vcpkg-registry", - "baseline": "088646fd8a16a062714842c8cd6748007ba5eef7", + "baseline": "c739f089ed2a03adad8344aa753d4199c5081251", "packages": [ "boost", "softfloat", From 02317536543bb566c6c39cb499350eda2ec628de Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Thu, 14 May 2026 12:38:49 -0500 Subject: [PATCH 06/22] AWS_KMS: Updated version --- vcpkg.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vcpkg.json b/vcpkg.json index 09a902db3b..b43cc61ee1 100644 --- a/vcpkg.json +++ b/vcpkg.json @@ -61,7 +61,7 @@ { "name": "aws-sdk-cpp", "default-features": false, - "version>=": "1.11.591", + "version>=": "1.11.665", "features": [ "kms" ] @@ -148,7 +148,7 @@ }, { "name": "aws-sdk-cpp", - "version": "1.11.591" + "version": "1.11.665" } ] } From ec7cc07a50e581930a6addf06d69916788c97638 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 15 May 2026 16:12:58 -0500 Subject: [PATCH 07/22] AWS_KMS: Removed accidental file add --- Waves4-6.list | 87 --------------------------------------------------- 1 file changed, 87 deletions(-) delete mode 100644 Waves4-6.list diff --git a/Waves4-6.list b/Waves4-6.list deleted file mode 100644 index ef07f9cb07..0000000000 --- a/Waves4-6.list +++ /dev/null @@ -1,87 +0,0 @@ -● PR Review Plan: feature/crank_queues_apy → master - - 85 findings across 5 severity buckets. Sorted into ordered waves so each one builds on the last. - - Wave 1 — Production blockers (must fix before merge) - - Security / data corruption: - - #10 default_interval_schedule = "* */1 * * *" → "0 * * * *" (plugin.cpp:85, README:80). Fires 60×/hour on mainnet. This is the one flagged production-blocker. - - #1 TLS hostname verification — add stream.set_verify_callback(asio::ssl::host_name_verification(host)) at plugin.cpp:132-140. - - #11 API-key leaks: mask beacon-chain-api-key in app.hpp:79-81; log URL before appending apikey at plugin.cpp:118. - - #4 JSON leading-zeros bug at json.cpp:321-328 — "00…00" (20+ zeros) produces uint256 variant instead of returning 0. One-line fix shown in review. - - #12 parts[1] OOB on --beacon-chain-interval myname (no comma) — add SYS_ASSERT(parts.size() == 2, …) at plugin.cpp:311. - - Concurrency (blocking_retry trifecta at cron_service.hpp): - - #7 race on scheduled_id write (line 255 vs 248) — needs promise/latch so scheduler can't fire before assignment. - - #8 busy-spin burns a core for up to 50 min — condition_variable or std::future::wait(). - - #9 num_threads=1 default aborts blocking_retry's precondition — pick: (a) default 2, (b) precondition check at plugin_startup, (c) README/help doc. - - Latent API bugs exposed by public headers: - - #2 block_tag(uint64_t bn) discards bn — fix number(bn) at ethereum_client.cpp:24-26. - - #3 block_tag::to_string() falls off end (UB) — add default: branch. - - #6 Nonce cache ignores address — decide fix shape (cache only for signer / cache-by-address / split methods). - - Repo hygiene: - - #13 Delete CRON_PARSER_SUMMARY.md (root-level status doc — prior feedback says no). - - #5 JSON overflow wraparound — needs policy decision (see below). - - Wave 2 — Defense-in-depth (should fix) - - - Bounds-clamp withdraw_delay_sec ≤ 180d, entry_queue_days ≤ 365, apy_bps ≤ 10000 in beacon_chain_config_updates.cpp. - - Negative/int64 EPA treated as huge uint at plugin.cpp:210-216 — restrict to is_uint64() or range-check. - - compute_apy_updates accept integer APY via is_numeric(), not just is_double(). - - apy_fraction_to_bps guard std::isfinite + upper clamp. - - HTTP body_limit via response_parser. - - DNS resolve after expires_after (or bounded via ASIO timer). - - fc::mutable_variant_object for {"chain": network} at plugin.cpp:174. - - README: add "Security considerations" section for KEY:0x on CLI; dlog mask at signature_provider_manager_plugin.cpp:426. - - Chain-ID pinning — warn when absent from --outpost-ethereum-client. - - Wave 3 — Correctness & consistency cleanup - - - Plugin naming inconsistency — review suggests renaming options to wire-eth-*. You already chose to keep beacon-chain-*; this is a standing decision to re-affirm or revisit. - - plugin_shutdown — cancel just_once_jid, curl_global_cleanup, wait for in-flight HTTPS. - - Add #include to public wire_eth_maintenance_plugin.hpp. - - README: "10 minutes" → "50 minutes"; drop "lightweight" or narrow chain_target link set. - - cron.add_job → cron.add in both .md files. - - block_tag class: enum class, drop const members, remove dead valid_label(), explicit ctor, rename block_tag_t typedef (dead). - - block_tag parameter-vs-type shadowing — rename parameter to tag/blk at 6 sites. - - Scrub em-dashes / emoji (codebase prohibits) in 4 files listed. - - Stale test comments test_json_variant.cpp:150-220 referencing non-existent int128 routing. - - Wave 4 — Minor / style (~25 items) - - Grouped by file so the diff is reviewable: - - json.cpp: drop check_int64/check_uint64 structs, redundant const_s. - - ethereum_client.cpp: double ;;, hex_prefix as string_view. - - ethereum_abi.cpp: typos, inconsistent hex prefix flag at line 518. - - outpost_ethereum_client_plugin.cpp: double-log, double-negation, trailing newline, const getters already good. - - wire_eth_maintenance_plugin.cpp: C-style cast, commented-out line 30, using namespace std, include order, "intervals vs actions" log message, TLS-context caching, response-body ilog → dlog. - - cron_parser.cpp: unused , value{}, tab-handling decision, step-larger-than-range warning. - - cron_service.hpp: variadic double-move note, e.dynamic_copy_exception() to avoid slicing, mutex split for nonce vs contracts. - - README: nits already listed. - - Wave 5 — Missing tests (review lists ~25) - - - json.cpp: all-zero inputs, 77/78/79-digit inputs, UINT256_MAX±1. - - cron_parser: range-lists, step-without-base, step > range, DOW=7, empty list entries, duplicates. - - cron_service::blocking_retry: whole API untested. - - wire_eth_maintenance_plugin: negative-EPA, overflow EPA, integer APY, NaN fraction, confirm_txs retry path, plugin_init/startup/shutdown, malformed interval spec, send_* / confirm_txs throwing. - - outpost_ethereum_client_plugin: get_abis_for_contract, get_client_for_chain, chain-id spec parsing (3 vs 4 fields). - - Wave 6 — Nitpicks (~20) - - Drive-by sweep: trailing ;, double spaces, missing newlines, hex_prefix → string_view, README "10 min" already listed, test typos ("INT64_MAX = -9223…" is INT64_MIN). Do all in one commit. - - - - - - - -
Skipped for Wave 5 (bigger-ticket items from the review):                                                                                                                   
-  - Plugin init/startup/shutdown test (needs appbase + cron + outpost fixture — significant harness).                                                                         
-  - confirm_txs retry path integration (needs ethereum mock).                                                                                                                 
-  - get_abis_for_contract / get_client_for_chain / chain-id spec parsing in outpost (needs plugin-level fixture; the existing outpost test file only does a tx-signing smoke  
-  test).                                                                                                                                                                      
-  - Step-larger-than-range cron validation — Wave 4 left this unresolved; writing a test that asserts current behavior would lock in the not-yet-fixed silent acceptance.
From adf55fc2fb872cf2a08e971ff7508321c8b4c2c3 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 15 May 2026 21:05:43 -0500 Subject: [PATCH 08/22] AWS_KMS: added standalone test readme --- .../test/README.md | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 plugins/signature_provider_manager_plugin/test/README.md diff --git a/plugins/signature_provider_manager_plugin/test/README.md b/plugins/signature_provider_manager_plugin/test/README.md new file mode 100644 index 0000000000..de7e59fac0 --- /dev/null +++ b/plugins/signature_provider_manager_plugin/test/README.md @@ -0,0 +1,157 @@ +# `test_signature_provider_manager_plugin` — running the live AWS KMS round-trip + +This directory hosts the Boost.Test suite for the signature-provider plugin, +including the AWS KMS provider tests. Most of those cases are offline — they +exercise the spec parser, DER → raw conversion, low-S normalisation, and +v-recovery using locally generated secp256k1 keys, so they need no AWS +credentials and no network. + +The one case that does touch AWS is +`kms_signature_provider_tests/kms_live_sign_round_trip`. This README is the +minimum runbook to make it pass. + +## What the live case actually verifies + +1. Construct a real `make_kms_signature_provider` closure for the supplied + `KMS:`. +2. Issue a real `KMS::Sign` call against a fixed deterministic digest + (`keccak256("wire-sysio kms live test 2026")`), `MessageType=DIGEST`, + `SigningAlgorithm=ECDSA_SHA_256`. +3. Convert the returned DER signature to the 65-byte Ethereum compact form, + recovering `v` locally by trial-recovery against the pubkey you supplied. +4. Recover the public key from the resulting signature **locally** (no + `kms:Verify` call) and `BOOST_CHECK` that it matches `KMS_LIVE_PUBKEY` + byte-for-byte. + +So "KMS is working" here means: KMS returned a valid ECDSA signature over +your digest, and the key it signed with matches the public key you pinned. + +If `KMS_LIVE_SPEC` or `KMS_LIVE_PUBKEY` is unset or empty, the case logs +`skipping live KMS test` and exits success — that is how default CI stays +free of network calls and KMS charges. + +## Prerequisites + +### 1. KMS key + +A KMS key in your AWS account with: + +- **Key spec:** `ECC_SECG_P256K1` (secp256k1 — the curve Ethereum uses) +- **Key usage:** `SIGN_VERIFY` + +```bash +aws kms create-key \ + --region us-east-1 \ + --key-spec ECC_SECG_P256K1 \ + --key-usage SIGN_VERIFY \ + --description "wire-sysio KMS live signing test" +# capture the KeyId from the response, then optionally alias it: +aws kms create-alias \ + --region us-east-1 \ + --alias-name alias/wire-ci-test-secp256k1 \ + --target-key-id +``` + +### 2. IAM permission + +The principal whose credentials the test runs under needs `kms:Sign` on the +key. (The plugin does not call `kms:GetPublicKey` — the expected pubkey is +supplied by the caller, not fetched from KMS.) Minimum inline policy: + +```json +{ + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": "kms:Sign", + "Resource": "arn:aws:kms:us-east-1::key/" + }] +} +``` + +### 3. AWS credentials in the runner's environment + +Anything the standard AWS credential chain accepts works: `AWS_ACCESS_KEY_ID` ++ `AWS_SECRET_ACCESS_KEY` (+ `AWS_SESSION_TOKEN` if you assumed a role), +`~/.aws/credentials`, IRSA, IMDS, SSO. Confirm with: + +```bash +aws sts get-caller-identity +``` + +### 4. The matching public key in hex + +Pull the SubjectPublicKeyInfo from KMS and extract the uncompressed +`04 || X || Y` (65 bytes, 130 hex chars): + +```bash +aws kms get-public-key \ + --region us-east-1 \ + --key-id alias/wire-ci-test-secp256k1 \ + --output text \ + --query PublicKey \ +| base64 -d \ +| openssl ec -pubin -inform DER -text -noout 2>/dev/null \ +| awk '/^pub:/{flag=1;next}/ASN1 OID/{flag=0}flag' \ +| tr -d ' :\n' +``` + +That prints e.g. `045a87…eef3`. The fc parser also accepts the 64-byte raw +`X || Y` form (no `04` prefix) and the 33-byte compressed form (`02`/`03` +prefix); leading `0x` is optional. + +## Build + +The live case lives in the same binary as the parser tests. Build only what +you need (substitute your own `$BUILD_DIR`, e.g. `build/claude`): + +```bash +ninja -C $BUILD_DIR test_signature_provider_manager_plugin +``` + +## Run + +Export the two env vars and the AWS credentials, then invoke the binary with +a `--run_test` filter so you only pay for the one Sign call: + +```bash +export KMS_LIVE_SPEC='us-east-1:alias/wire-ci-test-secp256k1' +# or the full ARN form: +# export KMS_LIVE_SPEC='arn:aws:kms:us-east-1:111122223333:alias/wire-ci-test-secp256k1' +export KMS_LIVE_PUBKEY='045a87...eef3' + +$BUILD_DIR/plugins/signature_provider_manager_plugin/test/test_signature_provider_manager_plugin \ + --run_test=kms_signature_provider_tests/kms_live_sign_round_trip \ + --log_level=test_suite +``` + +Expected output on success: + +``` +Entering test case "kms_live_sign_round_trip" +Leaving test case "kms_live_sign_round_trip" +*** No errors detected +``` + +If you want to confirm the skip path works without spending money, unset the +env vars and run the same command — Boost prints +`skipping live KMS test` and the case still reports `No errors detected`. + +## Cost & rate-limit notes + +`KMS::Sign` is billed at roughly $0.03 per 10 000 calls, plus the per-key +monthly storage fee for the asymmetric key. A single live run is one Sign +call. KMS throttles at 10 000 req/s per region, far above what a single +test needs. + +## Common failure modes + +| Symptom | Likely cause | +|----------------------------------------------------|-----------------------------------------------------------| +| `Unable to locate credentials` | AWS credential chain came up empty — see Prereq #3 | +| `AccessDeniedException: not authorized to perform: kms:Sign` | IAM policy missing — see Prereq #2 | +| `NotFoundException: Invalid keyId` | Wrong region in `KMS_LIVE_SPEC`, or alias not created | +| `BOOST_CHECK(recovered == em_expected) failed` | `KMS_LIVE_PUBKEY` does not match the key KMS holds | +| `parse_kms_spec` exception at startup | Spec body malformed — see `KMS_SIGNING_DESIGN.md` §3 | + +See `../KMS_SIGNING_DESIGN.md` for the full design context. From f6624ac9b91a9d70ad14a6d50c4f10c27c0ac885 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 15 May 2026 21:08:56 -0500 Subject: [PATCH 09/22] AWS_KMS: added to standalone test readme --- .../signature_provider_manager_plugin/test/README.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/test/README.md b/plugins/signature_provider_manager_plugin/test/README.md index de7e59fac0..04d4310588 100644 --- a/plugins/signature_provider_manager_plugin/test/README.md +++ b/plugins/signature_provider_manager_plugin/test/README.md @@ -55,15 +55,20 @@ aws kms create-alias \ ### 2. IAM permission The principal whose credentials the test runs under needs `kms:Sign` on the -key. (The plugin does not call `kms:GetPublicKey` — the expected pubkey is -supplied by the caller, not fetched from KMS.) Minimum inline policy: +key. `kms:GetPublicKey` is also granted so the same principal can extract +the matching pubkey hex (Prereq #4) without juggling a second role; the +plugin itself does not call `GetPublicKey` (the expected pubkey is supplied +by the caller). Minimum inline policy: ```json { "Version": "2012-10-17", "Statement": [{ "Effect": "Allow", - "Action": "kms:Sign", + "Action": [ + "kms:Sign", + "kms:GetPublicKey" + ], "Resource": "arn:aws:kms:us-east-1::key/" }] } From 38441cf79d62417343c1fbeddf05eb0293ddf1fa Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Tue, 19 May 2026 22:29:49 -0500 Subject: [PATCH 10/22] AWS_KMS: Fixed comments and removed design doc for PR --- .../KMS_SIGNING_DESIGN.md | 337 ------------------ .../src/kms_signature_provider.cpp | 7 +- .../src/kms_signature_provider.hpp | 37 +- 3 files changed, 32 insertions(+), 349 deletions(-) delete mode 100644 plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md diff --git a/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md b/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md deleted file mode 100644 index a2d71e32c8..0000000000 --- a/plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md +++ /dev/null @@ -1,337 +0,0 @@ -# Design Note: AWS KMS signing for `signature_provider_manager_plugin` - -Status: In progress — registry overlays landed (file:// verified, GitHub-URL verification gated on master merge); CMake wiring underway -Author: brian.johnson@wire.network -Date: 2026-05-08 (initial draft 2026-05-06) -Plugin: `plugins/signature_provider_manager_plugin/` -External dep: [`aws-sdk-cpp`](https://vcpkg.io/en/package/aws-sdk-cpp) feature `kms` - -## 1. Motivation - -Today the only ways to attach signing material to a `signature_provider_manager_plugin` spec are: - -| Provider | Where the secret lives | Spec form | -|----------|------------------------------------------|---------------------------------| -| `KEY` | In the `nodeop`/cranker config / env var | `KEY:` | -| `KIOD` | Local `kiod` daemon over HTTP/Unix | `KIOD:` | - -Both require the operator either to put a raw private key on the host or to run a local key daemon. For production cranker / outpost-client deployments we want the signing key to live in **AWS KMS** so: - -- the private key material never appears on the host or in process memory, -- access is gated by IAM / IMDS / IRSA (no key files to rotate), -- audit of every `Sign` call is centralised in CloudTrail, -- key rotation and disablement do not require redeploying the binary. - -The first concrete user is the Ethereum cranker (`bin/cranker`), where the signer address (`0x8950…0800`) currently comes from a `KEY:0x…` literal in `~/scripts/cranker.sh`. Replacing it with a KMS-backed provider closes the only place where a hot Ethereum private key lives on the operator's box. - -## 2. Goals / Non-goals - -**Goals** - -1. Add a new `KMS:` provider type to the existing spec grammar, with no breaking change to `KEY:` / `KIOD:`. -2. Cover the immediate need: **secp256k1** signing for `chain_key_type_ethereum` and `chain_key_type_wire` (sysio K1). -3. Use the standard AWS credential chain — no new config knobs unless we discover we need them. -4. Keep the `signature_provider_t` model intact: `private_key` stays `std::optional` and is `std::nullopt` for KMS-backed providers (same shape as `KIOD`). -5. Ship with unit tests that don't require a live AWS account (mocked `KMSClient`). - -**Non-goals** - -1. **R1 / NIST P-256 (`chain_key_type_*` for R1)**: KMS supports `ECC_NIST_P256`, but no current Wire plugin needs it. Add later behind the same plumbing. -2. **Ed25519 / Solana keys**: KMS does not support Ed25519 asymmetric signing. Out of scope. -3. **BLS keys**: KMS does not support BLS12-381. Out of scope. -4. **Producer-block signing on the hot path**: KMS Sign is ~30-100 ms / call and ~$0.03 / 10k calls — fine for crank/maintenance traffic, **not** intended for `producer_plugin` block signing. Documented in §8 as a deliberate restriction. -5. **Multi-region failover.** - -## 3. Spec grammar - -Extend the existing `` grammar. The full signature-provider spec is unchanged: - -``` -,,,, -``` - -`` adds one form: - -``` -KMS: -``` - -where `` is **either** a full ARN - -``` -arn:aws:kms:::key/ -arn:aws:kms:::alias/ -``` - -**or** the shorthand `:` for environments that prefer not to embed an ARN: - -``` -KMS:us-east-1:alias/wire-cranker-eth-01 -KMS:us-east-1:1234abcd-12ab-34cd-56ef-1234567890ab -``` - -Region is mandatory because `Aws::Client::ClientConfiguration::region` must be set explicitly; we do not silently inherit `AWS_REGION` from the environment so that misconfiguration is loud. - -Cranker example, drop-in replacement for the current `KEY:` form: - -``` ---signature-provider eth-01,ethereum,ethereum,0x045a87…eef3,KMS:us-east-1:alias/wire-cranker-eth-01 -``` - -## 4. End-to-end flow - -``` -caller (e.g. cranker) ──► signature_provider_t::sign(digest) - │ - ▼ - KMS provider closure (per-spec) - │ - ┌─────────────┴─────────────┐ - │ │ - ▼ ▼ - Aws::KMS::KMSClient::Sign cached recovered_pubkey - MessageType=DIGEST (from GetPublicKey) - SigningAlgorithm=ECDSA_SHA_256 - │ - ▼ - DER (r, s) - │ - ▼ - DER → raw (r, s) - │ - ▼ - normalise s to low-S (EIP-2 / canonical secp256k1) - │ - ▼ - recover v ∈ {0, 1} by trial-recovery against pubkey - │ - ▼ - chain::signature_type - (em::compact_signature for ethereum, - ecc::signature_shim for wire k1) -``` - -Notes: - -- The `sign_fn` signature is `fc::crypto::signature(const sha256&)` — the digest is already 32 bytes, so we set `MessageType=DIGEST` and **never** ask KMS to do its own hashing. -- KMS does not return a recovery id. We must derive it locally by trying both parities and comparing the recovered public key to the known one (which we fetch once from KMS via `GetPublicKey` on first use and pin against the spec's `` field — see §6). -- DER signatures from KMS occasionally have leading zero padding on `r`/`s`; the parser must strip and zero-extend to 32 bytes. -- Low-S normalisation is required: Ethereum rejects high-S (EIP-2), and `fc::crypto::em::compact_signature` round-trips assume canonical form. - -## 5. C++ surface - -### 5.1 New translation unit - -``` -plugins/signature_provider_manager_plugin/ - src/ - kms_signature_provider.cpp (new) - kms_signature_provider.hpp (new, plugin-private) - signature_provider_manager_plugin.cpp (modified — one new branch) -``` - -Anonymous-namespace constants live at the top of `kms_signature_provider.cpp`: - -```cpp -namespace { - constexpr auto kms_spec_prefix = "KMS"; - constexpr auto kms_signing_algorithm = "ECDSA_SHA_256"; - constexpr auto kms_message_type_digest = "DIGEST"; - constexpr auto kms_arn_prefix = "arn:aws:kms:"; -} -``` - -### 5.2 Public entry point (plugin-private header) - -```cpp -namespace sysio::sigprov::kms { - -/// Opaque target — either an ARN or a parsed (region, key-id) pair. -struct kms_key_ref { - std::string region; - std::string key_id; ///< raw KMS KeyId, alias name, or ARN tail -}; - -/// Parses `KMS:` body (everything after `KMS:`). -kms_key_ref parse_kms_spec(std::string_view spec_data); - -/// Builds a `sign_fn` closure backed by AWS KMS for the given key reference, -/// validating that the recovered public key matches `expected_pubkey`. -fc::crypto::sign_fn make_kms_signature_provider( - const kms_key_ref& ref, - fc::crypto::chain_key_type_t key_type, - const fc::crypto::public_key& expected_pubkey); - -} // namespace sysio::sigprov::kms -``` - -### 5.3 Hook into `create_provider_from_spec` - -In `signature_provider_manager_plugin.cpp::signature_provider_manager_plugin_impl::create_provider_from_spec`, add a third branch alongside `KEY` / `KIOD`: - -```cpp -if (spec_type_str == kms_spec_prefix) { - auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); - return { sysio::sigprov::kms::make_kms_signature_provider(ref, key_type, public_key), - std::nullopt }; -} -``` - -Same shape as `KIOD`: returns `std::nullopt` for the optional `private_key` because nothing leaves AWS. - -### 5.4 SDK lifecycle - -`Aws::InitAPI` / `Aws::ShutdownAPI` are process-global and not safe to call concurrently. Wrap in a Meyers-singleton guard owned by the plugin: - -```cpp -class aws_sdk_lifecycle { -public: - static aws_sdk_lifecycle& instance() { - static aws_sdk_lifecycle s; - return s; - } -private: - aws_sdk_lifecycle() { Aws::InitAPI(_options); } - ~aws_sdk_lifecycle() { Aws::ShutdownAPI(_options); } - Aws::SDKOptions _options{}; -}; -``` - -`make_kms_signature_provider` calls `aws_sdk_lifecycle::instance()` once before constructing its first `KMSClient`. Static destruction order guarantees `ShutdownAPI` runs after the last `KMSClient` is destroyed (closures hold the client by `shared_ptr`). - -### 5.5 `KMSClient` reuse - -One `std::shared_ptr` per `(region, credentials)` pair, kept in a process-wide map keyed by region. Construction is cheap but not free; multiple cranker specs in the same region must not each spin up a fresh client. - -### 5.6 Threading - -`Aws::KMS::KMSClient::Sign` is thread-safe (per AWS SDK docs and source). The closure captures `std::shared_ptr` by value and is itself thread-safe. - -### 5.7 Public-key pinning - -On first sign for a given closure, call `KMSClient::GetPublicKey` once, parse the DER X.509 SubjectPublicKeyInfo to raw secp256k1 (uncompressed `04 || X || Y`), compare to `expected_pubkey`. Throw `chain::plugin_config_exception` on mismatch. Cache the parsed pubkey for subsequent v-recovery. - -This catches the common operator error of putting a different `` in the spec than the KMS key actually holds. - -## 6. Build wiring - -### 6.1 `vcpkg.json` - -Add to `dependencies`: - -```json -{ - "name": "aws-sdk-cpp", - "default-features": false, - "features": ["kms"] -} -``` - -`default-features: false` is important — the upstream port defaults to `dynamodb`, `kinesis`, `s3` which we do not need (the upstream `vcpkg.in.json` confirms this). - -The aws-sdk-cpp port already pulls in `curl`, `openssl`, and `zlib`, all of which we have. `aws-crt-cpp` is a transitive dep and uses `s2n` on Linux — verify it doesn't conflict with our `boringssl-custom` curl feature (see §9 risks). - -### 6.2 Plugin `CMakeLists.txt` - -```cmake -find_package(aws-cpp-sdk-kms CONFIG REQUIRED) - -plugin_target( - ${TARGET_NAME} - SOURCE_GLOBS - src/*.cpp - src/*.hpp - include/*.hpp - - LIBRARIES - custom_appbase - http_client_plugin - wallet_plugin_headers - aws-cpp-sdk-kms -) -``` - -The `aws-cpp-sdk-kms` config (vcpkg-installed at `share/aws-cpp-sdk-kms/`) imports a STATIC `aws-cpp-sdk-kms` target and `find_dependency(aws-cpp-sdk-core)`. Transitive deps (`aws-crt-cpp` → `aws-c-cal` → `boringssl-custom`, `s2n`, etc.) propagate via `INTERFACE_LINK_LIBRARIES`. The umbrella `find_package(AWSSDK REQUIRED COMPONENTS kms)` is also viable but pulls in extra setup files (`sdksCommon.cmake`, `platformDeps.cmake`, `compiler_settings.cmake`) we do not need. - -## 7. Tests - -New test source: `plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp`, registered as a `boost_test`-style suite (`kms_signature_provider_tests`) and run from `tests/plugin_test`. - -| Suite case | What it pins | -|---------------------------------------------|---------------------------------------------------------| -| `parse_kms_spec_arn` | full-ARN form parses to (region, key-id) | -| `parse_kms_spec_region_keyid` | `:` form parses | -| `parse_kms_spec_rejects_no_region` | bare key-id throws `plugin_config_exception` | -| `der_to_raw_strips_leading_zero` | DER (r,s) round-trips to 32+32 raw | -| `low_s_normalisation` | high-S input flips to low-S, untouched if already low | -| `recover_v_zero_and_one` | both parities recovered correctly via mocked pubkey | -| `pubkey_mismatch_throws` | mocked `GetPublicKey` returning wrong key throws | -| `sign_round_trip_against_secp256k1_fixture` | mocked Sign with a known DER blob → expected `signature`| -| `sdk_init_idempotent` | constructing two providers in the same region init's once| - -The mock implements `Aws::KMS::KMSClient` by overriding `Sign(...)` and `GetPublicKey(...)`. Fixtures are precomputed offline with a fresh secp256k1 key so the test does not need network or AWS creds. - -A separate live-account test (`KMS_LIVE_TEST=1` env-gated) hits a sandbox alias `alias/wire-ci-test-secp256k1` for end-to-end verification — opt-in only, never run in default CI. - -## 8. Operational guidance (will land in `signature_provider_help_text()`) - -``` -KMS: is either an ARN - (arn:aws:kms:::(key|alias)/) - or :. - - The KMS key must be an asymmetric ECC_SECG_P256K1 key - with usage SIGN_VERIFY. AWS credentials are taken from - the standard chain (env, shared config, IRSA, IMDS). - - Recommended for outpost cranker / maintenance signers. - NOT recommended for producer block signing — adds - ~30-100ms per signature and is rate-limited per AWS - account / region. -``` - -## 9. Risks & open questions - -1. **`aws-sdk-cpp` / `aws-c-cal` / `s2n` ↔ `boringssl-custom` collision (RESOLVED).** The original observation: `boringssl-custom` claims OpenSSL's install footprint (same header paths, same `libssl.a`), so any port that declares an upstream `openssl` dep collides at port-install time. Three layers in the AWS dep graph declare `openssl`: - - ``` - aws-sdk-cpp ──► aws-crt-cpp - ├── aws-c-cal ──► (declares openssl) - └── aws-c-io - └── s2n ──► (declares openssl) - aws-sdk-cpp itself ─────────────────► (declares openssl) - ``` - - **Resolution (landed):** overlay ports in `wire-network/wire-vcpkg-registry` on branch `feature/aws-sdk-cpp-boringssl` (commits `641bb7f`, `d1e4cc9`, `088646f`, `c739f08`): - - - `ports/aws-sdk-cpp/` — manifest only; drops the `openssl` dep (the SDK's per-OS `OpenSSLImpl` was removed before 1.11.591, crypto delegates to `aws-crt-cpp`). - - `ports/aws-c-cal/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `aws-c-cal-config.cmake` to use `boringssl::crypto`. - - `ports/s2n/` — drops the `openssl` dep, replaces it with `boringssl-custom`; patches `CMakeLists.txt` and the installed `s2n-config.cmake` to use `boringssl::crypto`; relaxes `-Werror` on the build (upstream tracks bleeding-edge compilers). - - `vcpkg-configuration.json` claims all three port names in the wire registry's `packages` array and pins `baseline: c739f089...` (`aws-sdk-cpp 1.11.665`, `aws-c-cal 0.9.3`, `s2n 1.5.27`). End-to-end verified against a `file:///home/swamp/dev/wire-vcpkg-registry` URL: all 50 ports install cleanly, `libaws-cpp-sdk-kms.a` + `libs2n.a` + `libbscrypto.a` materialise in `vcpkg_installed/x64-linux/lib/`, no port-install collision, `cmake -B build -S .` exits 0 in 105.9 s. - - **Pending durability step:** `feature/aws-sdk-cpp-boringssl` must merge to `master` in the registry. vcpkg's git-registry uses the registry's HEAD branch for port discovery (independent of the configured baseline SHA), so consumers fetching from `https://github.com/wire-network/wire-vcpkg-registry` will get `error: aws-sdk-cpp does not exist` until the branch lands on master. PR or fast-forward push to master closes this. -2. **Determinism.** secp256k1 ECDSA in KMS is **not** RFC-6979 deterministic. Each call returns a fresh `(r, s)`. Fine for transactions (nonce + chainId pin replay), but a hard "no" for any signing path that consensus assumes is deterministic across nodes. Documented in §8. -3. **Cold-start latency.** First sign on a fresh process can take 200-800 ms while the credential provider chain resolves IMDS / IRSA. **Mitigation:** call `GetPublicKey` once at plugin startup to warm the client, fail fast if creds are missing. -4. **Cost.** $0.03 per 10k Sign calls plus per-key monthly storage. Trivial for cranker (≪ 1 sign/min); flag in §8. -5. **Throttling.** KMS default is 10k req/s shared per region. Far above what any single signer needs, but AWS SDK's default retry strategy already handles `ThrottlingException` — confirmed acceptable, no custom retry policy needed. -6. **Static-destruction order of `aws_sdk_lifecycle`.** Closures captured into `signature_provider_t::sign` hold the `KMSClient` by `shared_ptr`. If a `signature_provider_t` outlives the plugin (e.g. captured into a long-lived appbase context) static destruction order matters. **Mitigation:** plugin clears its provider map in `plugin_shutdown` before SDK shutdown runs. - -## 10. Implementation plan - -Each step is independently reviewable. Order matters — earlier steps unblock later ones. - -| # | Step | Files touched | Verification | -|---|------|---------------|--------------| -| 1a | **DONE (locally verified, pending master merge).** Overlay ports `aws-sdk-cpp`, `aws-c-cal`, `s2n` in `wire-network/wire-vcpkg-registry`, swapping `openssl` → `boringssl-custom`; baseline bumped; port names registered in `vcpkg-configuration.json`'s `packages` array. See §9 risk #1. | `wire-vcpkg-registry/ports/{aws-sdk-cpp,aws-c-cal,s2n}/`, `wire-vcpkg-registry/versions/`, `vcpkg-configuration.json` | All three ports install via `cmake -B build -S .` against a `file://` clone (verified). GitHub-URL access pending merge of `feature/aws-sdk-cpp-boringssl` → `master`. | -| 1b | **DONE.** `aws-sdk-cpp` (`kms` feature, no defaults) added to `vcpkg.json`. | `vcpkg.json` | vcpkg installs `libaws-cpp-sdk-kms.a` + transitive deps with `--default-features=false`; configure exits 0 in 105.9 s. | -| 2 | Smoke-link AWS SDK into the plugin's `CMakeLists.txt` (no code yet). | `plugins/signature_provider_manager_plugin/CMakeLists.txt` | `ninja -C $BUILD_DIR signature_provider_manager_plugin nodeop` succeeds; build graph references `aws-cpp-sdk-kms`. | -| 3 | Spec parser: `parse_kms_spec` + unit tests for ARN / shorthand / error paths. | `kms_signature_provider.{hpp,cpp}`, `test_kms_signature_provider.cpp` | New `parse_kms_spec_*` tests pass under `plugin_test`. | -| 4 | DER→raw + low-S normalisation + v-recovery helpers, with secp256k1 fixtures (no AWS calls). | `kms_signature_provider.cpp`, test file | `der_to_raw_*`, `low_s_*`, `recover_v_*` cases pass. | -| 5 | `aws_sdk_lifecycle` singleton + per-region `KMSClient` cache. | `kms_signature_provider.cpp` | `sdk_init_idempotent` test passes. | -| 6 | `make_kms_signature_provider` end-to-end with mocked `KMSClient` (Sign + GetPublicKey). | `kms_signature_provider.cpp`, test file | `sign_round_trip_*`, `pubkey_mismatch_throws` pass. | -| 7 | Wire `KMS` branch into `signature_provider_manager_plugin_impl::create_provider_from_spec`; update help text. | `signature_provider_manager_plugin.cpp` | `--signature-provider …,KMS:…` parses; existing `KEY` / `KIOD` paths untouched. | -| 8 | Verify on Hoodi against a real KMS key — point cranker at `KMS:us-east-1:alias/wire-cranker-eth-01`, confirm signer address matches the existing `0x8950…0800` (or whatever the new authorised address is, depending on how the role-grant from yesterday's diagnosis is resolved). | `~/scripts/cranker.sh` (operator config, not committed) | Cranker submits a successful tx (no `AccessManagedUnauthorized` revert from §1 of yesterday's diagnosis, no `KMSException`). | -| 9 | **DONE.** Documentation pass: `signature_provider_help_text()` extended in step 7; `BUILD.md` now notes the `aws-sdk-cpp[kms]` + overlay-port chain pulled by vcpkg on first configure; `OVERVIEW.md` Batch Operators section mentions the new `KMS:` option alongside `KEY:`/`KIOD:`. | `signature_provider_manager_plugin.cpp`, `BUILD.md`, `OVERVIEW.md` | Docs reflect the landed implementation. | - -Steps 1-7 are pure-local, no AWS account needed. Step 8 is the only one that touches a real KMS key and is gated on having a provisioned secp256k1 KMS key + an IAM grant for the runner. diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index e85740e8f1..9b303ff426 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -70,10 +70,9 @@ constexpr unsigned char eth_v_offset = 27; /// destroyed at static destruction (after the client cache, since the cache /// is touched by `get_kms_client` *after* this lifecycle, making it the /// younger Meyers singleton; younger statics are destroyed first). The -/// remaining ordering risk — a `signature_provider_t` outliving the plugin -/// while still holding a `KMSClient` shared_ptr — is handled at the plugin -/// layer by clearing providers in `plugin_shutdown` (KMS_SIGNING_DESIGN.md -/// §9 risk #6). +/// safe because the application object owns the plugin and is destroyed +/// before atexit static teardown; do not hand a KMS-backed `sign_fn` to an +/// owner that outlives the application. struct aws_sdk_lifecycle { static aws_sdk_lifecycle& instance() { static aws_sdk_lifecycle s; diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index b4e1110a31..d3e1894bc0 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -150,14 +150,35 @@ std::shared_ptr get_kms_client(const std::string& region); /** * @brief Build a `sign_fn` closure that signs digests with AWS KMS. * - * Stub in this revision: throws `chain::pending_impl_exception` on first - * invocation. Implementation lands in step 6 of `KMS_SIGNING_DESIGN.md` §10 - * (DER → raw, low-S normalisation, v-recovery, and `KMSClient` lifecycle). - * - * @param ref the parsed `(region, key_id)` pair - * @param key_type chain key type (must be ethereum / wire k1 — secp256k1) - * @param expected_pubkey public key the operator placed in the spec; the - * provider validates it against `KMSClient::GetPublicKey` on first sign + * Validates `key_type` and the public-key variant, resolves the shared + * `KMSClient` for `ref.region` via `get_kms_client`, and captures the client, + * key id, and expected public key into the returned closure. No network I/O + * happens here; the first KMS request occurs only when the closure is + * invoked. + * + * Each invocation sends an `ECDSA_SHA_256` `Sign` request with + * `MessageType=DIGEST` (so KMS treats the 32-byte input as already hashed + * rather than re-hashing with SHA-256), decodes KMS's DER signature, + * normalises it to low-S, recovers the Ethereum `v` byte by trying both + * parities and matching against `expected_pubkey`, and returns a 65-byte + * compact signature. If neither parity recovers to the expected key the call + * throws `chain::plugin_config_exception` — effectively the per-sign + * integrity check that the KMS key still matches the public key pinned in + * the spec. There is no `KMSClient::GetPublicKey` call; the pubkey match is + * proven implicitly by successful ECDSA recovery. + * + * v1 only supports secp256k1 keys held as Ethereum public keys + * (`chain_key_type_ethereum` + `fc::em::public_key_shim`). Other key types + * raise `chain::pending_impl_exception` at construction; a `public_key` + * variant that does not hold the Ethereum shim raises + * `chain::plugin_config_exception`. See KMS_SIGNING_DESIGN.md §2 for the + * non-goal curves. + * + * @param ref parsed `(region, key_id)` pair + * @param key_type chain key type; must be `chain_key_type_ethereum` + * @param expected_pubkey public key the operator pinned in the spec; used at + * each sign call to recover the `v` byte and to assert that the + * signature KMS produced matches that key * @return a `sign_fn` matching `fc::crypto::sign_fn`'s signature */ fc::crypto::sign_fn make_kms_signature_provider( From 57b3021fa8cdba9234063ffe20bdc72e34535eaa Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Tue, 19 May 2026 22:52:52 -0500 Subject: [PATCH 11/22] AWS_KMS: Added error support for non-AWS kms keys --- .../src/kms_signature_provider.cpp | 52 ++++++++++++++++++- .../test/test_kms_signature_provider.cpp | 32 ++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index 9b303ff426..99f671ec36 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -38,13 +39,21 @@ namespace { /// other than `aws` becomes a deployment target). constexpr std::string_view kms_arn_prefix = "arn:aws:kms:"; +/// Case-insensitive lead-in shared by every ARN. A spec that begins with this +/// but does not match `kms_arn_prefix` is a malformed or out-of-scope ARN — +/// never the shorthand `:` form — and must fail loudly rather +/// than fall through to the shorthand parser. See `parse_kms_spec`. +constexpr std::string_view arn_lead_in = "arn:"; + /// Number of colon-separated segments in a well-formed KMS ARN: /// `arn`, `aws`, `kms`, ``, ``, `(key|alias)/`. constexpr std::size_t kms_arn_segment_count = 6; /// Indices into the split ARN. -constexpr std::size_t arn_idx_region = 3; -constexpr std::size_t arn_idx_tail = 5; +constexpr std::size_t arn_idx_partition = 1; +constexpr std::size_t arn_idx_service = 2; +constexpr std::size_t arn_idx_region = 3; +constexpr std::size_t arn_idx_tail = 5; /// Tail prefixes the KMS API accepts for the `KeyId` field. constexpr std::string_view tail_prefix_key = "key/"; @@ -123,6 +132,20 @@ kms_client_cache& kms_clients() { return c; } +/// Case-insensitive ASCII prefix test. ARN partitions and services are +/// lowercase by convention, but an operator may paste a mis-cased +/// `ARN:AWS:KMS:...`; we still want to recognise it as an ARN so it fails +/// loudly in `parse_kms_spec` rather than being mistaken for the shorthand +/// `:` form. +bool starts_with_ci(std::string_view s, std::string_view prefix) { + if (s.size() < prefix.size()) + return false; + return std::equal(prefix.begin(), prefix.end(), s.begin(), + [](unsigned char a, unsigned char b) { + return std::tolower(a) == std::tolower(b); + }); +} + } // namespace kms_key_ref parse_kms_spec(std::string_view spec_data) { @@ -157,6 +180,31 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { return kms_key_ref{region, tail}; } + // A spec that begins with `arn:` (any casing) but did not match the + // supported `arn:aws:kms:` form above is a malformed or out-of-scope ARN, + // never shorthand. Falling through to the `:` parser below + // would split on the first colon and silently yield region="arn"; AWS then + // rejects that only at first sign — with an opaque `InvalidRegion`/endpoint + // error, after a billable attempt. Fail loudly here instead, naming the + // offending partition and service. Non-`aws` partitions (`aws-cn`, + // `aws-us-gov`) are deliberately out of scope; this is the boundary that + // enforces it. A mis-cased `ARN:AWS:KMS:...` and a typo'd service + // (`arn:aws:ksm:...`) land here too — the message points at the canonical + // form in every case. + if (starts_with_ci(spec_data, arn_lead_in)) { + const auto parts = fc::split(std::string{spec_data}, ':', kms_arn_segment_count); + std::string partition, service; + if (parts.size() > arn_idx_partition) + partition = parts[arn_idx_partition]; + if (parts.size() > arn_idx_service) + service = parts[arn_idx_service]; + FC_THROW_EXCEPTION(chain::plugin_config_exception, + "Unsupported KMS ARN \"{}\": only the 'arn:aws:kms:' partition/service " + "is supported (got partition \"{}\", service \"{}\"). Non-'aws' " + "partitions such as 'aws-cn' and 'aws-us-gov' are out of scope.", + spec_data, partition, service); + } + // Shorthand `:`. We only split on the first colon // so aliases that themselves contain colons round-trip unchanged (KMS // alias names are operator-chosen, and while AWS docs disallow colons in diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index 7aa48b873a..6b16091ada 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -177,6 +177,38 @@ BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_too_few_segments) { sysio::chain::plugin_config_exception); } +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_china_partition) { + // `aws-cn` partition is out of scope. Must fail loudly at parse time + // rather than mis-parsing region as "arn" and failing later at first sign. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws-cn:kms:cn-north-1:111122223333:key/abc"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_govcloud_partition) { + // `aws-us-gov` partition is likewise out of scope. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws-us-gov:kms:us-gov-west-1:111122223333:key/abc"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_uppercase_arn) { + // A mis-cased ARN is not the supported lowercase `arn:aws:kms:` form; + // recognised case-insensitively as an ARN so it fails loudly here. + BOOST_CHECK_THROW(parse_kms_spec("ARN:AWS:KMS:us-east-1:111122223333:key/abc"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_typoed_service) { + // `ksm` instead of `kms` — would otherwise mis-parse region as "arn". + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:ksm:us-east-1:111122223333:key/abc"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_bare_arn_prefix) { + // Just `arn:` with nothing after it is still an ARN-shaped spec, not + // shorthand with a region literally named "arn". + BOOST_CHECK_THROW(parse_kms_spec("arn:"), sysio::chain::plugin_config_exception); +} + BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_shorthand_empty_region) { // Leading colon means no region. BOOST_CHECK_THROW(parse_kms_spec(":alias/wire-cranker-eth-01"), From ca6d1423c2e447c4a67a64a9fedfd0899c87ac36 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 00:30:30 -0500 Subject: [PATCH 12/22] AWS_KMS: No `GetPublicKey` pinning --- .../src/kms_signature_provider.cpp | 226 +++++++++++++++++- .../src/kms_signature_provider.hpp | 37 ++- .../test/test_kms_signature_provider.cpp | 90 +++++++ 3 files changed, 343 insertions(+), 10 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index 99f671ec36..1a8e164f44 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -96,12 +97,28 @@ struct aws_sdk_lifecycle { /// Per-closure state for a KMS-backed signer. Captured by `std::shared_ptr` /// so that `std::function` copies remain cheap and the same `KMSClient` / /// expected pubkey are shared across copies of the closure. +/// +/// A user-provided constructor is required because `std::once_flag` is neither +/// copyable nor movable, which rules out the aggregate / designated-initializer +/// construction the struct would otherwise allow. struct kms_signer_state { + kms_signer_state(std::shared_ptr client_, + std::string key_id_, + fc::em::public_key expected_em_pubkey_) + : client(std::move(client_)) + , key_id(std::move(key_id_)) + , expected_em_pubkey(std::move(expected_em_pubkey_)) {} + std::shared_ptr client; std::string key_id; - /// secp256k1 uncompressed public key the spec pinned. Used by + /// secp256k1 uncompressed public key the spec pinned. Verified once against + /// the live KMS key by `verify_kms_pubkey` (design §5.7), then used by /// `recover_v` to discriminate between recovery_id 0 and 1. fc::em::public_key expected_em_pubkey; + /// One-shot guard for the GetPublicKey pinning check. The check runs on the + /// first `Sign`; `std::call_once` re-runs it only if it threw — e.g. a + /// transient GetPublicKey API error — and never again once it has passed. + std::once_flag pin_once; }; /// Translate an AWS KMS error outcome into a fc::exception with a stable @@ -146,6 +163,175 @@ bool starts_with_ci(std::string_view s, std::string_view prefix) { }); } +// --------------------------------------------------------------------------- +// X.509 SubjectPublicKeyInfo (DER) decoding — design §5.7 public-key pinning. +// +// AWS KMS `GetPublicKey` returns the key as a DER-encoded SubjectPublicKeyInfo +// (RFC 5280 §4.1). We walk just enough of that structure to verify the key is +// secp256k1 and to lift out the raw EC point. +// --------------------------------------------------------------------------- + +/// ASN.1 DER universal tags that appear inside an EC SubjectPublicKeyInfo. +constexpr unsigned char der_tag_sequence = 0x30; +constexpr unsigned char der_tag_oid = 0x06; +constexpr unsigned char der_tag_bit_string = 0x03; + +/// DER length encoding: when the high bit of the leading length octet is set, +/// the low 7 bits give the number of subsequent big-endian length octets. +constexpr unsigned char der_length_long_form_bit = 0x80; +constexpr unsigned char der_length_value_mask = 0x7F; + +/// DER OBJECT IDENTIFIER bodies (the content of the OID TLV — tag and length +/// stripped) for the two OIDs that a secp256k1 SPKI must carry: +/// `1.2.840.10045.2.1` id-ecPublicKey and `1.3.132.0.10` secp256k1. +constexpr std::array oid_ec_public_key = { + 0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01}; +constexpr std::array oid_secp256k1 = { + 0x2B, 0x81, 0x04, 0x00, 0x0A}; + +/// An uncompressed secp256k1 EC point is `0x04 || X[32] || Y[32]`. +constexpr unsigned char ec_point_uncompressed_prefix = 0x04; +constexpr std::size_t ec_point_uncompressed_len = 65; + +/// Minimal ASN.1 DER reader over a byte span. DER is a canonical, unambiguous +/// encoding, so a structural tag-length-value walk is a genuine parse — not a +/// heuristic. Every malformed input raises `plugin_config_exception`. +struct der_reader { + std::span buf; + std::size_t pos = 0; + + /// True once every byte of `buf` has been consumed. + bool at_end() const { return pos >= buf.size(); } + + /// One decoded tag-length-value triple. `content` views into the reader's + /// underlying buffer (no copy is made). + struct element { + unsigned char tag; + std::span content; + }; + + /// Read the next TLV element and advance past it. + element next() { + SYS_ASSERT(pos + 2 <= buf.size(), chain::plugin_config_exception, + "Malformed KMS public-key DER: truncated tag/length header"); + const unsigned char tag = buf[pos++]; + + std::size_t len = buf[pos++]; + if ((len & der_length_long_form_bit) != 0) { + const std::size_t len_octets = len & der_length_value_mask; + SYS_ASSERT(len_octets >= 1 && len_octets <= sizeof(std::size_t), + chain::plugin_config_exception, + "Malformed KMS public-key DER: unsupported {}-octet length field", + len_octets); + SYS_ASSERT(pos + len_octets <= buf.size(), chain::plugin_config_exception, + "Malformed KMS public-key DER: truncated long-form length"); + len = 0; + for (std::size_t i = 0; i < len_octets; ++i) { + len = (len << 8) | buf[pos++]; + } + } + // `buf.size() - pos` cannot underflow: `pos <= buf.size()` is an + // invariant here, and the subtraction form avoids `pos + len` wrapping + // on a maliciously large long-form length. + SYS_ASSERT(len <= buf.size() - pos, chain::plugin_config_exception, + "Malformed KMS public-key DER: element body of {} bytes overruns buffer", + len); + + const auto content = buf.subspan(pos, len); + pos += len; + return {tag, content}; + } +}; + +/// Walk a DER X.509 SubjectPublicKeyInfo and return its raw uncompressed +/// secp256k1 EC point (`0x04 || X || Y`). Verifies the algorithm is +/// `id-ecPublicKey` over the `secp256k1` named curve, so an operator who +/// pointed the spec at an RSA key, a P-256 key, etc. gets a precise error +/// instead of a downstream surprise. +std::array +parse_spki_ec_point(std::span spki_der) { + der_reader top{spki_der}; + const auto spki = top.next(); + SYS_ASSERT(spki.tag == der_tag_sequence, chain::plugin_config_exception, + "KMS public-key DER: expected outer SEQUENCE, got tag {:#x}", + static_cast(spki.tag)); + SYS_ASSERT(top.at_end(), chain::plugin_config_exception, + "KMS public-key DER: unexpected trailing bytes after SubjectPublicKeyInfo"); + + // SubjectPublicKeyInfo ::= SEQUENCE { algorithm AlgorithmIdentifier, + // subjectPublicKey BIT STRING } + der_reader body{spki.content}; + const auto algorithm = body.next(); + const auto subject_pk = body.next(); + SYS_ASSERT(algorithm.tag == der_tag_sequence, chain::plugin_config_exception, + "KMS public-key DER: expected AlgorithmIdentifier SEQUENCE, got tag {:#x}", + static_cast(algorithm.tag)); + SYS_ASSERT(subject_pk.tag == der_tag_bit_string, chain::plugin_config_exception, + "KMS public-key DER: expected subjectPublicKey BIT STRING, got tag {:#x}", + static_cast(subject_pk.tag)); + SYS_ASSERT(body.at_end(), chain::plugin_config_exception, + "KMS public-key DER: unexpected trailing bytes inside SubjectPublicKeyInfo"); + + // AlgorithmIdentifier ::= SEQUENCE { algorithm OID, parameters (curve OID) } + der_reader alg{algorithm.content}; + const auto algo_oid = alg.next(); + const auto curve_oid = alg.next(); + SYS_ASSERT(algo_oid.tag == der_tag_oid && curve_oid.tag == der_tag_oid, + chain::plugin_config_exception, + "KMS public-key DER: AlgorithmIdentifier is not a pair of OBJECT IDENTIFIERs"); + SYS_ASSERT(std::ranges::equal(algo_oid.content, oid_ec_public_key), + chain::plugin_config_exception, + "KMS public-key DER: algorithm is not id-ecPublicKey — the KMS key is not an " + "elliptic-curve key"); + SYS_ASSERT(std::ranges::equal(curve_oid.content, oid_secp256k1), + chain::plugin_config_exception, + "KMS public-key DER: EC curve is not secp256k1 — the KMS key must be created " + "with key spec ECC_SECG_P256K1"); + + // subjectPublicKey BIT STRING: a leading "unused bits" octet (0 for a + // byte-aligned key) followed by the EC point itself. + SYS_ASSERT(!subject_pk.content.empty() && subject_pk.content.front() == 0x00, + chain::plugin_config_exception, + "KMS public-key DER: subjectPublicKey BIT STRING has a non-zero unused-bit count"); + const auto point = subject_pk.content.subspan(1); + SYS_ASSERT(point.size() == ec_point_uncompressed_len, chain::plugin_config_exception, + "KMS public-key DER: EC point is {} bytes, expected {} (uncompressed 0x04 form)", + point.size(), ec_point_uncompressed_len); + SYS_ASSERT(point.front() == ec_point_uncompressed_prefix, chain::plugin_config_exception, + "KMS public-key DER: EC point is not in uncompressed (0x04) form"); + + std::array out{}; + std::ranges::copy(point, out.begin()); + return out; +} + +/// Design §5.7 public-key pinning. Fetch the KMS key's public key with the +/// free, non-billable `GetPublicKey` API, decode its SubjectPublicKeyInfo, and +/// assert it matches the key the operator pinned in the spec. On mismatch this +/// throws `plugin_config_exception` early — before any billable `Sign` — with a +/// message that names the misconfiguration directly. Invoked exactly once per +/// closure through `kms_signer_state::pin_once`. +void verify_kms_pubkey(kms_signer_state& state) { + Aws::KMS::Model::GetPublicKeyRequest req; + req.SetKeyId(Aws::String{state.key_id.c_str()}); + + auto outcome = state.client->GetPublicKey(req); + if (!outcome.IsSuccess()) { + throw_kms_error("GetPublicKey", state.key_id, outcome.GetError()); + } + + const auto& spki_buf = outcome.GetResult().GetPublicKey(); + const std::span spki_der{ + spki_buf.GetUnderlyingData(), spki_buf.GetLength()}; + + const auto kms_pubkey = spki_der_to_public_key(spki_der); + SYS_ASSERT(kms_pubkey == state.expected_em_pubkey, chain::plugin_config_exception, + "AWS KMS key \"{}\" holds a public key that does not match the public key " + "pinned in the signature-provider spec. Correct the spec's to the " + "key this KMS key actually holds, or point the spec at the intended KMS key.", + state.key_id); +} + } // namespace kms_key_ref parse_kms_spec(std::string_view spec_data) { @@ -293,6 +479,27 @@ fc::em::compact_signature der_to_eth_signature( return out; } +fc::em::public_key spki_der_to_public_key(std::span spki_der) { + const auto point = parse_spki_ec_point(spki_der); + + // `public_key_data_uncompressed` is std::array; the EC point is + // std::array. The element-wise copy is a value-preserving + // unsigned-char -> char conversion (bit pattern unchanged). + fc::em::public_key_data_uncompressed uncompressed{}; + std::ranges::copy(point, uncompressed.begin()); + + // The fc::em::public_key constructor re-validates the point on the curve via + // libsecp256k1 and raises an fc assertion on a bad point. Translate that to + // the module's standard exception type so every failure here is uniform. + try { + return fc::em::public_key{uncompressed}; + } catch (const fc::exception& e) { + FC_THROW_EXCEPTION(chain::plugin_config_exception, + "KMS public-key DER carries an EC point that is not a valid " + "secp256k1 public key: {}", e.to_detail_string()); + } +} + fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& ref, fc::crypto::chain_key_type_t key_type, const fc::crypto::public_key& expected_pubkey) { @@ -314,13 +521,20 @@ fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& r const auto& shim = expected_pubkey.get(); - auto state = std::make_shared(kms_signer_state{ - .client = get_kms_client(ref.region), - .key_id = ref.key_id, - .expected_em_pubkey = shim.unwrapped(), - }); + auto state = std::make_shared( + get_kms_client(ref.region), ref.key_id, shim.unwrapped()); return [state](const chain::digest_type& digest) -> chain::signature_type { + // Public-key pinning (design §5.7). Before the first — and only the + // first — billable Sign, fetch the KMS key's own public key with the + // free GetPublicKey API and assert it matches the key pinned in the + // spec. This turns the common "wrong in the spec" mistake + // into a fast, direct error instead of an opaque recovery failure that + // would otherwise surface only after a paid Sign. `std::call_once` runs + // the check exactly once on success; if it throws (e.g. a transient + // GetPublicKey API error) the next Sign retries it. + std::call_once(state->pin_once, [&] { verify_kms_pubkey(*state); }); + // Build a Sign request. MessageType=DIGEST tells KMS the 32 bytes are // already a hash; otherwise it would re-hash with SHA-256 and break // any chain that hashes with anything other than SHA-256. diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index d3e1894bc0..9e29509a00 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -127,6 +127,28 @@ fc::em::compact_signature der_to_eth_signature( std::span digest, const fc::em::public_key& expected_pubkey); +/** + * @brief Decode an X.509 SubjectPublicKeyInfo (DER) into a secp256k1 public key. + * + * AWS KMS `GetPublicKey` returns the public key as a DER-encoded + * SubjectPublicKeyInfo (RFC 5280 §4.1): an outer `SEQUENCE` wrapping an + * `AlgorithmIdentifier` and a `BIT STRING`. This helper walks that structure, + * verifies the algorithm is `id-ecPublicKey` over the `secp256k1` named curve, + * and lifts the trailing uncompressed `0x04 || X || Y` point into an + * `fc::em::public_key`. It backs the design §5.7 public-key pinning check. + * + * Because DER is a canonical encoding, the walk is an exact parse rather than a + * heuristic: anything that is not a well-formed secp256k1 SPKI is rejected. + * + * @param spki_der DER bytes of the SubjectPublicKeyInfo (88 bytes for a + * well-formed uncompressed secp256k1 key) + * @throws sysio::chain::plugin_config_exception if the DER is malformed, the + * algorithm or curve is not secp256k1, or the EC point is not a valid + * uncompressed secp256k1 public key + * @return the decoded public key + */ +fc::em::public_key spki_der_to_public_key(std::span spki_der); + /** * @brief Get (or lazily create) a process-wide `KMSClient` for `region`. * @@ -156,16 +178,23 @@ std::shared_ptr get_kms_client(const std::string& region); * happens here; the first KMS request occurs only when the closure is * invoked. * + * On its first invocation the closure performs design §5.7 public-key + * pinning: it calls `KMSClient::GetPublicKey` exactly once, decodes the + * returned X.509 SubjectPublicKeyInfo via `spki_der_to_public_key`, and + * asserts the KMS key's public key matches `expected_pubkey`. A mismatch + * throws `chain::plugin_config_exception` immediately — before any billable + * `Sign` — so a spec that pins the wrong `` fails fast with a + * direct error. The pinning check runs once on success; a transient + * `GetPublicKey` failure is retried on the next `Sign`. + * * Each invocation sends an `ECDSA_SHA_256` `Sign` request with * `MessageType=DIGEST` (so KMS treats the 32-byte input as already hashed * rather than re-hashing with SHA-256), decodes KMS's DER signature, * normalises it to low-S, recovers the Ethereum `v` byte by trying both * parities and matching against `expected_pubkey`, and returns a 65-byte * compact signature. If neither parity recovers to the expected key the call - * throws `chain::plugin_config_exception` — effectively the per-sign - * integrity check that the KMS key still matches the public key pinned in - * the spec. There is no `KMSClient::GetPublicKey` call; the pubkey match is - * proven implicitly by successful ECDSA recovery. + * throws `chain::plugin_config_exception`; once pinning has passed this is a + * defence-in-depth check that should never fire. * * v1 only supports secp256k1 keys held as Ethereum public keys * (`chain_key_type_ethereum` + `fc::em::public_key_shim`). Other key types diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index 6b16091ada..f75dc7f3f1 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -80,6 +80,33 @@ std::vector compact_to_der(const std::array& c return der; } +/** + * Test helper: wrap a 65-byte uncompressed secp256k1 EC point (0x04 || X || Y) + * in a DER X.509 SubjectPublicKeyInfo, byte-for-byte identical to what AWS KMS + * `GetPublicKey` returns for an `ECC_SECG_P256K1` signing key. Lets us exercise + * the production `spki_der_to_public_key` decoder without calling AWS. + * + * The 23-byte prefix is fixed because every field length is small enough for + * single-byte DER length encoding and the algorithm identifiers are constant: + * + * SEQUENCE (0x56) { + * SEQUENCE (0x10) { OID id-ecPublicKey, OID secp256k1 } + * BIT STRING (0x42) { 00 <65-byte point> } + * } + */ +std::vector point_to_spki_der( + const fc::em::public_key_data_uncompressed& point) { + static constexpr std::array spki_prefix = { + 0x30, 0x56, 0x30, 0x10, 0x06, 0x07, 0x2A, 0x86, + 0x48, 0xCE, 0x3D, 0x02, 0x01, 0x06, 0x05, 0x2B, + 0x81, 0x04, 0x00, 0x0A, 0x03, 0x42, 0x00}; + std::vector der(spki_prefix.begin(), spki_prefix.end()); + for (const char c : point) { + der.push_back(static_cast(c)); + } + return der; +} + /// Curve order N for secp256k1 (big-endian). Used to construct deliberately /// high-S signatures from a known low-S form by computing s' = N - s. constexpr std::array secp256k1_curve_order = { @@ -343,6 +370,69 @@ BOOST_AUTO_TEST_CASE(der_to_eth_signature_normalises_high_s_input) { BOOST_CHECK(out == local); } +// --------------------------------------------------------------------------- +// spki_der_to_public_key — design §5.7 public-key pinning decoder +// +// These tests synthesise the X.509 SubjectPublicKeyInfo that AWS KMS +// `GetPublicKey` returns by wrapping a locally generated EC point with +// `point_to_spki_der` above. No AWS credentials, no network. +// --------------------------------------------------------------------------- + +BOOST_AUTO_TEST_CASE(spki_der_to_public_key_round_trips) { + // A well-formed secp256k1 SPKI must decode back to the exact key it wraps. + for (int i = 0; i < 4; ++i) { + const auto priv = fc::em::private_key::generate(); + const auto pub = priv.get_public_key(); + + const auto der = point_to_spki_der(pub.serialize_uncompressed()); + const auto decoded = sysio::sigprov::kms::spki_der_to_public_key( + std::span(der)); + BOOST_CHECK(decoded == pub); + } +} + +BOOST_AUTO_TEST_CASE(spki_der_to_public_key_rejects_garbage) { + // Non-DER bytes must fail the structural walk, not be silently accepted. + const std::array garbage{0xde, 0xad, 0xbe, 0xef}; + BOOST_CHECK_THROW( + sysio::sigprov::kms::spki_der_to_public_key(std::span(garbage)), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(spki_der_to_public_key_rejects_wrong_curve) { + // Flip the final byte of the curve OID (0x0A -> 0x07): the structure stays + // valid DER but the named curve is no longer secp256k1. An operator who + // pointed the spec at a non-secp256k1 EC key must get a loud parse error. + const auto priv = fc::em::private_key::generate(); + auto der = point_to_spki_der(priv.get_public_key().serialize_uncompressed()); + der[19] = 0x07; // index 19 = last octet of the secp256k1 curve OID body + BOOST_CHECK_THROW( + sysio::sigprov::kms::spki_der_to_public_key(std::span(der)), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(spki_der_to_public_key_rejects_truncated) { + // A buffer shorter than its declared DER lengths must fail, not over-read. + const auto priv = fc::em::private_key::generate(); + auto der = point_to_spki_der(priv.get_public_key().serialize_uncompressed()); + der.resize(der.size() - 10); // chop the tail of the EC point + BOOST_CHECK_THROW( + sysio::sigprov::kms::spki_der_to_public_key(std::span(der)), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(spki_der_to_public_key_rejects_off_curve_point) { + // Structurally valid SPKI whose 65-byte point (0x04 followed by all zeros) + // is not on the secp256k1 curve. libsecp256k1 rejects it; the helper must + // surface that as the module's standard plugin_config_exception. + fc::em::public_key_data_uncompressed off_curve{}; + off_curve[0] = 0x04; + const auto der = point_to_spki_der(off_curve); + BOOST_CHECK_THROW( + sysio::sigprov::kms::spki_der_to_public_key(std::span(der)), + sysio::chain::plugin_config_exception); +} + // --------------------------------------------------------------------------- // KMSClient cache + AWS SDK lifecycle // From c3822f589c8acdcd046ea45ba03bcc07facf472e Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 08:45:57 -0500 Subject: [PATCH 13/22] AWS_KMS: startup credential check --- .../signature_provider_manager_plugin.hpp | 13 ++- .../src/kms_signature_provider.cpp | 58 +++++++---- .../src/kms_signature_provider.hpp | 46 +++++++-- .../src/signature_provider_manager_plugin.cpp | 97 +++++++++++++++++-- .../test/test_kms_signature_provider.cpp | 21 ++-- 5 files changed, 190 insertions(+), 45 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/include/sysio/signature_provider_manager_plugin/signature_provider_manager_plugin.hpp b/plugins/signature_provider_manager_plugin/include/sysio/signature_provider_manager_plugin/signature_provider_manager_plugin.hpp index 59d0faaa6f..bfbb67a555 100644 --- a/plugins/signature_provider_manager_plugin/include/sysio/signature_provider_manager_plugin/signature_provider_manager_plugin.hpp +++ b/plugins/signature_provider_manager_plugin/include/sysio/signature_provider_manager_plugin/signature_provider_manager_plugin.hpp @@ -38,7 +38,18 @@ class signature_provider_manager_plugin : public appbase::plugin client; std::string key_id; /// secp256k1 uncompressed public key the spec pinned. Verified once against - /// the live KMS key by `verify_kms_pubkey` (design §5.7), then used by - /// `recover_v` to discriminate between recovery_id 0 and 1. + /// the live KMS key by `verify_kms_pubkey`, then used by `recover_v` to + /// discriminate between recovery_id 0 and 1. fc::em::public_key expected_em_pubkey; /// One-shot guard for the GetPublicKey pinning check. The check runs on the /// first `Sign`; `std::call_once` re-runs it only if it threw — e.g. a @@ -164,7 +164,7 @@ bool starts_with_ci(std::string_view s, std::string_view prefix) { } // --------------------------------------------------------------------------- -// X.509 SubjectPublicKeyInfo (DER) decoding — design §5.7 public-key pinning. +// X.509 SubjectPublicKeyInfo (DER) decoding for KMS public-key pinning. // // AWS KMS `GetPublicKey` returns the key as a DER-encoded SubjectPublicKeyInfo // (RFC 5280 §4.1). We walk just enough of that structure to verify the key is @@ -305,10 +305,10 @@ parse_spki_ec_point(std::span spki_der) { return out; } -/// Design §5.7 public-key pinning. Fetch the KMS key's public key with the -/// free, non-billable `GetPublicKey` API, decode its SubjectPublicKeyInfo, and -/// assert it matches the key the operator pinned in the spec. On mismatch this -/// throws `plugin_config_exception` early — before any billable `Sign` — with a +/// Public-key pinning check. Fetch the KMS key's public key with the free, +/// non-billable `GetPublicKey` API, decode its SubjectPublicKeyInfo, and assert +/// it matches the key the operator pinned in the spec. On mismatch this throws +/// `plugin_config_exception` early — before any billable `Sign` — with a /// message that names the misconfiguration directly. Invoked exactly once per /// closure through `kms_signer_state::pin_once`. void verify_kms_pubkey(kms_signer_state& state) { @@ -332,6 +332,14 @@ void verify_kms_pubkey(kms_signer_state& state) { state.key_id); } +/// Run the public-key pinning check exactly once for `state`. Both the first +/// `Sign` and the opt-in startup probe funnel through here, so `std::call_once` +/// guarantees a single GetPublicKey round-trip — and retries it only if it +/// threw (e.g. a transient GetPublicKey API error). +void ensure_kms_pubkey_pinned(kms_signer_state& state) { + std::call_once(state.pin_once, [&] { verify_kms_pubkey(state); }); +} + } // namespace kms_key_ref parse_kms_spec(std::string_view spec_data) { @@ -500,9 +508,9 @@ fc::em::public_key spki_der_to_public_key(std::span spki_de } } -fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& ref, - fc::crypto::chain_key_type_t key_type, - const fc::crypto::public_key& expected_pubkey) { +kms_signer make_kms_signature_provider(const kms_key_ref& ref, + fc::crypto::chain_key_type_t key_type, + const fc::crypto::public_key& expected_pubkey) { // v1 only supports secp256k1 keys held as Ethereum-flavoured public keys // (ECC_SECG_P256K1 in KMS, signed with ECDSA_SHA_256). Wire K1 (sysio's // own secp256k1 path), R1, BLS, Ed25519, and webauthn use different key @@ -524,16 +532,16 @@ fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& r auto state = std::make_shared( get_kms_client(ref.region), ref.key_id, shim.unwrapped()); - return [state](const chain::digest_type& digest) -> chain::signature_type { - // Public-key pinning (design §5.7). Before the first — and only the - // first — billable Sign, fetch the KMS key's own public key with the - // free GetPublicKey API and assert it matches the key pinned in the - // spec. This turns the common "wrong in the spec" mistake - // into a fast, direct error instead of an opaque recovery failure that - // would otherwise surface only after a paid Sign. `std::call_once` runs - // the check exactly once on success; if it throws (e.g. a transient - // GetPublicKey API error) the next Sign retries it. - std::call_once(state->pin_once, [&] { verify_kms_pubkey(*state); }); + fc::crypto::sign_fn sign = [state](const chain::digest_type& digest) -> chain::signature_type { + // Public-key pinning. Before the first — and only the first — billable + // Sign, fetch the KMS key's own public key with the free GetPublicKey + // API and assert it matches the key pinned in the spec. This turns the + // common "wrong in the spec" mistake into a fast, direct + // error instead of an opaque recovery failure that would otherwise + // surface only after a paid Sign. If the opt-in startup probe already + // ran the check, this is a no-op — both paths share `state->pin_once` + // through `ensure_kms_pubkey_pinned`. + ensure_kms_pubkey_pinned(*state); // Build a Sign request. MessageType=DIGEST tells KMS the 32 bytes are // already a hash; otherwise it would re-hash with SHA-256 and break @@ -561,6 +569,16 @@ fc::crypto::sign_fn make_kms_signature_provider(const kms_key_ref& r return fc::crypto::signature( fc::crypto::signature::storage_type{fc::em::signature_shim{compact}}); }; + + // Startup probe: runs the same one-shot pinning check as the first Sign, + // but issues only the free GetPublicKey — no billable Sign. An opt-in + // plugin_startup() calls this so a missing credential, bad region, absent + // IAM grant, or wrong pinned key fails loudly at boot instead of deep in + // production. It shares `state` (hence `pin_once`) with `sign`, so enabling + // the probe never doubles the check. + std::function warm_up = [state] { ensure_kms_pubkey_pinned(*state); }; + + return kms_signer{.sign = std::move(sign), .warm_up = std::move(warm_up)}; } std::shared_ptr get_kms_client(const std::string& region) { diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index 9e29509a00..69ab36269d 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -135,7 +136,7 @@ fc::em::compact_signature der_to_eth_signature( * `AlgorithmIdentifier` and a `BIT STRING`. This helper walks that structure, * verifies the algorithm is `id-ecPublicKey` over the `secp256k1` named curve, * and lifts the trailing uncompressed `0x04 || X || Y` point into an - * `fc::em::public_key`. It backs the design §5.7 public-key pinning check. + * `fc::em::public_key`. It backs the KMS public-key pinning check. * * Because DER is a canonical encoding, the walk is an exact parse rather than a * heuristic: anything that is not a well-formed secp256k1 SPKI is rejected. @@ -170,18 +171,43 @@ fc::em::public_key spki_der_to_public_key(std::span spki_de std::shared_ptr get_kms_client(const std::string& region); /** - * @brief Build a `sign_fn` closure that signs digests with AWS KMS. + * @brief A KMS-backed signer: the signing closure plus its one-shot startup + * probe. + * + * `make_kms_signature_provider` returns this pair so a caller can choose to + * validate the KMS key eagerly at startup instead of lazily on the first sign. + * Both members share the same underlying state, so the public-key pinning + * check runs at most once regardless of which one triggers it. + */ +struct kms_signer { + /// Signing closure, usable wherever `fc::crypto::sign_fn` is expected. Each + /// call issues one `KMS::Sign`; the first call also runs the public-key + /// pinning check, unless `warm_up` has already run it. + fc::crypto::sign_fn sign; + + /// Eagerly run the startup probe: a single `KMS::GetPublicKey` + /// that resolves AWS credentials, warms the client, and verifies the KMS + /// key matches the pinned public key — without signing. Optional; if never + /// called, the same check happens lazily on the first `sign`. Idempotent — + /// it shares the closure's one-shot guard, so calling it never makes the + /// check run twice. Throws `sysio::chain::plugin_config_exception` on a + /// missing credential, bad region, absent IAM grant, or mismatched key. + std::function warm_up; +}; + +/** + * @brief Build a KMS-backed signer (closure + startup probe) for a key. * * Validates `key_type` and the public-key variant, resolves the shared * `KMSClient` for `ref.region` via `get_kms_client`, and captures the client, * key id, and expected public key into the returned closure. No network I/O - * happens here; the first KMS request occurs only when the closure is - * invoked. + * happens here; the first KMS request occurs only when the closure — or the + * returned `warm_up` probe — is invoked. * - * On its first invocation the closure performs design §5.7 public-key - * pinning: it calls `KMSClient::GetPublicKey` exactly once, decodes the - * returned X.509 SubjectPublicKeyInfo via `spki_der_to_public_key`, and - * asserts the KMS key's public key matches `expected_pubkey`. A mismatch + * On its first invocation the closure performs public-key pinning: it calls + * `KMSClient::GetPublicKey` exactly once, decodes the returned X.509 + * SubjectPublicKeyInfo via `spki_der_to_public_key`, and asserts the KMS + * key's public key matches `expected_pubkey`. A mismatch * throws `chain::plugin_config_exception` immediately — before any billable * `Sign` — so a spec that pins the wrong `` fails fast with a * direct error. The pinning check runs once on success; a transient @@ -208,9 +234,9 @@ std::shared_ptr get_kms_client(const std::string& region); * @param expected_pubkey public key the operator pinned in the spec; used at * each sign call to recover the `v` byte and to assert that the * signature KMS produced matches that key - * @return a `sign_fn` matching `fc::crypto::sign_fn`'s signature + * @return a `kms_signer` bundling the signing closure and the startup probe */ -fc::crypto::sign_fn make_kms_signature_provider( +kms_signer make_kms_signature_provider( const kms_key_ref& ref, fc::crypto::chain_key_type_t key_type, const fc::crypto::public_key& expected_pubkey); diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index d8f731f069..3f8bcb422b 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -21,7 +21,14 @@ namespace sysio { namespace { -constexpr auto option_name_kiod_timeout_us = "signature-provider-kiod-timeout-us"; +constexpr auto option_name_kiod_timeout = "signature-provider-kiod-timeout"; + +/// Opt-in flag: when true, plugin_startup() probes every KMS-backed signing key +/// with a GetPublicKey call so a credential / region / IAM misconfiguration +/// fails loudly at boot rather than on the first production sign. Default +/// false, so nodes with no KMS keys and offline test environments are +/// unaffected. +constexpr auto option_name_kms_startup_check = "signature-provider-kms-startup-check"; std::filesystem::path default_signature_provider_spec_file() { return app().config_dir() / "default_signature_providers.json"; @@ -37,6 +44,12 @@ class signature_provider_manager_plugin_impl { */ fc::microseconds _kiod_provider_timeout_us; + /** + * When true, `plugin_startup()` runs the opt-in AWS KMS startup credential + * check. Set from `option_name_kms_startup_check` in `plugin_initialize`. + */ + bool _kms_startup_check{false}; + fc::crypto::sign_fn make_key_signature_provider(const chain::private_key_type& key) const { return [key](const chain::digest_type& digest) { return key.sign(digest); }; } @@ -118,9 +131,17 @@ class signature_provider_manager_plugin_impl { // closure issues KMS::Sign on each invocation, so credentials and // network access are deferred to first sign rather than spec parse. auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); - auto signer = sysio::sigprov::kms::make_kms_signature_provider( - ref, key_type, public_key); - return {std::move(signer), std::nullopt}; + auto kms = sysio::sigprov::kms::make_kms_signature_provider(ref, key_type, public_key); + + // Retain the startup probe so plugin_startup() can — when the opt-in + // KMS startup check is enabled — fail fast on a missing credential, + // bad region, absent IAM grant, or wrong pinned key, instead of + // discovering it deep into production on the first sign. + { + std::scoped_lock lock(_signing_providers_mutex); + _kms_startup_probes.push_back(std::move(kms.warm_up)); + } + return {std::move(kms.sign), std::nullopt}; } SYS_THROW(chain::plugin_config_exception, "Unsupported key provider type \"{}\"", spec_type_str); @@ -379,6 +400,43 @@ class signature_provider_manager_plugin_impl { return set_provider(provider); } + /** + * Run the opt-in AWS KMS startup credential check. + * + * For every KMS-backed signing key registered before startup, issue a + * single `GetPublicKey` call. That resolves AWS credentials, warms the + * client, and verifies the KMS key matches the pinned public key. Any + * misconfiguration — missing credentials, bad region, absent IAM grant, + * unreachable IMDS, or a mismatched public key — throws + * `chain::plugin_config_exception` here, aborting startup loudly instead of + * failing on the first production sign. + * + * A no-op unless `_kms_startup_check` is set, and a no-op when no KMS-backed + * keys are registered. + */ + void run_kms_startup_check() { + if (!_kms_startup_check) { + return; + } + + // Copy the probe list so the network-bound GetPublicKey calls run + // without holding the providers mutex. + std::vector> probes; + { + std::scoped_lock lock(_signing_providers_mutex); + probes = _kms_startup_probes; + } + if (probes.empty()) { + return; + } + + ilog("Verifying AWS KMS credentials for {} signing key(s) at startup", probes.size()); + for (auto& probe : probes) { + probe(); // throws chain::plugin_config_exception on any misconfiguration + } + ilog("AWS KMS startup credential check passed"); + } + private: std::atomic_uint32_t _anon_key_counter{0}; @@ -398,6 +456,13 @@ class signature_provider_manager_plugin_impl { */ std::map _signing_providers_by_name{}; std::map _signing_providers_by_pubkey{}; + + /** + * One-shot startup probes, one per KMS-backed signing key, collected as + * providers are created. Run by `run_kms_startup_check()` when + * `_kms_startup_check` is enabled. Guarded by `_signing_providers_mutex`. + */ + std::vector> _kms_startup_probes{}; }; signature_provider_manager_plugin::signature_provider_manager_plugin() @@ -407,12 +472,20 @@ signature_provider_manager_plugin::~signature_provider_manager_plugin() {} void signature_provider_manager_plugin::set_program_options(options_description&, options_description& cfg) { cfg.add_options()( - "signature-provider-kiod-timeout", boost::program_options::value()->default_value(5), + option_name_kiod_timeout, boost::program_options::value()->default_value(5), "Limits the maximum time (in milliseconds) that is allowed for sending requests to a kiod provider for signing"); cfg.add_options()( "signature-provider", boost::program_options::value>()->multitoken(), "Signature provider spec formatted as (check docs for details): " "`,,,,`");; + cfg.add_options()( + option_name_kms_startup_check, + boost::program_options::value()->default_value(false), + "When true, the signature provider plugin issues an AWS KMS GetPublicKey " + "call at startup for every KMS-backed (`KMS:` spec) signing key, failing " + "fast if credentials, region, IAM permissions, or the pinned public key " + "are misconfigured. Off by default, so nodes without KMS keys and offline " + "test environments are unaffected."); } const char* signature_provider_manager_plugin::signature_provider_help_text() const { @@ -437,8 +510,11 @@ const char* signature_provider_manager_plugin::signature_provider_help_text() co } void signature_provider_manager_plugin::plugin_initialize(const variables_map& options) { - if (options.contains(option_name_kiod_timeout_us)) - my->_kiod_provider_timeout_us = fc::milliseconds(options.at(option_name_kiod_timeout_us).as()); + if (options.contains(option_name_kiod_timeout)) + my->_kiod_provider_timeout_us = fc::milliseconds(options.at(option_name_kiod_timeout).as()); + + if (options.contains(option_name_kms_startup_check)) + my->_kms_startup_check = options.at(option_name_kms_startup_check).as(); if (options.contains(option_name_provider)) { auto specs = options.at(option_name_provider).as>(); @@ -451,6 +527,13 @@ void signature_provider_manager_plugin::plugin_initialize(const variables_map& o } } +void signature_provider_manager_plugin::plugin_startup() { + // Opt-in AWS KMS credential probe. A no-op unless + // `signature-provider-kms-startup-check` is enabled; when enabled, any KMS + // misconfiguration throws here and aborts startup loudly. + my->run_kms_startup_check(); +} + fc::crypto::signature_provider_ptr signature_provider_manager_plugin::create_provider(const std::string& spec) { return my->create_provider(spec); } diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index f75dc7f3f1..bf0c6abc5a 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -371,7 +371,7 @@ BOOST_AUTO_TEST_CASE(der_to_eth_signature_normalises_high_s_input) { } // --------------------------------------------------------------------------- -// spki_der_to_public_key — design §5.7 public-key pinning decoder +// spki_der_to_public_key — KMS public-key pinning decoder // // These tests synthesise the X.509 SubjectPublicKeyInfo that AWS KMS // `GetPublicKey` returns by wrapping a locally generated EC point with @@ -475,13 +475,15 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_returns_callable_for_ethereum) const auto chain_pub = fc::crypto::private_key::generate(fc::crypto::private_key::key_type::em).get_public_key(); - const auto sign = sysio::sigprov::kms::make_kms_signature_provider( + const auto kms = sysio::sigprov::kms::make_kms_signature_provider( kms_key_ref{"us-east-1", "alias/wire-cranker-eth-01"}, fc::crypto::chain_key_type_ethereum, chain_pub); - // Closure must be set; we deliberately do not invoke it (would hit live KMS). - BOOST_CHECK(static_cast(sign)); + // Both the signing closure and the startup probe must be set; we deliberately + // invoke neither here (either would hit live KMS). + BOOST_CHECK(static_cast(kms.sign)); + BOOST_CHECK(static_cast(kms.warm_up)); } BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_wire_k1) { @@ -544,10 +546,15 @@ BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { fc::crypto::from_native_string_to_public_key(pub_hex); const auto em_expected = chain_pub.get().unwrapped(); - const auto ref = sysio::sigprov::kms::parse_kms_spec(spec_env); - const auto sign = sysio::sigprov::kms::make_kms_signature_provider( + const auto ref = sysio::sigprov::kms::parse_kms_spec(spec_env); + const auto kms = sysio::sigprov::kms::make_kms_signature_provider( ref, fc::crypto::chain_key_type_ethereum, chain_pub); + // Exercise the startup probe: a GetPublicKey call plus the public-key pin, + // with no signing. It must pass before we attempt a (billable) Sign — and a + // passing probe pre-pins the closure. + BOOST_CHECK_NO_THROW(kms.warm_up()); + // Deterministic digest so the test is replayable and the AWS account audit // log entries are recognisable across runs. const auto keccak = fc::crypto::keccak256::hash(std::string{"wire-sysio kms live test 2026"}); @@ -558,7 +565,7 @@ BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { sysio::chain::digest_type chain_digest; std::memcpy(chain_digest.data(), keccak.data(), 32); - const auto sig = sign(chain_digest); + const auto sig = kms.sign(chain_digest); const auto& em_sig = sig.get(); const auto recovered = em_sig.recover_eth(keccak).unwrapped(); From 8e292e492027ee9f99151f8639b6ff247c29b62c Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 08:51:30 -0500 Subject: [PATCH 14/22] AWS_KMS: Comment cleanup --- .../src/kms_signature_provider.cpp | 4 ++-- .../src/kms_signature_provider.hpp | 6 ++---- .../signature_provider_manager_plugin/test/README.md | 4 +--- .../test/test_create_provider_specs.cpp | 4 ++-- .../test/test_kms_signature_provider.cpp | 11 ++++++----- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index a496ade828..d9f48c41f1 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -514,8 +514,8 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, // v1 only supports secp256k1 keys held as Ethereum-flavoured public keys // (ECC_SECG_P256K1 in KMS, signed with ECDSA_SHA_256). Wire K1 (sysio's // own secp256k1 path), R1, BLS, Ed25519, and webauthn use different key - // shapes or curves and need separate plumbing — see KMS_SIGNING_DESIGN.md - // §2 non-goals for the contract. + // shapes or curves and need separate plumbing, so they are out of scope + // for v1. SYS_ASSERT(key_type == fc::crypto::chain_key_type_ethereum, chain::pending_impl_exception, "KMS signing currently supports only chain_key_type_ethereum, got {}", diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index 69ab36269d..d8a44e51ec 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -4,8 +4,7 @@ * AWS KMS-backed signature provider — plugin-private header. * * The KMS provider extends the existing `KEY:` / `KIOD:` spec grammar with a - * third form, `KMS:`, where the signing key never leaves AWS. See - * `KMS_SIGNING_DESIGN.md` (this directory) for the full design. + * third form, `KMS:`, where the signing key never leaves AWS. * * This header is consumed only by the plugin's own translation units and by * its tests; it is intentionally not installed under `include/`. @@ -226,8 +225,7 @@ struct kms_signer { * (`chain_key_type_ethereum` + `fc::em::public_key_shim`). Other key types * raise `chain::pending_impl_exception` at construction; a `public_key` * variant that does not hold the Ethereum shim raises - * `chain::plugin_config_exception`. See KMS_SIGNING_DESIGN.md §2 for the - * non-goal curves. + * `chain::plugin_config_exception`. * * @param ref parsed `(region, key_id)` pair * @param key_type chain key type; must be `chain_key_type_ethereum` diff --git a/plugins/signature_provider_manager_plugin/test/README.md b/plugins/signature_provider_manager_plugin/test/README.md index 04d4310588..2eef4bb9aa 100644 --- a/plugins/signature_provider_manager_plugin/test/README.md +++ b/plugins/signature_provider_manager_plugin/test/README.md @@ -157,6 +157,4 @@ test needs. | `AccessDeniedException: not authorized to perform: kms:Sign` | IAM policy missing — see Prereq #2 | | `NotFoundException: Invalid keyId` | Wrong region in `KMS_LIVE_SPEC`, or alias not created | | `BOOST_CHECK(recovered == em_expected) failed` | `KMS_LIVE_PUBKEY` does not match the key KMS holds | -| `parse_kms_spec` exception at startup | Spec body malformed — see `KMS_SIGNING_DESIGN.md` §3 | - -See `../KMS_SIGNING_DESIGN.md` for the full design context. +| `parse_kms_spec` exception at startup | Spec body malformed — check the `KMS:` spec format | diff --git a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp index d2bcab1782..f52e1c3906 100644 --- a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp @@ -344,8 +344,8 @@ BOOST_AUTO_TEST_CASE(create_provider_ethereum_kms_spec_routes_through_parser) { // End-to-end check that the plugin's spec parser routes `KMS:` through // `parse_kms_spec` + `make_kms_signature_provider` and returns a provider // whose sign closure is callable. The closure itself is *not* invoked - // here — invocation issues a real KMS::Sign request, which lives behind - // the env-gated live test in KMS_SIGNING_DESIGN.md §7. + // here — invocation issues a real KMS::Sign request, which is covered + // only by the env-gated live test. using namespace fc::test; using namespace fc::crypto; diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index bf0c6abc5a..1623843be3 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -3,9 +3,10 @@ * * These cases cover only the offline parse path — `parse_kms_spec` does no * network I/O and constructs no `KMSClient`, so the suite runs without AWS - * credentials and without an internet connection. End-to-end signing tests - * live in a separate suite (gated behind `KMS_LIVE_TEST`, see - * `KMS_SIGNING_DESIGN.md` §7). + * credentials and without an internet connection. The one end-to-end signing + * test in this file (`kms_live_sign_round_trip`) is gated on the + * `KMS_LIVE_SPEC` / `KMS_LIVE_PUBKEY` environment variables and is skipped + * unless both are set. */ #include @@ -488,8 +489,8 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_returns_callable_for_ethereum) BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_wire_k1) { // Wire K1 (`chain_key_type_wire`) is also secp256k1, but its public-key - // and signature shapes differ from Ethereum's. Goal #2 of the design note - // tracks adding it; until then, fail loud. + // and signature shapes differ from Ethereum's, so it is not yet supported + // — fail loud rather than sign with the wrong format. const auto chain_pub = fc::crypto::private_key::generate(fc::crypto::private_key::key_type::k1).get_public_key(); From f3c4bb92e0b38d4cf918f845cdd27563add8a974 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 13:36:29 -0500 Subject: [PATCH 15/22] AWS_KMS: Better error reporting for transient aws-kms --- .../chain/include/sysio/chain/exceptions.hpp | 2 + .../src/kms_signature_provider.cpp | 44 +++++++++++------ .../src/kms_signature_provider.hpp | 45 ++++++++++++++++-- .../src/signature_provider_manager_plugin.cpp | 37 ++++++++++++--- .../test/test_kms_signature_provider.cpp | 47 +++++++++++++++++++ 5 files changed, 150 insertions(+), 25 deletions(-) diff --git a/libraries/chain/include/sysio/chain/exceptions.hpp b/libraries/chain/include/sysio/chain/exceptions.hpp index a01976af33..0e32e7f9bc 100644 --- a/libraries/chain/include/sysio/chain/exceptions.hpp +++ b/libraries/chain/include/sysio/chain/exceptions.hpp @@ -435,6 +435,8 @@ namespace sysio { namespace chain { 3110006, "Incorrect plugin configuration" ) FC_DECLARE_DERIVED_EXCEPTION( missing_trace_api_plugin_exception, plugin_exception, 3110007, "Missing Trace API Plugin" ) + FC_DECLARE_DERIVED_EXCEPTION( signing_transient_exception, plugin_exception, + 3110008, "Transient signing-provider failure; the operation should be retried" ) FC_DECLARE_DERIVED_EXCEPTION( wallet_exception, chain_exception, diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index d9f48c41f1..882e982edb 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -19,6 +19,8 @@ #include #include +#include + #include #include @@ -121,21 +123,6 @@ struct kms_signer_state { std::once_flag pin_once; }; -/// Translate an AWS KMS error outcome into a fc::exception with a stable -/// shape. The error-type enum name is the most actionable signal (e.g. -/// `AccessDenied`, `KeyUnavailable`, `Throttling`); the message and HTTP -/// status round out the diagnostic. -[[noreturn]] void throw_kms_error(std::string_view op, std::string_view key_id, - const Aws::Client::AWSError& err) { - FC_THROW_EXCEPTION(chain::plugin_config_exception, - "AWS KMS {} for key \"{}\" failed: {} (status {}, {}): {}", - op, key_id, - magic_enum::enum_name(err.GetErrorType()), - static_cast(err.GetResponseCode()), - std::string{err.GetExceptionName().c_str()}, - std::string{err.GetMessage().c_str()}); -} - /// Per-region cache of KMS clients. Lock once on lookup; the SDK's HTTP /// pool inside the client is itself thread-safe, so multiple sign closures /// sharing a client may submit `Sign` requests concurrently. @@ -342,6 +329,33 @@ void ensure_kms_pubkey_pinned(kms_signer_state& state) { } // namespace +[[noreturn]] void throw_kms_error(std::string_view op, std::string_view key_id, + const Aws::Client::AWSError& err) { + // The AWS SDK tags every deserialised error with a retryability class when + // it parses the response. Transient classes (throttling, KMSInternal, + // dependency / network timeouts, service-unavailable) report ShouldRetry(); + // permanent ones (access denied, key not found, disabled key, invalid + // state, bad parameters) do not. Map that split onto two exception types so + // a caller can retry the transient class with backoff and treat the rest as + // a fatal misconfiguration. The SDK's own classification is authoritative — + // it is what the SDK's retry strategy uses — so there is no hand-maintained + // table of error codes here to drift out of date. + const bool transient = err.ShouldRetry(); + const auto message = fmt::format( + "AWS KMS {} for key \"{}\" failed: {} (status {}, {}) [{}]: {}", + op, key_id, + magic_enum::enum_name(err.GetErrorType()), + static_cast(err.GetResponseCode()), + std::string{err.GetExceptionName().c_str()}, + transient ? "transient, retryable" : "permanent", + std::string{err.GetMessage().c_str()}); + + if (transient) { + FC_THROW_EXCEPTION(chain::signing_transient_exception, "{}", message); + } + FC_THROW_EXCEPTION(chain::plugin_config_exception, "{}", message); +} + kms_key_ref parse_kms_spec(std::string_view spec_data) { SYS_ASSERT(!spec_data.empty(), chain::plugin_config_exception, "KMS spec body is empty; expected an ARN or ':'"); diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index d8a44e51ec..c484c1e37b 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -11,6 +11,7 @@ */ #include +#include #include #include @@ -169,6 +170,33 @@ fc::em::public_key spki_der_to_public_key(std::span spki_de */ std::shared_ptr get_kms_client(const std::string& region); +/** + * @brief Translate a failed AWS KMS API outcome into an fc exception, split by + * whether the failure is transient. + * + * The AWS SDK classifies every deserialised error as retryable or not — the + * same classification its own retry strategy uses. `throw_kms_error` maps that + * split onto two distinct exception types so a caller can react correctly: + * + * - Transient (throttling, `KMSInternal`, dependency / network timeouts, + * service-unavailable) -> `sysio::chain::signing_transient_exception`. The + * operation may be retried with backoff; the credentials and key are fine. + * - Permanent (access denied, key not found, disabled key, invalid state, + * bad parameters) -> `sysio::chain::plugin_config_exception`. Retrying will + * not help — the operator must fix credentials, IAM, region, or the spec. + * + * The two types are siblings, not parent and child, so a handler that catches + * only `plugin_config_exception` will not silently swallow a retryable error. + * + * @param op short label for the failed operation (e.g. "Sign", "GetPublicKey") + * @param key_id the KMS key id / alias / ARN tail the call targeted + * @param err the failed outcome's AWS error + * @throws sysio::chain::signing_transient_exception if `err` is retryable + * @throws sysio::chain::plugin_config_exception otherwise + */ +[[noreturn]] void throw_kms_error(std::string_view op, std::string_view key_id, + const Aws::Client::AWSError& err); + /** * @brief A KMS-backed signer: the signing closure plus its one-shot startup * probe. @@ -181,7 +209,10 @@ std::shared_ptr get_kms_client(const std::string& region); struct kms_signer { /// Signing closure, usable wherever `fc::crypto::sign_fn` is expected. Each /// call issues one `KMS::Sign`; the first call also runs the public-key - /// pinning check, unless `warm_up` has already run it. + /// pinning check, unless `warm_up` has already run it. A transient KMS + /// failure throws `sysio::chain::signing_transient_exception` (safe to retry + /// with backoff); a permanent one throws + /// `sysio::chain::plugin_config_exception` (fatal — fix the configuration). fc::crypto::sign_fn sign; /// Eagerly run the startup probe: a single `KMS::GetPublicKey` @@ -189,8 +220,11 @@ struct kms_signer { /// key matches the pinned public key — without signing. Optional; if never /// called, the same check happens lazily on the first `sign`. Idempotent — /// it shares the closure's one-shot guard, so calling it never makes the - /// check run twice. Throws `sysio::chain::plugin_config_exception` on a - /// missing credential, bad region, absent IAM grant, or mismatched key. + /// check run twice. A permanent misconfiguration (missing credential, bad + /// region, absent IAM grant, mismatched key) throws + /// `sysio::chain::plugin_config_exception`; a transient failure (throttle, + /// timeout, `KMSInternal`) throws `sysio::chain::signing_transient_exception` + /// and the check is left to run again on the first `sign`. std::function warm_up; }; @@ -221,6 +255,11 @@ struct kms_signer { * throws `chain::plugin_config_exception`; once pinning has passed this is a * defence-in-depth check that should never fire. * + * A failed `Sign` or `GetPublicKey` call is classified by retryability: a + * transient failure (throttle, `KMSInternal`, timeout) throws + * `chain::signing_transient_exception` and may be retried with backoff; a + * permanent one throws `chain::plugin_config_exception`. See `throw_kms_error`. + * * v1 only supports secp256k1 keys held as Ethereum public keys * (`chain_key_type_ethereum` + `fc::em::public_key_shim`). Other key types * raise `chain::pending_impl_exception` at construction; a `public_key` diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index 3f8bcb422b..085a5f8d33 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -405,11 +405,15 @@ class signature_provider_manager_plugin_impl { * * For every KMS-backed signing key registered before startup, issue a * single `GetPublicKey` call. That resolves AWS credentials, warms the - * client, and verifies the KMS key matches the pinned public key. Any - * misconfiguration — missing credentials, bad region, absent IAM grant, - * unreachable IMDS, or a mismatched public key — throws - * `chain::plugin_config_exception` here, aborting startup loudly instead of - * failing on the first production sign. + * client, and verifies the KMS key matches the pinned public key. + * + * A permanent misconfiguration — missing credentials, bad region, absent + * IAM grant, or a mismatched public key — throws + * `chain::plugin_config_exception`, which propagates out of + * `plugin_startup()` and aborts startup loudly instead of failing on the + * first production sign. A transient error (throttle, timeout, KMSInternal) + * is logged and skipped — it is not a misconfiguration, so the lazy + * first-sign check is left to retry it rather than killing node startup. * * A no-op unless `_kms_startup_check` is set, and a no-op when no KMS-backed * keys are registered. @@ -431,10 +435,29 @@ class signature_provider_manager_plugin_impl { } ilog("Verifying AWS KMS credentials for {} signing key(s) at startup", probes.size()); + std::size_t deferred = 0; for (auto& probe : probes) { - probe(); // throws chain::plugin_config_exception on any misconfiguration + try { + probe(); + } catch (const chain::signing_transient_exception& e) { + // A transient KMS error (throttle, timeout, KMSInternal) at startup + // is not a misconfiguration — don't abort the node over it. Log it + // and continue; the lazy first-sign check re-runs the same probe. + ++deferred; + wlog("AWS KMS startup credential check: transient error for one " + "key, deferring its check to the first sign: {}", + e.to_detail_string()); + } + // A permanent misconfiguration throws chain::plugin_config_exception, + // which propagates out of plugin_startup() and aborts startup loudly. + } + if (deferred == 0) { + ilog("AWS KMS startup credential check passed"); + } else { + ilog("AWS KMS startup credential check passed; {} key(s) hit a " + "transient error and will be re-checked on the first sign", + deferred); } - ilog("AWS KMS startup credential check passed"); } private: diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index 1623843be3..77744e4854 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -30,6 +30,8 @@ #include #include +#include + #include "../src/kms_signature_provider.hpp" using sysio::sigprov::kms::kms_key_ref; @@ -588,4 +590,49 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_pubkey_variant_mismatch sysio::chain::plugin_config_exception); } +// --------------------------------------------------------------------------- +// throw_kms_error — transient vs permanent classification +// +// `throw_kms_error` maps an AWS error onto two exception types using the SDK's +// own retryability classification. Constructing an `AWSError` is offline — it +// is a plain value type, so these tests need no SDK init and no network. +// --------------------------------------------------------------------------- + +BOOST_AUTO_TEST_CASE(throw_kms_error_maps_retryable_to_transient) { + // A throttling error is retryable — the caller should back off and retry. + const Aws::Client::AWSError err( + Aws::KMS::KMSErrors::THROTTLING, "ThrottlingException", "Rate exceeded", + /* isRetryable */ true); + BOOST_CHECK_THROW(sysio::sigprov::kms::throw_kms_error("Sign", "alias/x", err), + sysio::chain::signing_transient_exception); +} + +BOOST_AUTO_TEST_CASE(throw_kms_error_maps_non_retryable_to_config) { + // Access-denied is permanent — retrying will not help, the IAM grant is + // missing. It must surface as a config exception, not a transient one. + const Aws::Client::AWSError err( + Aws::KMS::KMSErrors::ACCESS_DENIED, "AccessDeniedException", + "not authorized to perform kms:Sign", /* isRetryable */ false); + BOOST_CHECK_THROW(sysio::sigprov::kms::throw_kms_error("Sign", "alias/x", err), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(throw_kms_error_transient_is_not_a_config_exception) { + // The transient and permanent classes must be siblings, not parent/child, + // so a handler that catches only `plugin_config_exception` cannot silently + // swallow a retryable error and defeat the caller's retry/backoff logic. + const Aws::Client::AWSError err( + Aws::KMS::KMSErrors::K_M_S_INTERNAL, "KMSInternalException", + "internal service error", /* isRetryable */ true); + bool caught_transient = false; + try { + sysio::sigprov::kms::throw_kms_error("GetPublicKey", "alias/x", err); + } catch (const sysio::chain::plugin_config_exception&) { + BOOST_FAIL("transient KMS error was caught as plugin_config_exception"); + } catch (const sysio::chain::signing_transient_exception&) { + caught_transient = true; + } + BOOST_CHECK(caught_transient); +} + BOOST_AUTO_TEST_SUITE_END() From 5b8f0ed8266a399cb3b5e5c9de29306bb6f12c2a Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 15:58:47 -0500 Subject: [PATCH 16/22] AWS_KMS: eth_client_signer integration gap fixed --- libraries/libfc/include/fc/crypto/signer.hpp | 61 +++++++++-- .../libfc/test/crypto/test_cypher_suites.cpp | 101 ++++++++++++++++++ 2 files changed, 154 insertions(+), 8 deletions(-) diff --git a/libraries/libfc/include/fc/crypto/signer.hpp b/libraries/libfc/include/fc/crypto/signer.hpp index cb364354b1..7e5d25b2fb 100644 --- a/libraries/libfc/include/fc/crypto/signer.hpp +++ b/libraries/libfc/include/fc/crypto/signer.hpp @@ -19,6 +19,55 @@ namespace fc::crypto { template struct signer_traits; +namespace detail { + +/// Sign a 32-byte keccak digest with an ethereum (secp256k1) key, transparently +/// supporting both `signature_provider_t` shapes: +/// +/// - A provider with a local `em` private key signs in-process. +/// - A provider with no local key — a remote signer such as AWS KMS — has no +/// key to sign with directly, so the digest is handed to its `sign` closure. +/// `sign` is typed on `fc::sha256`, but the closure treats that argument as +/// an opaque 32-byte digest, so the keccak digest is carried across +/// byte-for-byte and raw-signed. +/// +/// Digest preparation (plain keccak vs. EIP-191 framing) is the caller's job — +/// it has already happened in `signer_traits::prepare` — so the remote signer +/// must raw-sign the digest as given. The remote path self-verifies this: +/// it recovers the public key from the returned signature and asserts it +/// matches the provider's key. A remote signer that applied its own message +/// framing instead of raw-signing, or that signed with the wrong key, recovers +/// to a different key and is rejected here — before an invalid transaction can +/// be emitted — rather than yielding a silently bad signature. +inline signature em_sign_keccak(const signature_provider_t& p, const keccak256& digest) { + if (p.private_key) { + FC_ASSERT(p.private_key->contains(), + "ETH signing requires an EM private key"); + const auto& em_key = p.private_key->get(); + return signature(signature::storage_type(em_key.sign_keccak256(digest))); + } + + FC_ASSERT(static_cast(p.sign), + "signature provider has neither a local private key nor a sign function"); + + // Carry the keccak digest into the sha256-typed closure argument unchanged. + const auto digest_bytes = digest.to_char_span(); + const signature sig = p.sign(sha256(digest_bytes.data(), digest_bytes.size())); + + // Self-verify: the remote signer must have raw-signed `digest` with the + // provider's key. Recover and compare; reject anything else loudly. + FC_ASSERT(sig.contains(), + "remote signer returned a non-Ethereum signature"); + const auto recovered = public_key(public_key::storage_type( + sig.get().recover_eth(digest))); + FC_ASSERT(recovered == p.public_key, + "remote signer produced a signature that does not recover to the " + "provider's public key — the signer's key or digest framing is wrong"); + return sig; +} + +} // namespace detail + // --------------------------------------------------------------------------- // (ethereum, ethereum) — ETH client transaction signing // Signs: keccak256(raw_bytes) via EM (secp256k1) @@ -34,10 +83,8 @@ struct signer_traits { } static signature do_sign(const signature_provider_t& p, const prepared_type& digest) { - FC_ASSERT(p.private_key, "ETH signing requires a local private key"); - FC_ASSERT(p.private_key->contains(), "ETH signing requires an EM private key"); - auto& em_key = p.private_key->get(); - return signature(signature::storage_type(em_key.sign_keccak256(digest))); + // Sign with a local em key, or delegate to a remote signer's closure. + return detail::em_sign_keccak(p, digest); } static public_key do_recover(const signature& sig, const prepared_type& digest) { @@ -62,10 +109,8 @@ struct signer_traits { } static signature do_sign(const signature_provider_t& p, const prepared_type& digest) { - FC_ASSERT(p.private_key, "ETH signing requires a local private key"); - FC_ASSERT(p.private_key->contains(), "ETH signing requires an EM private key"); - auto& em_key = p.private_key->get(); - return signature(signature::storage_type(em_key.sign_keccak256(digest))); + // Sign with a local em key, or delegate to a remote signer's closure. + return detail::em_sign_keccak(p, digest); } static public_key do_recover(const signature& sig, const prepared_type& digest) { diff --git a/libraries/libfc/test/crypto/test_cypher_suites.cpp b/libraries/libfc/test/crypto/test_cypher_suites.cpp index 3d7d0bc4d3..f002f9f132 100644 --- a/libraries/libfc/test/crypto/test_cypher_suites.cpp +++ b/libraries/libfc/test/crypto/test_cypher_suites.cpp @@ -645,4 +645,105 @@ BOOST_AUTO_TEST_CASE(test_signer_rejects_wrong_key_type) try { BOOST_CHECK_THROW(eth_client_signer{provider}, fc::exception); } FC_LOG_AND_RETHROW(); +// =========================================================================== +// Remote-signer support — eth_client_signer / wire_eth_signer driving a +// signature_provider_t that has no local private key (e.g. an AWS KMS key), +// where the `sign` closure is the signer. +// =========================================================================== + +BOOST_AUTO_TEST_CASE(test_eth_client_signer_signs_via_closure_without_private_key) try { + // A provider with no local private key — its `sign` closure does the + // signing, mirroring an AWS KMS-backed provider. + auto key = private_key::generate(private_key::key_type::em); + auto pub = key.get_public_key(); + auto em_key = key.get(); + + auto payload = ethereum::to_uint8_span("eth transaction bytes"); + auto kc = keccak256::hash(payload); + + signature_provider_t remote; + remote.key_type = chain_key_type_ethereum; + remote.public_key = pub; + // remote.private_key intentionally left empty. + remote.sign = [em_key, kc](const sha256&) { + // A KMS-style raw signer: a recoverable signature over the 32-byte + // digest it is handed (here the known keccak digest of `payload`). + return signature(signature::storage_type(em_key.sign_keccak256(kc))); + }; + BOOST_REQUIRE(!remote.private_key.has_value()); + + eth_client_signer s(remote); + auto sig = s.sign(payload); + auto recovered = s.recover(sig, payload); + BOOST_CHECK_EQUAL(recovered.to_string({}), pub.to_string({})); +} FC_LOG_AND_RETHROW(); + +BOOST_AUTO_TEST_CASE(test_eth_client_signer_closure_matches_local_key) try { + // The remote-signer path must produce the identical signature the local-key + // path produces — libsecp256k1 ECDSA is RFC-6979 deterministic. + auto key = private_key::generate(private_key::key_type::em); + auto em_key = key.get(); + + auto payload = ethereum::to_uint8_span("eth transaction bytes"); + auto kc = keccak256::hash(payload); + + auto local = make_provider(key, chain_key_type_ethereum); // has private_key + + signature_provider_t remote; + remote.key_type = chain_key_type_ethereum; + remote.public_key = key.get_public_key(); + remote.sign = [em_key, kc](const sha256&) { + return signature(signature::storage_type(em_key.sign_keccak256(kc))); + }; + + auto local_sig = eth_client_signer(local).sign(payload); + auto remote_sig = eth_client_signer(remote).sign(payload); + BOOST_CHECK_EQUAL(local_sig.to_string({}), remote_sig.to_string({})); +} FC_LOG_AND_RETHROW(); + +BOOST_AUTO_TEST_CASE(test_eth_client_signer_rejects_remote_signature_for_wrong_key) try { + // A remote signer whose closure signs with a different key than the provider + // advertises must be rejected by the self-verifying recover check, rather + // than emitting a transaction with an invalid signature. + auto key_a = private_key::generate(private_key::key_type::em); + auto key_b = private_key::generate(private_key::key_type::em); + auto em_b = key_b.get(); + + auto payload = ethereum::to_uint8_span("eth transaction bytes"); + auto kc = keccak256::hash(payload); + + signature_provider_t bad; + bad.key_type = chain_key_type_ethereum; + bad.public_key = key_a.get_public_key(); // provider advertises key A + bad.sign = [em_b, kc](const sha256&) { // but the closure signs with key B + return signature(signature::storage_type(em_b.sign_keccak256(kc))); + }; + + eth_client_signer s(bad); + BOOST_CHECK_THROW(s.sign(payload), fc::exception); +} FC_LOG_AND_RETHROW(); + +BOOST_AUTO_TEST_CASE(test_wire_eth_signer_signs_via_closure_without_private_key) try { + // wire_eth_signer shares the same remote-signer path; its EIP-191 framing is + // applied in `prepare`, so the closure still just raw-signs the digest. + auto key = private_key::generate(private_key::key_type::em); + auto pub = key.get_public_key(); + auto em_key = key.get(); + + auto digest = sha256::hash("wire transaction digest"); + auto prepared = ethereum::hash_user_message(digest.to_uint8_span()); + + signature_provider_t remote; + remote.key_type = chain_key_type_ethereum; + remote.public_key = pub; + remote.sign = [em_key, prepared](const sha256&) { + return signature(signature::storage_type(em_key.sign_keccak256(prepared))); + }; + + wire_eth_signer s(remote); + auto sig = s.sign(digest); + auto recovered = s.recover(sig, digest); + BOOST_CHECK_EQUAL(recovered.to_string({}), pub.to_string({})); +} FC_LOG_AND_RETHROW(); + BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file From fbcf59dea62a2cfbee0792c56446e02cd130670e Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 17:57:47 -0500 Subject: [PATCH 17/22] AWS_KMS: replaced includes with forward declarations --- .../src/kms_signature_provider.hpp | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index c484c1e37b..56b79fff67 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -10,9 +10,6 @@ * its tests; it is intentionally not installed under `include/`. */ -#include -#include - #include #include #include @@ -26,6 +23,31 @@ #include #include +/** + * Forward declarations for the AWS SDK types named in this header's function + * signatures. The full `` headers pull in a large dependency + * tree; declaring these types opaquely keeps that tree out of every + * translation unit that includes this header only for `parse_kms_spec` / + * `make_kms_signature_provider` — notably `signature_provider_manager_plugin.cpp`, + * which uses no AWS type at all. The full AWS headers are included only where + * the complete types are actually needed: `kms_signature_provider.cpp` and the + * plugin's test translation unit. + * + * `KMSClient` is named only as `std::shared_ptr` and `AWSError` + * only behind a reference, so an incomplete type suffices for the declarations + * below. This couples the header to the AWS SDK's (very stable) declaration of + * these names; the SDK ships no forward-declaration header of its own. + */ +namespace Aws { +namespace KMS { + class KMSClient; + enum class KMSErrors; +} // namespace KMS +namespace Client { + template class AWSError; +} // namespace Client +} // namespace Aws + namespace sysio::sigprov::kms { /** From 8b42c61489d50075657b8d2c76861ef005024192 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Wed, 20 May 2026 23:02:12 -0500 Subject: [PATCH 18/22] AWS_KMS: Updated baseline and reference for vcpkg --- vcpkg-configuration.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vcpkg-configuration.json b/vcpkg-configuration.json index 114d85713e..863c96aec0 100644 --- a/vcpkg-configuration.json +++ b/vcpkg-configuration.json @@ -10,8 +10,8 @@ { "kind": "git", "repository": "https://github.com/wire-network/wire-vcpkg-registry", - "baseline": "23aa4018992d85602a4b5c30104a623e708b4b3e", - "reference": "23aa4018992d85602a4b5c30104a623e708b4b3e", + "baseline": "fcfa97213e4d965b58871a8325a9b5ef54259d12", + "reference": "fcfa97213e4d965b58871a8325a9b5ef54259d12", "packages": [ "boost", "softfloat", From af87e6841b7e8c40045f702f617f088f6ab49fa5 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Thu, 21 May 2026 17:44:50 -0500 Subject: [PATCH 19/22] AWS_KMS: Fixed claude identified issues --- OVERVIEW.md | 2 +- libraries/libfc/include/fc/crypto/signer.hpp | 49 +++++-- .../src/network/ethereum/ethereum_client.cpp | 8 +- .../src/kms_signature_provider.cpp | 132 ++++++++++++------ .../src/kms_signature_provider.hpp | 32 ++--- .../src/signature_provider_manager_plugin.cpp | 43 +++--- .../test/README.md | 13 +- .../test/test_create_provider_specs.cpp | 4 +- .../test/test_kms_signature_provider.cpp | 69 ++++++--- 9 files changed, 238 insertions(+), 114 deletions(-) diff --git a/OVERVIEW.md b/OVERVIEW.md index 19c1ee3386..86d3d2b434 100644 --- a/OVERVIEW.md +++ b/OVERVIEW.md @@ -87,7 +87,7 @@ Elected via Appointed Proof of Stake (APoS). They validate transactions and prod - Building and submitting outbound envelopes to external chains - Responding to challenges if disputed -Signing keys for outbound submissions can be held locally (`KEY:`), in a `kiod` daemon (`KIOD:`), or in AWS KMS (`KMS:` — secp256k1 asymmetric keys). See `plugins/signature_provider_manager_plugin/KMS_SIGNING_DESIGN.md` for the KMS spec format and operational notes. +Signing keys for outbound submissions can be held locally (`KEY:`), in a `kiod` daemon (`KIOD:`), or in AWS KMS (`KMS:` — secp256k1 asymmetric keys). See `plugins/signature_provider_manager_plugin/test/README.md` for KMS key setup, IAM requirements, the `KMS:` spec format, and operational notes. ### Underwriters Independent operators running the `underwriter_plugin`. They provide collateral-backed liquidity for cross-chain swaps: diff --git a/libraries/libfc/include/fc/crypto/signer.hpp b/libraries/libfc/include/fc/crypto/signer.hpp index 7e5d25b2fb..54e2b552b1 100644 --- a/libraries/libfc/include/fc/crypto/signer.hpp +++ b/libraries/libfc/include/fc/crypto/signer.hpp @@ -7,15 +7,17 @@ #include #include +#include + #include namespace fc::crypto { // =========================================================================== -// signer_traits — compile-time signing dispatch per (TargetChain, KeyType) +// signer_traits -- compile-time signing dispatch per (TargetChain, KeyType) // =========================================================================== -/// Primary template — must be specialized for each valid (TargetChain, KeyType) pair +/// Primary template -- must be specialized for each valid (TargetChain, KeyType) pair template struct signer_traits; @@ -25,20 +27,20 @@ namespace detail { /// supporting both `signature_provider_t` shapes: /// /// - A provider with a local `em` private key signs in-process. -/// - A provider with no local key — a remote signer such as AWS KMS — has no +/// - A provider with no local key -- a remote signer such as AWS KMS -- has no /// key to sign with directly, so the digest is handed to its `sign` closure. /// `sign` is typed on `fc::sha256`, but the closure treats that argument as /// an opaque 32-byte digest, so the keccak digest is carried across /// byte-for-byte and raw-signed. /// -/// Digest preparation (plain keccak vs. EIP-191 framing) is the caller's job — -/// it has already happened in `signer_traits::prepare` — so the remote signer +/// Digest preparation (plain keccak vs. EIP-191 framing) is the caller's job -- +/// it has already happened in `signer_traits::prepare` -- so the remote signer /// must raw-sign the digest as given. The remote path self-verifies this: /// it recovers the public key from the returned signature and asserts it /// matches the provider's key. A remote signer that applied its own message /// framing instead of raw-signing, or that signed with the wrong key, recovers -/// to a different key and is rejected here — before an invalid transaction can -/// be emitted — rather than yielding a silently bad signature. +/// to a different key and is rejected here -- before an invalid transaction can +/// be emitted -- rather than yielding a silently bad signature. inline signature em_sign_keccak(const signature_provider_t& p, const keccak256& digest) { if (p.private_key) { FC_ASSERT(p.private_key->contains(), @@ -62,14 +64,14 @@ inline signature em_sign_keccak(const signature_provider_t& p, const keccak256& sig.get().recover_eth(digest))); FC_ASSERT(recovered == p.public_key, "remote signer produced a signature that does not recover to the " - "provider's public key — the signer's key or digest framing is wrong"); + "provider's public key -- the signer's key or digest framing is wrong"); return sig; } } // namespace detail // --------------------------------------------------------------------------- -// (ethereum, ethereum) — ETH client transaction signing +// (ethereum, ethereum) -- ETH client transaction signing // Signs: keccak256(raw_bytes) via EM (secp256k1) // --------------------------------------------------------------------------- template<> @@ -95,7 +97,7 @@ struct signer_traits { }; // --------------------------------------------------------------------------- -// (wire, ethereum) — Wire transactions signed via MetaMask / personal_sign +// (wire, ethereum) -- Wire transactions signed via MetaMask / personal_sign // Signs: keccak256(EIP-191 prefix + sha256_raw) via EM (secp256k1) // --------------------------------------------------------------------------- template<> @@ -121,7 +123,7 @@ struct signer_traits { }; // --------------------------------------------------------------------------- -// (solana, solana) — Solana client transaction signing +// (solana, solana) -- Solana client transaction signing // Signs: raw_bytes via ED25519 // --------------------------------------------------------------------------- template<> @@ -151,7 +153,7 @@ struct signer_traits { }; // =========================================================================== -// signer — typed cross-chain signing wrapper +// signer -- typed cross-chain signing wrapper // =========================================================================== template @@ -163,7 +165,7 @@ struct signer { explicit signer(const signature_provider_t& p) : provider(p) { FC_ASSERT(p.key_type == KeyType, "signer: provider key_type mismatch (expected {}, got {})", - static_cast(KeyType), static_cast(p.key_type)); + magic_enum::enum_name(KeyType), magic_enum::enum_name(p.key_type)); } signature sign(typename traits::input_type data) const { @@ -189,10 +191,29 @@ struct signer { }; // =========================================================================== -// wire_signer — key-type agnostic Wire transaction signing +// wire_signer -- key-type agnostic Wire transaction signing // Passes through to provider.sign(sha256); handles K1/EM/ED polymorphically // =========================================================================== +/** + * @brief Key-type-agnostic Wire transaction signer. + * + * Hands the 32-byte digest straight to `provider.sign(...)`, which dispatches + * polymorphically over K1 / EM / ED keys inside the provider's closure. + * + * Unlike `signer<>` (and its `wire_eth_signer` alias), `wire_signer` has no + * `prepare()` hook: it raw-signs exactly the 32 bytes it is given and applies + * no EIP-191 framing. For an Ethereum key that framing + * (`ethereum::hash_user_message`) lives in + * `signer_traits::prepare`, not in + * the signature provider -- a `KEY:` provider's `em::private_key_shim` and a + * `KMS:` provider alike raw-sign the digest as handed in. Consequently + * `wire_signer` and `wire_eth_signer` are NOT interchangeable even for an + * identical provider: swapping one for the other changes which bytes are + * signed. Use `wire_eth_signer` when the Wire transaction must carry a + * MetaMask-compatible `personal_sign` signature; use `wire_signer` only when + * the caller has already produced the exact digest to be signed. + */ struct wire_signer { const signature_provider_t& provider; diff --git a/libraries/libfc/src/network/ethereum/ethereum_client.cpp b/libraries/libfc/src/network/ethereum/ethereum_client.cpp index 5de017f576..079a152d6f 100644 --- a/libraries/libfc/src/network/ethereum/ethereum_client.cpp +++ b/libraries/libfc/src/network/ethereum/ethereum_client.cpp @@ -116,7 +116,13 @@ fc::variant ethereum_client::execute_contract_tx_fn(const eip1559_tx& source_tx, auto& tx_sig_data = tx_sig.get().serialize(); std::copy_n(tx_sig_data.begin(), 32, tx.r.begin()); std::copy_n(tx_sig_data.begin() + 32, 32, tx.s.begin()); - tx.v = tx_sig_data[64] - 27; // recovery id + // Byte 64 of the recoverable signature is the Ethereum `v`, encoded + // pre-EIP-155 as `27 + recovery_id` (Yellow Paper Appendix F) by every + // signing path -- local `em` keys and the AWS KMS signer alike (cf. + // `eth_v_offset` in kms_signature_provider.cpp). EIP-1559 typed + // transactions carry the bare recovery id, so strip the 27 offset here; + // this is the exact inverse of the packing done at signing time. + tx.v = tx_sig_data[64] - 27; // recovery id (pre-EIP-155 27 offset removed) tx_encoded = rlp::encode_eip1559_signed_typed(tx); } diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index 882e982edb..34656652f8 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -1,3 +1,16 @@ +// magic_enum resolves an enum value to its name only when the value falls +// inside [MAGIC_ENUM_RANGE_MIN, MAGIC_ENUM_RANGE_MAX] (default -128..128). +// AWS KMS's service-specific error codes (Aws::KMS::KMSErrors) begin at +// Aws::Client::CoreErrors::SERVICE_EXTENSION_START_RANGE + 1 = 129 and run up +// to 176, so under the default ceiling magic_enum::enum_name() would return "" +// for every KMS-specific error in throw_kms_error() -- precisely the transient +// vs. permanent triage cases that matter most. Raise the ceiling to 256 (well +// above the highest KMS error value, 176). This must be defined before +// is first included anywhere in this translation +// unit -- including transitively through the headers below -- so it sits ahead +// of every #include. +#define MAGIC_ENUM_RANGE_MAX 256 + #include "kms_signature_provider.hpp" #include @@ -37,14 +50,14 @@ namespace sysio::sigprov::kms { namespace { /// Anchor for ARN detection. ARNs always start with `arn:aws:kms:` (no -/// regional suffix on the partition for kms — `arn:aws-cn:kms:` and +/// regional suffix on the partition for kms -- `arn:aws-cn:kms:` and /// `arn:aws-us-gov:kms:` are not currently in scope; revisit if a partition /// other than `aws` becomes a deployment target). constexpr std::string_view kms_arn_prefix = "arn:aws:kms:"; /// Case-insensitive lead-in shared by every ARN. A spec that begins with this -/// but does not match `kms_arn_prefix` is a malformed or out-of-scope ARN — -/// never the shorthand `:` form — and must fail loudly rather +/// but does not match `kms_arn_prefix` is a malformed or out-of-scope ARN -- +/// never the shorthand `:` form -- and must fail loudly rather /// than fall through to the shorthand parser. See `parse_kms_spec`. constexpr std::string_view arn_lead_in = "arn:"; @@ -56,6 +69,7 @@ constexpr std::size_t kms_arn_segment_count = 6; constexpr std::size_t arn_idx_partition = 1; constexpr std::size_t arn_idx_service = 2; constexpr std::size_t arn_idx_region = 3; +constexpr std::size_t arn_idx_account = 4; constexpr std::size_t arn_idx_tail = 5; /// Tail prefixes the KMS API accepts for the `KeyId` field. @@ -66,7 +80,10 @@ constexpr std::string_view tail_prefix_alias = "alias/"; /// lazily on first use; libsecp256k1 contexts are thread-safe for the /// signing-verification operations we use here. Lives separate from libfc's /// internal context (`fc::em::detail::_get_context`) because that one is -/// not exposed across translation units. +/// not exposed across translation units. It is created with +/// `SECP256K1_CONTEXT_NONE` -- no precomputation tables -- so this second +/// long-lived context costs only a few hundred bytes, not the few hundred KiB +/// a precomputed context would. const secp256k1_context* kms_secp_ctx() { static secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE); return ctx; @@ -81,18 +98,36 @@ constexpr unsigned char eth_v_offset = 27; /// Process-wide AWS SDK lifecycle. Constructed lazily on first KMS access, /// destroyed at static destruction (after the client cache, since the cache /// is touched by `get_kms_client` *after* this lifecycle, making it the -/// younger Meyers singleton; younger statics are destroyed first). The -/// safe because the application object owns the plugin and is destroyed -/// before atexit static teardown; do not hand a KMS-backed `sign_fn` to an -/// owner that outlives the application. +/// younger Meyers singleton; younger statics are destroyed first). Holding a +/// `KMSClient` shared_ptr inside a long-lived `sign_fn` closure is safe +/// because the application object owns the plugin and is destroyed before +/// atexit static teardown; do not hand a KMS-backed `sign_fn` to an owner +/// that outlives the application. struct aws_sdk_lifecycle { static aws_sdk_lifecycle& instance() { static aws_sdk_lifecycle s; return s; } + + // This is a Meyers singleton: there is exactly one lifecycle per process. + // Deleting copy / move makes that intent explicit and stops a stray + // `aws_sdk_lifecycle copy = ...` from compiling and running a second + // InitAPI / ShutdownAPI pair. + aws_sdk_lifecycle(const aws_sdk_lifecycle&) = delete; + aws_sdk_lifecycle(aws_sdk_lifecycle&&) = delete; + aws_sdk_lifecycle& operator=(const aws_sdk_lifecycle&) = delete; + aws_sdk_lifecycle& operator=(aws_sdk_lifecycle&&) = delete; + private: aws_sdk_lifecycle() { Aws::InitAPI(_options); } ~aws_sdk_lifecycle() { Aws::ShutdownAPI(_options); } + + // TODO: the default-constructed SDKOptions leaves the AWS SDK's internal + // logger disabled. To diagnose an AWS-side retry storm or credential-chain + // failure from the node's own logs -- without restarting the node under the + // AWS_LOG_LEVEL environment variable -- install an + // Aws::Utils::Logging::LogSystemInterface here that forwards to fc::log + // before the Aws::InitAPI call above. Aws::SDKOptions _options{}; }; @@ -118,8 +153,8 @@ struct kms_signer_state { /// discriminate between recovery_id 0 and 1. fc::em::public_key expected_em_pubkey; /// One-shot guard for the GetPublicKey pinning check. The check runs on the - /// first `Sign`; `std::call_once` re-runs it only if it threw — e.g. a - /// transient GetPublicKey API error — and never again once it has passed. + /// first `Sign`; `std::call_once` re-runs it only if it threw -- e.g. a + /// transient GetPublicKey API error -- and never again once it has passed. std::once_flag pin_once; }; @@ -154,7 +189,7 @@ bool starts_with_ci(std::string_view s, std::string_view prefix) { // X.509 SubjectPublicKeyInfo (DER) decoding for KMS public-key pinning. // // AWS KMS `GetPublicKey` returns the key as a DER-encoded SubjectPublicKeyInfo -// (RFC 5280 §4.1). We walk just enough of that structure to verify the key is +// (RFC 5280 section 4.1). We walk just enough of that structure to verify the key is // secp256k1 and to lift out the raw EC point. // --------------------------------------------------------------------------- @@ -168,7 +203,7 @@ constexpr unsigned char der_tag_bit_string = 0x03; constexpr unsigned char der_length_long_form_bit = 0x80; constexpr unsigned char der_length_value_mask = 0x7F; -/// DER OBJECT IDENTIFIER bodies (the content of the OID TLV — tag and length +/// DER OBJECT IDENTIFIER bodies (the content of the OID TLV -- tag and length /// stripped) for the two OIDs that a secp256k1 SPKI must carry: /// `1.2.840.10045.2.1` id-ecPublicKey and `1.3.132.0.10` secp256k1. constexpr std::array oid_ec_public_key = { @@ -181,7 +216,7 @@ constexpr unsigned char ec_point_uncompressed_prefix = 0x04; constexpr std::size_t ec_point_uncompressed_len = 65; /// Minimal ASN.1 DER reader over a byte span. DER is a canonical, unambiguous -/// encoding, so a structural tag-length-value walk is a genuine parse — not a +/// encoding, so a structural tag-length-value walk is a genuine parse -- not a /// heuristic. Every malformed input raises `plugin_config_exception`. struct der_reader { std::span buf; @@ -198,7 +233,7 @@ struct der_reader { }; /// Read the next TLV element and advance past it. - element next() { + [[nodiscard]] element next() { SYS_ASSERT(pos + 2 <= buf.size(), chain::plugin_config_exception, "Malformed KMS public-key DER: truncated tag/length header"); const unsigned char tag = buf[pos++]; @@ -268,11 +303,11 @@ parse_spki_ec_point(std::span spki_der) { "KMS public-key DER: AlgorithmIdentifier is not a pair of OBJECT IDENTIFIERs"); SYS_ASSERT(std::ranges::equal(algo_oid.content, oid_ec_public_key), chain::plugin_config_exception, - "KMS public-key DER: algorithm is not id-ecPublicKey — the KMS key is not an " + "KMS public-key DER: algorithm is not id-ecPublicKey -- the KMS key is not an " "elliptic-curve key"); SYS_ASSERT(std::ranges::equal(curve_oid.content, oid_secp256k1), chain::plugin_config_exception, - "KMS public-key DER: EC curve is not secp256k1 — the KMS key must be created " + "KMS public-key DER: EC curve is not secp256k1 -- the KMS key must be created " "with key spec ECC_SECG_P256K1"); // subjectPublicKey BIT STRING: a leading "unused bits" octet (0 for a @@ -295,12 +330,12 @@ parse_spki_ec_point(std::span spki_der) { /// Public-key pinning check. Fetch the KMS key's public key with the free, /// non-billable `GetPublicKey` API, decode its SubjectPublicKeyInfo, and assert /// it matches the key the operator pinned in the spec. On mismatch this throws -/// `plugin_config_exception` early — before any billable `Sign` — with a +/// `plugin_config_exception` early -- before any billable `Sign` -- with a /// message that names the misconfiguration directly. Invoked exactly once per /// closure through `kms_signer_state::pin_once`. void verify_kms_pubkey(kms_signer_state& state) { Aws::KMS::Model::GetPublicKeyRequest req; - req.SetKeyId(Aws::String{state.key_id.c_str()}); + req.SetKeyId(Aws::String{state.key_id}); auto outcome = state.client->GetPublicKey(req); if (!outcome.IsSuccess()) { @@ -321,7 +356,7 @@ void verify_kms_pubkey(kms_signer_state& state) { /// Run the public-key pinning check exactly once for `state`. Both the first /// `Sign` and the opt-in startup probe funnel through here, so `std::call_once` -/// guarantees a single GetPublicKey round-trip — and retries it only if it +/// guarantees a single GetPublicKey round-trip -- and retries it only if it /// threw (e.g. a transient GetPublicKey API error). void ensure_kms_pubkey_pinned(kms_signer_state& state) { std::call_once(state.pin_once, [&] { verify_kms_pubkey(state); }); @@ -337,18 +372,18 @@ void ensure_kms_pubkey_pinned(kms_signer_state& state) { // permanent ones (access denied, key not found, disabled key, invalid // state, bad parameters) do not. Map that split onto two exception types so // a caller can retry the transient class with backoff and treat the rest as - // a fatal misconfiguration. The SDK's own classification is authoritative — - // it is what the SDK's retry strategy uses — so there is no hand-maintained + // a fatal misconfiguration. The SDK's own classification is authoritative -- + // it is what the SDK's retry strategy uses -- so there is no hand-maintained // table of error codes here to drift out of date. const bool transient = err.ShouldRetry(); const auto message = fmt::format( "AWS KMS {} for key \"{}\" failed: {} (status {}, {}) [{}]: {}", op, key_id, magic_enum::enum_name(err.GetErrorType()), - static_cast(err.GetResponseCode()), - std::string{err.GetExceptionName().c_str()}, + magic_enum::enum_integer(err.GetResponseCode()), + err.GetExceptionName(), transient ? "transient, retryable" : "permanent", - std::string{err.GetMessage().c_str()}); + err.GetMessage()); if (transient) { FC_THROW_EXCEPTION(chain::signing_transient_exception, "{}", message); @@ -365,24 +400,36 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { // stray colons inside the trailing `key/` segment stay glued to it // (KMS key ids are uuids, no colons today, but aliases are operator- // chosen and we should not silently truncate). - auto parts = fc::split(std::string{spec_data}, ':', kms_arn_segment_count); + auto parts = fc::split(spec_data, ':', kms_arn_segment_count); SYS_ASSERT(parts.size() == kms_arn_segment_count, chain::plugin_config_exception, "Malformed KMS ARN \"{}\": expected {} colon-separated segments, got {}", spec_data, kms_arn_segment_count, parts.size()); - const auto& region = parts[arn_idx_region]; - const auto& tail = parts[arn_idx_tail]; + const auto& region = parts[arn_idx_region]; + const auto& account = parts[arn_idx_account]; + const auto& tail = parts[arn_idx_tail]; + // `arn`, `aws`, and `kms` are guaranteed non-empty and correct by the + // `kms_arn_prefix` match above. The region, account, and tail segments + // are operator-supplied; an empty one means a stray colon collapsed two + // segments (e.g. `arn:aws:kms:us-east-1::key/x`) and the `key_id` we + // would hand KMS is wrong. Reject that here with a precise message + // rather than after a billable Sign against a bad endpoint. SYS_ASSERT(!region.empty(), chain::plugin_config_exception, "KMS ARN \"{}\" has empty region segment", spec_data); + SYS_ASSERT(!account.empty(), chain::plugin_config_exception, + "KMS ARN \"{}\" has empty account-id segment", spec_data); SYS_ASSERT(tail.starts_with(tail_prefix_key) || tail.starts_with(tail_prefix_alias), chain::plugin_config_exception, "KMS ARN tail must start with 'key/' or 'alias/', got \"{}\" in \"{}\"", tail, spec_data); - // Reject bare prefixes: `key/` or `alias/` with nothing after them. - SYS_ASSERT(tail.size() > tail_prefix_key.size() && - (!tail.starts_with(tail_prefix_alias) || tail.size() > tail_prefix_alias.size()), - chain::plugin_config_exception, + // Reject bare prefixes: `key/` or `alias/` with nothing after them. The + // assertion above guarantees `tail` starts with exactly one of the two, + // so the prefix actually present determines how much to strip. + const auto name = tail.starts_with(tail_prefix_key) + ? tail.substr(tail_prefix_key.size()) + : tail.substr(tail_prefix_alias.size()); + SYS_ASSERT(!name.empty(), chain::plugin_config_exception, "KMS ARN tail \"{}\" has empty key/alias name", tail); return kms_key_ref{region, tail}; @@ -392,15 +439,15 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { // supported `arn:aws:kms:` form above is a malformed or out-of-scope ARN, // never shorthand. Falling through to the `:` parser below // would split on the first colon and silently yield region="arn"; AWS then - // rejects that only at first sign — with an opaque `InvalidRegion`/endpoint + // rejects that only at first sign -- with an opaque `InvalidRegion`/endpoint // error, after a billable attempt. Fail loudly here instead, naming the // offending partition and service. Non-`aws` partitions (`aws-cn`, // `aws-us-gov`) are deliberately out of scope; this is the boundary that // enforces it. A mis-cased `ARN:AWS:KMS:...` and a typo'd service - // (`arn:aws:ksm:...`) land here too — the message points at the canonical + // (`arn:aws:ksm:...`) land here too -- the message points at the canonical // form in every case. if (starts_with_ci(spec_data, arn_lead_in)) { - const auto parts = fc::split(std::string{spec_data}, ':', kms_arn_segment_count); + const auto parts = fc::split(spec_data, ':', kms_arn_segment_count); std::string partition, service; if (parts.size() > arn_idx_partition) partition = parts[arn_idx_partition]; @@ -468,6 +515,13 @@ bool normalise_low_s(std::array& compact) { unsigned char recover_v(const std::array& compact, std::span digest, const fc::em::public_key& expected) { + // ECDSA recovery ids span {0, 1, 2, 3}. Ids 2 and 3 only arise when the + // signature's `r` had to be reduced modulo the curve order `n` because the + // ephemeral point's x-coordinate exceeded `n` -- a ~2^-128 event that no + // compliant ECDSA implementation, AWS KMS included, ever emits. The + // recoverable set is therefore exactly {0, 1}. Do not widen this bound + // speculatively: a genuine id of 2/3 would mean KMS returned a + // non-canonical signature, which is a defect to surface, not to absorb. for (unsigned char rec_id = 0; rec_id < 2; ++rec_id) { fc::em::compact_signature trial{}; std::ranges::copy(compact, trial.begin()); @@ -547,13 +601,13 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, get_kms_client(ref.region), ref.key_id, shim.unwrapped()); fc::crypto::sign_fn sign = [state](const chain::digest_type& digest) -> chain::signature_type { - // Public-key pinning. Before the first — and only the first — billable + // Public-key pinning. Before the first -- and only the first -- billable // Sign, fetch the KMS key's own public key with the free GetPublicKey // API and assert it matches the key pinned in the spec. This turns the // common "wrong in the spec" mistake into a fast, direct // error instead of an opaque recovery failure that would otherwise // surface only after a paid Sign. If the opt-in startup probe already - // ran the check, this is a no-op — both paths share `state->pin_once` + // ran the check, this is a no-op -- both paths share `state->pin_once` // through `ensure_kms_pubkey_pinned`. ensure_kms_pubkey_pinned(*state); @@ -561,7 +615,7 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, // already a hash; otherwise it would re-hash with SHA-256 and break // any chain that hashes with anything other than SHA-256. Aws::KMS::Model::SignRequest req; - req.SetKeyId(Aws::String{state->key_id.c_str()}); + req.SetKeyId(Aws::String{state->key_id}); req.SetMessage(Aws::Utils::ByteBuffer{ digest.to_uint8_span().data(), digest.to_uint8_span().size()}); @@ -585,7 +639,7 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, }; // Startup probe: runs the same one-shot pinning check as the first Sign, - // but issues only the free GetPublicKey — no billable Sign. An opt-in + // but issues only the free GetPublicKey -- no billable Sign. An opt-in // plugin_startup() calls this so a missing credential, bad region, absent // IAM grant, or wrong pinned key fails loudly at boot instead of deep in // production. It shares `state` (hence `pin_once`) with `sign`, so enabling @@ -611,7 +665,7 @@ std::shared_ptr get_kms_client(const std::string& region) { auto& slot = c.by_region[region]; if (!slot) { Aws::Client::ClientConfiguration cfg; - cfg.region = Aws::String{region.c_str()}; + cfg.region = Aws::String{region}; slot = std::make_shared(cfg); } return slot; diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index 56b79fff67..8fa77f9bcc 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -1,7 +1,7 @@ #pragma once /** - * AWS KMS-backed signature provider — plugin-private header. + * AWS KMS-backed signature provider -- plugin-private header. * * The KMS provider extends the existing `KEY:` / `KIOD:` spec grammar with a * third form, `KMS:`, where the signing key never leaves AWS. @@ -28,7 +28,7 @@ * signatures. The full `` headers pull in a large dependency * tree; declaring these types opaquely keeps that tree out of every * translation unit that includes this header only for `parse_kms_spec` / - * `make_kms_signature_provider` — notably `signature_provider_manager_plugin.cpp`, + * `make_kms_signature_provider` -- notably `signature_provider_manager_plugin.cpp`, * which uses no AWS type at all. The full AWS headers are included only where * the complete types are actually needed: `kms_signature_provider.cpp` and the * plugin's test translation unit. @@ -53,7 +53,7 @@ namespace sysio::sigprov::kms { /** * @brief Parsed KMS key reference. * - * `key_id` is whatever follows the resolved region — either the bare key id / + * `key_id` is whatever follows the resolved region -- either the bare key id / * alias as the operator typed it (`:` form), or the * `key/` / `alias/` tail of an ARN. AWS KMS accepts both shapes * in the `KeyId` field of `Sign` / `GetPublicKey`, so we hand it through @@ -111,7 +111,7 @@ std::array der_to_compact(std::span der) bool normalise_low_s(std::array& compact); /** - * @brief Find the `recovery_id ∈ {0, 1}` that recovers `expected` from `compact`. + * @brief Find the `recovery_id in {0, 1}` that recovers `expected` from `compact`. * * KMS does not return a recovery byte. We derive it locally by trying both * parities, recovering the candidate public key, and comparing to the pubkey @@ -127,14 +127,14 @@ bool normalise_low_s(std::array& compact); * @param digest the 32-byte digest the signature was produced over * @param expected the public key the signer is supposed to control * @throws sysio::chain::plugin_config_exception if neither parity recovers `expected` - * @return `recovery_id` ∈ {0, 1} (Ethereum's `v` byte is `27 + recovery_id`) + * @return `recovery_id` in {0, 1} (Ethereum's `v` byte is `27 + recovery_id`) */ unsigned char recover_v(const std::array& compact, std::span digest, const fc::em::public_key& expected); /** - * @brief End-to-end: DER → low-S compact + recovered v → 65-byte Ethereum sig. + * @brief End-to-end: DER -> low-S compact + recovered v -> 65-byte Ethereum sig. * * Wraps `der_to_compact` + `normalise_low_s` + `recover_v` and packs the * result as `[ r | s | (27 + recovery_id) ]`, the format consumed by @@ -154,7 +154,7 @@ fc::em::compact_signature der_to_eth_signature( * @brief Decode an X.509 SubjectPublicKeyInfo (DER) into a secp256k1 public key. * * AWS KMS `GetPublicKey` returns the public key as a DER-encoded - * SubjectPublicKeyInfo (RFC 5280 §4.1): an outer `SEQUENCE` wrapping an + * SubjectPublicKeyInfo (RFC 5280 section 4.1): an outer `SEQUENCE` wrapping an * `AlgorithmIdentifier` and a `BIT STRING`. This helper walks that structure, * verifies the algorithm is `id-ecPublicKey` over the `secp256k1` named curve, * and lifts the trailing uncompressed `0x04 || X || Y` point into an @@ -196,7 +196,7 @@ std::shared_ptr get_kms_client(const std::string& region); * @brief Translate a failed AWS KMS API outcome into an fc exception, split by * whether the failure is transient. * - * The AWS SDK classifies every deserialised error as retryable or not — the + * The AWS SDK classifies every deserialised error as retryable or not -- the * same classification its own retry strategy uses. `throw_kms_error` maps that * split onto two distinct exception types so a caller can react correctly: * @@ -205,7 +205,7 @@ std::shared_ptr get_kms_client(const std::string& region); * operation may be retried with backoff; the credentials and key are fine. * - Permanent (access denied, key not found, disabled key, invalid state, * bad parameters) -> `sysio::chain::plugin_config_exception`. Retrying will - * not help — the operator must fix credentials, IAM, region, or the spec. + * not help -- the operator must fix credentials, IAM, region, or the spec. * * The two types are siblings, not parent and child, so a handler that catches * only `plugin_config_exception` will not silently swallow a retryable error. @@ -234,13 +234,13 @@ struct kms_signer { /// pinning check, unless `warm_up` has already run it. A transient KMS /// failure throws `sysio::chain::signing_transient_exception` (safe to retry /// with backoff); a permanent one throws - /// `sysio::chain::plugin_config_exception` (fatal — fix the configuration). + /// `sysio::chain::plugin_config_exception` (fatal -- fix the configuration). fc::crypto::sign_fn sign; /// Eagerly run the startup probe: a single `KMS::GetPublicKey` /// that resolves AWS credentials, warms the client, and verifies the KMS - /// key matches the pinned public key — without signing. Optional; if never - /// called, the same check happens lazily on the first `sign`. Idempotent — + /// key matches the pinned public key -- without signing. Optional; if never + /// called, the same check happens lazily on the first `sign`. Idempotent -- /// it shares the closure's one-shot guard, so calling it never makes the /// check run twice. A permanent misconfiguration (missing credential, bad /// region, absent IAM grant, mismatched key) throws @@ -256,15 +256,15 @@ struct kms_signer { * Validates `key_type` and the public-key variant, resolves the shared * `KMSClient` for `ref.region` via `get_kms_client`, and captures the client, * key id, and expected public key into the returned closure. No network I/O - * happens here; the first KMS request occurs only when the closure — or the - * returned `warm_up` probe — is invoked. + * happens here; the first KMS request occurs only when the closure -- or the + * returned `warm_up` probe -- is invoked. * * On its first invocation the closure performs public-key pinning: it calls * `KMSClient::GetPublicKey` exactly once, decodes the returned X.509 * SubjectPublicKeyInfo via `spki_der_to_public_key`, and asserts the KMS * key's public key matches `expected_pubkey`. A mismatch - * throws `chain::plugin_config_exception` immediately — before any billable - * `Sign` — so a spec that pins the wrong `` fails fast with a + * throws `chain::plugin_config_exception` immediately -- before any billable + * `Sign` -- so a spec that pins the wrong `` fails fast with a * direct error. The pinning check runs once on success; a transient * `GetPublicKey` failure is retried on the next `Sign`. * diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index 085a5f8d33..757d1ebac5 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -133,8 +133,8 @@ class signature_provider_manager_plugin_impl { auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); auto kms = sysio::sigprov::kms::make_kms_signature_provider(ref, key_type, public_key); - // Retain the startup probe so plugin_startup() can — when the opt-in - // KMS startup check is enabled — fail fast on a missing credential, + // Retain the startup probe so plugin_startup() can -- when the opt-in + // KMS startup check is enabled -- fail fast on a missing credential, // bad region, absent IAM grant, or wrong pinned key, instead of // discovering it deep into production on the first sign. { @@ -407,30 +407,34 @@ class signature_provider_manager_plugin_impl { * single `GetPublicKey` call. That resolves AWS credentials, warms the * client, and verifies the KMS key matches the pinned public key. * - * A permanent misconfiguration — missing credentials, bad region, absent - * IAM grant, or a mismatched public key — throws + * A permanent misconfiguration -- missing credentials, bad region, absent + * IAM grant, or a mismatched public key -- throws * `chain::plugin_config_exception`, which propagates out of * `plugin_startup()` and aborts startup loudly instead of failing on the * first production sign. A transient error (throttle, timeout, KMSInternal) - * is logged and skipped — it is not a misconfiguration, so the lazy + * is logged and skipped -- it is not a misconfiguration, so the lazy * first-sign check is left to retry it rather than killing node startup. * * A no-op unless `_kms_startup_check` is set, and a no-op when no KMS-backed * keys are registered. + * + * The probe list is one-shot: this drains it (whether or not the check is + * enabled) so it never lingers as a misleading registry of signers. A KMS + * key registered after `plugin_startup()` -- e.g. via a future runtime spec + * reload -- is not retroactively probed; its pinning check still runs lazily + * on the first sign through the closure's shared one-shot guard. */ void run_kms_startup_check() { - if (!_kms_startup_check) { - return; - } - - // Copy the probe list so the network-bound GetPublicKey calls run - // without holding the providers mutex. + // Drain the probe list under the lock. Moving it out both hands the + // network-bound GetPublicKey calls a private copy to run without holding + // the providers mutex, and empties the member so the one-shot nature of + // the startup check is structural rather than just documented. std::vector> probes; { std::scoped_lock lock(_signing_providers_mutex); - probes = _kms_startup_probes; + probes = std::exchange(_kms_startup_probes, {}); } - if (probes.empty()) { + if (!_kms_startup_check || probes.empty()) { return; } @@ -441,7 +445,7 @@ class signature_provider_manager_plugin_impl { probe(); } catch (const chain::signing_transient_exception& e) { // A transient KMS error (throttle, timeout, KMSInternal) at startup - // is not a misconfiguration — don't abort the node over it. Log it + // is not a misconfiguration -- don't abort the node over it. Log it // and continue; the lazy first-sign check re-runs the same probe. ++deferred; wlog("AWS KMS startup credential check: transient error for one " @@ -504,11 +508,10 @@ void signature_provider_manager_plugin::set_program_options(options_description& cfg.add_options()( option_name_kms_startup_check, boost::program_options::value()->default_value(false), - "When true, the signature provider plugin issues an AWS KMS GetPublicKey " - "call at startup for every KMS-backed (`KMS:` spec) signing key, failing " - "fast if credentials, region, IAM permissions, or the pinned public key " - "are misconfigured. Off by default, so nodes without KMS keys and offline " - "test environments are unaffected."); + "Probe every `KMS:` signing key with a GetPublicKey call at startup; " + "fail fast if credentials, region, IAM, or the pinned public key are " + "wrong. Off by default; has no effect when no KMS-backed keys are " + "configured."); } const char* signature_provider_manager_plugin::signature_provider_help_text() const { @@ -520,7 +523,7 @@ const char* signature_provider_manager_plugin::signature_provider_help_text() co " key format to parse\n\n" " is a string form of a valid \n\n" " is a string in the form :\n\n" - " is KEY, KIOD, KMS, or SE\n\n" + " is KEY, KIOD, or KMS\n\n" " KEY: is a string containing a private key of the key-type specified\n\n" " KIOD: is the URL where kiod is available and the appropriate wallet(s) are unlocked\n\n" " KMS: is either a full AWS KMS ARN\n" diff --git a/plugins/signature_provider_manager_plugin/test/README.md b/plugins/signature_provider_manager_plugin/test/README.md index 2eef4bb9aa..894e2d2a6f 100644 --- a/plugins/signature_provider_manager_plugin/test/README.md +++ b/plugins/signature_provider_manager_plugin/test/README.md @@ -54,11 +54,14 @@ aws kms create-alias \ ### 2. IAM permission -The principal whose credentials the test runs under needs `kms:Sign` on the -key. `kms:GetPublicKey` is also granted so the same principal can extract -the matching pubkey hex (Prereq #4) without juggling a second role; the -plugin itself does not call `GetPublicKey` (the expected pubkey is supplied -by the caller). Minimum inline policy: +The principal whose credentials the test runs under needs both `kms:Sign` +and `kms:GetPublicKey` on the key. `kms:GetPublicKey` is required at runtime, +not just for setup: on the first sign of each key — and again from the +optional startup probe — the plugin calls `GetPublicKey` exactly once to pin +the operator-supplied public key against the one KMS actually holds. Without +that grant the lazy pin and the startup probe both fail with +`AccessDeniedException` on first use. The same call is also how you extract +the matching pubkey hex for the spec (Prereq #4). Minimum inline policy: ```json { diff --git a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp index f52e1c3906..74c205d7b1 100644 --- a/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_create_provider_specs.cpp @@ -344,7 +344,7 @@ BOOST_AUTO_TEST_CASE(create_provider_ethereum_kms_spec_routes_through_parser) { // End-to-end check that the plugin's spec parser routes `KMS:` through // `parse_kms_spec` + `make_kms_signature_provider` and returns a provider // whose sign closure is callable. The closure itself is *not* invoked - // here — invocation issues a real KMS::Sign request, which is covered + // here -- invocation issues a real KMS::Sign request, which is covered // only by the env-gated live test. using namespace fc::test; using namespace fc::crypto; @@ -374,7 +374,7 @@ BOOST_AUTO_TEST_CASE(create_provider_ethereum_kms_spec_routes_through_parser) { BOOST_AUTO_TEST_CASE(create_provider_kms_spec_rejects_solana) { // The plugin must reject a KMS spec for a non-secp256k1 chain at parse - // time, not at first sign — operators should learn early that KMS + // time, not at first sign -- operators should learn early that KMS // can't sign Solana ed25519. using namespace fc::test; using namespace fc::crypto; diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index 77744e4854..1d3e1b179e 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -1,7 +1,7 @@ /** * Unit tests for the AWS KMS signature provider parser. * - * These cases cover only the offline parse path — `parse_kms_spec` does no + * These cases cover only the offline parse path -- `parse_kms_spec` does no * network I/O and constructs no `KMSClient`, so the suite runs without AWS * credentials and without an internet connection. The one end-to-end signing * test in this file (`kms_live_sign_round_trip`) is gated on the @@ -177,7 +177,7 @@ BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_empty) { } BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_no_region_no_arn) { - // Bare key id — ambiguous about which region the key lives in. Must throw. + // Bare key id -- ambiguous about which region the key lives in. Must throw. BOOST_CHECK_THROW(parse_kms_spec("1234abcd-12ab-34cd-56ef-1234567890ab"), sysio::chain::plugin_config_exception); } @@ -188,8 +188,22 @@ BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_missing_tail) { sysio::chain::plugin_config_exception); } +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_empty_account) { + // A stray colon collapsed the account-id segment to empty. The six segment + // count still holds, so this is caught by the explicit account check, not + // the segment-count assertion. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1::key/abc"), + sysio::chain::plugin_config_exception); +} + +BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_empty_region) { + // Likewise for an empty region segment. + BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms::111122223333:key/abc"), + sysio::chain::plugin_config_exception); +} + BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_arn_bad_tail_prefix) { - // ARN tail that doesn't start with `key/` or `alias/` — KMS rejects this + // ARN tail that doesn't start with `key/` or `alias/` -- KMS rejects this // server-side, but we can fail loud at parse time. BOOST_CHECK_THROW(parse_kms_spec("arn:aws:kms:us-east-1:111122223333:foo/bar"), sysio::chain::plugin_config_exception); @@ -228,7 +242,7 @@ BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_uppercase_arn) { } BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_typoed_service) { - // `ksm` instead of `kms` — would otherwise mis-parse region as "arn". + // `ksm` instead of `kms` -- would otherwise mis-parse region as "arn". BOOST_CHECK_THROW(parse_kms_spec("arn:aws:ksm:us-east-1:111122223333:key/abc"), sysio::chain::plugin_config_exception); } @@ -252,7 +266,7 @@ BOOST_AUTO_TEST_CASE(parse_kms_spec_rejects_shorthand_empty_key_id) { } // --------------------------------------------------------------------------- -// DER → raw + low-S + v-recovery helpers +// DER -> raw + low-S + v-recovery helpers // // These tests synthesise the inputs that AWS KMS would normally produce by // signing locally with `fc::em::private_key::sign_compact` and re-encoding @@ -374,7 +388,7 @@ BOOST_AUTO_TEST_CASE(der_to_eth_signature_normalises_high_s_input) { } // --------------------------------------------------------------------------- -// spki_der_to_public_key — KMS public-key pinning decoder +// spki_der_to_public_key -- KMS public-key pinning decoder // // These tests synthesise the X.509 SubjectPublicKeyInfo that AWS KMS // `GetPublicKey` returns by wrapping a locally generated EC point with @@ -492,7 +506,7 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_returns_callable_for_ethereum) BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_wire_k1) { // Wire K1 (`chain_key_type_wire`) is also secp256k1, but its public-key // and signature shapes differ from Ethereum's, so it is not yet supported - // — fail loud rather than sign with the wrong format. + // -- fail loud rather than sign with the wrong format. const auto chain_pub = fc::crypto::private_key::generate(fc::crypto::private_key::key_type::k1).get_public_key(); @@ -505,7 +519,7 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_wire_k1) { } BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_solana) { - // Solana keys are Ed25519 — KMS does not support that signing algorithm. + // Solana keys are Ed25519 -- KMS does not support that signing algorithm. const auto chain_pub = fc::crypto::private_key::generate(fc::crypto::private_key::key_type::ed).get_public_key(); @@ -518,7 +532,7 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_solana) { } // --------------------------------------------------------------------------- -// Live KMS round-trip — env-gated, skipped in default CI. +// Live KMS round-trip -- env-gated, skipped in default CI. // // Set both env vars to exercise this case against a real AWS KMS key: // KMS_LIVE_SPEC body of a `KMS:` spec, e.g. `us-east-1:alias/wire-ci-test` @@ -540,7 +554,7 @@ BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { const auto* spec_env = std::getenv("KMS_LIVE_SPEC"); const auto* pub_env = std::getenv("KMS_LIVE_PUBKEY"); if (!spec_env || !pub_env || *spec_env == '\0' || *pub_env == '\0') { - BOOST_TEST_MESSAGE("KMS_LIVE_SPEC / KMS_LIVE_PUBKEY not set — skipping live KMS test"); + BOOST_TEST_MESSAGE("KMS_LIVE_SPEC / KMS_LIVE_PUBKEY not set -- skipping live KMS test"); return; } @@ -554,7 +568,7 @@ BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { ref, fc::crypto::chain_key_type_ethereum, chain_pub); // Exercise the startup probe: a GetPublicKey call plus the public-key pin, - // with no signing. It must pass before we attempt a (billable) Sign — and a + // with no signing. It must pass before we attempt a (billable) Sign -- and a // passing probe pre-pins the closure. BOOST_CHECK_NO_THROW(kms.warm_up()); @@ -562,7 +576,7 @@ BOOST_AUTO_TEST_CASE(kms_live_sign_round_trip) { // log entries are recognisable across runs. const auto keccak = fc::crypto::keccak256::hash(std::string{"wire-sysio kms live test 2026"}); - // chain::digest_type is fc::sha256 — a 32-byte holder. We copy the + // chain::digest_type is fc::sha256 -- a 32-byte holder. We copy the // keccak256 bytes verbatim because KMS treats the message as opaque // when MessageType=DIGEST. sysio::chain::digest_type chain_digest; @@ -591,15 +605,15 @@ BOOST_AUTO_TEST_CASE(make_kms_signature_provider_rejects_pubkey_variant_mismatch } // --------------------------------------------------------------------------- -// throw_kms_error — transient vs permanent classification +// throw_kms_error -- transient vs permanent classification // // `throw_kms_error` maps an AWS error onto two exception types using the SDK's -// own retryability classification. Constructing an `AWSError` is offline — it +// own retryability classification. Constructing an `AWSError` is offline -- it // is a plain value type, so these tests need no SDK init and no network. // --------------------------------------------------------------------------- BOOST_AUTO_TEST_CASE(throw_kms_error_maps_retryable_to_transient) { - // A throttling error is retryable — the caller should back off and retry. + // A throttling error is retryable -- the caller should back off and retry. const Aws::Client::AWSError err( Aws::KMS::KMSErrors::THROTTLING, "ThrottlingException", "Rate exceeded", /* isRetryable */ true); @@ -608,7 +622,7 @@ BOOST_AUTO_TEST_CASE(throw_kms_error_maps_retryable_to_transient) { } BOOST_AUTO_TEST_CASE(throw_kms_error_maps_non_retryable_to_config) { - // Access-denied is permanent — retrying will not help, the IAM grant is + // Access-denied is permanent -- retrying will not help, the IAM grant is // missing. It must surface as a config exception, not a transient one. const Aws::Client::AWSError err( Aws::KMS::KMSErrors::ACCESS_DENIED, "AccessDeniedException", @@ -635,4 +649,27 @@ BOOST_AUTO_TEST_CASE(throw_kms_error_transient_is_not_a_config_exception) { BOOST_CHECK(caught_transient); } +BOOST_AUTO_TEST_CASE(throw_kms_error_message_carries_enum_name) { + // Regression guard for the magic_enum range. KMS service-specific error + // codes (Aws::KMS::KMSErrors) begin at 129; magic_enum's default ceiling is + // 128, so without MAGIC_ENUM_RANGE_MAX raised the enum-name field of the + // diagnostic is blank for every KMS-specific error. DISABLED has value 141, + // squarely in the formerly-dead range. + const Aws::Client::AWSError err( + Aws::KMS::KMSErrors::DISABLED, "DisabledException", "Key is disabled", + /* isRetryable */ false); + try { + sysio::sigprov::kms::throw_kms_error("Sign", "alias/x", err); + BOOST_FAIL("throw_kms_error did not throw"); + } catch (const fc::exception& e) { + // "DISABLED" is the magic_enum::enum_name spelling. The wire name + // ("DisabledException") and the human message ("Key is disabled") use + // mixed / lower case, so the all-caps form appears only if enum_name + // resolved -- asserting on it specifically pins the range fix. + const auto detail = e.to_detail_string(); + BOOST_CHECK_MESSAGE(detail.find("DISABLED") != std::string::npos, + "KMS diagnostic is missing the KMSErrors enum name: " << detail); + } +} + BOOST_AUTO_TEST_SUITE_END() From 0ea94464c9c1854023b87e160ff86b2dd8f1f1e6 Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Thu, 21 May 2026 19:08:43 -0500 Subject: [PATCH 20/22] AWS_KMS: Fixed KMS ARNs --- .../src/kms_signature_provider.cpp | 26 +++++++++++++------ .../src/kms_signature_provider.hpp | 22 +++++++++++----- .../test/test_kms_signature_provider.cpp | 17 +++++++++--- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index 34656652f8..db45103179 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -396,10 +396,12 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { "KMS spec body is empty; expected an ARN or ':'"); if (spec_data.starts_with(kms_arn_prefix)) { - // Full ARN form. Capture exactly `kms_arn_segment_count` parts so any - // stray colons inside the trailing `key/` segment stay glued to it - // (KMS key ids are uuids, no colons today, but aliases are operator- - // chosen and we should not silently truncate). + // Full ARN form. Split into exactly `kms_arn_segment_count` parts so any + // stray colons inside the trailing `key/` / `alias/` segment + // stay glued to it (KMS key ids are uuids with no colons today, but + // aliases are operator-chosen and must not be silently truncated). The + // split is only for *validation* below -- the value handed to KMS is the + // unmodified ARN. auto parts = fc::split(spec_data, ':', kms_arn_segment_count); SYS_ASSERT(parts.size() == kms_arn_segment_count, chain::plugin_config_exception, "Malformed KMS ARN \"{}\": expected {} colon-separated segments, got {}", @@ -412,9 +414,9 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { // `arn`, `aws`, and `kms` are guaranteed non-empty and correct by the // `kms_arn_prefix` match above. The region, account, and tail segments // are operator-supplied; an empty one means a stray colon collapsed two - // segments (e.g. `arn:aws:kms:us-east-1::key/x`) and the `key_id` we - // would hand KMS is wrong. Reject that here with a precise message - // rather than after a billable Sign against a bad endpoint. + // segments (e.g. `arn:aws:kms:us-east-1::key/x`), producing a malformed + // ARN. Reject that here with a precise message rather than after a + // billable Sign against a bad endpoint. SYS_ASSERT(!region.empty(), chain::plugin_config_exception, "KMS ARN \"{}\" has empty region segment", spec_data); SYS_ASSERT(!account.empty(), chain::plugin_config_exception, @@ -432,7 +434,15 @@ kms_key_ref parse_kms_spec(std::string_view spec_data) { SYS_ASSERT(!name.empty(), chain::plugin_config_exception, "KMS ARN tail \"{}\" has empty key/alias name", tail); - return kms_key_ref{region, tail}; + // Hand KMS the full ARN, not the `key/` / `alias/` tail. AWS + // KMS accepts a bare key id, a key ARN, an alias name, or an alias ARN + // as `KeyId` -- but NOT the bare `key/` tail. Passing the whole + // ARN both keeps key ARNs valid and preserves the account id: an alias + // ARN stripped to `alias/` would resolve against the *caller's* + // account and could silently bind a same-named alias for a different + // key. `region` is still taken from the ARN to build the regional + // client; it matches the region embedded in the ARN we pass through. + return kms_key_ref{region, std::string{spec_data}}; } // A spec that begins with `arn:` (any casing) but did not match the diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp index 8fa77f9bcc..0cde712cc1 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.hpp @@ -53,11 +53,19 @@ namespace sysio::sigprov::kms { /** * @brief Parsed KMS key reference. * - * `key_id` is whatever follows the resolved region -- either the bare key id / - * alias as the operator typed it (`:` form), or the - * `key/` / `alias/` tail of an ARN. AWS KMS accepts both shapes - * in the `KeyId` field of `Sign` / `GetPublicKey`, so we hand it through - * verbatim and let the SDK validate it. + * `region` selects the regional `KMSClient`. `key_id` is handed verbatim to + * the `KeyId` field of KMS `Sign` / `GetPublicKey`. + * + * AWS KMS accepts four `KeyId` forms -- a bare key id, a key ARN, an alias + * name (`alias/`), or an alias ARN -- but NOT the bare `key/` + * tail of an ARN. Accordingly: + * - For an ARN spec, `key_id` is the full ARN, unmodified. Keeping the ARN + * intact preserves the account id; stripping an alias ARN down to the + * bare `alias/` would resolve the alias in the *caller's* account + * and could silently bind a same-named alias for a different key. + * - For the shorthand `:` spec, `key_id` is the + * bare key id or `alias/` the operator supplied; that form + * deliberately resolves within the caller's own account. */ struct kms_key_ref { std::string region; @@ -69,8 +77,8 @@ struct kms_key_ref { * * Accepted forms: * - Full ARN: `arn:aws:kms:::(key|alias)/` - * Region is taken from the ARN's region segment; `key_id` is the trailing - * `key/` or `alias/` portion. + * Region is taken from the ARN's region segment; `key_id` is the full ARN + * itself, passed to KMS verbatim so the account id is preserved. * - Shorthand: `:` * Region is the leading token; everything after the first `:` is `key_id`. * diff --git a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp index 1d3e1b179e..d6bf51ad19 100644 --- a/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/test/test_kms_signature_provider.cpp @@ -149,15 +149,24 @@ std::array drop_v(const fc::em::compact_signature& sig) { BOOST_AUTO_TEST_SUITE(kms_signature_provider_tests) BOOST_AUTO_TEST_CASE(parse_kms_spec_arn_key) { - const auto ref = parse_kms_spec("arn:aws:kms:us-east-1:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab"); + // A key ARN must round-trip into `key_id` whole. AWS KMS accepts a key ARN + // as `KeyId` but rejects the bare `key/` tail, and keeping the full + // ARN preserves the account id. + constexpr auto arn = + "arn:aws:kms:us-east-1:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab"; + const auto ref = parse_kms_spec(arn); BOOST_CHECK_EQUAL(ref.region, "us-east-1"); - BOOST_CHECK_EQUAL(ref.key_id, "key/1234abcd-12ab-34cd-56ef-1234567890ab"); + BOOST_CHECK_EQUAL(ref.key_id, arn); } BOOST_AUTO_TEST_CASE(parse_kms_spec_arn_alias) { - const auto ref = parse_kms_spec("arn:aws:kms:eu-west-2:111122223333:alias/wire-cranker-eth-01"); + // An alias ARN must likewise round-trip whole: stripping it to the bare + // `alias/` would drop the account id and could resolve a same-named + // alias in the caller's own account. + constexpr auto arn = "arn:aws:kms:eu-west-2:111122223333:alias/wire-cranker-eth-01"; + const auto ref = parse_kms_spec(arn); BOOST_CHECK_EQUAL(ref.region, "eu-west-2"); - BOOST_CHECK_EQUAL(ref.key_id, "alias/wire-cranker-eth-01"); + BOOST_CHECK_EQUAL(ref.key_id, arn); } BOOST_AUTO_TEST_CASE(parse_kms_spec_shorthand_uuid) { From 2e43baf3b70909315a1b767c1c6c6c85468c06ae Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Thu, 21 May 2026 19:30:58 -0500 Subject: [PATCH 21/22] AWS_KMS: KMS store after provider --- .../src/signature_provider_manager_plugin.cpp | 57 ++++++++++++++----- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index 757d1ebac5..ec311c1a70 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -76,7 +76,25 @@ class signature_provider_manager_plugin_impl { }; } - std::pair> create_provider_from_spec( + /** + * Result of building a signing provider from a ``: + * the signing closure, the local private key (only for `KEY:` specs), and -- + * for `KMS:` specs only -- the one-shot startup probe. + * + * `kms_warm_up` is handed back rather than registered inline so the caller + * can defer appending it to `_kms_startup_probes` until the provider has + * been successfully inserted. A provider rejected by `set_provider()` (a + * duplicate key_name or public_key) must not leave an orphan probe behind: + * `plugin_startup()` would otherwise probe -- or fail node startup on -- a + * KMS key whose provider was never registered. + */ + struct provider_spec_result { + fc::crypto::sign_fn signer; + std::optional private_key; + std::function kms_warm_up; ///< empty for non-KMS specs + }; + + provider_spec_result create_provider_from_spec( fc::crypto::chain_key_type_t key_type, const fc::crypto::public_key& public_key, const std::string& spec) { @@ -118,11 +136,11 @@ class signature_provider_manager_plugin_impl { FC_ASSERT(public_key == privkey.get_public_key(), "Private key does not match given public key for {}", fc::json::to_log_string(public_key)); - return {make_key_signature_provider(privkey), privkey}; + return {.signer = make_key_signature_provider(privkey), .private_key = privkey}; } if (spec_type_str == "KIOD") { - return {make_kiod_signature_provider(spec_data, public_key), std::nullopt}; + return {.signer = make_kiod_signature_provider(spec_data, public_key)}; } if (spec_type_str == "KMS") { @@ -133,15 +151,14 @@ class signature_provider_manager_plugin_impl { auto ref = sysio::sigprov::kms::parse_kms_spec(spec_data); auto kms = sysio::sigprov::kms::make_kms_signature_provider(ref, key_type, public_key); - // Retain the startup probe so plugin_startup() can -- when the opt-in - // KMS startup check is enabled -- fail fast on a missing credential, - // bad region, absent IAM grant, or wrong pinned key, instead of - // discovering it deep into production on the first sign. - { - std::scoped_lock lock(_signing_providers_mutex); - _kms_startup_probes.push_back(std::move(kms.warm_up)); - } - return {std::move(kms.sign), std::nullopt}; + // Hand the startup probe back to the caller rather than registering it + // here. It is appended to `_kms_startup_probes` only once the provider + // is successfully inserted, so a provider rejected as a duplicate + // leaves no probe for plugin_startup() to run. The probe lets + // plugin_startup() -- when the opt-in KMS startup check is enabled -- + // fail fast on a missing credential, bad region, absent IAM grant, or + // wrong pinned key instead of discovering it on the first sign. + return {.signer = std::move(kms.sign), .kms_warm_up = std::move(kms.warm_up)}; } SYS_THROW(chain::plugin_config_exception, "Unsupported key provider type \"{}\"", spec_type_str); @@ -392,12 +409,24 @@ class signature_provider_manager_plugin_impl { } } - auto [signer, privkey] = create_provider_from_spec(key_type, pubkey, private_key_provider_spec); + auto [signer, privkey, kms_warm_up] = + create_provider_from_spec(key_type, pubkey, private_key_provider_spec); auto provider = std::make_shared( signature_provider_t{target_chain, key_type, key_name, pubkey, privkey, signer} ); - return set_provider(provider); + // Register first. set_provider() throws plugin_config_exception on a + // duplicate key_name / public_key; reaching the statement after it means + // the provider is in the maps. Only then is the KMS startup probe (if + // any) retained -- a rejected provider leaves nothing behind for + // plugin_startup() to probe. + set_provider(provider); + + if (kms_warm_up) { + std::scoped_lock lock(_signing_providers_mutex); + _kms_startup_probes.push_back(std::move(kms_warm_up)); + } + return provider; } /** From 1161e4b4cd207944e34bf90269a1451d069014ca Mon Sep 17 00:00:00 2001 From: Brian Johnson Date: Fri, 22 May 2026 14:02:06 -0500 Subject: [PATCH 22/22] AWS_KMS: Fix signing interface for sha256 and keccak256 --- .../include/fc/crypto/signature_provider.hpp | 33 +++++++++++++++++-- libraries/libfc/include/fc/crypto/signer.hpp | 13 ++++---- .../libfc/test/crypto/test_cypher_suites.cpp | 15 +++++---- .../include/sysio/testing/bls_utils.hpp | 6 ++-- .../producer_plugin/src/producer_plugin.cpp | 4 ++- .../src/kms_signature_provider.cpp | 12 ++++--- .../src/signature_provider_manager_plugin.cpp | 17 +++++++--- 7 files changed, 73 insertions(+), 27 deletions(-) diff --git a/libraries/libfc/include/fc/crypto/signature_provider.hpp b/libraries/libfc/include/fc/crypto/signature_provider.hpp index 6ed4572b9e..d3fdde9f11 100644 --- a/libraries/libfc/include/fc/crypto/signature_provider.hpp +++ b/libraries/libfc/include/fc/crypto/signature_provider.hpp @@ -1,12 +1,41 @@ #pragma once #include #include +#include +#include +#include + +#include namespace fc::crypto { using signature_provider_id_t = std::variant; -/// Wire default signing function (sha256 digest) -using sign_fn = std::function; +/// A 32-byte digest tagged with the hash algorithm that produced it. A signing +/// closure receives one of these and can dispatch on the active alternative -- +/// e.g. an Ethereum remote signer expects `keccak256`, block signing `sha256`. +using hash256 = std::variant; + +/// Signing closure stored on every `signature_provider_t`. Takes the digest by +/// value: a `hash256` is two 32-byte hashes in a variant -- trivially cheap to +/// copy -- and the closure owns its copy. +using sign_fn = std::function; + +/// The raw 32 digest bytes of whichever hash `h` carries. For closures that +/// sign an opaque 32-byte digest (AWS KMS, kiod) regardless of hash algorithm. +inline std::span digest_span(const hash256& h) { + return std::visit([](const auto& d) -> std::span { + return d.to_uint8_span(); + }, h); +} + +/// The `sha256` alternative of `h`. Throws if `h` carries a `keccak256`. For +/// closures that feed a SHA-256-only API (`private_key::sign`) and are only +/// ever handed a SHA-256 digest -- the assertion catches a wrong-hash bug. +inline const sha256& as_sha256(const hash256& h) { + const sha256* s = std::get_if(&h); + FC_ASSERT(s, "expected a sha256 digest, but the hash256 carries a keccak256"); + return *s; +} /** * `signature_provider_entry` constructed provider diff --git a/libraries/libfc/include/fc/crypto/signer.hpp b/libraries/libfc/include/fc/crypto/signer.hpp index 54e2b552b1..2e01050346 100644 --- a/libraries/libfc/include/fc/crypto/signer.hpp +++ b/libraries/libfc/include/fc/crypto/signer.hpp @@ -28,10 +28,9 @@ namespace detail { /// /// - A provider with a local `em` private key signs in-process. /// - A provider with no local key -- a remote signer such as AWS KMS -- has no -/// key to sign with directly, so the digest is handed to its `sign` closure. -/// `sign` is typed on `fc::sha256`, but the closure treats that argument as -/// an opaque 32-byte digest, so the keccak digest is carried across -/// byte-for-byte and raw-signed. +/// key to sign with directly, so the digest is handed to its `sign` closure +/// as a `hash256` carrying the `keccak256` alternative; the closure raw-signs +/// those 32 bytes. /// /// Digest preparation (plain keccak vs. EIP-191 framing) is the caller's job -- /// it has already happened in `signer_traits::prepare` -- so the remote signer @@ -52,9 +51,9 @@ inline signature em_sign_keccak(const signature_provider_t& p, const keccak256& FC_ASSERT(static_cast(p.sign), "signature provider has neither a local private key nor a sign function"); - // Carry the keccak digest into the sha256-typed closure argument unchanged. - const auto digest_bytes = digest.to_char_span(); - const signature sig = p.sign(sha256(digest_bytes.data(), digest_bytes.size())); + // Hand the keccak digest to the remote signer's closure, correctly typed as + // the keccak256 alternative of hash256 -- no byte reinterpretation. + const signature sig = p.sign(digest); // Self-verify: the remote signer must have raw-signed `digest` with the // provider's key. Recover and compare; reject anything else loudly. diff --git a/libraries/libfc/test/crypto/test_cypher_suites.cpp b/libraries/libfc/test/crypto/test_cypher_suites.cpp index f002f9f132..220800c644 100644 --- a/libraries/libfc/test/crypto/test_cypher_suites.cpp +++ b/libraries/libfc/test/crypto/test_cypher_suites.cpp @@ -509,7 +509,7 @@ signature_provider_t make_provider(const private_key& key, chain_key_type_t key_ p.key_type = key_type; p.public_key = key.get_public_key(); p.private_key = key; - p.sign = [key](const sha256& d) { return key.sign(d); }; + p.sign = [key](hash256 d) { return key.sign(as_sha256(d)); }; return p; } @@ -665,9 +665,12 @@ BOOST_AUTO_TEST_CASE(test_eth_client_signer_signs_via_closure_without_private_ke remote.key_type = chain_key_type_ethereum; remote.public_key = pub; // remote.private_key intentionally left empty. - remote.sign = [em_key, kc](const sha256&) { + remote.sign = [em_key, kc](hash256 digest) { // A KMS-style raw signer: a recoverable signature over the 32-byte - // digest it is handed (here the known keccak digest of `payload`). + // digest it is handed. The eth signing path now delivers a keccak256- + // tagged hash256 -- verify the typed argument arrives correctly. + BOOST_REQUIRE(std::holds_alternative(digest)); + BOOST_CHECK(std::get(digest) == kc); return signature(signature::storage_type(em_key.sign_keccak256(kc))); }; BOOST_REQUIRE(!remote.private_key.has_value()); @@ -692,7 +695,7 @@ BOOST_AUTO_TEST_CASE(test_eth_client_signer_closure_matches_local_key) try { signature_provider_t remote; remote.key_type = chain_key_type_ethereum; remote.public_key = key.get_public_key(); - remote.sign = [em_key, kc](const sha256&) { + remote.sign = [em_key, kc](hash256) { return signature(signature::storage_type(em_key.sign_keccak256(kc))); }; @@ -715,7 +718,7 @@ BOOST_AUTO_TEST_CASE(test_eth_client_signer_rejects_remote_signature_for_wrong_k signature_provider_t bad; bad.key_type = chain_key_type_ethereum; bad.public_key = key_a.get_public_key(); // provider advertises key A - bad.sign = [em_b, kc](const sha256&) { // but the closure signs with key B + bad.sign = [em_b, kc](hash256) { // but the closure signs with key B return signature(signature::storage_type(em_b.sign_keccak256(kc))); }; @@ -736,7 +739,7 @@ BOOST_AUTO_TEST_CASE(test_wire_eth_signer_signs_via_closure_without_private_key) signature_provider_t remote; remote.key_type = chain_key_type_ethereum; remote.public_key = pub; - remote.sign = [em_key, prepared](const sha256&) { + remote.sign = [em_key, prepared](hash256) { return signature(signature::storage_type(em_key.sign_keccak256(prepared))); }; diff --git a/libraries/testing/include/sysio/testing/bls_utils.hpp b/libraries/testing/include/sysio/testing/bls_utils.hpp index 391a26b625..3248b3c30c 100644 --- a/libraries/testing/include/sysio/testing/bls_utils.hpp +++ b/libraries/testing/include/sysio/testing/bls_utils.hpp @@ -35,8 +35,10 @@ inline std::pairsign(digest); + // Block signing uses a SHA-256 digest; wrap it as the sha256 + // alternative of the provider closure's hash256 argument. + return sig_provider->sign(fc::crypto::hash256{digest}); } else { return chain::signature_type(); } diff --git a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp index db45103179..f595b9e5a8 100644 --- a/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp +++ b/plugins/signature_provider_manager_plugin/src/kms_signature_provider.cpp @@ -610,7 +610,7 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, auto state = std::make_shared( get_kms_client(ref.region), ref.key_id, shim.unwrapped()); - fc::crypto::sign_fn sign = [state](const chain::digest_type& digest) -> chain::signature_type { + fc::crypto::sign_fn sign = [state](fc::crypto::hash256 digest) -> chain::signature_type { // Public-key pinning. Before the first -- and only the first -- billable // Sign, fetch the KMS key's own public key with the free GetPublicKey // API and assert it matches the key pinned in the spec. This turns the @@ -621,14 +621,16 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, // through `ensure_kms_pubkey_pinned`. ensure_kms_pubkey_pinned(*state); + // The Ethereum signing path hands this closure a keccak256 digest; KMS + // signs the opaque 32-byte value regardless of which hash produced it. + const auto digest_bytes = fc::crypto::digest_span(digest); + // Build a Sign request. MessageType=DIGEST tells KMS the 32 bytes are // already a hash; otherwise it would re-hash with SHA-256 and break // any chain that hashes with anything other than SHA-256. Aws::KMS::Model::SignRequest req; req.SetKeyId(Aws::String{state->key_id}); - req.SetMessage(Aws::Utils::ByteBuffer{ - digest.to_uint8_span().data(), - digest.to_uint8_span().size()}); + req.SetMessage(Aws::Utils::ByteBuffer{digest_bytes.data(), digest_bytes.size()}); req.SetMessageType(Aws::KMS::Model::MessageType::DIGEST); req.SetSigningAlgorithm(Aws::KMS::Model::SigningAlgorithmSpec::ECDSA_SHA_256); @@ -642,7 +644,7 @@ kms_signer make_kms_signature_provider(const kms_key_ref& ref, der_buf.GetUnderlyingData(), der_buf.GetLength()}; const auto compact = der_to_eth_signature( - der, digest.to_uint8_span(), state->expected_em_pubkey); + der, digest_bytes, state->expected_em_pubkey); return fc::crypto::signature( fc::crypto::signature::storage_type{fc::em::signature_shim{compact}}); diff --git a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp index ec311c1a70..259e172bac 100644 --- a/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp +++ b/plugins/signature_provider_manager_plugin/src/signature_provider_manager_plugin.cpp @@ -51,7 +51,11 @@ class signature_provider_manager_plugin_impl { bool _kms_startup_check{false}; fc::crypto::sign_fn make_key_signature_provider(const chain::private_key_type& key) const { - return [key](const chain::digest_type& digest) { return key.sign(digest); }; + // KEY: providers (wire / K1 / BLS block signing) always sign a SHA-256 + // digest; as_sha256 asserts that and feeds it to private_key::sign. + return [key](fc::crypto::hash256 digest) { + return key.sign(fc::crypto::as_sha256(digest)); + }; } fc::crypto::sign_fn make_kiod_signature_provider(const string& url_str, const chain::public_key_type& pubkey) const { @@ -64,9 +68,13 @@ class signature_provider_manager_plugin_impl { else kiod_url = fc::url(url_str); - return [to = _kiod_provider_timeout_us, kiod_url, pubkey](const chain::digest_type& digest) { + return [to = _kiod_provider_timeout_us, kiod_url, pubkey](fc::crypto::hash256 digest) { + // The kiod daemon protocol expects a sha256-typed digest. Rebuild one + // from the raw 32 bytes of whichever hash the closure was handed, so a + // kiod-backed Ethereum key (keccak digest) keeps working and the kiod + // wire format stays byte-identical. fc::variant params; - fc::to_variant(std::make_pair(digest, pubkey), params); + fc::to_variant(std::make_pair(as_sha256(digest), pubkey), params); auto deadline = to.count() >= 0 ? fc::time_point::now() + to : fc::time_point::maximum(); return app() .get_plugin() @@ -602,7 +610,8 @@ fc::crypto::signature_provider_ptr signature_provider_manager_plugin::create_pro fc::crypto::sign_fn signature_provider_manager_plugin::create_anonymous_provider_from_private_key(chain::private_key_type priv) const { - return [priv](const fc::sha256& d) { return priv.sign(d); }; + // An anonymous KEY-style provider: signs a SHA-256 digest with a local key. + return [priv](fc::crypto::hash256 d) { return priv.sign(fc::crypto::as_sha256(d)); }; } bool signature_provider_manager_plugin::has_provider(const fc::crypto::signature_provider_id_t& key) {