Skip to content

feat(ledger): safe ledger signing and verification provider diagnostics (XWING-2440) - #148

Merged
smith-xyz merged 4 commits into
traust-security:developmentfrom
smith-xyz:feat/safe-ledger-signing-and-verification-provider-di
Oct 31, 2026
Merged

smith-xyz merged 4 commits into
traust-security:developmentfrom
smith-xyz:feat/safe-ledger-signing-and-verification-provider-di

Conversation

@smith-xyz

@smith-xyz smith-xyz commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Plan: plans/traust-standardization/7-ledger/07-shaun-ledger-provider-diagnostics.md

What

  • add one bounded terminal standard-library logging event to each Cosign and Sigstore signing/verification provider boundary
  • report only operation, provider, generic method, outcome, reason and duration while preserving existing SignResult/VerifyResult values and error details for callers
  • keep Merkle/frontier/canonicalization code silent and configure no process handler; the library installs only a NullHandler
  • distinguish cryptographic invalidity from operational failure: malformed bundles are invalid, while opaque CLI exits and provider/setup exceptions are failed
  • emit failed/unexpected_error before re-raising unexpected boundary exceptions so every provider path remains observable without changing exception behavior

Review resolutions

Three independent read-only reviews covered terminal-path completeness, sensitive-data flow, logger behavior and test coverage.

  • wrapped complete provider boundaries so unexpected file/import/setup exceptions emit exactly one bounded event before being re-raised
  • changed opaque Cosign verification exits and Sigstore setup/provider errors from invalid to failed
  • made rendered and structured operation names agree (ledger.sign / ledger.verify)
  • expanded tests across levels, duration, success, nonzero/timeout, missing/empty/published bundles, dependency/configuration/credential/policy refusal, provider failure, invalid bundle and unexpected exceptions

Verification

  • make lint-fix
  • TMPDIR=/tmp make test: 306 passed
  • push hooks: lint, format and 306 tests passed
  • no SQL or repository query changed; no perf run required

Reviewer checks (from the plan)

  • Does every provider path terminate in at most one useful event, and can any signing secret, identity, document, command or provider diagnostic escape?

Quality checklist

  • 1. Comments only for the non-trivial
  • 2. Docstrings don't bloat
  • 3. Don't reinvent
  • 4. Shared logic is a helper, in one place
  • 5. Tests cover the edges
  • 6. Integration tests are thorough and use real data, sanitized
  • 7. Minimal, clean code
  • 8. Queries are low latency, and you checked (no query changed)
  • 9. Tests change only on purpose, and a person said so (new test file only; no existing test changed)

Legacy files

File Verdict Where it went / why it's gone
None reuse Provider adapters and typed result contracts remain in place; only their external boundaries gain diagnostics

Changed tests

No existing test expectation changed. tests/test_provider_logging.py is new coverage for the added diagnostic contract.

Dependencies

This PR is independently reviewable. Core consumer registration and end-to-end correlated integration are delivered separately.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2026
@openshift-ci

openshift-ci Bot commented Oct 30, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smith-xyz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2026
@smith-xyz
smith-xyz marked this pull request as ready for review October 30, 2026 19:43
@smith-xyz
smith-xyz requested a review from a team as a code owner October 30, 2026 19:43
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2026
@qodo-for-traust-security

Copy link
Copy Markdown

PR Summary by Qodo

Add safe terminal diagnostics to ledger signing providers

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes


AI Description

• Emit bounded terminal diagnostics for Cosign and Sigstore signing and verification without logging
 sensitive inputs.
• Distinguish invalid bundles, provider failures, and policy refusals while preserving caller-facing
 results and errors.
• Add tests for event completeness, classifications, log levels, and unexpected exceptions.
Diagram

graph TD
  Caller["Ledger callers"] --> Boundary["Sign or verify"] --> Cosign["Cosign CLI"] --> Outcome{"Terminal outcome"} --> Logger["Module logger"] --> Host["Host logging"]
  Boundary --> Sigstore["Sigstore SDK"] --> Outcome
  Outcome --> Results["Typed results"]
Loading
High-Level Assessment

Boundary-local diagnostics fit this change: providers can classify their own failures without exposing inputs or changing result contracts. Logging only in callers would miss unexpected provider-boundary exceptions, so it offers no clear advantage.

Files changed (2) +549 / -53

Enhancement (1) +259 / -53
signing.pyEmit bounded diagnostics at Cosign and Sigstore boundaries +259/-53

Emit bounded diagnostics at Cosign and Sigstore boundaries

• Adds shared terminal-event helpers and a module NullHandler. All four sign and verify methods report classified outcomes and durations, including unexpected exceptions, while retaining existing result values and caller-facing error details.

src/traust_ledger/integrity/signing.py

Tests (1) +290 / -0
test_provider_logging.pyTest safe, single-event provider diagnostics +290/-0

Test safe, single-event provider diagnostics

• Adds coverage for successful operations, provider and process failures, bundle and configuration conditions, refusals, and re-raised exceptions. Tests assert event fields, levels, duration, single-event behavior, and exclusion of sensitive inputs from logs.

tests/test_provider_logging.py

@qodo-for-traust-security

qodo-for-traust-security Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Code Review by Qodo


Resolved findings
1. Logging errors can replace signing results ✓ Resolved
Description
_emit_terminal creates a log record with unconditional extra fields, and
_terminal_on_unexpected also catches exceptions raised while emitting a normal result's event. If
an application's logging configuration already defines one of those fields on its records, record
construction raises; the wrapper tries to emit again and the logging exception escapes instead of
the provider result or original exception.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A logging record-field collision can raise during terminal emission, replacing a successful or failed provider result and prompting a second emission attempt.

## Fix Focus Areas
- src/traust_ledger/integrity/signing.py[38-55]
- src/traust_ledger/integrity/signing.py[63-78]
- src/traust_ledger/integrity/signing.py[85-103]

## Recommended Fix
Make terminal emission best-effort without allowing logging exceptions to replace provider results or exceptions. Ensure the unexpected-exception wrapper does not treat an emission failure as a new provider failure, and test with a colliding log-record field.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Rejected identities appear as provider failures ✓ Resolved
Description
SigstoreOIDCBackend.verify labels every exception from verifier.verify_artifact as
failed/provider_error, including a certificate identity or issuer mismatch. When a parsed bundle
fails those verification checks, the terminal event reports an operational failure rather than a
rejected signature, making the two cases indistinguishable in diagnostics.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Sigstore verification records identity and issuer mismatches as operational provider failures because its broad exception handler assigns every verification exception the same outcome.

## Fix Focus Areas
- src/traust_ledger/integrity/signing.py[631-646]
- tests/test_provider_logging.py[248-290]

## Recommended Fix
Distinguish the verification library's cryptographic or policy rejection from setup and operational exceptions, emit an `invalid` outcome for rejection, and retain `failed` for operational errors. Add a logging test for a rejected identity or issuer.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Comment thread src/traust_ledger/integrity/signing.py Resolved
Comment thread src/traust_ledger/integrity/signing.py Resolved
@smith-xyz
smith-xyz merged commit 5f10352 into traust-security:development Oct 31, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant