feat: add signed package manifest flow - #2287
Conversation
|
claude's review: Review:
|
| File | Purpose |
|---|---|
| trestle/core/signing_manifest.py (new) | Manifest loading/validation, SigningManifest/ManifestArtifact dataclasses, Statement building, verification logic |
| trestle/core/commands/sign_manifest.py, verify_manifest.py (new) | CLI command classes, thin wrappers over the above |
| trestle/core/signing.py (extended) | Reused sign_in_toto_statement, verify_dsse_payload, load_in_toto_statement, DSSE PAE helpers |
| trestle/core/beta_features.py | New json-manifest-signing beta flag |
| trestle/cli.py | Command registration |
| docs/predicates/oscal-package/v1.md, docs/tutorials/cli.md, docs/index.md | Docs for the predicate schema and CLI usage |
| Two new test files (~740 lines) | Unit tests for signing_manifest.py and CLI-level round-trip/tamper tests |
Strengths
- Good reuse of existing primitives. Rather than reinventing DSSE/PAE handling, this builds on the existing
signing.pyhelpers (sign_in_toto_statement,verify_dsse_payload,dsse_pae), keeping the crypto path in one place. - Careful path handling.
_artifact_pathrejects absolute URIs and URIs with a scheme, and confirms the resolved path stays within the manifest's directory (relative_tocheck) — a sensible guard against path traversal via a malicious or malformed manifest. - Constant-time digest comparisons. Both
signing.pyandsigning_manifest.pyusehmac.compare_digestfor digest/signature comparisons instead of==. - Output-path safety in
write_dsse_envelope. Refuses to overwrite an existing file, refuses a symlink target, and refuses when output path collides with the manifest or key path (checked insign_manifest.py) — avoids silently clobbering artifacts. - Strict, closed-world schema validation.
_reject_unknown_fieldsrejects unrecognized manifest/artifact keys rather than silently ignoring them, and duplicate artifact names/subject names are explicitly rejected. - Predicate equality check is exact-match, not subset.
verify_manifest_envelopecompares the entire predicate dict for equality, so an attacker can't smuggle in an extra field or omitprimaryArtifactwithout detection. - Beta-gating is applied consistently via the existing
@beta_featuredecorator pattern, matching howtrestle sign/verifyare gated — no special-casing. - Decent test coverage across both the pure-logic layer (
signing_manifest_test.py) and CLI/round-trip behavior (tampered envelope, wrong key, changed artifact, changed manifest, encrypted private key, beta-flag gating).
Points worth checking / discussing
-
YAML loading uses
YAML(typ='safe')— good, this avoids arbitrary object deserialization. Worth confirming ruamel'ssafeloader is pinned to a version where this guarantee holds, but this is a reasonable default. -
Artifact digesting re-parses and re-canonicalizes on every verify.
_artifact_digestcallsload_canonical_json_fileper artifact per verification — fine functionally, but for large packages with many artifacts this means re-reading/re-serializing every file on eachverify-manifestcall. Not a correctness issue, just a scalability note if manifests grow large. -
No explicit artifact count / manifest size limit. Unlike
signing.py'sMAX_SIGNATURES = 16cap on DSSE signatures,signing_manifest.pydoesn't cap the number of artifacts in a manifest. Probably fine since this is a local CLI tool (not a network-facing verifier), but worth a thought if manifests could ever come from untrusted sources. -
primaryArtifactis recorded but not otherwise enforced. It's just a labeled field in the predicate — verification doesn't appear to treat the primary artifact any differently from the rest (e.g., no requirement that it be verified first or exclusively). That seems intentional given the "digest-only" scope, but the semantic meaning of "primary" beyond documentation purposes isn't obvious from the code — might be worth a one-line clarifying comment on intended future use. -
Predicate type URI is a GitHub Pages URL (
https://oscal-compass.github.io/compliance-trestle/predicates/oscal-package/v1) that, per the docs, is "not fetched during verification" — good, this avoids a live network dependency during signature checks, and is called out explicitly indocs/predicates/oscal-package/v1.md. -
Minor: exception handling in
_runmethods is broad (except Exception as e: # pragma: no cover) — consistent with the existingsign/verifycommands, so not a regression, just inherited style. -
Docs are clear and cross-linked (CLI tutorial ↔ predicate spec ↔ index), and the "Before you merge" checklist items are all ticked. The PR is explicitly scoped ("does not auto-discover... does not enforce per-document signatures") which sets good expectations for reviewers and future maintainers.
Overall
This is a focused, well-tested addition that mostly reuses proven code from the existing single-file signing path rather than introducing new cryptographic logic. The path-traversal guard, constant-time comparisons, and strict schema validation are the right defensive choices for a manifest format that could plausibly be hand-edited or come from a less-trusted source. No blocking issues found; the notes above are mostly scalability/clarity observations rather than correctness concerns.
butler54
left a comment
butler54
left a comment
There was a problem hiding this comment.
Overal it looks pretty solid to me. I've got one big question which is how we define the manifest as yaml vs json.
| '--signature', help='Path to the detached DSSE package envelope.', required=True, type=pathlib.Path | ||
| ) | ||
| self.add_argument( | ||
| '--key', help='Path to the PEM public key for verification.', required=True, type=pathlib.Path |
There was a problem hiding this comment.
A nit - we have used key for the private key. Can we be consistent across all commands for public vs private key arguments?
e.g. this can become --public-key
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """Trestle Sign Manifest Command.""" |
There was a problem hiding this comment.
There is one meta discussion point that' I'm confused with - which is why are our manifests for our json artifacts yaml?
Also a formal schema for the manifest should exist. (if it's json that is easier).
There was a problem hiding this comment.
initially I chose YAML because it's more readable, but if we’re aiming toward automatically generated manifests and using tooling rather than manual editing, JSON makes more sense.
Signed-off-by: Matteo Fari <matteofari06@gmail.com>
|
Switched package manifests from YAML to strict JSON, added a JSON Schema, renamed verification’s key option to |
b8198dc to
f3c1dcf
Compare
3 participants
Types of changes
develop->main)Quality assurance (all should be covered).
Summary
Adds beta support for signing and verifying YAML-defined JSON package manifests.
This introduces trestle
sign-manifestand trestleverify-manifest. The manifest explicitly lists package artifacts, trestle canonicalizes each listed JSON file, computes SHA-256 digests, builds an in-toto Statement containing all artifact subjects, and signs that Statement as a detached DSSE envelope.This PR is digest-only package verification. It does not auto-discover OSCAL dependencies, enforce per-document .dsse signatures.
Key links:
Before you merge