fix: close signature-verification auth bypass + budget bypasses; swap secp256k1 for coincurve (0.4.0) - #7
Merged
Conversation
- Tagline: "Discover, negotiate, and settle" -> "Discover, request, and settle" (the SDK has no negotiate API; ASA flow is discover -> request -> settle -> attest). Also fixed the same wording in the package docstring. - Protocol section: add kind 38403 (Agent Attestation) alongside 38400/38401/38402. - API Reference: add AgentAttestation model row plus a Reputation methods table (publish_attestation / get_attestations / get_reputation_score, signatures verified against agent/manager.py) and a runnable attest-and-check-reputation example. - Bump version to 0.3.2 for a docs republish; sync __version__ in __init__.py (was stale at 0.3.0 vs pyproject 0.3.1). No tag pushed. Closes #5 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FeVJ1dSozozhHDmNHiPj5f
Four confirmed defects, each fixed at the root rather than the symptom. 1. Auth bypass: NostrEvent.verify() returned True whenever secp256k1 was unimportable. The event ID is a plain SHA-256 over public fields with no secret input, so an attacker computes it offline -- forged capability ads and forged attestations verified under any pubkey. secp256k1 needs a native build and is genuinely not importable in a stock env (pip install fails on this platform), so the branch was live, not theoretical. verify() now raises Secp256k1UnavailableError, consistent with sign() and pubkey_from_private_key(), which already raised on the same condition. Only verify() failed open. The new type subclasses RuntimeError, so existing handlers keep working, while letting callers distinguish an environment fault from an operational one. 2. pay_and_access() ignored max_amount_sats entirely: a client built with max_amount_sats=100 paid a 10,000,000-sat invoice. Root cause was a divergent duplicate of the payment logic; it is now routed through a shared _execute_payment() with an effective max, mirroring the TS SDK, so the two entry points cannot drift apart again. It also gains the per-call max_amount_sats override that access() already had. 3. Budget check was skipped when the amount was unparseable: the guard read None as "no limit applies" and paid. Since the wallet is a caller-supplied callback, that handed an unbounded invoice to arbitrary code. Unknown amount is now refused whenever a limit is configured, matching the invariant the MCP already enforces. With no limit configured the caller has explicitly opted out of budget enforcement, so that path is unchanged. Also fixes the parser that made the guard unsound: the amount pattern was not anchored to the BOLT-11 human-readable part, so an amountless (i.e. unbounded) invoice whose data part contained <digits><multiplier>1 was read as a small amount and passed the budget check. Fixing the None-handling alone would not have caught this, because the parser returned a wrong number rather than None. Amounts are now read only from the HRP (split at the final bech32 separator per BIP-173), computed in integer pico-BTC, and rounded up so a budget check is never handed an under-reported value. Validated against the BOLT-11 spec vectors. 4. Relay events were never verified: discover(), get_attestations() and listen_requests() passed raw relay JSON straight into the models while verify() went uncalled. Relay lists are caller-configurable and merged, so one malicious relay could inject events attributed to any pubkey. Verification is now wired into all three. Failures drop only the offending event, so one bad relay cannot fail an otherwise good query; a missing native dep propagates instead, since silently returning zero results would misreport it as "nothing found". In listen_requests() that error is explicitly excluded from the reconnect handler, which would otherwise swallow it and burn the whole reconnect budget before surfacing a misleading "lost connection". Reputation averaging (get_reputation_score) was audited and is correct -- it already filters to 1..5 before averaging. Left as-is, with a regression test to lock the behaviour in. Tests: 22 new security regression tests, each verified to fail against the pre-fix code. The real BIP-340 path had no coverage at all, so signatures are now exercised with genuine coincurve-signed fixtures (pure wheels, dev-only) covering forged, tampered, reattributed, and wrong-key events. Suite passes both with secp256k1 absent and present: 161 passed. Also fixes the User-Agent, which advertised 0.1.0 on every release since 0.1.0; it now tracks __version__, so server-side telemetry can show upgrade adoption for this release. Version bumped to 0.3.3 with a CHANGELOG entry and a README note.
The previous commit closed a fail-open in verify(), but left the fix
unreachable for a large share of users: secp256k1 is a declared hard
dependency that needs a native build, and `pip install le-agent-sdk` fails
outright on Windows. Shipping it as-is converted "silently insecure" into
"cannot install", which is not a fix so much as a relocation of the problem.
coincurve provides the same BIP-340 Schnorr primitives over the same curve
and ships prebuilt wheels, so the dependency installs everywhere and the bug
stays closed. Verified in a stock venv: `pip install -e .` succeeds with no
toolchain, while `pip install "secp256k1>=0.14.0"` -- what the released 0.3.2
actually declares -- still fails in that same env. CI never caught this
because it is ubuntu-only, where secp256k1 builds fine.
Interop, which is the risk this change carries: signatures are a wire
protocol, and quietly breaking compatibility across the ecosystem would be
worse than the bug being fixed. Agreement is proven in both directions
against implementations sharing no code with coincurve, not just by
round-tripping against itself:
- .NET SDK (NBitcoin.Secp256k1) -> Python: verified, including non-ASCII
and astral-plane content where independently written canonical
serializers realistically diverge.
- Python -> .NET SDK: verified, with forged and reattributed events
confirmed rejected, so the check is discriminating rather than
permissive.
- Python and .NET -> the MCP's verifier, which also agrees on event IDs.
- x-only pubkey derivation agrees for all four keys tried, including
odd-y-parity keys where a parity leak would silently produce events
attributed to a pubkey nobody else computes.
Four .NET-signed events are committed under tests/fixtures/ so the Python
suite proves wire compatibility on every run without a .NET toolchain, and
tests/test_interop.py adds the BIP-340 published vectors to pin the SDK to
the spec rather than to one library's behaviour. Vector 0 initially failed:
the cause was a bad transcription on my side, not the implementation --
confirmed by having NBitcoin deterministically re-sign all four vectors and
emit bytes identical to coincurve's, which is also what keeps the committed
vectors from being circular evidence.
No test was weakened to accommodate the swap. The signature tests previously
shimmed coincurve over the secp256k1 binding, since the runtime dep was
routinely unimportable and the path could not otherwise be covered; the
backends are now the same library, so the shim is deleted and those tests
drive the real production path end to end.
Secp256k1UnavailableError -> CryptoBackendUnavailableError, since the name
now misdescribes the backend. Kept as an alias, and pinned by a test: the
name only ever existed in the unreleased 0.3.3, so nothing published can be
catching it, but the alias costs nothing.
Version: 0.4.0, not 0.3.3. This changes a declared dependency for every
downstream install, and under 0.x the minor is the compatibility signal -- a
patch bump would imply a drop-in change that does not touch the dependency
tree, which is exactly wrong here. Anyone who relied on secp256k1 being
pulled in transitively loses it. 0.3.3 was never published, so its entry is
folded into 0.4.0 rather than leaving a changelog entry for a version that
will never exist on PyPI.
Tests: 188 passed (was 161).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CHANGELOG pointed at /commits/main; this remote has no main branch (only master), so the link 404s — and it is the pointer the CHANGELOG explicitly defers to for pre-0.4.0 history. The interop docstring claimed two of the four vectors have odd y. Only 0b432b26... does. The test is effective today, but a maintainer trimming the vector list on the strength of 'two' could drop the only odd-y key and silently zero the parity coverage — so name the vector and say why it must stay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # pyproject.toml # src/le_agent_sdk/__init__.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auth bypass, live on PyPI right now
nostr/event.pyverify()returnedTruewhensecp256k1was unavailable.This was not a theoretical branch.
secp256k1requires a native build and does not install on Windows — so on every Windows install, the import failed,_HAS_SECP256K1was false, and the fail-open branch was the branch that ran.The ID check that "passed" in that branch is a plain SHA-256 over public event fields with no secret input — an attacker computes it as easily as we do. It authenticates nothing. So the practical result is: forged Nostr events verified as genuine — forged capability advertisements and fabricated reputation/attestations under any pubkey the attacker chooses.
The most damning detail:
sign()andpubkey_from_private_key()already raised on the identical condition. Onlyverifyfailed open — the one caller where the answer is a security decision rather than an error. Verification is now hard-required.Also fixed
pay_and_accessignoredmax_amount_satsentirely, while its docstring promised it would reject invoices above the cap. The budget ceiling was documentation, not code.For upgraders, the headline is the dependency swap
secp256k1→coincurve(prebuilt wheels, no native toolchain).This is not just ergonomics.
pip install "secp256k1>=0.14.0"— exactly what the currently published 0.3.2 declares — hard-fails in a stock Windows venv. Every Windows install of the current release is broken today. The security fix is only real if it installs, so the swap is part of the fix, not a nicety.Version 0.4.0, deliberately not 0.3.3: a dependency change is not a patch. 0.3.3 was never published, so it's folded in here.
Interop is proven, not assumed
Swapping the crypto library on a wire-format-compatible SDK is exactly where silent breakage hides, so this was verified in both directions against the .NET SDK (NBitcoin.Secp256k1) and the MCP verifier — including astral-plane unicode (the serialization edge most likely to diverge) — with forged and re-attributed events correctly rejected.
Four .NET-signed events are committed as fixtures (
tests/fixtures/dotnet_signed_events.json) so CI proves cross-SDK wire compatibility on every run, rather than trusting a one-time manual check.Tests: 161 → 188.
CI gap that let this ship
CI is ubuntu-only — which is precisely why a Windows-only fail-open reached PyPI and stayed there. The bug was invisible to the only platform we test. Recommend adding
windows-latestto the matrix; otherwise the next platform-conditional bug ships the same way.Note for the reviewer
This branch conflicts with
masterinpyproject.tomlandsrc/le_agent_sdk/__init__.py. Both are the same trivial collision:masterreached0.3.2via the squash-merge of #6 while this branch carries its own copy of that bump plus the0.4.0bump. Resolution is to keep0.4.0in both files. No code conflict — left unresolved rather than force-pushing over the branch.🤖 Generated with Claude Code