Skip to content

fix: async Web3 calls + nonce manager to prevent event loop blocking#2

Closed
HaraldeRoessler wants to merge 1 commit into
MoltyCel:mainfrom
HaraldeRoessler:fix/async-web3-nonce-manager
Closed

fix: async Web3 calls + nonce manager to prevent event loop blocking#2
HaraldeRoessler wants to merge 1 commit into
MoltyCel:mainfrom
HaraldeRoessler:fix/async-web3-nonce-manager

Conversation

@HaraldeRoessler

Copy link
Copy Markdown
Contributor

Summary

  • Add app/nonce_manager.py with per-address asyncio.Lock preventing nonce collisions on concurrent transactions
  • Wrap all blocking Web3 RPC calls in asyncio.to_thread() across main.py, erc8004.py, and provenance/anchor.py
  • Convert post_reputation_feedback, register_onchain_agent, get_onchain_reputation from sync to async
  • Replace music VC anchoring via cast CLI subprocess with web3.py (eliminates private key in process env)

Problem

All Web3 calls were synchronous, blocking the FastAPI async event loop. This caused latency spikes for all concurrent API users whenever an on-chain operation ran. Additionally, concurrent transactions from the same wallet fetched the same nonce, causing one to fail silently.

How it works

Nonce manager: Uses an asyncio.Lock per wallet address. First call fetches the pending nonce from chain, subsequent calls increment locally. On failure, the cache resets to force a re-fetch.

Async wrapping: Every w3.eth.* call now runs in asyncio.to_thread() instead of blocking the event loop.

Test plan

  • Verify API starts without errors
  • Test /reputation/rate with ERC-8004 agent (triggers async post_reputation_feedback)
  • Test concurrent on-chain operations produce no nonce collisions
  • Verify music credential anchoring works without cast CLI dependency

Generated with Claude Code

All synchronous Web3 RPC calls (get_transaction_count, gas_price,
send_raw_transaction, wait_for_transaction_receipt, is_connected) were
blocking the FastAPI async event loop, causing latency spikes for all
concurrent users.

Changes:
- Add app/nonce_manager.py with per-address asyncio.Lock to prevent
  nonce collisions when multiple async handlers submit transactions
  concurrently from the same wallet
- Wrap all blocking Web3 calls in asyncio.to_thread() across main.py,
  erc8004.py, and provenance/anchor.py
- Convert erc8004.py functions (post_reputation_feedback,
  register_onchain_agent, get_onchain_reputation) from sync to async
  and update all call sites with await
- Replace music VC anchoring via cast CLI subprocess with web3.py,
  eliminating private key exposure in process environment

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@HaraldeRoessler

Copy link
Copy Markdown
Contributor Author

Merge order

Independent — can merge in any order relative to #4 and #7. Recommended: after #1.

Sequence: #1#3#2#4#7

@HaraldeRoessler

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #3, which contains the same app/main.py / app/erc8004.py / app/nonce_manager.py / app/provenance/anchor.py changes verbatim (identical hunks at the same line ranges), plus pytest unit tests, docker-compose.yml, and a smoke test. No content from this PR is missing from #3.

@HaraldeRoessler HaraldeRoessler deleted the fix/async-web3-nonce-manager branch May 12, 2026 00:08
HaraldeRoessler referenced this pull request in HaraldeRoessler/moltrust-api May 12, 2026
… CSP

Follow-up to the previous commit on this branch (89bec1d). Adds the 13
items reviewers flagged as still-open. Bundled into one branch since
they're all defence-in-depth / mechanical, and splitting would just
multiply review cycles.

FAIL-FAST ON REQUIRED CREDENTIALS (these are BREAKING for deploys
that haven't set the env vars):

  M2  NONCE_SECRET — empty default removed; raises at startup if unset
                     (the runtime checks at the call sites caught it
                     before, but better to refuse to start than to
                     serve traffic with trivially-forgeable nonces).
  M10 MOLTSTACK_DB_PW — empty default removed; raises at startup.
  HIGH#3 MOLTBOOK_APP_KEY — "moltdev_PENDING" sentinel default
                     removed; endpoint returns 503 when unset.
  L15 GITHUB_CLIENT_ID — "PENDING" sentinel removed; endpoint
                     returns 503 when unset.

OPERATOR MIGRATION (one-time before merging into prod):
  export MOLTRUST_ADMIN_USERS="..."         # from previous commit
  export NONCE_SECRET="..."                  # any high-entropy string
  export MOLTSTACK_DB_PW="..."               # the existing DB password
  # MOLTBOOK_APP_KEY / GITHUB_CLIENT_ID: optional, leave unset to disable

LOCALIZED HARDENING (no migration impact):

  M4  ipaddress.ip_address() validation on /admin/traffic/caller/{ip}.
      Rejects malformed input before the DB LIKE-prefix lookup.
  L3  random.choice annotations — noqa: S311 with "non-security
      content selection" justification at 4 sites (cosmetic uses;
      no security-adjacent sampling).
  M9  DATABASE_URL default cleaned of the $(cat /dev/null) shell
      antipattern. Was never shelled out, but confusing.
  HIGH#5 _reg_tracker registration-rate hash: 16-char (64-bit)
      truncation → full SHA-256. Per-API-key rate limits are no
      longer collision-bypassable.
  HIGH#6 scrub_secrets pattern expansion: GitHub tokens, Stripe
      live keys (sk_live_, rk_live_, whsec_), OpenAI/Anthropic
      sk-proj-*, generic JWTs, AWS secret access keys, plus more
      PRIVATE KEY header variants. Deliberately NOT adding broad
      hex / base58 patterns — they would scrub legitimate response
      payloads (Ethereum TX hashes, IPR commitment hashes, wallet
      addresses).
  M12 KMS signer plaintext-fallback gating: DID_PRIVATE_KEY_HEX
      and ~/.moltrust_did_private_key are accepted in dev, but
      a hard error is raised when MOLTRUST_ENV=production is set
      and no KMS-encrypted blob is provided.
  M13 update_last_seen / update_last_active: bare `except: pass`
      replaced with `logger.warning(...)`. Still fire-and-forget
      (doesn't block the request) but failures are observable now.
  M1  _get_client_ip: X-Real-IP / X-Forwarded-For are honoured ONLY
      when request.client.host falls in MOLTRUST_TRUSTED_PROXIES
      (default: RFC1918 + loopback + ULA, which covers a load
      balancer with a private IP). Operators with public-IP LBs
      should override the env var; set "0.0.0.0/0,::/0" to fall
      back to the previous "trust everyone" behaviour.
  L16 Swagger UI: CSP header added; CSS link pinned to @5.17.14
      with SRI integrity hash (matches the JS pin from the
      previous commit).

EXPLICITLY STILL OUT OF SCOPE (separate PRs / your call):

  HIGH#4 SESSIONS dict in app/admin_auth.py — needs a design
         decision (TTLCache vs background sweep vs Redis).
  M11    print() → logger across 15+ sites in app/main.py — too
         large to mix into this PR.
  L2     Foundry `subprocess` for on-chain anchoring → Python
         web3 lib — touches the core anchoring path; similar
         refactors closed before (PR #2).
  L1     subprocess for openssl SSL check in /health — low value.
  C1     .env.dilithium — VERIFIED never in git history, only on
         local disk. No repo change applicable; operator should
         delete the file and rotate the key locally.

VERIFICATION
  Python 3.12 AST parse — all 8 touched files compile cleanly.
  (System Python 3.9 can't parse the project — pre-existing
  f-string syntax requires 3.10+.)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HaraldeRoessler referenced this pull request in HaraldeRoessler/moltrust-api May 12, 2026
Follow-up to commit d25e70c (SSRF). After running CodeQL default-setup
on the fork, 17 additional findings surfaced. Triage outcome:

  Already closed by earlier commits this PR:   1 (SSRF)
  False positives (dismissed via CodeQL UI):   4
  Real findings fixed in this commit:          5
  Stack-trace-exposure (deferred to design):   7

FIXES IN THIS COMMIT

  #1 [LOG SANITISATION] credit_middleware exception swallows DB password
     - app/main.py (logger.error in credit_middleware)
     `logger.error("…: %s", caller_did, e)` — the raw exception `e`
     can be an asyncpg ConnectionError whose repr() includes the
     Postgres connection string (with the password). Log only
     `type(e).__name__` instead.

  #2 [DEFENSIVE URL ENCODING] /join?ref= referrer parameter
     - app/main.py /join handler
     The redirect target is HARDCODED to https://moltrust.ch — the
     host is not user-controlled. But `f"https://moltrust.ch?ref={ref}"`
     interpolates `ref` raw, and a payload like `ref="x&malparam=…"`
     could corrupt the query string. Use `urllib.parse.quote(ref)` to
     percent-encode the value before interpolation.

  MoltyCel#3 [STDOUT TOKEN LEAK] telegram_hn_remind print(r.text)
     - scripts/telegram_hn_remind.py
     `print(f'Status: {r.status_code}, Response: {r.text}')` — if
     Telegram error responses ever echo the request URL (which contains
     the bot token in the path), the body lands in stdout / CI scrollback.
     Print only the status code.

  MoltyCel#4 [ReDoS] mpp authorization header regex
     - packages/mpp/index.js
     `auth.match(/^(?:Payment|MPP)\s+(.+)$/i)` on an unbounded header
     is polynomial-quadratic. This package is published to npm, so
     consumer servers carry the risk. Cap header at 8 KiB and use
     bounded `\s{1,8}` with a non-greedy first char.

  MoltyCel#5 [ReDoS] moltrust-openclaw-v2 base URL trim
     - moltrust-openclaw-v2/src/client.ts
     `.replace(/\/+$/, "")` is polynomial on pathological inputs.
     Replace with a `while (str.endsWith("/")) str = str.slice(0, -1)`
     loop, which is linear.

DISMISSED AS FALSE POSITIVES (no code change)

  MoltyCel#14 py/clear-text-logging-sensitive-data at SPIFFE bind log
      Logs spiffe_uri, did, caller_did — none are passwords. CodeQL
      misfires on the "did" → "id" → "password" name-similarity heuristic.

  MoltyCel#13, MoltyCel#12 py/clear-text-logging-sensitive-data in scripts/threadwatch.py
      Telegram bot token flows into the request URL but never into a
      logger or print() call — only to requests.post (which doesn't
      log URLs by default).

  MoltyCel#16 py/weak-sensitive-data-hashing in _reg_tracker
      This is in-memory rate-limit bucket-key derivation, not password
      storage. bcrypt/argon2 would be wrong here (slow + salted breaks
      the lookup). SHA-256 of the full API key is the correct primitive
      for an O(1) tracker.

EXPLICITLY DEFERRED (7 stack-trace-exposure findings)

  Multiple endpoints currently return `{"error": str(e)[:100]}` to
  callers. CodeQL flags these as info disclosure. Fixing them means
  changing the API contract — clients that parse the `error` field
  would break. This is a design call for the maintainer; deferring
  to a separate PR + discussion rather than including in this hardening
  pass.

VERIFICATION

  Python 3.12 AST parse — app/main.py + scripts/telegram_hn_remind.py
  compile cleanly. `node -c packages/mpp/index.js` clean. The TS file
  change is a syntactically-trivial loop, not type-impacting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HaraldeRoessler referenced this pull request in HaraldeRoessler/moltrust-api May 18, 2026
… CSP

Follow-up to the previous commit on this branch (89bec1d). Adds the 13
items reviewers flagged as still-open. Bundled into one branch since
they're all defence-in-depth / mechanical, and splitting would just
multiply review cycles.

FAIL-FAST ON REQUIRED CREDENTIALS (these are BREAKING for deploys
that haven't set the env vars):

  M2  NONCE_SECRET — empty default removed; raises at startup if unset
                     (the runtime checks at the call sites caught it
                     before, but better to refuse to start than to
                     serve traffic with trivially-forgeable nonces).
  M10 MOLTSTACK_DB_PW — empty default removed; raises at startup.
  HIGH#3 MOLTBOOK_APP_KEY — "moltdev_PENDING" sentinel default
                     removed; endpoint returns 503 when unset.
  L15 GITHUB_CLIENT_ID — "PENDING" sentinel removed; endpoint
                     returns 503 when unset.

OPERATOR MIGRATION (one-time before merging into prod):
  export MOLTRUST_ADMIN_USERS="..."         # from previous commit
  export NONCE_SECRET="..."                  # any high-entropy string
  export MOLTSTACK_DB_PW="..."               # the existing DB password
  # MOLTBOOK_APP_KEY / GITHUB_CLIENT_ID: optional, leave unset to disable

LOCALIZED HARDENING (no migration impact):

  M4  ipaddress.ip_address() validation on /admin/traffic/caller/{ip}.
      Rejects malformed input before the DB LIKE-prefix lookup.
  L3  random.choice annotations — noqa: S311 with "non-security
      content selection" justification at 4 sites (cosmetic uses;
      no security-adjacent sampling).
  M9  DATABASE_URL default cleaned of the $(cat /dev/null) shell
      antipattern. Was never shelled out, but confusing.
  HIGH#5 _reg_tracker registration-rate hash: 16-char (64-bit)
      truncation → full SHA-256. Per-API-key rate limits are no
      longer collision-bypassable.
  HIGH#6 scrub_secrets pattern expansion: GitHub tokens, Stripe
      live keys (sk_live_, rk_live_, whsec_), OpenAI/Anthropic
      sk-proj-*, generic JWTs, AWS secret access keys, plus more
      PRIVATE KEY header variants. Deliberately NOT adding broad
      hex / base58 patterns — they would scrub legitimate response
      payloads (Ethereum TX hashes, IPR commitment hashes, wallet
      addresses).
  M12 KMS signer plaintext-fallback gating: DID_PRIVATE_KEY_HEX
      and ~/.moltrust_did_private_key are accepted in dev, but
      a hard error is raised when MOLTRUST_ENV=production is set
      and no KMS-encrypted blob is provided.
  M13 update_last_seen / update_last_active: bare `except: pass`
      replaced with `logger.warning(...)`. Still fire-and-forget
      (doesn't block the request) but failures are observable now.
  M1  _get_client_ip: X-Real-IP / X-Forwarded-For are honoured ONLY
      when request.client.host falls in MOLTRUST_TRUSTED_PROXIES
      (default: RFC1918 + loopback + ULA, which covers a load
      balancer with a private IP). Operators with public-IP LBs
      should override the env var; set "0.0.0.0/0,::/0" to fall
      back to the previous "trust everyone" behaviour.
  L16 Swagger UI: CSP header added; CSS link pinned to @5.17.14
      with SRI integrity hash (matches the JS pin from the
      previous commit).

EXPLICITLY STILL OUT OF SCOPE (separate PRs / your call):

  HIGH#4 SESSIONS dict in app/admin_auth.py — needs a design
         decision (TTLCache vs background sweep vs Redis).
  M11    print() → logger across 15+ sites in app/main.py — too
         large to mix into this PR.
  L2     Foundry `subprocess` for on-chain anchoring → Python
         web3 lib — touches the core anchoring path; similar
         refactors closed before (PR #2).
  L1     subprocess for openssl SSL check in /health — low value.
  C1     .env.dilithium — VERIFIED never in git history, only on
         local disk. No repo change applicable; operator should
         delete the file and rotate the key locally.

VERIFICATION
  Python 3.12 AST parse — all 8 touched files compile cleanly.
  (System Python 3.9 can't parse the project — pre-existing
  f-string syntax requires 3.10+.)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HaraldeRoessler referenced this pull request in HaraldeRoessler/moltrust-api May 18, 2026
Follow-up to commit d25e70c (SSRF). After running CodeQL default-setup
on the fork, 17 additional findings surfaced. Triage outcome:

  Already closed by earlier commits this PR:   1 (SSRF)
  False positives (dismissed via CodeQL UI):   4
  Real findings fixed in this commit:          5
  Stack-trace-exposure (deferred to design):   7

FIXES IN THIS COMMIT

  #1 [LOG SANITISATION] credit_middleware exception swallows DB password
     - app/main.py (logger.error in credit_middleware)
     `logger.error("…: %s", caller_did, e)` — the raw exception `e`
     can be an asyncpg ConnectionError whose repr() includes the
     Postgres connection string (with the password). Log only
     `type(e).__name__` instead.

  #2 [DEFENSIVE URL ENCODING] /join?ref= referrer parameter
     - app/main.py /join handler
     The redirect target is HARDCODED to https://moltrust.ch — the
     host is not user-controlled. But `f"https://moltrust.ch?ref={ref}"`
     interpolates `ref` raw, and a payload like `ref="x&malparam=…"`
     could corrupt the query string. Use `urllib.parse.quote(ref)` to
     percent-encode the value before interpolation.

  MoltyCel#3 [STDOUT TOKEN LEAK] telegram_hn_remind print(r.text)
     - scripts/telegram_hn_remind.py
     `print(f'Status: {r.status_code}, Response: {r.text}')` — if
     Telegram error responses ever echo the request URL (which contains
     the bot token in the path), the body lands in stdout / CI scrollback.
     Print only the status code.

  MoltyCel#4 [ReDoS] mpp authorization header regex
     - packages/mpp/index.js
     `auth.match(/^(?:Payment|MPP)\s+(.+)$/i)` on an unbounded header
     is polynomial-quadratic. This package is published to npm, so
     consumer servers carry the risk. Cap header at 8 KiB and use
     bounded `\s{1,8}` with a non-greedy first char.

  MoltyCel#5 [ReDoS] moltrust-openclaw-v2 base URL trim
     - moltrust-openclaw-v2/src/client.ts
     `.replace(/\/+$/, "")` is polynomial on pathological inputs.
     Replace with a `while (str.endsWith("/")) str = str.slice(0, -1)`
     loop, which is linear.

DISMISSED AS FALSE POSITIVES (no code change)

  MoltyCel#14 py/clear-text-logging-sensitive-data at SPIFFE bind log
      Logs spiffe_uri, did, caller_did — none are passwords. CodeQL
      misfires on the "did" → "id" → "password" name-similarity heuristic.

  MoltyCel#13, MoltyCel#12 py/clear-text-logging-sensitive-data in scripts/threadwatch.py
      Telegram bot token flows into the request URL but never into a
      logger or print() call — only to requests.post (which doesn't
      log URLs by default).

  MoltyCel#16 py/weak-sensitive-data-hashing in _reg_tracker
      This is in-memory rate-limit bucket-key derivation, not password
      storage. bcrypt/argon2 would be wrong here (slow + salted breaks
      the lookup). SHA-256 of the full API key is the correct primitive
      for an O(1) tracker.

EXPLICITLY DEFERRED (7 stack-trace-exposure findings)

  Multiple endpoints currently return `{"error": str(e)[:100]}` to
  callers. CodeQL flags these as info disclosure. Fixing them means
  changing the API contract — clients that parse the `error` field
  would break. This is a design call for the maintainer; deferring
  to a separate PR + discussion rather than including in this hardening
  pass.

VERIFICATION

  Python 3.12 AST parse — app/main.py + scripts/telegram_hn_remind.py
  compile cleanly. `node -c packages/mpp/index.js` clean. The TS file
  change is a syntactically-trivial loop, not type-impacting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MoltyCel pushed a commit that referenced this pull request May 28, 2026
Re-§12-Review für alpha.1 (run 2026-05-28 17:27 UTC, output
~/moltstack/reviews/20260528_172832_openclaw-plugin-v2.0.0-alpha.1_review.md)
votierte ÜBERARBEITEN. Drei neue Blocker für alpha.2 — alle in
diesem Commit adressiert.

Blocker #1 (Test-Lücke): kombinatorischer Pfad "own DID OK +
counterparty lookup fail + failOpen=true → ALLOW" war nicht getestet.
Test ergänzt in before-tool-call.test.ts.

Blocker #2 (Performance): sequenzielle Counterparty-Lookups in
before-tool-call.ts waren O(N × API-latency) — 4 DIDs × 50ms = 200ms+
Event-Loop-Block. Fix: Promise.allSettled + post-evaluation. Block-
Priority ist deterministisch (erste Counterparty im array order wins).
Own-DID-Check bleibt VOR counterparties (early-exit bei eigener
Insufficienz).

Blocker #3 (Air-Gap Lücke): die moltrust_verify / moltrust_trust_score /
moltrust_endorse Agent-Tools blieben auch bei minTrustScore=0 +
verifyOnStart=false beim Agent-Runtime registriert — LLM-Halluzinationen
konnten ungewollte DID-Lookups triggern. Neue Config-Option
`registerMoltrustTools` (default true). Wenn false: die 3 registerTool-
Calls werden geskipped, Tools sind für die LLM nicht aufrufbar. Slash-
Commands + RPC + Lifecycle-Hooks bleiben unverändert (explizite
Operator/User-Aktionen, nicht LLM-aufrufbar).

README: Privacy-Section schärft Disable-Anweisung — "Disabling
automatic outbound calls" (alt) vs neue "True air-gap mode" mit
registerMoltrustTools=false. Configuration-Beispiel zeigt neuen
Default-Wert.

Tests: 31 → 32 (1 neuer für kombinatorischen Pfad). Alle 32 grün,
tsc grün.

Re-Re-§12-Review läuft mit alpha.2-Briefing-Append vor npm publish.
MoltyCel added a commit that referenced this pull request Jun 3, 2026
* docs(specs): D-1 Acceptance-Gate architecture brief (design-only)

Scope: AAE draft-04 §5 Step 1 (signature verify + signing-authority) + Step 2 (payload/schema/cty). Decisions: #1 JWS-wrapped VC submit-contract (extract blocks from verified payload; component-1 API/raw_canonical impact named); #2 did:web + did:moltrust launch (did:key follow-on); #3 resolve-and-verify with trust-tiering (trusted vs unverified_issuer, no hard-allowlist); #4 scope = steps 1+2 only (step 4 subject-binding + step 9 delegation = follow-ons). PyJWT 2.12.1 (no new dep). Canonicalization clarity: D-1 verifies JOSE-JWS bytes, not JCS raw_canonical. Open sign-off: DID-resolution depth/SSRF/caching, raw_canonical redefinition, trust-tier persistence.

* docs(specs): resolve 4 D-1 sign-off points (design-only)

1) DID-resolution SSRF/DoS = same egress-proxy as revocation_check (no new mitigation); did:web gated on proxy, D-1 LAUNCHES did:moltrust-only (no outbound, not proxy-gated). 2) raw_canonical = JWS-payload (trigger structurally unchanged); breaking submit-contract change, only smoke-rows affected. 3) trust-tier = new additive column issuer_trust_tier (trusted/unverified_issuer, analog value_source). 4) did:web VM-dereferencing = new layer (resolver gives raw DID-doc only). Phased launch: A did:moltrust-only now, B did:web when egress-proxy live.

* docs(specs): D-1 review-hardening — 4 criticals + 2 mediums resolved (design-only)

alg-confusion (explicit algorithms=[EdDSA] allowlist, never trust header alg); kid strict DID-URL validation + path-traversal/look-alike protection; canonicalization = exact b64url-decoded payload bytes (never re-serialize); submit rate-limit + per-issuer quota (PK already blocks exact replays); did:moltrust registry SPOF -> key rotation; JSON duplicate-keys reject via object_pairs_hook. Implementation contract, not architecture change.

---------

Co-authored-by: Lars Kroehl <kersten.kroehl@cryptokri.ch>
MoltyCel added a commit that referenced this pull request Jul 5, 2026
…nt off) (#210)

* feat: JCS canonicalization + post-quantum dual-signature v2 (rebased, blockers fixed)

Re-implementation of PR #7 against current main (VC Data Model v2.0), fixing
both blockers from the 3-model security review and the side findings.

Upstream review blockers fixed:

1. Downgrade/stripping attack (app/crypto/hybrid.py::verify_proof)
   Verification now uses AND-logic: when a credential carries a list of
   proofs, EVERY proof must be present and valid. A broken Dilithium leg
   fails the whole credential — an attacker cannot strip the PQC leg and
   rely on Ed25519 alone. Composite-signature semantics (BSI TR-02102-1).
   The proof signs the credential body (proof field stripped), so both legs
   bind the same canonical bytes.

2. Private-key stdout leak (scripts/generate_dilithium_keys.py)
   The ML-DSA-65 secret key is NEVER printed to stdout/stderr. It is written
   only to a chmod 600 file (dilithium_secret_key.hex), created with an
   explicit fchmod so it is never group/world-readable even momentarily. A
   --kms mode emits base64 raw bytes for piping into `aws kms encrypt`.

Side findings fixed:

- JCS fail-closed: if the jcs library is missing, dual_sign raises rather
  than emit a proof labelled canonicalizationAlgorithm=JCS that was
  actually produced with json.dumps(sort_keys=True) (false-negative/DoS
  vector).
- Hardcoded proof[0] indexing removed: new app/crypto/proof_utils.py
  extracts proofs by type. The 3 DB-insert sites in main.py + the aae_id
  fallback now use get_primary_proof_value(), which finds the Ed25519
  proof by type — never accidentally persisting a Dilithium proofValue into
  the proof_value column.
- liboqs-python pinned in requirements.txt (>=0.10.2). Imported lazily by
  dilithium.py so the app starts without it; required only when
  DILITHIUM_* env vars are set.

VC v2.0 integration (the old PR was written against v1):

- credentials.py: issue_credential uses dual_sign(); verify_credential uses
  verify_proof(). Both keep the existing validFrom/validUntil v2.0 shape and
  the vc_valid_from/vc_valid_until helpers. Legacy sort_keys credentials
  still verify (backward compat) via the canonicalizationAlgorithm field.
- endorsement.py: signing block uses dual_sign() against the v2.0 VC.
- main.py: DID_WEB_DOCUMENT is built dynamically (_build_issuer_did_document)
  and includes the Dilithium verification method + context when configured.
  /.well-known/did.json rebuilds per-request so a key enabled after startup
  appears without a restart. #key-1 kept as a legacy alias for verifiers
  that still resolve the old id.

Zero-risk deploy unchanged: without DILITHIUM_PRIVATE_KEY_HEX (and no KMS
blob) the system is Ed25519-only, exactly as before.

Locally verified (venv with pynacl+jcs, oqs stubbed):
- Ed25519-only issue + verify (real JCS)
- tampered body rejected
- legacy sort_keys credentials still verify
- dual-signature list issued
- AND-logic: one bad leg fails the whole credential
- JCS fail-closed raises when jcs missing
- py_compile clean on all 8 files including main.py (8273 lines)

* fix(pqc): bind proof skeleton into signature — close downgrade attack

The 3-model review blocker #1 (downgrade/stripping) was not fully solved
by AND-logic alone: my first implementation stripped the proof field
before signing, so an attacker who removed the Dilithium leg left the
Ed25519 signature valid over unchanged bytes — the exact OR-downgrade the
review flagged. A prior test even asserted the wrong behaviour (stripped
single verifies standalone).

Fix per the review explicit requirement (proof field must be included in
the signed message):

- dual_sign now builds a proof SKELETON (every intended proof dict with
  proofValue blanked) and signs the credential body PLUS that skeleton.
  Both Ed25519 and Dilithium sign the same canonicalized skeleton bytes.
- verify_proof reconstructs the same skeleton from the presented proofs.
  If a leg is missing, the skeleton differs from what the issuer signed,
  so the surviving Ed25519 signature fails too. Stripping a leg now
  breaks every remaining signature — composite-signature semantics
  (IETF composite-sigs, BSI TR-02102-1).
- dual_sign refuses to silently fall back to Ed25519-only when Dilithium
  is configured but signing fails (that would re-open the downgrade path:
  an attacker cannot distinguish a deliberate Ed25519-only cred from a
  failed-dual one). It raises instead.
- Legacy sort_keys credentials (no canonicalizationAlgorithm) still verify
  over the body only — backward compat unchanged, and they only ever had
  one leg so there was nothing to strip.

Also fix blocker #2 fully: scripts/generate_dilithium_keys.py --kms mode
previously printed base64(secret_key) to stdout. It now writes the
KMS-ready base64 to a chmod 600 file (dilithium_secret_key.kms.b64)
instead. Runtime proof: script stdout contains only the PUBLIC key; no
secret (hex or base64) reaches stdout/stderr. Secret files are mode 0600.

Verified:
- Downgrade attack (PQC-enabled issuer -> strip Dilithium -> verify) now
  FAILS: Ed25519 signature invalid because skeleton changed.
- Legit Ed25519-only (non-PQC issuer) still verifies.
- Legacy sort_keys credentials still verify.
- Dual both-legs-valid -> valid; one bad leg -> whole cred invalid.
- JCS fail-closed raises when jcs missing.
- py_compile clean on all 8 files.

* fix(pqc): harden input validation, exact-match type dispatch, verifyMethod binding

Fresh security review with a new model found input-validation gaps and
type-confusion risks that the previous version did not cover. None of
these are forgery attacks (the attacker still cannot produce a valid
signature without the key), but they would cause 500 errors on malformed
input and one was a type-confusion code smell.

Input validation — all malformed inputs now return valid=False instead
of raising (which would surface as 500 errors and be a minor DoS vector):

verify_proof:
  - Non-dict credential (None, str, int, list, bool) rejected
  - Non-dict/non-list proof field rejected
  - Non-dict proof entry in a list rejected
  - None / wrong-type verificationMethod rejected
  - None / wrong-type proofValue (None, int, float, list, bytes, "")
    rejected
  - Non-hex proofValue rejected
  - Wrong-length proofValue rejected
verify_credential: same validations plus type checks for the proof
  list normalization.
proof_utils: get_proofs/find_proof/get_primary_proof_value now accept
  non-dict credentials and return empty / raise KeyError instead of
  AttributeError.

Type confusion — proof type dispatch changed from substring to exact
match. Previously, `"Ed25519" in ptype` would match any string containing
"Ed25519" (e.g. "EvilEd25519NotReally"). The attacker still could not
forge a signature, but the type field was effectively untrustworthy.
Now: `ptype == "Ed25519Signature2020"` exact match. Anything else is
rejected as "Unknown proof type".

Defense-in-depth — proof verificationMethod is now cross-checked against
the key being used:
  - Ed25519 proof: verificationMethod must start with
    did:web:api.moltrust.ch#key-ed25519 or #key-1 (legacy alias)
  - Dilithium proof: verificationMethod must start with
    did:web:api.moltrust.ch#key-dilithium
A valid signature over a valid body is no longer enough if the proof
claims to be from a key we are not using.

Dead code removed: use_skeleton (set but never used), dil_sig (set to
the function object, never called).

Coverage: 54 new tests in tests/test_pqc_security.py covering:
  - Downgrade attack (PQC-enabled -> strip Dilithium -> rejected)
  - AND-logic (one bad leg fails whole credential)
  - Swapped proof order rejected
  - Fake Dilithium added to Ed25519-only rejected
  - Legitimate credentials verify (Ed25519-only and dual)
  - Body tamper rejected
  - Extra field in proof rejected
  - Legacy sort_keys backward compat
  - Non-dict / None / wrong-type credential rejected
  - Non-dict / None / wrong-type proof field rejected
  - None / wrong-type verificationMethod rejected
  - None / wrong-type proofValue rejected
  - Non-hex / short / wrong-length proofValue rejected
  - Type confusion ("EvilEd25519NotReally", bare "Ed25519",
    "ed25519signature2020") rejected
  - proof_utils helpers handle non-dict credentials

All 54 tests pass. py_compile clean on all files. The downgrade attack
remains blocked (verified again in the new test suite).

* test: reload credentials module in stub fixture so verify_credential picks up reloaded hybrid

When stub_oqs reloads app.crypto.hybrid, the app.credentials module still
holds the old reference to verify_proof. Reload credentials too so the
TestVerifyCredentialWrapper tests work correctly.

Fixes the 2 failures that appeared when test_credentials_vc_v2.py and
test_pqc_security.py run together.

* fix(pqc): exact-match verificationMethod and proof type, remove latent substring/prefix bugs

Fresh review found three additional hardening gaps:

1. verificationMethod binding used startswith(). An ID like
   did:web:api.moltrust.ch#key-ed25519-attacker would pass the
   defense-in-depth check (no forgery without the key, but the type
   field became untrustworthy). Now exact equality against the allowed
   key-id sets { #key-ed25519, #key-1 } and { #key-dilithium }.

2. proof_utils.find_proof() still used substring matching. Removed:
   exact equality only, so "EvilEd25519NotReally" no longer matches a
   search for "Ed25519Signature2020". No security-critical caller was
   using it, but it was a latent bug.

3. hybrid.verify_proof() had `isinstance(ed25519_verify_key, object)`,
   which is always True for any non-None Python value. Removed the
   useless check; bad keys are caught by the verify() exception handler.

Also removed unused get_ed25519_proof imports from credentials.py and
main.py.

Tests: 71 pass (54 new PQC security + 10 existing VC v2 + 7 new exact-match
regressions). Downgrade attack and all input-validation paths remain
blocked.

* fix(pqc): wrap _load_keypair plaintext hex parsing in try/except

The KMS path already had a try/except around bytes.fromhex(), but the
plaintext env-var fallback did not. A misconfigured DILITHIUM_PUBLIC_KEY_HEX
or DILITHIUM_PRIVATE_KEY_HEX (non-hex string) would raise ValueError,
propagate through is_available() -> dual_sign, and crash credential
issuance with a 500.

Now returns None (PQC off) and logs the error, matching the KMS path
pattern. Added 2 tests covering bad public and bad private key hex.

* fix(pqc): enforce dual-signature policy for PQC-capable issuers + hard-pin liboqs

Addresses both findings from the 3-model review (Gemini + Perplexity):

1. BLOCKER (Perplexity Sonar Pro): verify_proof/verify_credential did not
   enforce that a PQC-capable issuer MUST dual-sign JCS credentials.
   has_dual_signature() existed but was never called. An Ed25519-only JCS
   credential from a PQC-enabled issuer was accepted — a policy downgrade.

   Fix: verify_proof now checks, before the per-proof loop, whether:
   (a) the verifier is PQC-capable (dilithium.is_available()), AND
   (b) the credential uses JCS (new format, detected via _has_skeleton).
   If both are true and the credential is NOT dual-signed, it is rejected
   with "PQC policy violation".

   Legacy credentials (sort_keys, no canonicalizationAlgorithm) are exempt
   — they predate the PQC policy and only ever had one leg.

   Threat model documented in the module docstring.

2. NACHBESSERN (Gemini + Perplexity consensus): liboqs-python>=0.10.2 was
   a minimum version, not a hard pin. Pre-1.0, not FIPS-validated C-binding
   → supply-chain risk. Hard-pinned to ==0.10.2.

Tests: 78 pass (5 new PQC policy tests + 1 new skeleton-binding test).
- PQC issuer Ed25519-only JCS → rejected (policy)
- PQC issuer dual-signed → accepted
- Non-PQC verifier Ed25519-only JCS → accepted
- PQC verifier legacy sort_keys → exempt (accepted)
- Strip Dilithium breaks Ed25519 signature even without policy (skeleton)

* fix(pqc): pin liboqs-python to actual released version + fix policy trigger

Two issues caught by CI:

1. liboqs-python==0.10.2 does not exist on PyPI. The 3-model review
   cited 0.10.2 as the pin target, but the actual released versions are
   0.14.1 and 0.15.0 (verified via `pip index versions liboqs-python`).
   The hard pin to a non-existent version broke `pip install` in CI:
   "Could not find a version that satisfies the requirement
   liboqs-python==0.10.2". Fixed to ==0.15.0 (latest stable).

2. The PQC policy trigger in verify_proof used
   `dilithium.is_available()`, which calls _load_keypair() and therefore
   requires the SECRET key (or working KMS). If the public key is
   configured but KMS is temporarily down, is_available() returns False
   and the policy is silently skipped — an Ed25519-only JCS credential
   from a PQC-capable issuer would be accepted. The policy should fire
   on public-key declaration alone, not on full keypair availability.

   Fix: added `dilithium.public_key_configured()` which checks the env
   var directly. Updated verify_proof to use it. Updated _setup_pqc test
   helper to stub the new function. Added 4 tests:
   - Policy fires when public key configured but is_available() False
   - public_key_configured returns False when env var unset
   - public_key_configured returns True when env var set
   - public_key_configured returns False for whitespace-only env var

Tests: 82 pass (was 78). py_compile clean. pip install -r requirements.txt
resolves cleanly with ==0.15.0.

* fix(pqc): error propagation, canonicalization exception handling, proofValue length cap

Fresh cross-check found three issues:

1. verify_credential dropped verify_proof's explicit error when no checks
   array existed. verify_proof early returns (e.g. PQC policy violation,
   "credential is not a dict", "No proof found") only set `error`, not
   `checks`. verify_credential aggregated errors from `checks`, producing
   an empty `error` string. Fixed by preferring `result["error"]` when
   present.

2. verify_proof only caught RuntimeError around _signed_payload. A
   malformed JCS credential (e.g. non-serializable value in the body) could
   raise ValueError/TypeError from _canonicalize and bubble up, causing a
   500 when verify_proof is called directly. Fixed by catching Exception
   and returning it as a check error.

3. No length limit on proofValue before bytes.fromhex. A malicious
   multi-megabyte hex string would be accepted and decoded, causing memory
   DoS. Added _MAX_PROOFVALUE_HEX_LEN = 20000 (generous margin over the
   largest expected ML-DSA-65 sig of 6618 hex chars). Rejected proofValue
   returns valid=False with "too long" error.

Tests: 85 pass (was 82). Added:
- test_verify_credential_preserves_policy_error
- test_verify_credential_preserves_early_error
- test_very_long_proofvalue_rejected

* feat(pqc): make dual-signature policy advisory by default (PQC_ENFORCE)

The PQC dual-signature capability stays in the credential format and the
verify path, but is no longer hard-enforced by default. A central switch
PQC_ENFORCE (env, default off) gates it:
  - off (default): the policy check runs and its outcome is surfaced
    (pqc_policy: would_reject) and logged, but a missing Dilithium leg does not
    fail verification -- existing Ed25519-only issuers keep working.
  - on: reject a PQC-capable issuer single-signed JCS credential, as before.
Legacy sort_keys credentials remain exempt. The liboqs pin, proofValue DoS cap
and error propagation from f3817d4 are unchanged.

Tests: split the policy test into advisory-accept + enforce-reject; the
reject-path tests now run under PQC_ENFORCE=true. Adds an ADR note.

Builds on Harald f3817d4 (his commits preserved as ancestors).

---------

Co-authored-by: Harald Roessler <harald.roessler@dsncon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant