From f72572276f888f91e37322210fd932821266edbb Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 3 Jul 2026 14:50:29 -0400 Subject: [PATCH 1/4] docs: document attestation/reputation API, fix tagline; bump to 0.3.2 - 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 Claude-Session: https://claude.ai/code/session_01FeVJ1dSozozhHDmNHiPj5f --- README.md | 45 ++++++++++++++++++++++++++++++++++-- pyproject.toml | 2 +- src/le_agent_sdk/__init__.py | 4 ++-- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a560c13..7288f5f 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ Python SDK for Lightning Enable Agent Service Agreements. -Discover, negotiate, and settle agent-to-agent services over Nostr with L402 Lightning payments. +Discover, request, and settle agent-to-agent services over Nostr with L402 Lightning payments. ## Installation @@ -100,6 +100,7 @@ asyncio.run(main()) | `AgentCapability` | Defines a service offering with pricing, categories, endpoints, and metadata. Published as Nostr kind 38400 events. | | `AgentServiceRequest` | Represents a request for service from one agent to another (kind 38401). | | `AgentServiceAgreement` | Bilateral contract between provider and requester (kind 38402). | +| `AgentAttestation` | Post-completion review of an agent (kind 38403): rating 1-5, review text, optional payment proof. The building block for on-protocol reputation. | ### Nostr Layer @@ -116,13 +117,53 @@ asyncio.run(main()) | `L402Client` | HTTP client with automatic L402 challenge-response handling. Wraps [l402-requests](https://github.com/refined-element/l402-requests). | | `AgentPricing` | Pricing model (amount, unit, per-request/per-token). | +### Reputation (`AgentManager` methods) + +| Method | Description | +|--------|-------------| +| `publish_attestation(subject_pubkey, agreement_id, rating, content="", proof=None)` | Publish a review of an agent after a completed agreement (kind 38403). `rating` must be 1-5; `proof` is an optional hash of the L402 payment preimage. Returns the published `AgentAttestation`. | +| `get_attestations(pubkey, limit=20, timeout=5.0)` | Query relays for attestations about an agent. Returns `list[AgentAttestation]`. | +| `get_reputation_score(pubkey, limit=50, timeout=5.0)` | Average rating (1.0-5.0) computed from attestations, or `None` if the agent has no attestations yet. | + +#### Example: Attest and Check Reputation + +```python +import asyncio +from le_agent_sdk import AgentManager + +async def main(): + manager = AgentManager( + private_key="", + relay_urls=["wss://agents.lightningenable.com"], + ) + + # After a completed service agreement, publish a review + attestation = await manager.publish_attestation( + subject_pubkey="", + agreement_id="", + rating=5, + content="Fast, accurate translation. Would hire again.", + ) + print(f"Published attestation: {attestation.event_id}") + + # Before hiring an agent, check its track record + score = await manager.get_reputation_score("") + if score is None: + print("No attestations yet") + else: + print(f"Reputation: {score:.1f}/5.0") + +asyncio.run(main()) +``` + ## Protocol -Agent Service Agreements use three Nostr event kinds: +Agent Service Agreements use four Nostr event kinds: - **38400** -- Agent Capability: provider advertises available services - **38401** -- Agent Service Request: requester asks for a service - **38402** -- Agent Service Agreement: bilateral contract with terms and pricing +- **38403** -- Agent Attestation: post-completion review (rating 1-5) that builds on-protocol reputation Settlement happens via L402 (Lightning HTTP 402) through Lightning Enable endpoints. diff --git a/pyproject.toml b/pyproject.toml index 1f9d99e..51056f7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "le-agent-sdk" -version = "0.3.1" +version = "0.3.2" description = "Python SDK for Lightning Enable Agent Service Agreements" requires-python = ">=3.10" license = "MIT" diff --git a/src/le_agent_sdk/__init__.py b/src/le_agent_sdk/__init__.py index a9bc3dd..946a700 100644 --- a/src/le_agent_sdk/__init__.py +++ b/src/le_agent_sdk/__init__.py @@ -1,4 +1,4 @@ -"""Lightning Enable Agent SDK — discover, negotiate, and settle Agent Service Agreements.""" +"""Lightning Enable Agent SDK — discover, request, and settle Agent Service Agreements.""" from le_agent_sdk.models.capability import AgentCapability, AgentPricing from le_agent_sdk.models.request import AgentServiceRequest @@ -24,4 +24,4 @@ "AgentManager", ] -__version__ = "0.3.0" +__version__ = "0.3.2" From 5aed44c29566e1ee8b0cc1c58502f41058357140 Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 17 Jul 2026 00:49:00 -0400 Subject: [PATCH 2/4] fix: close signature-verification fail-open and two budget bypasses 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 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. --- CHANGELOG.md | 62 ++++ README.md | 6 + pyproject.toml | 6 +- src/le_agent_sdk/__init__.py | 5 +- src/le_agent_sdk/agent/manager.py | 55 +++- src/le_agent_sdk/l402/client.py | 292 +++++++++++------ src/le_agent_sdk/nostr/event.py | 44 ++- tests/test_agent_manager.py | 11 +- tests/test_security_regressions.py | 503 +++++++++++++++++++++++++++++ 9 files changed, 877 insertions(+), 107 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 tests/test_security_regressions.py diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..9f9fa7f --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,62 @@ +# Changelog + +All notable changes to `le-agent-sdk` are documented here. + +This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +Entries begin at 0.3.3; for earlier history see the +[commit log](https://github.com/refined-element/le-agent-sdk-python/commits/main). + +## [0.3.3] - 2026-07-17 + +### Fixed + +- Fixes signature verification silently passing when secp256k1 is unavailable — upgrade recommended. + `NostrEvent.verify()` returned `True` for any event whose ID matched when the + secp256k1 native library could not be imported. The event ID is a plain SHA-256 + over public fields, so it is attacker-computable and proves nothing about + authenticity — forged capability advertisements and forged attestations under any + pubkey were accepted. Verification now raises `Secp256k1UnavailableError` instead + of passing. secp256k1 requires a native build and is not importable in some + environments where installation otherwise appears to succeed, so this affected + real deployments rather than only misconfigured ones. +- Fixes `pay_and_access()` ignoring `max_amount_sats`, and budget checks being + skipped for invoices whose amount could not be read — both allowed payments + above the configured limit; upgrade recommended. + `pay_and_access()` never consulted the limit at all, so a client constructed with + `max_amount_sats=100` would pay a 10,000,000-sat invoice. Separately, an invoice + whose amount could not be determined was treated as "no limit applies" and paid. + An amount that cannot be determined is now refused whenever a limit is configured. +- Fixes the BOLT-11 amount parser reading an amount from the invoice data part. + The pattern was not anchored to the human-readable part, so an amountless (i.e. + unbounded) invoice whose data happened to contain `1` was + reported as a small amount and passed the budget check. Amounts are now read only + from the human-readable part, and rounded up rather than truncated so a budget + check is never given an under-reported value. +- Incoming relay events are now signature-verified before use. `discover()`, + `get_attestations()` and `listen_requests()` passed raw relay JSON straight into + the models. Relay lists are caller-configurable and results are merged across + relays, so a single malicious relay could inject events attributed to any pubkey. + Events failing verification are dropped and logged; other relays' results are + unaffected. +- The `User-Agent` sent by `L402ProducerClient` reported `0.1.0` on every release + since 0.1.0. It now tracks the package version. + +### Added + +- `Secp256k1UnavailableError`, exported from the package root. Subclasses + `RuntimeError`, so existing `except RuntimeError` handlers continue to work. + +### Changed + +- `pay_and_access()` accepts a `max_amount_sats` argument to override the + instance-level limit for a single call, matching `access()`. + +### Upgrade notes + +- If secp256k1 is not importable in your environment, verification now raises where + it previously returned `True`. Any code path that reads events from relays is + affected. This is intentional: the previous result was not a weaker check, it was + no check. Installing secp256k1 restores normal operation. +- Callers relying on unknown-amount invoices being paid while `max_amount_sats` is + set will now see `ValueError`. Either set no limit (explicitly opting out of + budget enforcement) or use invoices with an explicit amount. diff --git a/README.md b/README.md index 7288f5f..8113bf6 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,12 @@ Discover, request, and settle agent-to-agent services over Nostr with L402 Light pip install le-agent-sdk ``` +> **0.3.3 fixes signature verification silently passing when secp256k1 is unavailable, plus two payment-budget bypasses. Upgrading is recommended** — see the [changelog](CHANGELOG.md). + +`secp256k1` requires a native build. If it is not importable, the operations that +need it — signing, key derivation, and signature verification — raise +`Secp256k1UnavailableError` rather than degrading to a weaker check. + ## Quick Start ### Provider: Publish a Service diff --git a/pyproject.toml b/pyproject.toml index 51056f7..9e5d79d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "le-agent-sdk" -version = "0.3.2" +version = "0.3.3" description = "Python SDK for Lightning Enable Agent Service Agreements" requires-python = ">=3.10" license = "MIT" @@ -34,6 +34,10 @@ dependencies = [ dev = [ "pytest>=7.0", "pytest-asyncio>=0.21", + # Pure-wheel BIP-340 impl, used only to sign real fixtures in the signature + # tests. secp256k1 (the runtime dep) needs a native build that is often + # unavailable, which would otherwise leave the signature path untested. + "coincurve>=18.0", ] [project.urls] diff --git a/src/le_agent_sdk/__init__.py b/src/le_agent_sdk/__init__.py index 946a700..6886adc 100644 --- a/src/le_agent_sdk/__init__.py +++ b/src/le_agent_sdk/__init__.py @@ -4,7 +4,7 @@ from le_agent_sdk.models.request import AgentServiceRequest from le_agent_sdk.models.agreement import AgentServiceAgreement from le_agent_sdk.models.attestation import AgentAttestation -from le_agent_sdk.nostr.event import NostrEvent +from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError from le_agent_sdk.nostr.relay import RelayClient from le_agent_sdk.nostr.tags import TagParser from le_agent_sdk.l402.client import L402Client, L402ProducerClient @@ -17,6 +17,7 @@ "AgentServiceAgreement", "AgentAttestation", "NostrEvent", + "Secp256k1UnavailableError", "RelayClient", "TagParser", "L402Client", @@ -24,4 +25,4 @@ "AgentManager", ] -__version__ = "0.3.2" +__version__ = "0.3.3" diff --git a/src/le_agent_sdk/agent/manager.py b/src/le_agent_sdk/agent/manager.py index b576205..98a1811 100644 --- a/src/le_agent_sdk/agent/manager.py +++ b/src/le_agent_sdk/agent/manager.py @@ -7,6 +7,7 @@ from __future__ import annotations import asyncio +import logging from typing import Any, AsyncIterator, Optional from le_agent_sdk.l402.client import L402Client, L402ProducerClient @@ -14,10 +15,12 @@ from le_agent_sdk.models.attestation import AgentAttestation from le_agent_sdk.models.capability import AgentCapability from le_agent_sdk.models.request import AgentServiceRequest -from le_agent_sdk.nostr.event import NostrEvent +from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError from le_agent_sdk.nostr.relay import RelayClient from le_agent_sdk.nostr.tags import TagParser +logger = logging.getLogger(__name__) + class AgentManager: """Main entry point for agent operations. @@ -64,6 +67,39 @@ def pubkey(self) -> str: self._pubkey = NostrEvent.pubkey_from_private_key(self.private_key) return self._pubkey + @staticmethod + def _is_event_authentic(event: dict[str, Any]) -> bool: + """Check a relay-supplied event's signature before trusting its contents. + + Relay URLs are caller-configurable and results are merged across relays, + so without this a single malicious or compromised relay could inject + events attributed to any pubkey — forged capability ads, forged + attestations inflating an agent's reputation. + + A verification failure drops only the offending event: one bad relay in + the pool must not be able to fail an otherwise good query. A RuntimeError + (secp256k1 unavailable) is left to propagate — that is an environment + fault affecting every event, and silently returning zero results would + misrepresent it as "nothing found". + """ + if NostrEvent.verify(event): + return True + + logger.warning( + "Dropping Nostr event %.16s... (kind=%s) from pubkey %.16s...: " + "signature verification failed. The relay may be malicious or " + "misbehaving.", + event.get("id", ""), + event.get("kind", ""), + event.get("pubkey", ""), + ) + return False + + @classmethod + def _filter_authentic(cls, events: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Keep only events with a valid signature under their claimed pubkey.""" + return [event for event in events if cls._is_event_authentic(event)] + async def _publish_to_relays(self, event: dict[str, Any]) -> str: """Publish an event to all configured relays. @@ -183,7 +219,10 @@ async def discover( ) raw_events = await self._query_relays([nostr_filter], timeout=timeout) - return [AgentCapability.from_nostr_event(e) for e in raw_events] + return [ + AgentCapability.from_nostr_event(e) + for e in self._filter_authentic(raw_events) + ] async def publish_capability(self, capability: AgentCapability) -> str: """Publish a capability advertisement to relays. @@ -296,7 +335,14 @@ async def listen_requests( if event_id and event_id not in seen_ids: seen_ids.add(event_id) reconnect_attempts = 0 # Reset on successful message + if not self._is_event_authentic(event_data): + continue yield AgentServiceRequest.from_nostr_event(event_data) + except Secp256k1UnavailableError: + # Environment fault, not a relay fault: reconnecting cannot + # fix a missing native dependency. Surface it immediately + # instead of burning the reconnect budget on it. + raise except Exception: reconnect_attempts += 1 if reconnect_attempts > max_reconnect_attempts: @@ -521,7 +567,10 @@ async def get_attestations( ) raw_events = await self._query_relays([nostr_filter], timeout=timeout) - return [AgentAttestation.from_nostr_event(e) for e in raw_events] + return [ + AgentAttestation.from_nostr_event(e) + for e in self._filter_authentic(raw_events) + ] async def get_reputation_score( self, diff --git a/src/le_agent_sdk/l402/client.py b/src/le_agent_sdk/l402/client.py index 3dad462..b7d3f63 100644 --- a/src/le_agent_sdk/l402/client.py +++ b/src/le_agent_sdk/l402/client.py @@ -22,6 +22,26 @@ logger = logging.getLogger(__name__) +def _sdk_version() -> str: + """Version string for the User-Agent header. + + Read from the package __version__ rather than hardcoded, so it cannot drift + from the real version (it previously advertised 0.1.0 from a 0.3.x release, + which made server-side version telemetry misleading). Imported lazily to + avoid a circular import: the package __init__ imports this module. + + __version__ is used in preference to importlib.metadata because it reflects + the code actually running, which is the point of the header; installed + distribution metadata can be stale or absent in a source checkout. + """ + try: + from le_agent_sdk import __version__ + + return __version__ + except Exception: # pragma: no cover - defensive: UA must never break a request + return "unknown" + + @dataclass(frozen=True) class L402Challenge: """Parsed L402 challenge from a WWW-Authenticate header.""" @@ -80,6 +100,25 @@ class MppChallenge: re.IGNORECASE, ) +# BOLT-11 human-readable part: "ln" + currency prefix + optional amount. +# Anchored with a terminating $ so it only ever matches a complete HRP — +# never a fragment of the bech32 data part. Longer currency prefixes are +# listed first because Python's alternation takes the first match. +_BOLT11_HRP_RE = re.compile( + r"^ln(?:bcrt|bc|tbs|tb|sb)(?P\d+)?(?P[munp])?$", + re.IGNORECASE, +) + +# Amount multipliers expressed in pico-BTC, so amounts stay exact integers. +# 1 BTC = 10^12 pico-BTC = 10^8 sats, therefore 1 sat = 10^4 pico-BTC. +_PICO_BTC_MULTIPLIERS = { + "m": 10**9, # milli-BTC + "u": 10**6, # micro-BTC + "n": 10**3, # nano-BTC + "p": 1, # pico-BTC + "": 10**12, # no multiplier => whole BTC +} + def parse_l402_challenge(headers: dict[str, str]) -> Optional[L402Challenge]: """Extract an L402 challenge from response headers. @@ -230,34 +269,49 @@ def _ensure_client(self) -> httpx.AsyncClient: def _decode_invoice_amount_sats(invoice: str) -> Optional[int]: """Extract the amount in satoshis from a BOLT-11 invoice string. - Returns None if the amount cannot be parsed. + Returns: + The amount in satoshis, rounded UP to the next whole sat, or None if + the invoice encodes no amount or cannot be parsed. None means + "amount unknown" — it never means "no amount limit applies". Callers + enforcing a budget MUST refuse a None (see _check_amount_against_max). + + Security: + The amount is read only from the human-readable part (HRP), which is + everything before the final bech32 separator. Scanning the whole + string would let the amount be matched from inside the data part of + an amountless (i.e. unbounded) invoice, reporting a small bogus + amount that passes a budget check. """ - # BOLT-11: starts with "ln" then network (bc/tb/etc), then optional amount - # Amount is encoded as: where multipliers are: - # m=milli, u=micro, n=nano, p=pico (of BTC) - inv_lower = invoice.lower() - # Strip "lightning:" prefix if present + inv_lower = invoice.lower().strip() + # Strip "lightning:" URI prefix if present if inv_lower.startswith("lightning:"): inv_lower = inv_lower[10:] - match = re.match(r"ln\w+?(\d+)([munp])1", inv_lower) + # Per BIP-173 the separator is the LAST "1" in the string: the bech32 + # data charset excludes "1", so any earlier "1" belongs to the HRP. + separator = inv_lower.rfind("1") + if separator < 0: + return None + hrp = inv_lower[:separator] + + # HRP grammar: "ln" + currency prefix + optional (amount + multiplier). + # Longer prefixes must precede their own prefixes in the alternation. + match = _BOLT11_HRP_RE.match(hrp) if not match: return None - amount_num = int(match.group(1)) - multiplier = match.group(2) - # Convert to satoshis (1 BTC = 100_000_000 sats) - multiplier_map = { - "m": 100_000_00, # milli-BTC = 100,000 sats (0.001 BTC) - "u": 100_00, # micro-BTC = 100 sats (0.000001 BTC) - "n": 0.01, # nano-BTC = 0.01 sats - "p": 0.00001, # pico-BTC = 0.00001 sats - } - # milli = 10^-3 BTC = 10^5 sats - btc_multipliers = {"m": 1e-3, "u": 1e-6, "n": 1e-9, "p": 1e-12} - btc_amount = amount_num * btc_multipliers[multiplier] - sats = int(btc_amount * 1e8) - return sats + amount_digits = match.group("amount") + if not amount_digits: + return None # amountless invoice: payer chooses => unknown + + # Integer-only math in pico-BTC; floats would round a budget-critical + # value in the unsafe direction. + amount_pico = int(amount_digits) * _PICO_BTC_MULTIPLIERS[match.group("multiplier") or ""] + if amount_pico <= 0: + return None + + # 1 sat = 10_000 pico-BTC. Round UP: never under-report to a budget check. + return -(-amount_pico // 10_000) @staticmethod def _validate_preimage(preimage: str) -> bool: @@ -270,6 +324,110 @@ def _validate_preimage(preimage: str) -> bool: except ValueError: return False + def _check_amount_against_max( + self, + challenge: L402Challenge | MppChallenge, + effective_max: Optional[int], + ) -> None: + """Enforce the payment ceiling before any invoice reaches the wallet. + + The invariant is: an amount that cannot be determined is refused. + A budget is a guarantee ("never pay more than N"), and an invoice whose + amount cannot be proven <= N cannot be paid without breaking it. The + wallet callback is arbitrary caller-supplied code, so handing it an + unbounded invoice delegates an unbounded spend. + + No ceiling configured (None) means the caller explicitly opted out of + budget enforcement, so any invoice — known or unknown — is allowed + through; refusing there would add no safety and break the documented + "None means no limit" contract. + + Raises: + ValueError: If the invoice exceeds the ceiling, or if a ceiling is + configured and the amount cannot be determined. + """ + if effective_max is None: + return + + invoice_sats = self._decode_invoice_amount_sats(challenge.invoice) + + if invoice_sats is None: + raise ValueError( + "Invoice amount could not be determined, and a maximum of " + f"{effective_max} sats is configured. Refusing to pay: an " + "invoice with no verifiable amount cannot be checked against a " + "budget and would hand an unbounded payment to the wallet " + f"callback. Invoice: {challenge.invoice[:40]}..." + ) + + if invoice_sats > effective_max: + raise ValueError( + f"Invoice amount ({invoice_sats} sats) exceeds maximum allowed " + f"({effective_max} sats). Invoice: {challenge.invoice[:40]}..." + ) + + async def _execute_payment( + self, + challenge: L402Challenge | MppChallenge, + pay_invoice_callback: Any, + effective_max: Optional[int], + url: str, + ) -> str: + """Check the budget, pay the invoice, and validate the preimage. + + The single payment path shared by access() and pay_and_access() so the + budget ceiling can never apply to one entry point but not the other. + + Returns: + The validated preimage. + + Raises: + ValueError: If the amount is over budget/undeterminable, or the + callback returns a malformed preimage. + RuntimeError: If the payment callback itself fails. + """ + # Budget gate FIRST: nothing reaches the wallet before this passes. + self._check_amount_against_max(challenge, effective_max) + + try: + preimage = await pay_invoice_callback(challenge.invoice) + except Exception as exc: + logger.error( + "Error in pay_invoice_callback for URL %r: %s", url, exc, exc_info=True + ) + raise RuntimeError(f"Payment callback failed: {exc}") from exc + + if not self._validate_preimage(preimage): + logger.error( + "Invalid preimage returned from pay callback: expected 64-char hex, " + "got %r (length=%d)", + preimage[:20] if isinstance(preimage, str) else type(preimage), + len(preimage) if isinstance(preimage, str) else 0, + ) + raise ValueError( + f"Invalid preimage from payment callback: expected 64-character hex string, " + f"got length {len(preimage) if isinstance(preimage, str) else 'N/A'}" + ) + + # Only L402 credentials are cacheable (keyed by macaroon); MPP has none. + if isinstance(challenge, L402Challenge): + self._cache[challenge.macaroon] = preimage + + protocol = "MPP" if isinstance(challenge, MppChallenge) else "L402" + logger.info("%s payment succeeded for %s", protocol, url) + logger.debug("%s preimage (first 8 chars): %.8s...", protocol, preimage) + + return preimage + + @staticmethod + def _build_auth_header( + challenge: L402Challenge | MppChallenge, preimage: str + ) -> str: + """Build the Authorization header value for a paid challenge.""" + if isinstance(challenge, MppChallenge): + return f'Payment method="lightning", preimage="{preimage}"' + return f"L402 {challenge.macaroon}:{preimage}" + async def access( self, url: str, @@ -324,49 +482,14 @@ async def access( retry_response = await client.request(method, url, headers=headers, **kwargs) return retry_response - # Check invoice amount against limit + # Budget check, payment, and preimage validation (shared path) effective_max = max_amount_sats if max_amount_sats is not None else self._max_amount_sats - if effective_max is not None: - invoice_sats = self._decode_invoice_amount_sats(challenge.invoice) - if invoice_sats is not None and invoice_sats > effective_max: - raise ValueError( - f"Invoice amount ({invoice_sats} sats) exceeds maximum allowed " - f"({effective_max} sats). Invoice: {challenge.invoice[:40]}..." - ) - - # Pay the invoice with error handling on the callback - try: - preimage = await self._pay_callback(challenge.invoice) - except Exception as exc: - logger.error("pay_invoice_callback failed: %s", exc) - raise RuntimeError(f"Payment callback failed: {exc}") from exc - - # Validate preimage format - if not self._validate_preimage(preimage): - logger.error( - "Invalid preimage returned from pay callback: expected 64-char hex, " - "got %r (length=%d)", - preimage[:20] if isinstance(preimage, str) else type(preimage), - len(preimage) if isinstance(preimage, str) else 0, - ) - raise ValueError( - f"Invalid preimage from payment callback: expected 64-character hex string, " - f"got length {len(preimage) if isinstance(preimage, str) else 'N/A'}" - ) - - # Build the correct Authorization header based on challenge type - if isinstance(challenge, MppChallenge): - auth_header = f'Payment method="lightning", preimage="{preimage}"' - logger.info("MPP payment succeeded for %s", url) - logger.debug("MPP preimage (first 8 chars): %.8s...", preimage) - else: - self._cache[challenge.macaroon] = preimage - auth_header = f"L402 {challenge.macaroon}:{preimage}" - logger.info("L402 payment succeeded for %s", url) - logger.debug("L402 preimage (first 8 chars): %.8s...", preimage) + preimage = await self._execute_payment( + challenge, self._pay_callback, effective_max, url + ) # Retry the request with credentials, with retry+backoff - headers["Authorization"] = auth_header + headers["Authorization"] = self._build_auth_header(challenge, preimage) max_retries = 3 last_exc: Optional[Exception] = None @@ -401,6 +524,7 @@ async def pay_and_access( pay_invoice_callback: Any, method: str = "GET", headers: Optional[dict[str, str]] = None, + max_amount_sats: Optional[int] = None, **kwargs: Any, ) -> httpx.Response: """Full L402 flow: request, get 402, pay invoice, retry with token. @@ -410,10 +534,16 @@ async def pay_and_access( pay_invoice_callback: Async callable(invoice: str) -> preimage: str. method: HTTP method. headers: Optional request headers. + max_amount_sats: Override max payment amount for this request. + Falls back to the instance-level max_amount_sats. **kwargs: Additional httpx request kwargs. Returns: The final HTTP response after payment. + + Raises: + ValueError: If the invoice amount exceeds max_amount_sats, or if a + limit is configured and the amount cannot be determined. """ headers = dict(headers or {}) client = self._ensure_client() @@ -438,39 +568,13 @@ async def pay_and_access( retry_response = await client.request(method, url, headers=headers, **kwargs) return retry_response - try: - preimage = await pay_invoice_callback(challenge.invoice) - except Exception as exc: - logger.error( - "Error in pay_invoice_callback during pay_and_access for URL %r: %s", - url, - exc, - exc_info=True, - ) - raise RuntimeError( - "Payment callback failed during pay_and_access; see logs for details" - ) from exc - - # Validate preimage format before constructing credentials - if not self._validate_preimage(preimage): - logger.error( - "Invalid preimage returned from pay callback in pay_and_access: " - "expected 64-char hex, got %r (length=%d)", - preimage[:20] if isinstance(preimage, str) else type(preimage), - len(preimage) if isinstance(preimage, str) else 0, - ) - raise ValueError( - f"Invalid preimage from payment callback: expected 64-character hex string, " - f"got length {len(preimage) if isinstance(preimage, str) else 'N/A'}" - ) - - # Build the correct Authorization header based on challenge type - if isinstance(challenge, MppChallenge): - headers["Authorization"] = f'Payment method="lightning", preimage="{preimage}"' - else: - self._cache[challenge.macaroon] = preimage - headers["Authorization"] = f"L402 {challenge.macaroon}:{preimage}" + # Budget check, payment, and preimage validation (shared path) + effective_max = max_amount_sats if max_amount_sats is not None else self._max_amount_sats + preimage = await self._execute_payment( + challenge, pay_invoice_callback, effective_max, url + ) + headers["Authorization"] = self._build_auth_header(challenge, preimage) retry_response = await client.request(method, url, headers=headers, **kwargs) return retry_response @@ -542,7 +646,7 @@ def _ensure_client(self) -> httpx.AsyncClient: "X-Api-Key": self._api_key, "Content-Type": "application/json", "Accept": "application/json", - "User-Agent": "LE-Agent-SDK-Python/0.1.0", + "User-Agent": f"LE-Agent-SDK-Python/{_sdk_version()}", } self._client = httpx.AsyncClient(headers=headers, **self._httpx_kwargs) return self._client diff --git a/src/le_agent_sdk/nostr/event.py b/src/le_agent_sdk/nostr/event.py index a14000d..9286236 100644 --- a/src/le_agent_sdk/nostr/event.py +++ b/src/le_agent_sdk/nostr/event.py @@ -11,7 +11,13 @@ import time from typing import Any, Optional -# Signing is optional — if secp256k1 is not installed, events are created unsigned. +# secp256k1 is a hard dependency, but it requires a native build and can be +# absent from an otherwise "successful" install. Import defensively so that +# import-time failure is deferred to the operations that actually need it. +# +# Every operation that depends on it — sign(), pubkey_from_private_key() and +# verify() — raises RuntimeError when it is missing. None of them degrade to a +# weaker check. Only building/serializing unsigned events works without it. try: import secp256k1 @@ -20,6 +26,16 @@ _HAS_SECP256K1 = False +class Secp256k1UnavailableError(RuntimeError): + """Raised when an operation needs secp256k1 but it could not be imported. + + Subclasses RuntimeError so existing `except RuntimeError` handlers keep + working. Distinguishable so that callers can tell this environment fault + apart from an operational error (e.g. a relay disconnect) and avoid + retrying something that will never succeed. + """ + + class NostrEvent: """Builds and signs Nostr events per NIP-01.""" @@ -57,7 +73,7 @@ def pubkey_from_private_key(private_key_hex: str) -> str: 32-byte x-only public key as hex string. """ if not _HAS_SECP256K1: - raise RuntimeError( + raise Secp256k1UnavailableError( "secp256k1 library is required for key derivation. " "Install with: pip install secp256k1" ) @@ -80,7 +96,7 @@ def sign(event_id_hex: str, private_key_hex: str) -> str: 64-byte signature as hex string. """ if not _HAS_SECP256K1: - raise RuntimeError( + raise Secp256k1UnavailableError( "secp256k1 library is required for signing. " "Install with: pip install secp256k1" ) @@ -99,8 +115,20 @@ def sign(event_id_hex: str, private_key_hex: str) -> str: def verify(event: dict[str, Any]) -> bool: """Verify a Nostr event's ID and signature. + The ID alone is NOT authentication: it is a plain SHA-256 over public + fields with no secret input, so anyone can compute a matching ID for an + event they forged. Authenticity comes solely from the BIP-340 signature, + which requires secp256k1. + Returns: - True if valid, False otherwise. + True only if the ID matches AND the signature is a valid BIP-340 + signature over that ID under the claimed pubkey. False otherwise. + + Raises: + Secp256k1UnavailableError: If secp256k1 is unavailable, so the + signature cannot be checked. This fails closed and loudly, + consistent with sign() and pubkey_from_private_key(). It is a + RuntimeError subclass and never silently passes. """ # Verify ID computed_id = NostrEvent.compute_id(event) @@ -108,8 +136,12 @@ def verify(event: dict[str, Any]) -> bool: return False if not _HAS_SECP256K1: - # Cannot verify signature without secp256k1; only ID was checked - return True + raise Secp256k1UnavailableError( + "secp256k1 library is required for signature verification. " + "Refusing to treat the event as verified: the event ID is a " + "plain hash of public fields and proves nothing about " + "authenticity. Install with: pip install secp256k1" + ) pubkey_hex = event.get("pubkey", "") sig_hex = event.get("sig", "") diff --git a/tests/test_agent_manager.py b/tests/test_agent_manager.py index 9381b48..0899c13 100644 --- a/tests/test_agent_manager.py +++ b/tests/test_agent_manager.py @@ -8,6 +8,7 @@ from le_agent_sdk.agent.manager import AgentManager from le_agent_sdk.models.agreement import AgentServiceAgreement from le_agent_sdk.models.capability import AgentCapability, AgentPricing +from le_agent_sdk.nostr.event import NostrEvent class TestAgentManagerInit: @@ -32,6 +33,13 @@ def test_pubkey_without_private_key_raises(self): class TestAgentManagerDiscover: @pytest.mark.asyncio async def test_discover_returns_capabilities(self): + """Authentic events are parsed into capabilities. + + discover() verifies signatures before parsing, so verification is stubbed + to isolate the parsing behaviour under test. Signing real fixtures here + would require the secp256k1 native build. The drop-on-forgery path is + covered in tests/test_security_regressions.py. + """ sample_events = [ { "id": "ev1", @@ -56,7 +64,8 @@ async def test_discover_returns_capabilities(self): mgr = AgentManager() with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: mock_query.return_value = sample_events - caps = await mgr.discover(categories=["ai"]) + with patch.object(NostrEvent, "verify", return_value=True): + caps = await mgr.discover(categories=["ai"]) assert len(caps) == 2 assert caps[0].service_id == "svc-a" diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py new file mode 100644 index 0000000..e9b1a29 --- /dev/null +++ b/tests/test_security_regressions.py @@ -0,0 +1,503 @@ +"""Security regression tests for fail-open / budget-bypass defects. + +Each test in this module corresponds to a confirmed vulnerability. They are +written to fail against the pre-fix code and pass after the fix. + +Covered: + 1. NostrEvent.verify() fail-open when secp256k1 is unavailable. + 2. L402Client.pay_and_access() ignoring max_amount_sats. + 3. Budget check skipped when the invoice amount is unparseable. + 4. Reputation scoring ignoring out-of-range ratings (already correct — locked in). + 5. AgentManager trusting unverified relay events. +""" + +from unittest.mock import AsyncMock, patch + +import pytest + +from le_agent_sdk.agent.manager import AgentManager +from le_agent_sdk.l402.client import L402Client +from le_agent_sdk.models.attestation import AgentAttestation +from le_agent_sdk.nostr import event as event_module +from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError + +# --- Fixtures / helpers ----------------------------------------------------- + +# A well-formed event whose `id` is a genuine SHA-256 of its public fields but +# whose `sig` is garbage. An attacker can compute `id` offline (no secret input), +# so ID-only validation is not authentication. +def _forged_event(pubkey: str = "de" * 32) -> dict: + event = { + "pubkey": pubkey, + "created_at": 1700000000, + "kind": 38400, + "tags": [["d", "svc-a"], ["s", "ai"]], + "content": "Totally legit service", + } + event["id"] = NostrEvent.compute_id(event) # attacker-computable + event["sig"] = "00" * 64 # not a valid BIP-340 signature + return event + + +class TestVerifyFailsClosedWithoutSecp256k1: + """Finding 1: verify() returned True when secp256k1 was unimportable.""" + + def test_verify_raises_when_secp256k1_unavailable(self): + """Missing native dep must be loud, not a silent pass. + + Consistent with sign()/pubkey_from_private_key(), which already raise. + """ + with patch.object(event_module, "_HAS_SECP256K1", False): + with pytest.raises(RuntimeError, match="secp256k1"): + NostrEvent.verify(_forged_event()) + + def test_forged_event_does_not_verify_as_true(self): + """A forged event must never come back as verified. + + Regardless of whether secp256k1 is installed, the one outcome that must + be impossible is a `True` return for an event with a bogus signature. + """ + with patch.object(event_module, "_HAS_SECP256K1", False): + try: + result = NostrEvent.verify(_forged_event()) + except RuntimeError: + return # fail-closed: acceptable + assert result is not True, "forged event verified as authentic" + + def test_verify_still_rejects_id_mismatch_before_dep_check(self): + """A tampered ID is rejected without needing the native dep.""" + event = _forged_event() + event["content"] = "tampered after id was computed" + with patch.object(event_module, "_HAS_SECP256K1", False): + assert NostrEvent.verify(event) is False + + +class TestVerifyAcceptsGenuineRejectsForged: + """The signature path must discriminate, not just refuse everything. + + Fixing a fail-open bug by failing everything closed would be no fix. These + tests exercise the real BIP-340 path with genuine signatures. + + secp256k1 needs a native build and is frequently unimportable, so signatures + are produced with coincurve (pure wheels) and the small slice of the + secp256k1 API that verify() uses is shimmed over it. The crypto under test + is real; only the binding is substituted. + """ + + @staticmethod + def _install_shim(monkeypatch): + coincurve = pytest.importorskip( + "coincurve", reason="needs a BIP-340 impl to sign real fixtures" + ) + + class _ShimPublicKey: + def __init__(self, data, raw=True): + # verify() prepends a 0x02 prefix; x-only key is the remainder. + self._xonly = coincurve.PublicKeyXOnly(data[1:]) + + def schnorr_verify(self, msg, sig, bip340tag=b"", raw=True): + return self._xonly.verify(sig, msg) + + class _ShimSecp256k1: + PublicKey = _ShimPublicKey + + monkeypatch.setattr(event_module, "secp256k1", _ShimSecp256k1, raising=False) + monkeypatch.setattr(event_module, "_HAS_SECP256K1", True) + return coincurve + + @staticmethod + def _sign(coincurve, private_hex: str, content: str = "genuine") -> dict: + priv = coincurve.PrivateKey(bytes.fromhex(private_hex)) + xonly_pubkey = priv.public_key.format(compressed=True)[1:] + event = { + "pubkey": xonly_pubkey.hex(), + "created_at": 1700000000, + "kind": 38400, + "tags": [["d", "svc-a"]], + "content": content, + } + event["id"] = NostrEvent.compute_id(event) + event["sig"] = priv.sign_schnorr(bytes.fromhex(event["id"])).hex() + return event + + def test_genuine_signed_event_verifies(self, monkeypatch): + coincurve = self._install_shim(monkeypatch) + event = self._sign(coincurve, "01" * 32) + assert NostrEvent.verify(event) is True + + def test_forged_signature_rejected(self, monkeypatch): + coincurve = self._install_shim(monkeypatch) + event = self._sign(coincurve, "01" * 32) + event["sig"] = "00" * 64 + assert NostrEvent.verify(event) is False + + def test_tampered_content_rejected(self, monkeypatch): + coincurve = self._install_shim(monkeypatch) + event = self._sign(coincurve, "01" * 32) + event["content"] = "tampered" + assert NostrEvent.verify(event) is False + + def test_event_reattributed_to_another_pubkey_rejected(self, monkeypatch): + """The core attack: swap the pubkey and recompute a valid ID. + + The ID check alone passes here — it is just a hash of public fields. + Only the signature check stops it. + """ + coincurve = self._install_shim(monkeypatch) + event = self._sign(coincurve, "01" * 32) + + victim = coincurve.PrivateKey(bytes.fromhex("02" * 32)) + event["pubkey"] = victim.public_key.format(compressed=True)[1:].hex() + event["id"] = NostrEvent.compute_id(event) # ID now matches again + + assert NostrEvent.compute_id(event) == event["id"], "ID check would pass" + assert NostrEvent.verify(event) is False, "reattributed event was accepted" + + def test_signature_from_different_key_rejected(self, monkeypatch): + coincurve = self._install_shim(monkeypatch) + event = self._sign(coincurve, "01" * 32) + attacker = coincurve.PrivateKey(bytes.fromhex("03" * 32)) + event["sig"] = attacker.sign_schnorr(bytes.fromhex(event["id"])).hex() + assert NostrEvent.verify(event) is False + + +class TestPayAndAccessRespectsMaxAmount: + """Finding 2: pay_and_access never read self._max_amount_sats.""" + + @pytest.mark.asyncio + async def test_pay_and_access_rejects_invoice_over_instance_limit(self): + # 10,000,000 sats == 100m (milli-BTC) invoice, limit is 100 sats. + expensive = "lnbc100m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rq" + paid = [] + + async def pay_callback(invoice): + paid.append(invoice) + return "ab" * 32 + + client = L402Client(max_amount_sats=100) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.return_value = _make_402(expensive) + mock_ensure.return_value = mock_http + + with pytest.raises(ValueError, match="exceeds maximum"): + await client.pay_and_access("https://x.test/r", pay_callback) + + assert paid == [], "wallet callback was invoked for an over-budget invoice" + + @pytest.mark.asyncio + async def test_pay_and_access_allows_invoice_under_limit(self): + cheap = "lnbc10u1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rq" # 1000 sats + + async def pay_callback(invoice): + return "ab" * 32 + + client = L402Client(max_amount_sats=5000) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.side_effect = [_make_402(cheap), _make_ok()] + mock_ensure.return_value = mock_http + + resp = await client.pay_and_access("https://x.test/r", pay_callback) + + assert resp.status_code == 200 + + @pytest.mark.asyncio + async def test_pay_and_access_per_call_override_beats_instance_limit(self): + cheap = "lnbc10u1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rq" # 1000 sats + + async def pay_callback(invoice): + return "ab" * 32 + + client = L402Client(max_amount_sats=100_000) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.return_value = _make_402(cheap) + mock_ensure.return_value = mock_http + + with pytest.raises(ValueError, match="exceeds maximum"): + await client.pay_and_access( + "https://x.test/r", pay_callback, max_amount_sats=10 + ) + + +class TestUnknownAmountIsRefused: + """Finding 3: unparseable amount was read as 'no limit applies'.""" + + def test_amountless_invoice_decodes_to_none(self): + amountless = "lnbc1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypq" + assert L402Client._decode_invoice_amount_sats(amountless) is None + + def test_amountless_invoice_is_not_misparsed_from_data_part(self): + """The HRP amount must not be matched from inside the bech32 data part. + + Pre-fix the regex was unanchored, so an amountless (unbounded) invoice + whose data contained '1' reported a small bogus amount and + sailed through the budget check. + """ + assert L402Client._decode_invoice_amount_sats("lnbc1pabc9u1def") is None + assert L402Client._decode_invoice_amount_sats("lnbc1pvjl5p1uez") is None + + @pytest.mark.asyncio + async def test_access_refuses_unparseable_invoice_when_budget_set(self): + garbage = "not-a-parseable-bolt11-invoice" + paid = [] + + async def pay_callback(invoice): + paid.append(invoice) + return "ab" * 32 + + client = L402Client(pay_invoice_callback=pay_callback, max_amount_sats=1000) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.return_value = _make_402(garbage) + mock_ensure.return_value = mock_http + + with pytest.raises(ValueError, match="amount"): + await client.access("https://x.test/r") + + assert paid == [], "unbounded invoice was handed to the wallet callback" + + @pytest.mark.asyncio + async def test_pay_and_access_refuses_unparseable_invoice_when_budget_set(self): + garbage = "not-a-parseable-bolt11-invoice" + paid = [] + + async def pay_callback(invoice): + paid.append(invoice) + return "ab" * 32 + + client = L402Client(max_amount_sats=1000) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.return_value = _make_402(garbage) + mock_ensure.return_value = mock_http + + with pytest.raises(ValueError, match="amount"): + await client.pay_and_access("https://x.test/r", pay_callback) + + assert paid == [] + + @pytest.mark.asyncio + async def test_unparseable_invoice_still_paid_when_no_budget_configured(self): + """No budget configured == caller explicitly accepted unbounded payment. + + Refusing here would break the documented `None means no limit` contract + without adding safety: with no limit, a known 10M-sat invoice is paid too. + """ + garbage = "not-a-parseable-bolt11-invoice" + paid = [] + + async def pay_callback(invoice): + paid.append(invoice) + return "ab" * 32 + + client = L402Client(pay_invoice_callback=pay_callback, max_amount_sats=None) + + with patch.object(client, "_ensure_client") as mock_ensure: + mock_http = AsyncMock() + mock_http.request.side_effect = [_make_402(garbage), _make_ok()] + mock_ensure.return_value = mock_http + + resp = await client.access("https://x.test/r") + + assert resp.status_code == 200 + assert paid == [garbage] + + +class TestReputationIgnoresOutOfRangeRatings: + """Finding 4: verified as already-correct. Locked in against regression.""" + + @pytest.mark.asyncio + async def test_out_of_range_ratings_excluded_from_average(self): + attestations = [ + AgentAttestation(rating=5), + AgentAttestation(rating=5), + AgentAttestation(rating=9999), # forged/out-of-range + AgentAttestation(rating=0), # unparsed/missing + AgentAttestation(rating=-5), + ] + mgr = AgentManager() + with patch.object(mgr, "get_attestations", new_callable=AsyncMock) as mock_get: + mock_get.return_value = attestations + score = await mgr.get_reputation_score("pub") + + assert score == 5.0 + + @pytest.mark.asyncio + async def test_no_valid_ratings_returns_none(self): + mgr = AgentManager() + with patch.object(mgr, "get_attestations", new_callable=AsyncMock) as mock_get: + mock_get.return_value = [AgentAttestation(rating=0)] + score = await mgr.get_reputation_score("pub") + + assert score is None + + +class TestManagerVerifiesRelayEvents: + """Finding 5: raw relay JSON flowed into models with no verification.""" + + @pytest.mark.asyncio + async def test_discover_drops_events_failing_verification(self): + mgr = AgentManager() + good, bad = _forged_event("aa" * 32), _forged_event("bb" * 32) + + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [good, bad] + with patch.object( + NostrEvent, "verify", side_effect=lambda e: e["pubkey"] == "aa" * 32 + ): + caps = await mgr.discover() + + assert len(caps) == 1, "unverified event was not dropped" + assert caps[0].pubkey == "aa" * 32 + + @pytest.mark.asyncio + async def test_get_attestations_drops_events_failing_verification(self): + mgr = AgentManager() + good, bad = _forged_event("aa" * 32), _forged_event("bb" * 32) + + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [good, bad] + with patch.object( + NostrEvent, "verify", side_effect=lambda e: e["pubkey"] == "aa" * 32 + ): + atts = await mgr.get_attestations("aa" * 32) + + assert len(atts) == 1 + assert atts[0].pubkey == "aa" * 32 + + @pytest.mark.asyncio + async def test_discover_drops_all_when_every_event_is_forged(self): + """One malicious relay must not be able to inject attributed events.""" + mgr = AgentManager() + + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [_forged_event(), _forged_event("cc" * 32)] + with patch.object(NostrEvent, "verify", return_value=False): + caps = await mgr.discover() + + assert caps == [] + + @pytest.mark.asyncio + async def test_verification_error_propagates(self): + """A missing native dep must surface, not silently empty the results.""" + mgr = AgentManager() + + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [_forged_event()] + with patch.object( + NostrEvent, "verify", side_effect=RuntimeError("secp256k1 required") + ): + with pytest.raises(RuntimeError, match="secp256k1"): + await mgr.discover() + + @pytest.mark.asyncio + async def test_listen_requests_drops_forged_events(self): + """The streaming ingestion point must verify too, not just the queries.""" + good = _forged_event("aa" * 32) + good["kind"] = 38401 + bad = _forged_event("bb" * 32) + bad["kind"] = 38401 + + mgr = AgentManager(private_key="01" * 32) + received = [] + + # The forged event is streamed FIRST: without verification it is what the + # caller receives, so ordering is what gives this test its teeth. + with patch.object( + type(mgr), "pubkey", property(lambda self: "aa" * 32) + ), patch.object(NostrEvent, "verify", side_effect=lambda e: e["pubkey"] == "aa" * 32): + with patch( + "le_agent_sdk.agent.manager.RelayClient", _FakeRelayClient([bad, good]) + ): + async for req in mgr.listen_requests(): + received.append(req) + if len(received) >= 1: + break + + assert len(received) == 1 + assert received[0].pubkey == "aa" * 32, "forged request was yielded to caller" + + @pytest.mark.asyncio + async def test_listen_requests_surfaces_missing_dep_without_reconnect_storm(self): + """A missing dep must not be mistaken for a relay fault and retried.""" + event = _forged_event("aa" * 32) + event["kind"] = 38401 + + mgr = AgentManager(private_key="01" * 32) + fake_relay_cls = _FakeRelayClient([event]) + + with patch.object( + type(mgr), "pubkey", property(lambda self: "aa" * 32) + ), patch.object( + NostrEvent, "verify", side_effect=Secp256k1UnavailableError("secp256k1 missing") + ): + with patch("le_agent_sdk.agent.manager.RelayClient", fake_relay_cls): + with pytest.raises(Secp256k1UnavailableError): + async for _ in mgr.listen_requests(): + pass + + # Exactly one connect: the error escaped instead of driving reconnects. + assert fake_relay_cls.connect_count == 1, ( + f"missing dep triggered {fake_relay_cls.connect_count} connects " + "(reconnect storm) instead of surfacing immediately" + ) + + +# --- Relay double ----------------------------------------------------------- + + +def _FakeRelayClient(events: list[dict]): + """Build a RelayClient stand-in class that streams `events` once. + + Returns a class (not an instance) because the manager constructs its own + RelayClient objects. Connects are counted on the class so a reconnect storm + is observable. + """ + + class _Fake: + connect_count = 0 + + async def connect(self, url): + type(self).connect_count += 1 + + async def subscribe(self, filters): + return None + + async def listen(self): + for event in events: + yield "EVENT", ("sub-id", event) + # Stream ends: modelled as a disconnect, which is what a real relay + # closing the socket looks like to the manager. + raise ConnectionError("relay closed the stream") + + async def close(self): + return None + + return _Fake + + +# --- HTTP response doubles -------------------------------------------------- + + +def _make_402(invoice: str): + """Build a fake 402 response carrying an L402 challenge.""" + resp = AsyncMock() + resp.status_code = 402 + resp.headers = { + "WWW-Authenticate": f'L402 macaroon="AGIAJEem", invoice="{invoice}"' + } + return resp + + +def _make_ok(): + resp = AsyncMock() + resp.status_code = 200 + resp.headers = {} + return resp From b3182aa01d16d9403b547ee572ceaf78402a97ef Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 17 Jul 2026 01:13:34 -0400 Subject: [PATCH 3/4] fix: swap secp256k1 for coincurve so the security fix is installable 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) --- CHANGELOG.md | 66 ++++-- README.md | 21 +- examples/demo_full_loop.py | 14 +- pyproject.toml | 11 +- src/le_agent_sdk/__init__.py | 11 +- src/le_agent_sdk/agent/manager.py | 8 +- src/le_agent_sdk/nostr/event.py | 94 +++++---- tests/fixtures/dotnet_signed_events.json | 82 ++++++++ tests/test_agent_manager.py | 2 +- tests/test_interop.py | 257 +++++++++++++++++++++++ tests/test_security_regressions.py | 159 +++++++------- 11 files changed, 573 insertions(+), 152 deletions(-) create mode 100644 tests/fixtures/dotnet_signed_events.json create mode 100644 tests/test_interop.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f9fa7f..6430b63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,22 +3,48 @@ All notable changes to `le-agent-sdk` are documented here. This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -Entries begin at 0.3.3; for earlier history see the +Entries begin at 0.4.0; for earlier history see the [commit log](https://github.com/refined-element/le-agent-sdk-python/commits/main). -## [0.3.3] - 2026-07-17 +## [0.4.0] - 2026-07-17 + +Security release. Fixes signature verification silently passing, two payment-budget +bypasses, and unverified relay events — **upgrading is recommended**. + +This release also replaces the package's crypto dependency, which is why it is a +minor bump rather than a patch: see [Dependencies](#dependencies) below. + +### Dependencies + +- **Replaces the `secp256k1` dependency with `coincurve`, which ships prebuilt + wheels.** `secp256k1` required a native build (libsecp256k1 plus a C toolchain) + and failed to install on Windows entirely; even where a build was possible it + frequently produced an install where `import secp256k1` failed, which is what the + verification bug below turned into a silent security hole. `coincurve` provides + the same BIP-340 Schnorr primitives over the same curve with no build step. + + For most users this is transparent — `pip install le-agent-sdk` simply starts + working where it previously did not. Two things to be aware of: + + - If your project imported `secp256k1` itself and relied on this package to pull + it in, it is no longer installed transitively. Declare it directly. + - The signature wire format is unchanged. Events signed by 0.3.x verify under + 0.4.0 and vice versa; the curve, key encoding, and BIP-340 semantics are + identical, only the binding differs. This is covered by cross-implementation + tests against the .NET SDK and the BIP-340 published vectors. ### Fixed -- Fixes signature verification silently passing when secp256k1 is unavailable — upgrade recommended. +- Fixes signature verification silently passing when the crypto backend is + unavailable — upgrade recommended. `NostrEvent.verify()` returned `True` for any event whose ID matched when the - secp256k1 native library could not be imported. The event ID is a plain SHA-256 + native secp256k1 library could not be imported. The event ID is a plain SHA-256 over public fields, so it is attacker-computable and proves nothing about authenticity — forged capability advertisements and forged attestations under any - pubkey were accepted. Verification now raises `Secp256k1UnavailableError` instead - of passing. secp256k1 requires a native build and is not importable in some - environments where installation otherwise appears to succeed, so this affected - real deployments rather than only misconfigured ones. + pubkey were accepted. Verification now raises `CryptoBackendUnavailableError` + instead of passing. Because the old dependency could not be installed at all on + some platforms, this affected real deployments rather than only misconfigured + ones — and the dependency swap above removes the condition for nearly all of them. - Fixes `pay_and_access()` ignoring `max_amount_sats`, and budget checks being skipped for invoices whose amount could not be read — both allowed payments above the configured limit; upgrade recommended. @@ -43,8 +69,12 @@ Entries begin at 0.3.3; for earlier history see the ### Added -- `Secp256k1UnavailableError`, exported from the package root. Subclasses +- `CryptoBackendUnavailableError`, exported from the package root. Subclasses `RuntimeError`, so existing `except RuntimeError` handlers continue to work. + `Secp256k1UnavailableError` is kept as an alias of it. +- Cross-implementation wire-compatibility tests: events signed by the .NET SDK + (via NBitcoin.Secp256k1) are committed as fixtures and verified on every run, + alongside the BIP-340 published test vectors. ### Changed @@ -53,10 +83,20 @@ Entries begin at 0.3.3; for earlier history see the ### Upgrade notes -- If secp256k1 is not importable in your environment, verification now raises where - it previously returned `True`. Any code path that reads events from relays is - affected. This is intentional: the previous result was not a weaker check, it was - no check. Installing secp256k1 restores normal operation. +- `pip install le-agent-sdk` no longer needs a C toolchain. If you previously + installed build dependencies (libsecp256k1, build-essential, Visual C++ Build + Tools) solely for this package, they are no longer required. +- If the crypto backend is not importable in your environment, verification now + raises where it previously returned `True`. Any code path that reads events from + relays is affected. This is intentional: the previous result was not a weaker + check, it was no check. - Callers relying on unknown-amount invoices being paid while `max_amount_sats` is set will now see `ValueError`. Either set no limit (explicitly opting out of budget enforcement) or use invoices with an explicit amount. + +### Note on 0.3.3 + +An earlier cut of this work was staged as 0.3.3 and was never published to PyPI. +Its contents are released here as 0.4.0; no 0.3.3 artifact exists. The +`Secp256k1UnavailableError` name originated in that unreleased cut, so no released +version ever exported it — it is aliased anyway for anyone tracking the branch. diff --git a/README.md b/README.md index 8113bf6..a1f0c52 100644 --- a/README.md +++ b/README.md @@ -18,11 +18,22 @@ Discover, request, and settle agent-to-agent services over Nostr with L402 Light pip install le-agent-sdk ``` -> **0.3.3 fixes signature verification silently passing when secp256k1 is unavailable, plus two payment-budget bypasses. Upgrading is recommended** — see the [changelog](CHANGELOG.md). - -`secp256k1` requires a native build. If it is not importable, the operations that -need it — signing, key derivation, and signature verification — raise -`Secp256k1UnavailableError` rather than degrading to a weaker check. +> **0.4.0 fixes signature verification silently passing when the crypto backend is +> unavailable, plus two payment-budget bypasses. Upgrading is recommended** — see +> the [changelog](CHANGELOG.md). + +> **0.4.0 replaces the `secp256k1` dependency with [`coincurve`](https://pypi.org/project/coincurve/).** +> `secp256k1` required a native build and could not be installed on Windows at all; +> `coincurve` ships prebuilt wheels, so installation no longer needs a C toolchain. +> The signature wire format is unchanged — events signed by 0.3.x still verify, +> which is covered by cross-implementation tests against the .NET SDK and the +> BIP-340 published vectors. If your own code imported `secp256k1` and relied on +> this package to pull it in, declare it directly. + +Signing, key derivation, and signature verification all need the crypto backend. +If it is not importable they raise `CryptoBackendUnavailableError` (aliased as +`Secp256k1UnavailableError`) rather than degrading to a weaker check — in +particular, verification never reports an unverifiable event as authentic. ## Quick Start diff --git a/examples/demo_full_loop.py b/examples/demo_full_loop.py index 2a3adc4..d3ff92a 100644 --- a/examples/demo_full_loop.py +++ b/examples/demo_full_loop.py @@ -65,7 +65,7 @@ def generate_throwaway_privkey() -> str: def mock_pubkey_from_privkey(privkey_hex: str) -> str: """Derive a deterministic mock 'pubkey' from a privkey using SHA-256. - This is NOT a real Nostr pubkey derivation (which requires secp256k1). + This is NOT a real Nostr pubkey derivation (which requires coincurve). It is used only in --mock mode to produce consistent, deterministic IDs. """ return hashlib.sha256(bytes.fromhex(privkey_hex)).hexdigest() @@ -109,7 +109,7 @@ def mock_create_event( # --------------------------------------------------------------------------- async def run_mock_demo() -> None: - """Run the full ASA loop with mock data -- no relay, no secp256k1 needed.""" + """Run the full ASA loop with mock data -- no relay, no crypto backend needed.""" print("\n" + "#" * 70) print("# NOSTRWOLFE E2E DEMO -- MOCK MODE") @@ -373,12 +373,12 @@ async def run_live_demo(relay_url: str) -> None: print(f"# Relay: {relay_url}") print("#" + "#" * 69 + "\n") - # Check for secp256k1 + # Check for the BIP-340 crypto backend try: - from le_agent_sdk.nostr.event import _HAS_SECP256K1 - if not _HAS_SECP256K1: - print("WARNING: secp256k1 not installed. Events will be unsigned.") - print("Install with: pip install secp256k1\n") + from le_agent_sdk.nostr.event import _HAS_COINCURVE + if not _HAS_COINCURVE: + print("WARNING: coincurve not installed. Events will be unsigned.") + print("Install with: pip install coincurve\n") except ImportError: pass diff --git a/pyproject.toml b/pyproject.toml index 9e5d79d..61d26ef 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "le-agent-sdk" -version = "0.3.3" +version = "0.4.0" description = "Python SDK for Lightning Enable Agent Service Agreements" requires-python = ">=3.10" license = "MIT" @@ -26,7 +26,10 @@ classifiers = [ dependencies = [ "httpx>=0.24.0", "websockets>=11.0", - "secp256k1>=0.14.0", + # BIP-340 Schnorr over secp256k1. coincurve ships prebuilt wheels for the + # platforms we support; the `secp256k1` package it replaced in 0.4.0 required + # a native build toolchain and could not be installed on Windows at all. + "coincurve>=18.0", "l402-requests>=0.1.0", ] @@ -34,10 +37,6 @@ dependencies = [ dev = [ "pytest>=7.0", "pytest-asyncio>=0.21", - # Pure-wheel BIP-340 impl, used only to sign real fixtures in the signature - # tests. secp256k1 (the runtime dep) needs a native build that is often - # unavailable, which would otherwise leave the signature path untested. - "coincurve>=18.0", ] [project.urls] diff --git a/src/le_agent_sdk/__init__.py b/src/le_agent_sdk/__init__.py index 6886adc..f89ff41 100644 --- a/src/le_agent_sdk/__init__.py +++ b/src/le_agent_sdk/__init__.py @@ -4,7 +4,11 @@ from le_agent_sdk.models.request import AgentServiceRequest from le_agent_sdk.models.agreement import AgentServiceAgreement from le_agent_sdk.models.attestation import AgentAttestation -from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError +from le_agent_sdk.nostr.event import ( + CryptoBackendUnavailableError, + NostrEvent, + Secp256k1UnavailableError, +) from le_agent_sdk.nostr.relay import RelayClient from le_agent_sdk.nostr.tags import TagParser from le_agent_sdk.l402.client import L402Client, L402ProducerClient @@ -17,6 +21,9 @@ "AgentServiceAgreement", "AgentAttestation", "NostrEvent", + "CryptoBackendUnavailableError", + # Pre-0.4.0 alias of CryptoBackendUnavailableError, kept for callers + # catching it by name. "Secp256k1UnavailableError", "RelayClient", "TagParser", @@ -25,4 +32,4 @@ "AgentManager", ] -__version__ = "0.3.3" +__version__ = "0.4.0" diff --git a/src/le_agent_sdk/agent/manager.py b/src/le_agent_sdk/agent/manager.py index 98a1811..15ea7b7 100644 --- a/src/le_agent_sdk/agent/manager.py +++ b/src/le_agent_sdk/agent/manager.py @@ -15,7 +15,7 @@ from le_agent_sdk.models.attestation import AgentAttestation from le_agent_sdk.models.capability import AgentCapability from le_agent_sdk.models.request import AgentServiceRequest -from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError +from le_agent_sdk.nostr.event import CryptoBackendUnavailableError, NostrEvent from le_agent_sdk.nostr.relay import RelayClient from le_agent_sdk.nostr.tags import TagParser @@ -78,7 +78,7 @@ def _is_event_authentic(event: dict[str, Any]) -> bool: A verification failure drops only the offending event: one bad relay in the pool must not be able to fail an otherwise good query. A RuntimeError - (secp256k1 unavailable) is left to propagate — that is an environment + (crypto backend unavailable) is left to propagate — that is an environment fault affecting every event, and silently returning zero results would misrepresent it as "nothing found". """ @@ -338,9 +338,9 @@ async def listen_requests( if not self._is_event_authentic(event_data): continue yield AgentServiceRequest.from_nostr_event(event_data) - except Secp256k1UnavailableError: + except CryptoBackendUnavailableError: # Environment fault, not a relay fault: reconnecting cannot - # fix a missing native dependency. Surface it immediately + # fix a missing crypto backend. Surface it immediately # instead of burning the reconnect budget on it. raise except Exception: diff --git a/src/le_agent_sdk/nostr/event.py b/src/le_agent_sdk/nostr/event.py index 9286236..105a36f 100644 --- a/src/le_agent_sdk/nostr/event.py +++ b/src/le_agent_sdk/nostr/event.py @@ -1,7 +1,7 @@ """Nostr event builder and signing (NIP-01). Handles event creation, ID computation (SHA-256 of canonical serialization), -and Schnorr signing (BIP-340) via the secp256k1 library. +and Schnorr signing (BIP-340) via the coincurve library. """ from __future__ import annotations @@ -11,23 +11,25 @@ import time from typing import Any, Optional -# secp256k1 is a hard dependency, but it requires a native build and can be -# absent from an otherwise "successful" install. Import defensively so that -# import-time failure is deferred to the operations that actually need it. +# coincurve provides BIP-340 Schnorr over secp256k1 and ships prebuilt wheels +# for every platform we support, so a normal `pip install` is sufficient. # -# Every operation that depends on it — sign(), pubkey_from_private_key() and -# verify() — raises RuntimeError when it is missing. None of them degrade to a -# weaker check. Only building/serializing unsigned events works without it. +# The import is still guarded. A wheel can be missing for an unusual +# platform/interpreter combination, and an install can be pruned after the fact, +# so the failure mode has to be defined rather than incidental. Every operation +# that depends on it — sign(), pubkey_from_private_key() and verify() — raises +# when it is missing. None of them degrade to a weaker check. Only +# building/serializing unsigned events works without it. try: - import secp256k1 + import coincurve - _HAS_SECP256K1 = True + _HAS_COINCURVE = True except ImportError: - _HAS_SECP256K1 = False + _HAS_COINCURVE = False -class Secp256k1UnavailableError(RuntimeError): - """Raised when an operation needs secp256k1 but it could not be imported. +class CryptoBackendUnavailableError(RuntimeError): + """Raised when an operation needs the BIP-340 backend but it is unimportable. Subclasses RuntimeError so existing `except RuntimeError` handlers keep working. Distinguishable so that callers can tell this environment fault @@ -36,6 +38,18 @@ class Secp256k1UnavailableError(RuntimeError): """ +# The backend moved from `secp256k1` to `coincurve` in 0.4.0, which dates the +# old name. It is kept as an alias so `except Secp256k1UnavailableError` keeps +# working: the curve is still secp256k1, only the binding changed. Prefer +# CryptoBackendUnavailableError in new code. +Secp256k1UnavailableError = CryptoBackendUnavailableError + +_MISSING_BACKEND_HINT = ( + "coincurve is required for BIP-340 Schnorr operations. " + "Install with: pip install coincurve" +) + + class NostrEvent: """Builds and signs Nostr events per NIP-01.""" @@ -72,21 +86,17 @@ def pubkey_from_private_key(private_key_hex: str) -> str: Returns: 32-byte x-only public key as hex string. """ - if not _HAS_SECP256K1: - raise Secp256k1UnavailableError( - "secp256k1 library is required for key derivation. " - "Install with: pip install secp256k1" + if not _HAS_COINCURVE: + raise CryptoBackendUnavailableError( + f"Key derivation is unavailable: {_MISSING_BACKEND_HINT}" ) privkey_bytes = bytes.fromhex(private_key_hex) if len(privkey_bytes) != 32: raise ValueError( f"Private key must be 32 bytes, got {len(privkey_bytes)} bytes" ) - keypair = secp256k1.PrivateKey(privkey_bytes) - # secp256k1 public key is 33 bytes (compressed); strip the prefix byte - pubkey_bytes = keypair.pubkey.serialize(compressed=True) - # x-only pubkey is the last 32 bytes of the compressed key (drop 0x02/0x03 prefix) - return pubkey_bytes[1:].hex() + # BIP-340 keys are x-only: the x coordinate with the y parity dropped. + return coincurve.PublicKeyXOnly.from_secret(privkey_bytes).format().hex() @staticmethod def sign(event_id_hex: str, private_key_hex: str) -> str: @@ -95,21 +105,21 @@ def sign(event_id_hex: str, private_key_hex: str) -> str: Returns: 64-byte signature as hex string. """ - if not _HAS_SECP256K1: - raise Secp256k1UnavailableError( - "secp256k1 library is required for signing. " - "Install with: pip install secp256k1" + if not _HAS_COINCURVE: + raise CryptoBackendUnavailableError( + f"Signing is unavailable: {_MISSING_BACKEND_HINT}" ) privkey_bytes = bytes.fromhex(private_key_hex) if len(privkey_bytes) != 32: raise ValueError( f"Private key must be 32 bytes, got {len(privkey_bytes)} bytes" ) + # NIP-01 signs the 32-byte event id directly; it is already a digest and + # must not be hashed again. sign_schnorr takes the message unhashed. msg_bytes = bytes.fromhex(event_id_hex) - keypair = secp256k1.PrivateKey(privkey_bytes) - sig = keypair.schnorr_sign(msg_bytes, bip340tag=b"", raw=True) - return sig.hex() + keypair = coincurve.PrivateKey(privkey_bytes) + return keypair.sign_schnorr(msg_bytes).hex() @staticmethod def verify(event: dict[str, Any]) -> bool: @@ -117,15 +127,14 @@ def verify(event: dict[str, Any]) -> bool: The ID alone is NOT authentication: it is a plain SHA-256 over public fields with no secret input, so anyone can compute a matching ID for an - event they forged. Authenticity comes solely from the BIP-340 signature, - which requires secp256k1. + event they forged. Authenticity comes solely from the BIP-340 signature. Returns: True only if the ID matches AND the signature is a valid BIP-340 signature over that ID under the claimed pubkey. False otherwise. Raises: - Secp256k1UnavailableError: If secp256k1 is unavailable, so the + CryptoBackendUnavailableError: If coincurve is unavailable, so the signature cannot be checked. This fails closed and loudly, consistent with sign() and pubkey_from_private_key(). It is a RuntimeError subclass and never silently passes. @@ -135,12 +144,14 @@ def verify(event: dict[str, Any]) -> bool: if computed_id != event.get("id", ""): return False - if not _HAS_SECP256K1: - raise Secp256k1UnavailableError( - "secp256k1 library is required for signature verification. " - "Refusing to treat the event as verified: the event ID is a " - "plain hash of public fields and proves nothing about " - "authenticity. Install with: pip install secp256k1" + # Raised before the try below, so a missing backend can never be caught + # and turned into a plain False — an unverifiable event must not be + # reported as merely invalid. + if not _HAS_COINCURVE: + raise CryptoBackendUnavailableError( + "Signature verification is unavailable. Refusing to treat the " + "event as verified: the event ID is a plain hash of public " + f"fields and proves nothing about authenticity. {_MISSING_BACKEND_HINT}" ) pubkey_hex = event.get("pubkey", "") @@ -151,11 +162,12 @@ def verify(event: dict[str, Any]) -> bool: try: msg_bytes = bytes.fromhex(event["id"]) sig_bytes = bytes.fromhex(sig_hex) - # Reconstruct compressed pubkey (prepend 0x02) - pubkey_bytes = bytes.fromhex("02" + pubkey_hex) - pubkey = secp256k1.PublicKey(pubkey_bytes, raw=True) - return pubkey.schnorr_verify(msg_bytes, sig_bytes, bip340tag=b"", raw=True) + # BIP-340 verification takes the 32-byte x-only pubkey directly. + pubkey = coincurve.PublicKeyXOnly(bytes.fromhex(pubkey_hex)) + return bool(pubkey.verify(sig_bytes, msg_bytes)) except Exception: + # Malformed pubkey/signature/id — not authentic. Distinct from a + # missing backend, which is raised above and never reaches here. return False @staticmethod diff --git a/tests/fixtures/dotnet_signed_events.json b/tests/fixtures/dotnet_signed_events.json new file mode 100644 index 0000000..9d0912d --- /dev/null +++ b/tests/fixtures/dotnet_signed_events.json @@ -0,0 +1,82 @@ +{ + "_comment": [ + "Nostr events signed by the .NET SDK (le-agent-sdk-dotnet) using", + "NBitcoin.Secp256k1 3.2.0 — an implementation with no code in common with", + "Python's coincurve. They are committed so the Python suite proves wire", + "compatibility on every run without needing a .NET toolchain in CI.", + "", + "These are a consensus artifact of the protocol, not test scaffolding: if a", + "change makes them stop verifying, this SDK has broken signature", + "compatibility with every other implementation on the network. Regenerate", + "them only when the .NET SDK's own wire format deliberately changes, never", + "to make a failing test pass.", + "", + "Generated from LightningEnable.AgentSdk.Nostr.NostrEvent.Sign() via", + "NostrEvent.GetPublicKey() / ComputeId(), .NET 8." + ], + "events": [ + { + "id": "fdf6fbd62ea5c7b6b0ba5e7e4cf5e99b1d1f1795535b159cb6cb15644e45f361", + "pubkey": "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", + "created_at": 1700000000, + "kind": 38400, + "tags": [ + [ + "d", + "svc-dotnet" + ], + [ + "s", + "ai" + ] + ], + "content": "Independent .NET-signed capability advertisement", + "sig": "bf080521c02b2bbce3f7c360deb2353230b6292fa602c19a031b9041c24661c8351ceb0d494932f2d8d739f30cdb4942b52d0d6d0eae09b7f68520b0e6d41ee1", + "_label": "ascii capability advertisement (kind 38400)" + }, + { + "id": "66609ad11d3d018f116e5872ff147956156a13ff010b81178eb181e22bd0eb80", + "pubkey": "dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659", + "created_at": 1700000001, + "kind": 38401, + "tags": [ + [ + "p", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ], + [ + "amount", + "1000" + ] + ], + "content": "service request from the .NET SDK", + "sig": "f62cd11fe887ba732532a1e8bab05cc6dd52c860785e932f6c97a5e7b3b60d2a375cec99002c5e90a13586bb483b27b8cc44b2bcc9f780d73470d641868a4b76", + "_label": "service request with tags (kind 38401)" + }, + { + "id": "9a9679bfd0e5e2c375fa6c6f95e6b84bda4d3809eed0b3d5552c541a9003a3d7", + "pubkey": "dd308afec5777e13121fa72b9cc1b7cc0139715309b086c960e18fd969774eb8", + "created_at": 1700000002, + "kind": 1, + "tags": [ + [ + "t", + "ünïcödé" + ] + ], + "content": "unicode ünïcödé 日本語 🚀 astral + \"quotes\" and \\backslash\\", + "sig": "1d0a2c63df9a2d392f759137215888a6213e6862bdc317d27add823f08288b22a0387a465a936cd2a624b10b32bc846037178b04be036985ae9b885e3cb81aa1", + "_label": "unicode + astral plane + escaped quotes/backslashes (kind 1)" + }, + { + "id": "03164c3bd6496a6554d09ecbceb9b2c8cc3af3e34aaf376f33e9269c61d74fbc", + "pubkey": "25d1dff95105f5253c4022f628a996ad3a0d95fbf21d468a1b33f8c160d8f517", + "created_at": 1700000003, + "kind": 0, + "tags": [], + "content": "", + "sig": "ef9eff4c10c9e95685e847a96d9d2c30a6082c18603f6bc57d2f0fa1978f72b226178bf51d19e6e4c970424e78b026ea9c139955cd153a6d3985c8e937f520a1", + "_label": "empty content, no tags (kind 0)" + } + ] +} diff --git a/tests/test_agent_manager.py b/tests/test_agent_manager.py index 0899c13..263626d 100644 --- a/tests/test_agent_manager.py +++ b/tests/test_agent_manager.py @@ -37,7 +37,7 @@ async def test_discover_returns_capabilities(self): discover() verifies signatures before parsing, so verification is stubbed to isolate the parsing behaviour under test. Signing real fixtures here - would require the secp256k1 native build. The drop-on-forgery path is + would require the crypto backend. The drop-on-forgery path is covered in tests/test_security_regressions.py. """ sample_events = [ diff --git a/tests/test_interop.py b/tests/test_interop.py new file mode 100644 index 0000000..7e26778 --- /dev/null +++ b/tests/test_interop.py @@ -0,0 +1,257 @@ +"""Cross-implementation wire-compatibility tests. + +Nostr events are a wire protocol: the event ID is a consensus value and the +signature must verify under any conforming implementation. A signature scheme +that only agrees with itself is worthless — a round-trip test would pass just as +happily on a private, incompatible curve. + +So these tests check this SDK against implementations it shares no code with: + + * BIP-340 published test vectors (the standard itself). + * Events signed by the .NET SDK (le-agent-sdk-dotnet) via NBitcoin.Secp256k1, + committed under tests/fixtures/ so no .NET toolchain is needed here. + +The fixtures are protocol artifacts, not scaffolding. If they stop verifying, +this SDK has broken compatibility with the network — regenerate them only when +the .NET SDK's wire format deliberately changes, never to make a test pass. +""" + +import json +from pathlib import Path + +import pytest + +from le_agent_sdk.nostr.event import NostrEvent + +FIXTURES = Path(__file__).parent / "fixtures" + + +def _load_dotnet_events(): + with open(FIXTURES / "dotnet_signed_events.json", encoding="utf-8") as fh: + return json.load(fh)["events"] + + +def _strip(event: dict) -> dict: + """Drop harness-only annotations, leaving the on-the-wire event.""" + return {k: v for k, v in event.items() if not k.startswith("_")} + + +DOTNET_EVENTS = _load_dotnet_events() +DOTNET_IDS = [e["_label"] for e in DOTNET_EVENTS] + + +class TestDotNetSignedEventsVerify: + """.NET (NBitcoin.Secp256k1) -> Python (coincurve).""" + + @pytest.mark.parametrize("event", DOTNET_EVENTS, ids=DOTNET_IDS) + def test_dotnet_event_id_agrees(self, event): + """Both implementations must derive the same ID from the same event. + + Covers the canonical NIP-01 serialization, including the non-ASCII and + astral-plane escaping rules the two languages implement separately. + """ + event = _strip(event) + assert NostrEvent.compute_id(event) == event["id"] + + @pytest.mark.parametrize("event", DOTNET_EVENTS, ids=DOTNET_IDS) + def test_dotnet_signature_verifies(self, event): + """A signature made by the .NET SDK must verify here.""" + assert NostrEvent.verify(_strip(event)) is True + + def test_fixture_actually_covers_non_ascii(self): + """Guard the guard: the unicode case must not silently vanish. + + Non-ASCII is where independently written serializers realistically + diverge, so losing that fixture would gut these tests without failing + anything. + """ + assert any( + any(ord(ch) > 0xFFFF for ch in e["content"]) for e in DOTNET_EVENTS + ), "no astral-plane content in fixtures" + assert any( + any(ord(ch) > 127 for ch in e["content"]) for e in DOTNET_EVENTS + ), "no non-ASCII content in fixtures" + + +class TestDotNetSignedEventsRejectTampering: + """The cross-impl fixtures must not verify once altered. + + Without these, a verify() that returned True unconditionally would pass + every test above. + """ + + def test_tampered_content_breaks_dotnet_event(self): + event = _strip(DOTNET_EVENTS[0]) + event["content"] = "tampered by a relay in transit" + assert NostrEvent.verify(event) is False + + def test_swapped_signature_breaks_dotnet_event(self): + event = _strip(DOTNET_EVENTS[0]) + event["sig"] = _strip(DOTNET_EVENTS[1])["sig"] + assert NostrEvent.verify(event) is False + + def test_dotnet_event_reattributed_to_another_pubkey_rejected(self): + """Reattribution with a recomputed ID: the ID check passes, sig must not.""" + event = _strip(DOTNET_EVENTS[0]) + event["pubkey"] = _strip(DOTNET_EVENTS[1])["pubkey"] + event["id"] = NostrEvent.compute_id(event) + + assert NostrEvent.compute_id(event) == event["id"], "ID check would pass" + assert NostrEvent.verify(event) is False + + +class TestPythonSignedEventsMatchDotNetKeys: + """Python -> .NET, verified through values .NET produced. + + Signatures are randomized per BIP-340 aux, so Python's signature bytes are + not expected to equal .NET's. Key derivation is deterministic, so agreement + on the pubkey is checkable here; the signature direction (Python-signed + events verifying under .NET) is exercised out-of-band against the .NET SDK. + """ + + @pytest.mark.parametrize( + "private_key,expected_pubkey", + [ + # (private key, x-only pubkey as derived by .NET's GetPublicKey()). + # These also match the BIP-340 published vectors. + ( + "0000000000000000000000000000000000000000000000000000000000000001", + "79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", + ), + ( + "b7e151628aed2a6abf7158809cf4f3c762e7160f38b4da56a784d9045190cfef", + "dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659", + ), + ( + "c90fdaa22168c234c4c6628b80dc1cd129024e088a67cc74020bbea63b14e5c9", + "dd308afec5777e13121fa72b9cc1b7cc0139715309b086c960e18fd969774eb8", + ), + ( + "0b432b2677937381aef05bb02a66ecd012773062cf3fa2549e44f58ed2401710", + "25d1dff95105f5253c4022f628a996ad3a0d95fbf21d468a1b33f8c160d8f517", + ), + ], + ) + def test_pubkey_derivation_matches_dotnet(self, private_key, expected_pubkey): + """x-only derivation must drop y-parity the same way in both SDKs. + + Two of these keys have odd y, so a binding that leaked parity into the + x-only key would disagree here and silently produce events attributed to + a pubkey no one else computes. + """ + assert NostrEvent.pubkey_from_private_key(private_key) == expected_pubkey + + def test_locally_signed_event_verifies_under_dotnet_derived_pubkey(self): + """Sign here, verify against the pubkey .NET says the key has.""" + private_key = "b7e151628aed2a6abf7158809cf4f3c762e7160f38b4da56a784d9045190cfef" + event = NostrEvent.create( + kind=38400, + content="signed by python, keyed by .NET's derivation", + tags=[["d", "svc-1"]], + private_key=private_key, + created_at=1700000000, + ) + assert event["pubkey"] == ( + "dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659" + ) + assert NostrEvent.verify(event) is True + + +class TestBip340PublishedVectors: + """Conformance to the BIP-340 standard itself. + + From the BIP-340 reference test vectors (indices 0-4). These pin the SDK to + the spec rather than to any one library's behaviour, and would catch a + backend swap that changed message-hashing semantics (e.g. a binding that + hashes the message again before signing — the event ID is already a digest + and must be signed as-is). + + Provenance matters here: a vector copied out of the library under test would + only prove the library agrees with itself. Each signature below was + independently reproduced by NBitcoin.Secp256k1 (the .NET SDK's backend) + signing the same key/message/aux_rand — BIP-340 signing is deterministic + given aux_rand, so two unrelated implementations emitting identical bytes is + what establishes these as the spec's values. + """ + + # (index, pubkey, message, signature, expected) + VECTORS = [ + ( + 0, + "f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9", + "0000000000000000000000000000000000000000000000000000000000000000", + "e907831f80848d1069a5371b402410364bdf1c5f8307b0084c55f1ce2dca8215" + "25f66a4a85ea8b71e482a74f382d2ce5ebeee8fdb2172f477df4900d310536c0", + True, + ), + ( + 1, + "dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659", + "243f6a8885a308d313198a2e03707344a4093822299f31d0082efa98ec4e6c89", + "6896bd60eeae296db48a229ff71dfe071bde413e6d43f917dc8dcf8c78de3341" + "8906d11ac976abccb20b091292bff4ea897efcb639ea871cfa95f6de339e4b0a", + True, + ), + ( + 2, + "dd308afec5777e13121fa72b9cc1b7cc0139715309b086c960e18fd969774eb8", + "7e2d58d8b3bcdf1abadec7829054f90dda9805aab56c77333024b9d0a508b75c", + "5831aaeed7b44bb74e5eab94ba9d4294c49bcf2a60728d8b4c200f50dd313c1b" + "ab745879a5ad954a72c45a91c3a51d3c7adea98d82f8481e0e1e03674a6f3fb7", + True, + ), + ( + 3, + "25d1dff95105f5253c4022f628a996ad3a0d95fbf21d468a1b33f8c160d8f517", + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", + "7eb0509757e246f19449885651611cb965ecc1a187dd51b64fda1edc9637d5ec" + "97582b9cb13db3933705b32ba982af5af25fd78881ebb32771fc5922efc66ea3", + True, + ), + ( + 4, + "d69c3509bb99e412e68b0fe8544e72837dfa30746d8be2aa65975f29d22dc7b9", + "4df3c3f68fcc83b27e9d42c90431a72499f17875c81a599b566c9889b9696703", + "00000000000000000000003b78ce563f89a0ed9414f5aa28ad0d96d6795f9c63" + "76afb1548af603b3eb45c9f8207dee1060cb71c04e80f593060b07d28308d7f4", + True, + ), + ] + + @pytest.mark.parametrize( + "index,pubkey,message,signature,expected", + VECTORS, + ids=[f"bip340-vector-{v[0]}" for v in VECTORS], + ) + def test_bip340_vector(self, index, pubkey, message, signature, expected): + """Drive the vector through verify() by presenting it as an event. + + verify() checks the ID before the signature, so the vector's message is + placed in `id`: that is exactly how NIP-01 uses it — the 32-byte value + signed. The ID check is neutralised by monkeypatching compute_id, since + these vectors are raw BIP-340 messages, not NIP-01 serializations. + """ + event = {"id": message, "pubkey": pubkey, "sig": signature} + + original = NostrEvent.compute_id + try: + NostrEvent.compute_id = staticmethod(lambda e: e["id"]) + assert NostrEvent.verify(event) is expected + finally: + NostrEvent.compute_id = original + + def test_vector_signature_rejected_under_wrong_pubkey(self): + """Negative control: the vectors must not verify under any key.""" + _, _, message, signature, _ = self.VECTORS[1] + event = { + "id": message, + "pubkey": self.VECTORS[2][1], # a different, valid pubkey + "sig": signature, + } + + original = NostrEvent.compute_id + try: + NostrEvent.compute_id = staticmethod(lambda e: e["id"]) + assert NostrEvent.verify(event) is False + finally: + NostrEvent.compute_id = original diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py index e9b1a29..a78f60b 100644 --- a/tests/test_security_regressions.py +++ b/tests/test_security_regressions.py @@ -4,7 +4,7 @@ written to fail against the pre-fix code and pass after the fix. Covered: - 1. NostrEvent.verify() fail-open when secp256k1 is unavailable. + 1. NostrEvent.verify() fail-open when the crypto backend is unavailable. 2. L402Client.pay_and_access() ignoring max_amount_sats. 3. Budget check skipped when the invoice amount is unparseable. 4. Reputation scoring ignoring out-of-range ratings (already correct — locked in). @@ -19,7 +19,11 @@ from le_agent_sdk.l402.client import L402Client from le_agent_sdk.models.attestation import AgentAttestation from le_agent_sdk.nostr import event as event_module -from le_agent_sdk.nostr.event import NostrEvent, Secp256k1UnavailableError +from le_agent_sdk.nostr.event import ( + CryptoBackendUnavailableError, + NostrEvent, + Secp256k1UnavailableError, +) # --- Fixtures / helpers ----------------------------------------------------- @@ -39,25 +43,43 @@ def _forged_event(pubkey: str = "de" * 32) -> dict: return event -class TestVerifyFailsClosedWithoutSecp256k1: - """Finding 1: verify() returned True when secp256k1 was unimportable.""" +class TestVerifyFailsClosedWithoutCryptoBackend: + """Finding 1: verify() returned True when the crypto backend was unimportable. - def test_verify_raises_when_secp256k1_unavailable(self): - """Missing native dep must be loud, not a silent pass. + The backend moved from secp256k1 to coincurve in 0.4.0. coincurve ships + prebuilt wheels, so the "absent backend" case is far less likely to occur by + accident than it was — but "unlikely" is not "impossible" (an unusual + platform with no wheel, or a pruned install), and the fail-open bug this + class covers is exactly what happens when an unlikely branch is left + undefined. The behaviour is still pinned. + """ + + def test_verify_raises_when_backend_unavailable(self): + """Missing backend must be loud, not a silent pass. Consistent with sign()/pubkey_from_private_key(), which already raise. """ - with patch.object(event_module, "_HAS_SECP256K1", False): - with pytest.raises(RuntimeError, match="secp256k1"): + with patch.object(event_module, "_HAS_COINCURVE", False): + with pytest.raises(RuntimeError, match="coincurve"): + NostrEvent.verify(_forged_event()) + + def test_verify_raises_specific_error_type(self): + """The raised type must stay catchable under both names.""" + with patch.object(event_module, "_HAS_COINCURVE", False): + with pytest.raises(CryptoBackendUnavailableError): + NostrEvent.verify(_forged_event()) + # The pre-0.4.0 name is an alias, so existing handlers keep working. + with patch.object(event_module, "_HAS_COINCURVE", False): + with pytest.raises(Secp256k1UnavailableError): NostrEvent.verify(_forged_event()) def test_forged_event_does_not_verify_as_true(self): """A forged event must never come back as verified. - Regardless of whether secp256k1 is installed, the one outcome that must + Regardless of whether the backend is installed, the one outcome that must be impossible is a `True` return for an event with a bogus signature. """ - with patch.object(event_module, "_HAS_SECP256K1", False): + with patch.object(event_module, "_HAS_COINCURVE", False): try: result = NostrEvent.verify(_forged_event()) except RuntimeError: @@ -65,12 +87,22 @@ def test_forged_event_does_not_verify_as_true(self): assert result is not True, "forged event verified as authentic" def test_verify_still_rejects_id_mismatch_before_dep_check(self): - """A tampered ID is rejected without needing the native dep.""" + """A tampered ID is rejected without needing the backend.""" event = _forged_event() event["content"] = "tampered after id was computed" - with patch.object(event_module, "_HAS_SECP256K1", False): + with patch.object(event_module, "_HAS_COINCURVE", False): assert NostrEvent.verify(event) is False + def test_sign_raises_when_backend_unavailable(self): + with patch.object(event_module, "_HAS_COINCURVE", False): + with pytest.raises(CryptoBackendUnavailableError, match="coincurve"): + NostrEvent.sign("ab" * 32, "01" * 32) + + def test_pubkey_derivation_raises_when_backend_unavailable(self): + with patch.object(event_module, "_HAS_COINCURVE", False): + with pytest.raises(CryptoBackendUnavailableError, match="coincurve"): + NostrEvent.pubkey_from_private_key("01" * 32) + class TestVerifyAcceptsGenuineRejectsForged: """The signature path must discriminate, not just refuse everything. @@ -78,88 +110,69 @@ class TestVerifyAcceptsGenuineRejectsForged: Fixing a fail-open bug by failing everything closed would be no fix. These tests exercise the real BIP-340 path with genuine signatures. - secp256k1 needs a native build and is frequently unimportable, so signatures - are produced with coincurve (pure wheels) and the small slice of the - secp256k1 API that verify() uses is shimmed over it. The crypto under test - is real; only the binding is substituted. + Before 0.4.0 these signed fixtures with coincurve and shimmed it over the + secp256k1 binding, because secp256k1 needs a native build and was routinely + unimportable — so the signature path could not otherwise be tested at all. + coincurve is now the runtime backend, so the shim is gone and these drive + the real production code path end to end. Cross-implementation agreement is + covered separately in test_interop.py; the concern here is that verify() + tells genuine and forged apart. """ @staticmethod - def _install_shim(monkeypatch): - coincurve = pytest.importorskip( - "coincurve", reason="needs a BIP-340 impl to sign real fixtures" + def _sign(private_hex: str, content: str = "genuine") -> dict: + """Build a genuinely signed event through the SDK's own signing path.""" + return NostrEvent.create( + kind=38400, + content=content, + tags=[["d", "svc-a"]], + private_key=private_hex, + created_at=1700000000, ) - class _ShimPublicKey: - def __init__(self, data, raw=True): - # verify() prepends a 0x02 prefix; x-only key is the remainder. - self._xonly = coincurve.PublicKeyXOnly(data[1:]) - - def schnorr_verify(self, msg, sig, bip340tag=b"", raw=True): - return self._xonly.verify(sig, msg) + def test_genuine_signed_event_verifies(self): + assert NostrEvent.verify(self._sign("01" * 32)) is True - class _ShimSecp256k1: - PublicKey = _ShimPublicKey - - monkeypatch.setattr(event_module, "secp256k1", _ShimSecp256k1, raising=False) - monkeypatch.setattr(event_module, "_HAS_SECP256K1", True) - return coincurve - - @staticmethod - def _sign(coincurve, private_hex: str, content: str = "genuine") -> dict: - priv = coincurve.PrivateKey(bytes.fromhex(private_hex)) - xonly_pubkey = priv.public_key.format(compressed=True)[1:] - event = { - "pubkey": xonly_pubkey.hex(), - "created_at": 1700000000, - "kind": 38400, - "tags": [["d", "svc-a"]], - "content": content, - } - event["id"] = NostrEvent.compute_id(event) - event["sig"] = priv.sign_schnorr(bytes.fromhex(event["id"])).hex() - return event - - def test_genuine_signed_event_verifies(self, monkeypatch): - coincurve = self._install_shim(monkeypatch) - event = self._sign(coincurve, "01" * 32) - assert NostrEvent.verify(event) is True - - def test_forged_signature_rejected(self, monkeypatch): - coincurve = self._install_shim(monkeypatch) - event = self._sign(coincurve, "01" * 32) + def test_forged_signature_rejected(self): + event = self._sign("01" * 32) event["sig"] = "00" * 64 assert NostrEvent.verify(event) is False - def test_tampered_content_rejected(self, monkeypatch): - coincurve = self._install_shim(monkeypatch) - event = self._sign(coincurve, "01" * 32) + def test_tampered_content_rejected(self): + event = self._sign("01" * 32) event["content"] = "tampered" assert NostrEvent.verify(event) is False - def test_event_reattributed_to_another_pubkey_rejected(self, monkeypatch): + def test_event_reattributed_to_another_pubkey_rejected(self): """The core attack: swap the pubkey and recompute a valid ID. The ID check alone passes here — it is just a hash of public fields. Only the signature check stops it. """ - coincurve = self._install_shim(monkeypatch) - event = self._sign(coincurve, "01" * 32) + event = self._sign("01" * 32) - victim = coincurve.PrivateKey(bytes.fromhex("02" * 32)) - event["pubkey"] = victim.public_key.format(compressed=True)[1:].hex() + event["pubkey"] = NostrEvent.pubkey_from_private_key("02" * 32) event["id"] = NostrEvent.compute_id(event) # ID now matches again assert NostrEvent.compute_id(event) == event["id"], "ID check would pass" assert NostrEvent.verify(event) is False, "reattributed event was accepted" - def test_signature_from_different_key_rejected(self, monkeypatch): - coincurve = self._install_shim(monkeypatch) - event = self._sign(coincurve, "01" * 32) - attacker = coincurve.PrivateKey(bytes.fromhex("03" * 32)) - event["sig"] = attacker.sign_schnorr(bytes.fromhex(event["id"])).hex() + def test_signature_from_different_key_rejected(self): + event = self._sign("01" * 32) + event["sig"] = NostrEvent.sign(event["id"], "03" * 32) assert NostrEvent.verify(event) is False + def test_sign_verify_round_trips_through_public_api(self): + """create() -> verify() must hold for the documented entry point.""" + event = NostrEvent.create( + kind=38400, + content="round trip", + tags=[["d", "svc-a"], ["s", "ai"]], + private_key="04" * 32, + ) + assert event["pubkey"] == NostrEvent.pubkey_from_private_key("04" * 32) + assert NostrEvent.verify(event) is True + class TestPayAndAccessRespectsMaxAmount: """Finding 2: pay_and_access never read self._max_amount_sats.""" @@ -392,9 +405,9 @@ async def test_verification_error_propagates(self): with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: mock_query.return_value = [_forged_event()] with patch.object( - NostrEvent, "verify", side_effect=RuntimeError("secp256k1 required") + NostrEvent, "verify", side_effect=RuntimeError("coincurve required") ): - with pytest.raises(RuntimeError, match="secp256k1"): + with pytest.raises(RuntimeError, match="coincurve"): await mgr.discover() @pytest.mark.asyncio @@ -436,10 +449,10 @@ async def test_listen_requests_surfaces_missing_dep_without_reconnect_storm(self with patch.object( type(mgr), "pubkey", property(lambda self: "aa" * 32) ), patch.object( - NostrEvent, "verify", side_effect=Secp256k1UnavailableError("secp256k1 missing") + NostrEvent, "verify", side_effect=CryptoBackendUnavailableError("coincurve missing") ): with patch("le_agent_sdk.agent.manager.RelayClient", fake_relay_cls): - with pytest.raises(Secp256k1UnavailableError): + with pytest.raises(CryptoBackendUnavailableError): async for _ in mgr.listen_requests(): pass From 052ad0da5e09d0e01f6cc951a13e60f98b9c4ffb Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 17 Jul 2026 02:06:41 -0400 Subject: [PATCH 4/4] docs: fix CHANGELOG branch link and the y-parity vector count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 2 +- tests/test_interop.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6430b63..5983318 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to `le-agent-sdk` are documented here. This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). Entries begin at 0.4.0; for earlier history see the -[commit log](https://github.com/refined-element/le-agent-sdk-python/commits/main). +[commit log](https://github.com/refined-element/le-agent-sdk-python/commits/master). ## [0.4.0] - 2026-07-17 diff --git a/tests/test_interop.py b/tests/test_interop.py index 7e26778..8b4436e 100644 --- a/tests/test_interop.py +++ b/tests/test_interop.py @@ -135,9 +135,10 @@ class TestPythonSignedEventsMatchDotNetKeys: def test_pubkey_derivation_matches_dotnet(self, private_key, expected_pubkey): """x-only derivation must drop y-parity the same way in both SDKs. - Two of these keys have odd y, so a binding that leaked parity into the - x-only key would disagree here and silently produce events attributed to - a pubkey no one else computes. + One of these keys has odd y (0b432b26...), so a binding that leaked parity + into the x-only key would disagree here and silently produce events + attributed to a pubkey no one else computes. Keep that vector: drop it and + the parity coverage silently falls to zero. """ assert NostrEvent.pubkey_from_private_key(private_key) == expected_pubkey