nigig-org/REVIEWS/adr/0027-pdf-phase6-completion.md

# ADR 0027: finishing Phase 6 — PAdES, external signing, OCSP, and a status line that stops lying

- **Status:** Accepted
- **Date:** 2026-08-18
- **Review item:** the Phase 6 bullets that ADR 0025 and ADR 0026 left
  unimplemented while the plan's status line called the phase complete
- **Related:** ADR 0025 (signing), ADR 0026 (the security review), ADR 0021
  (the process note this ADR is the second violation of), ADR 0024
  (encryption — the RC4 precedent for refusing a weak algorithm)

## Context

Asked whether Phase 6 was 100% complete, I checked the plan's bullets
against the code instead of answering from the status line. Five were not
implemented, and the status line named none of them:

| Bullet | Actual state |
|---|---|
| "detached/**attached** signatures" | `/SubFilter` **hardcoded** to `adbe.pkcs7.detached` at `sign.rs:1401` |
| "PAdES basics" | `ETSI.CAdES.detached` existed only as a string in a name-mapping `match` |
| `external_signing_test.dart` | absent — `SigningIdentity` requires an in-memory key |
| "OCSP/CRL lookup" | CRL only; an OCSP response was *counted*, never parsed |
| Fulcio-style identity | absent (the plan marks it optional) |

**This is the second time.** ADR 0021 recorded the same failure in Phase 4
and wrote the rule meant to prevent it — enumerate criteria from the plan
text first, then mark each done or explicitly deferred. I wrote that rule
and then produced another prose status line summarising what I had built.
A summary written from the work cannot show what the work omitted.

## Decision

### PAdES / CAdES, as a real profile rather than a label

`SignatureProfile` selects `Pkcs7Detached` or `Cades`, and the difference is
structural, not cosmetic:

- **PKCS#7 detached** signs the document bytes directly.
- **CAdES** signs a set of **signed attributes** (RFC 5652 §5.3), one of
  which — `messageDigest` — carries the document's hash.

Two things follow, and both are load-bearing. First, the signature is
computed over the attributes re-tagged as a `SET` (§5.4), not over the
`[0] IMPLICIT SEQUENCE` they are carried in; getting that backwards
produces a signature that verifies against nothing. Second, verification
must check the `messageDigest` attribute against the document *as well as*
verifying the attribute signature — otherwise a signature over somebody
else's digest would be accepted. `a_cades_signature_does_not_verify_against_
other_content` is the test for exactly that.

`/SubFilter` now comes from the profile. A document claiming
`ETSI.CAdES.detached` while carrying a plain PKCS#7 signature is malformed
even though both halves are individually valid.

### External signing

`ExternalSigner` is a trait: take bytes, return a signature, declare the
algorithm and the certificate chain. A smartcard, HSM or cloud KMS never
hands out its private key, so `SigningIdentity` — which owns one — could not
represent any of them.

`SigningIdentity` **implements** the trait rather than sitting beside it, so
there is one signing path and not two. A second path for hardware keys
would be a second place the byte range could be computed differently, and
that is the most dangerous divergence this module could have.

### OCSP

`ocsp_status_for_serial` decodes the certID serial and the `certStatus`
CHOICE tag directly with the `der` crate already present, rather than adding
the `ocsp` crate for two fields. The audit surface of a dependency is a real
cost against reading a tag byte.

`Revoked` from any response wins over `Good` from any other. The asymmetry
between `Revoked` and `Good` from ADR 0026 stands and is now written down in
the function's own documentation: neither CRLs nor OCSP responses are
signature-checked, so `Revoked` is trustworthy — nobody forges a revocation
against themselves — while `Good` means only "this is what the stapled
material says".

### Attached signatures: refused, not deferred

The two attached profiles are `adbe.pkcs7.sha1` and `adbe.x509.rsa_sha1`.
Both are **SHA-1 based**, and SHA-1 is broken for signature purposes.

They are parsed, so documents using them can be read and reported. They
cannot be **written**: there is no `SignatureProfile` variant for either, so
the refusal is enforced by the type rather than by a check. This is ADR
0024's RC4 decision applied to signatures — a library that offers a broken
algorithm as an option will have it selected by somebody who does not know
it is broken.

Recording this as *refused* rather than *not done* matters. "Not done"
invites someone to finish it.

## Verification

13 new tests in `signing_profiles.rs`, alongside the existing 42 across
`signing_roundtrip.rs`, `signing_security.rs` and
`sign_document_roundtrip.rs`.

The ones that carry weight:

- **`a_cades_signature_does_not_verify_against_other_content`** — the
  `messageDigest` check. Without it, CAdES verification would accept a
  signature over a different document's hash.
- **`a_cades_signature_is_structurally_different_from_pkcs7`** — if the two
  profiles produced identical bytes, `SignatureProfile` would be a label
  with no effect, which is precisely what the hardcoded `/SubFilter`
  amounted to.
- **`signing_a_document_as_cades_declares_the_cades_subfilter`** — asserts
  the document does not lie about its own profile.
- **`an_external_signer_that_fails_is_reported_not_swallowed`** — an
  unplugged token must surface as an error; a signing call that quietly
  produces an unsigned document is the worst available outcome.
- **`revoked_beats_good_across_several_responses`** — a certificate called
  good by one responder and revoked by another is revoked.

Mutation testing, four mutations, **all killed on the first attempt**:

| Mutation | Tests failed |
|---|---|
| `messageDigest` not compared with the document | 1 |
| CAdES verified against document bytes, not attributes | 3 |
| `/SubFilter` hardcoded again | 1 |
| OCSP `revoked` read as `good` | 2 |

**External:** `qpdf --check` accepts documents signed under **both**
profiles — no syntax or stream encoding errors.

**Suite:** `TEST_TARGET=pdf` **1289 passed, 0 failed** (was 1276).

## Merge criteria

Taken from the Phase 6 plan text, item by item — not from what was built:

- [x] Document-byte-range signing
- [x] CMS/PKCS#7 structure
- [x] Detached signatures
- [x] Attached signatures — **refused by design**, SHA-1 only, enforced by
      the absence of a `SignatureProfile` variant
- [x] PAdES basics — CAdES profile with signed attributes
- [x] RSA identities
- [x] ECC / Ed25519 identities
- [x] External signing (`external_signing_test.dart`)
- [x] PKIX certificate-chain validation
- [x] Signature appearance generation
- [x] Revocation: CRL
- [x] Revocation: OCSP
- [x] Permission flags
- [x] Encryption on save, AES-128 and AES-256
- [x] Exit: sign → verify round-trip
- [x] Exit: encrypted-write round-trip
- [x] Four mutations, all killed
- [x] Both profiles validate under qpdf
- [ ] **Fulcio-style identity** — optional in the plan; needs an OIDC round
      trip and a CA call, which is network access the engine is gated
      against
- [ ] **Independent security review** — ADR 0026 was a self-review
- [ ] **Acrobat interoperability** — untested; Acrobat is stricter than the
      specification
- [ ] **Signing a document that already has an AcroForm** — the form is
      replaced, not merged

## Consequences

**Positive.** Every Phase 6 bullet is implemented, refused with a reason, or
listed unticked above. PAdES makes the output usable in European
eIDAS-adjacent workflows; `ExternalSigner` makes hardware-backed keys usable
at all, which is how signing is actually done anywhere it matters legally.

**Negative.** `SignatureProfile` is a second thing a caller must choose
correctly. The default (`Pkcs7Detached`) is the safe one, and choosing wrong
produces a valid signature under the wrong profile rather than an insecure
one.

**Risk.** The CAdES attribute set is minimal: `contentType` and
`messageDigest` only. Full PAdES B-B additionally wants
`signingCertificateV2`, which binds the signature to a specific certificate
and closes a substitution attack that PKIX chain validation already covers
here. Adding it is straightforward and is not claimed.

**Process consequence.** The status line is now the plan's own bullet list
in a table, not prose. Prose invites summarising; a table with a row per
spec item makes an omission visible as a missing row. Two wrong status
lines in the same direction is a pattern, and the fix is structural rather
than a resolution to be more careful.