chore(security): Ed25519 signing review + 3 fixes by remyluslosius · Pull Request #397 · Hanalyx/OpenWatch

remyluslosius · 2026-04-14T12:14:05Z

Mandatory security review per Q1-Q3 plan governance.

Summary

Manual review of the Ed25519 evidence signing service, routes, and audit-export integration. Tooling clean (Bandit + Semgrep 0 findings). Manual review found 1 HIGH, 2 MEDIUM, 2 LOW. HIGH and both MEDIUMs fixed in this PR; LOWs filed as #395 and #396.

Findings fixed

ID	Severity	Issue	Fix
SEC-SIGN-01	HIGH	Private key fallback to plain base64 when EncryptionService missing — silently insecure-at-rest, violates spec AC-8	Hard-fail unless explicit `OPENWATCH_SIGNING_DEV_MODE=true` env var set
SEC-SIGN-02	MEDIUM	Race condition on `generate_key()` — UPDATE+INSERT in two unwrapped statements, concurrent rotations could leave two active rows	`SELECT ... FOR UPDATE` row lock before deactivation
SEC-SIGN-03	MEDIUM	Silent signing failure on audit export — caught Exception, logged warning, produced unsigned export with no signal to auditor	Write explicit `signed_bundle: null` + `signing_error` so unsigned state is machine-detectable

Findings deferred

SEC-SIGN-04: Per-endpoint rate limit + size cap on /api/signing/verify #395 SEC-SIGN-04: per-endpoint rate limit + size cap on `/api/signing/verify` (LOW)
SEC-SIGN-05: Add key revocation flag for compromise scenarios #396 SEC-SIGN-05: key revocation flag for compromise scenarios (LOW)

Tool results

Tool	Scope	Findings
Bandit 1.9.4 (high+medium)	signing service + routes (416 LOC)	0
Semgrep p/security-audit + p/owasp-top-ten + p/python + p/secrets	same	0 / 239 rules

Manual review — positive controls confirmed

