feat(kmsClient): ECDSA-attested decrypt, environment presets, and creator-bound ECDSA /secrets#109
feat(kmsClient): ECDSA-attested decrypt, environment presets, and creator-bound ECDSA /secrets#109seanmcgary wants to merge 14 commits into
Conversation
|
Claude finished @seanmcgary's task in 3m 31s —— View job Code Review
SummaryThis is a well-structured PR that genuinely closes a real security gap (ECDSA attestation was effectively un-gated before this). The implementation is clean and the spec → plan → TDD approach shows in the test coverage. A few items need attention before merge: Security1. Nil context in 2. Correctness3. 4. Testing5. Missing 502 test for 6. Nits
What's solid
The blocking items are #1 (nil context) and #3 (stat error handling); the others are low-severity improvements. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| signer := ethcrypto.PubkeyToAddress(*pub) | ||
|
|
||
| creator, err := s.node.baseContractCaller.GetAppCreator(common.HexToAddress(appID), nil) |
There was a problem hiding this comment.
[Context propagation] GetLatestReleaseAsRelease (called a few lines below) correctly uses r.Context(), but GetAppCreator here passes nil opts. If the HTTP request is cancelled the on-chain call won't abort. You'd need to thread the context through verifyECDSAOwnership, e.g.:
func (s *Server) verifyECDSAOwnership(ctx context.Context, appID string, publicKey []byte) (int, error) {
...
creator, err := s.node.baseContractCaller.GetAppCreator(
common.HexToAddress(appID),
&bind.CallOpts{Context: ctx},
)Low severity for now (the chain call is fast), but consistent with how the rest of the handler handles context.
| // 0x-prefixed output that `encrypt --output` writes; TrimSpace handles trailing | ||
| // newlines from editors or `echo`. | ||
| func parseEncryptedInput(input string) ([]byte, error) { | ||
| if _, statErr := os.Stat(input); statErr == nil { |
There was a problem hiding this comment.
[Bug - subtle] os.Stat can fail for reasons other than "file does not exist" (e.g. permission denied). In those cases the code silently falls through and tries to interpret input as a hex string — which will produce a confusing "failed to decode hex data" error instead of "permission denied reading file".
Prefer checking errors.Is(statErr, os.ErrNotExist) explicitly:
if _, statErr := os.Stat(input); statErr == nil {
// file path
} else if !errors.Is(statErr, os.ErrNotExist) {
return nil, fmt.Errorf("cannot stat encrypted data path: %w", statErr)
} else {
// hex string path
}
| Usage: "Output file for decrypted data", | ||
| Value: "", | ||
| }, | ||
| &cli.StringFlag{ |
There was a problem hiding this comment.
[Security - Medium] Passing a raw ECDSA private key as a CLI flag (--ecdsa-private-key) exposes the key in:
ps aux//proc/<pid>/cmdlineon Linux- shell history files (bash/zsh)
- system audit logs
--ecdsa-private-key-file is the safer default. Consider adding a Usage warning here, or making the file path the recommended path and the inline hex an explicit "not for production" shortcut:
Usage: "Hex-encoded secp256k1 private key for ECDSA attestation. WARNING: visible in process listings and shell history — prefer --ecdsa-private-key-file for production use.",
| creatorKey, _ := ethcrypto.GenerateKey() | ||
| f.contractCallerStub.SetAppCreator(appAddr, ethcrypto.PubkeyToAddress(creatorKey.PublicKey)) | ||
|
|
||
| wrongKey, err := ethcrypto.GenerateKey() |
There was a problem hiding this comment.
[Missing test coverage] The verifyECDSAOwnership path returns HTTP 502 when GetAppCreator itself errors. There's no test for this branch. Worth adding a subtest that configures the stub to return an error (via a mock or a short-circuit in TestableContractCallerStub) and asserts the 502. Without it, a regression that swallows the error or maps it to the wrong status would go undetected.
| // GetAppCreator returns the configured creator for an app, or the zero address | ||
| // if none was set. It never errors — release lookups and creator lookups are | ||
| // independent in tests. | ||
| func (m *TestableContractCallerStub) GetAppCreator(app common.Address, opts *bind.CallOpts) (common.Address, error) { |
There was a problem hiding this comment.
[Testing note] GetAppCreator here returns (common.Address{}, nil) — the zero address — when no creator has been set. The zero address is a valid common.Address, so calling verifyECDSAOwnership without first calling SetAppCreator will produce a 403 (signer ≠ zero address) rather than a 502. This is acceptable test behaviour, but it's worth documenting that tests that want to exercise the "app not found" → 502 path need a stub that returns an error, not this one.
| var environments = map[string]environment{ | ||
| "sepolia": { | ||
| AVSAddress: "0x47c9806e7DC4e6fE9a0a2399831F32d06DaE5730", | ||
| OperatorSetID: 0, |
There was a problem hiding this comment.
Nit: the source of this address is mentioned only in the PR description ("operator deployment charts"). A short inline comment here would save future maintainers a grep:
// sepolia: canonical AVS deployment, sourced from operator deployment charts.
"sepolia": {
| return data, nil | ||
| } | ||
|
|
||
| fmt.Printf("Using encrypted input %s\n", input) |
There was a problem hiding this comment.
[Noise / potential output pollution] This Printf prints the raw encrypted-data hex string to stdout. When the user passes a large ciphertext inline (which is common — encrypt writes 0x + hex and decrypt consumes the same), this line dumps the entire hex blob to the terminal before the decryption result, which can be multiple kilobytes of noise.
Suggest removing it, or replacing it with a shorter confirmation:
| fmt.Printf("Using encrypted input %s\n", input) | |
| fmt.Println("Treating encrypted-data as hex string (not a file path)") |
If you want to keep the full value, gate it on a --verbose flag so it's opt-in. As written it will fire for every non-file invocation.
| } | ||
| signer := ethcrypto.PubkeyToAddress(*pub) | ||
|
|
||
| creator, err := s.node.baseContractCaller.GetAppCreator(common.HexToAddress(appID), nil) |
There was a problem hiding this comment.
[Confirmed: context propagation] Confirming the existing review comment: GetAppCreator is passed nil opts here while GetLatestReleaseAsRelease (called just below) correctly uses r.Context(). If the HTTP request is cancelled between the two calls, the chain call won't abort. Since verifyECDSAOwnership is a pure helper with no access to the request, the cleanest fix is adding a ctx context.Context parameter:
func (s *Server) verifyECDSAOwnership(ctx context.Context, appID string, publicKey []byte) (int, error) {
...
creator, err := s.node.baseContractCaller.GetAppCreator(
common.HexToAddress(appID),
&bind.CallOpts{Context: ctx},
)Then the call site becomes s.verifyECDSAOwnership(r.Context(), req.AppID, claims.PublicKey). Fix this →
| req := kmsTypes.SecretsRequestV1{ | ||
| AppID: appID, | ||
| AttestationMethod: "ecdsa", | ||
| Attestation: []byte("sig-placeholder"), // StubECDSAMethod ignores it |
There was a problem hiding this comment.
[Testing clarity] The postECDSASecrets helper omits the Challenge field. This is fine because newTestSecretsFixture wires in NewStubManager() (which uses StubECDSAMethod), and StubECDSAMethod.Verify() ignores the challenge entirely — it just echoes request.PublicKey back into claims.PublicKey. So the unit tests here correctly exercise the handler's ownership-binding logic in isolation.
The tradeoff is that no unit test in pkg/node exercises the challenge expiry / replay path for ECDSA; that coverage comes from the integration tests in internal/tests/integration which use the real ECDSAAttestationMethod. Consider adding a brief comment here so a future reader doesn't wonder why Challenge is missing:
// No Challenge field — StubECDSAMethod bypasses real ECDSA verification;
// challenge expiry and replay are covered by integration tests.
| // Resolution: | ||
| // - avs-address: explicit flag → preset → else error. | ||
| // - operator-set-id: explicit flag → preset → else 0. | ||
| func resolveConnection(envName, avsFlag string, avsSet bool, setIDFlag uint32, setIDSet bool) (string, uint32, error) { |
There was a problem hiding this comment.
[Minor: preset AVS address not validated] resolveConnection propagates the preset's AVSAddress directly without checking whether it's a valid hex address. Since the value is hardcoded today this is low risk, but if someone adds a malformed entry to environments the error will surface later during the RPC call with a less helpful message than "invalid AVS address in preset".
Consider a cheap guard when a preset is used:
if havePreset && !common.IsHexAddress(avsAddress) {
return "", 0, fmt.Errorf("preset %q has invalid avs-address %q", envName, avsAddress)
}This doesn't affect the explicit-flag path (the user controls their own input) and is essentially free for the hardcoded preset path.
1 participant
Overview
This PR makes the
kms-clientCLI usable with ECDSA attestation, adds a named-environment shortcut so connection flags don't have to be retyped, and — most importantly — fixes a security gap in how operators authorize ECDSA/secretsrequests.It bundles three related changes (each developed spec → plan → TDD, docs under
docs/superpowers/):decrypt(cmd/kmsClient)--environmentconnection presets (cmd/kmsClient)/secretsto the app creator + drop the release requirement (pkg/node)1. ECDSA-attested decrypt (client)
The CLI's
decryptpreviously only used the unauthenticated/app/signendpoint. It now optionally authenticates with ECDSA challenge-response attestation against the/secretsendpoint.New flags on
decrypt:--attestation—""(default, legacy/app/sign) orecdsa. Any other value is a usage error.--ecdsa-private-key— hex-encoded secp256k1 key (optional0xprefix). Takes priority over the file flag.--ecdsa-private-key-file— path to a file holding the hex key.When
--attestation ecdsais set, the CLI loads the key, generates an ephemeral RSA transit keypair, calls the library's existingRetrieveSecretsWithOptionsto recover the app private key, then decrypts the supplied ciphertext viacrypto.DecryptForApp. When unset, behavior is byte-for-byte unchanged. This is a thin wiring layer — no library (pkg/clients/kmsClient) changes.2.
--environmentconnection presets (client)New global flag
--environment/-efills--avs-addressand--operator-set-idfrom a named preset, so they don't need to be passed every invocation.sepolia→avs-address=0x47c9806e7DC4e6fE9a0a2399831F32d06DaE5730,operator-set-id=0(sourced from the operator deployment charts).--avs-addresslost its hardRequired: trueand is now required only when no preset supplies it.--rpc-url.resolveConnection) and runs at the very top ofcreateClient, so a config error (e.g. missing avs-address) fails fast without needing a reachable RPC.3. Server: ECDSA
/secretsbinds to the app creator (security fix)The gap: ECDSA attestation authenticated nothing app-specific. The operator verified the request signature against the client-supplied public key, but never tied that key to the app. Because an
appIDis an app contract address with no private key, "prove you control a key" was satisfied by any freshly generated keypair — so anyone could request any app's key material over the ECDSA path. (In practice it failed for unrelated reasons: ECDSA setsImageDigest="ecdsa:unverified", which never matches a real release, so every ECDSA request against a real app 404'd or 403'd.)The fix (
pkg/node/handlers.go, ECDSA path ofhandleSecretsRequest):claims.PublicKeyand require it to equal the app's on-chain creator (GetAppCreator(appID), already on the contract-caller interface). The supplied ECDSA key must be the EOA that deployed/created the app. TheappIDmust be a valid contract address.encrypted_env/public_envare returned; otherwise empty env is returned alongside the recovered key. The partial signature is always returned on success.HTTP statuses on the ECDSA path: non-address appID → 400; unparseable pubkey → 400;
GetAppCreatorfailure → 502; signer ≠ creator → 403; success → 200.pkg/attestation/ecdsa.gois intentionally untouched — it already surfaces the verified public key.Testing
cmd/kmsClient—TestLoadECDSAKey(8 cases: file/flag precedence,0xprefix, whitespace, errors) andTestResolveConnection/TestSupportedEnvironmentsString(preset fill, explicit override, unknown env, missing-avs error).pkg/contractCaller—TestTestableStubGetAppCreatorfor the new configurable-creator test hook.pkg/node— 6 new ECDSA/secretssubtests: owner+env, owner+no-release (→ 200 empty env, the no-404 fix), owner+empty-env-release, wrong-signer (403), non-address appID (400), bad pubkey (400) — plus aNonECDSAStillRequiresReleaseregression guard proving non-ECDSA enforcement is intact.All suites green locally:
pkg/node,pkg/contractCaller,cmd/kmsClientallok. The fullpkg/nodesuite (including the anvil-backed persistence tests) passes.Docs
cmd/kmsClient/README.mddocuments the attestation flags, the--environmentpreset (incl. the RPC-secret rationale), and the ECDSA security model — notably that the key must be the app creator's, and that the attested path no longer needs a release. Design specs and implementation plans live underdocs/superpowers/.🤖 Generated with Claude Code