Area	Finding
Algorithm	Ed25519 (modern, no parameter choices, fixed output, fast)
Key generation	`Ed25519PrivateKey.generate()` (CSPRNG)
Canonical signing	`json.dumps(..., sort_keys=True, separators=(",", ":"))` — deterministic
Verify failure mode	Returns False, no leak of why verification failed
RBAC on sign	`@require_role([SECURITY_ADMIN, SUPER_ADMIN])`
Public verify	Unauthenticated by design (auditors don't need OpenWatch creds)
Rotation	Old keys remain in DB for verifying historical bundles

Test coverage

Added regression test `test_no_silent_plain_base64_fallback` in `test_evidence_signing_spec.py` (AC-8 section) that enforces the dev-mode gate convention. Source-inspection pattern matches the codebase convention.

`check-spec-coverage.py --enforce-active`: 813/813 ACs covered.

Governance carve-outs (not addressed by automated review)

Key management runbook — operators are responsible for the lifecycle of `OPENWATCH_MASTER_KEY` (the EncryptionService root key); compromise of that key compromises all signing keys
External auditor verification flow — the public verification endpoint and any companion `verify-bundle.py` script need a human-readable runbook published alongside signed exports
Cryptoperiod policy — NIST SP 800-57 §5.3.6 recommends Ed25519 cryptoperiods ≤2 years; OpenWatch should establish a rotation schedule (suggest annual)

Test plan

Bandit + Semgrep scan results documented
Manual code review completed (3 modules, 416 LOC)
Three fixes applied with regression test for HIGH
Follow-up issues SEC-SIGN-04: Per-endpoint rate limit + size cap on /api/signing/verify #395 and SEC-SIGN-05: Add key revocation flag for compromise scenarios #396 filed
Spec coverage 813/813 maintained
black + isort + flake8 + mypy + bandit pre-commit hooks pass
CI pipeline passes

Mandatory security review per Q1-Q3 plan governance. Tooling clean: Bandit 0 findings, Semgrep 0 findings on 239 rules. Manual review found 5 items. Fixes in this PR: SEC-SIGN-01 (HIGH): private key fallback to plain base64 - signing_service.py silently stored private keys as plain base64 when EncryptionService was missing. Production deploys that misconfigured the service got insecure-at-rest storage with no signal. - Fix: hard-fail in generate_key() and sign_envelope() unless explicit OPENWATCH_SIGNING_DEV_MODE=true env var is set (test/dev only). - Spec AC-8 was being violated whenever the fallback path was hit. SEC-SIGN-02 (MEDIUM): race condition on key generation - generate_key() ran UPDATE then INSERT in two separate statements with no transaction wrapping. Concurrent rotations could leave two rows with active=true. - Fix: SELECT ... FOR UPDATE on the active row before deactivating, taking a row-level lock that serialises concurrent calls. SEC-SIGN-03 (MEDIUM): silent signing failure on audit export - audit_export.py caught all exceptions from sign_envelope() and proceeded with an unsigned export. Auditors downloaded what they thought was signed evidence without any signal of the failure. - Fix: write explicit signed_bundle: null + signing_error fields to the export so the unsigned state is machine-detectable from the artifact itself. Follow-ups filed (LOW, not exploitable): - SEC-SIGN-04: verify endpoint DoS surface (per-endpoint rate limit) - SEC-SIGN-05: no key revocation flag (rotation-only today) Tests: added regression test for SEC-SIGN-01 to enforce the dev-mode gate convention and prevent future silent fallbacks. Review document: docs/SIGNING_SECURITY_REVIEW_2026-04-14.md

+        OPENWATCH_SIGNING_DEV_MODE so production misconfiguration surfaces
+        loudly instead of silently producing forgeable bundles.
+        """
+        import app.services.signing.signing_service as mod


Direct action from the Kensa↔OpenWatch coordination. Per-transaction evidence envelope signing moves to Kensa (where the evidence originates); OpenWatch signs only aggregate artifacts it itself produces. Kensa response §2.2 confirmed the trust-layer boundary: Kensa signs: per-transaction evidence envelope at capture/execute time. Attests: "This execution happened on this host at this time." OpenWatch signs: audit exports, quarterly posture reports, State-of- Production releases. Attests: "OpenWatch aggregated this data from N hosts and produced this artifact." Changes: - Remove POST /api/transactions/{id}/sign endpoint. Handler function deleted; module docstring documents the removal and the boundary. - SigningService docstring rewritten with a trust-layer boundary diagram and a clear "aggregate artifacts only" statement. - specs/services/signing/evidence-signing.spec.yaml bumped to v2.0: - Former AC-6 (per-transaction sign endpoint) replaced with AC-6 for /api/signing/verify - New AC-9: per-transaction signing explicitly out of scope - AC-7/AC-8 expanded with the SEC-SIGN-01 dev-mode gate and SEC-SIGN-03 machine-detectable unsigned-state guarantees - context: block documents the scope-narrow rationale and Kensa convergence at Week 22 (Kensa.VerifyEnvelope) - Regression tests: AC-9 added enforcing the endpoint is gone and the module docstring names the boundary. AC-8 tightened with the explicit signed_bundle=None + signing_error assertions from audit_export. - docs/SIGNING_SECURITY_REVIEW_2026-04-14.md updated with a "Scope narrow" section at the top documenting what was removed, what remains, and how OpenWatch verifies Kensa-signed envelopes at Kensa Week 22. No change to the five original security findings (SEC-SIGN-01/-02/-03 remain fixed in PR #397; SEC-SIGN-04/-05 remain open as #395/#396). Spec coverage: 824/824 ACs (was 823, +1 for AC-9).

…cts (#400) Direct action from the Kensa↔OpenWatch coordination. Per-transaction evidence envelope signing moves to Kensa (where the evidence originates); OpenWatch signs only aggregate artifacts it itself produces. Kensa response §2.2 confirmed the trust-layer boundary: Kensa signs: per-transaction evidence envelope at capture/execute time. Attests: "This execution happened on this host at this time." OpenWatch signs: audit exports, quarterly posture reports, State-of- Production releases. Attests: "OpenWatch aggregated this data from N hosts and produced this artifact." Changes: - Remove POST /api/transactions/{id}/sign endpoint. Handler function deleted; module docstring documents the removal and the boundary. - SigningService docstring rewritten with a trust-layer boundary diagram and a clear "aggregate artifacts only" statement. - specs/services/signing/evidence-signing.spec.yaml bumped to v2.0: - Former AC-6 (per-transaction sign endpoint) replaced with AC-6 for /api/signing/verify - New AC-9: per-transaction signing explicitly out of scope - AC-7/AC-8 expanded with the SEC-SIGN-01 dev-mode gate and SEC-SIGN-03 machine-detectable unsigned-state guarantees - context: block documents the scope-narrow rationale and Kensa convergence at Week 22 (Kensa.VerifyEnvelope) - Regression tests: AC-9 added enforcing the endpoint is gone and the module docstring names the boundary. AC-8 tightened with the explicit signed_bundle=None + signing_error assertions from audit_export. - docs/SIGNING_SECURITY_REVIEW_2026-04-14.md updated with a "Scope narrow" section at the top documenting what was removed, what remains, and how OpenWatch verifies Kensa-signed envelopes at Kensa Week 22. No change to the five original security findings (SEC-SIGN-01/-02/-03 remain fixed in PR #397; SEC-SIGN-04/-05 remain open as #395/#396). Spec coverage: 824/824 ACs (was 823, +1 for AC-9).

github-advanced-security AI found potential problems Apr 14, 2026

View reviewed changes

Comment thread tests/backend/unit/services/signing/test_evidence_signing_spec.py

OPENWATCH_SIGNING_DEV_MODE so production misconfiguration surfaces

loudly instead of silently producing forgeable bundles.

"""

import app.services.signing.signing_service as mod

remyluslosius merged commit 24fd72e into main Apr 14, 2026
20 of 21 checks passed

remyluslosius deleted the chore/signing-security-review branch April 14, 2026 12:56

remyluslosius mentioned this pull request Apr 14, 2026

chore(signing): narrow scope to OpenWatch-originated aggregate artifacts #400

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(security): Ed25519 signing review + 3 fixes#397

chore(security): Ed25519 signing review + 3 fixes#397
remyluslosius merged 1 commit into
mainfrom
chore/signing-security-review

remyluslosius commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

remyluslosius commented Apr 14, 2026

Summary

Findings fixed

Findings deferred

Tool results

Manual review — positive controls confirmed

Test coverage

Governance carve-outs (not addressed by automated review)

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants