Skip to content

feat: G2 code-signing verifier (Mode-1) with legacy G1 transition#41680

Open
DeepDiver1975 wants to merge 27 commits into
masterfrom
feature/g2-code-signing-verifier
Open

feat: G2 code-signing verifier (Mode-1) with legacy G1 transition#41680
DeepDiver1975 wants to merge 27 commits into
masterfrom
feature/g2-code-signing-verifier

Conversation

@DeepDiver1975

Copy link
Copy Markdown
Member

Description

Reimplements the verification half of OC\IntegrityCheck\Checker for the new
"G2" code-signing PKI (ECDSA-P384 primary, RSA-4096-PSS fallback), with CRL-based
revocation and a time-boxed transition path for legacy G1 signatures. The signing
half (writeAppSignature/writeCoreSignature, occ integrity:sign-*) is left
untouched — it is superseded by the standalone ocsign tool.

Verification now accepts the new signature.json v2 format
({v, alg, hashes, signature, certificates:{leaf, chain[]}}) as well as the legacy
format, validates the certificate chain against bundled trust anchors, verifies the
manifest signature over the canonical bytes M, enforces CRL revocation
(fail-closed), checks appId/CN identity, and produces the same structured
integrity diff the admin UI already consumes.

The new logic lives in focused, independently-tested collaborators under
lib/private/IntegrityCheck/Verifier/; Checker::verify() delegates to them,
preserving the public API, cache, result-shape contract, and all callers.
Mode-2 attestation is intentionally out of scope (blocked upstream on the
undecided bind(H,T) token encoding).

Components

  • CanonicalManifest — byte-exact reconstruction of the signed manifest M
    (gated by shared golden vectors copied from the ocsign repo).
  • SignatureEnvelope — v2 + legacy parser; extracts the verbatim hashes byte-span
    (no re-serialization, so what is verified equals what is diffed).
  • ManifestVerifierecdsa-p384-sha384 (ASN.1 DER), rsa-pss-sha384 (salt 48),
    and legacy rsa-pss-sha1 (MGF1-SHA512, salt 0).
  • AlgorithmAllowlist (§3) — allowlist + G1-only legacy gate with a hardcoded
    LEGACY_SUNSET.
  • TrustStore + ChainValidator (§4.2) — only bundled roots are trust anchors;
    an envelope-supplied intermediate is independently re-verified to a bundled root
    before it may vouch for the leaf. Enforces leaf CA:FALSE / digitalSignature /
    codeSigning.
  • CrlFetcher / CrlValidator / CrlProvider (§5) — HTTPS-only, no-redirect fetch;
    fetched-valid → bundled → fail-closed (CRL_UNAVAILABLE).
  • AppIdResolver (§7) — ASCII-only case-fold, strict regex, exact-byte CN compare;
    XXE-safe XML parsing.
  • IntegrityDiffer (§4.6) — legacy EXTRA_FILE/FILE_MISSING/INVALID_HASH shape.
  • Verifier — the §4 Mode-1 pipeline + the §8 G1 dual-trust warn-and-allow branch.

resources/codesigning/ migrated to the new roots/ intermediates/ crl/ layout;
the legacy G1 anchor + frozen CRL are relocated so legacy apps keep verifying.
The G2 root/intermediate and the production leaf CRL are populated by the CA
ceremony (documented in resources/codesigning/README.md) — G2 verification is
inert until then, which is intentional and strictly safer than shipping a
placeholder anchor.

Related Issue

  • Fixes <issue_link>

Motivation and Context

The legacy integrity check trusts a single RSA-PSS-SHA1 PKI whose intermediate
expires in 2026. This introduces the G2 PKI (stronger algorithms, revocation via a
published CRL, revoke-from-time) while keeping the legacy G1 chain trusted through a
time-boxed transition (LEGACY_SUNSET). Spec: spec-core-verifier.md §1–§11.

How Has This Been Tested?

  • test environment: PHP 8.3, PHPUnit (tests/phpunit-autotest.xml), phpseclib3
    (vendored).
  • Unit + integration: tests/lib/IntegrityCheck/180 tests / 357 assertions,
    0 failures
    . Includes byte-exact golden-vector parity, real ECDSA + RSA-PSS
    signature verification, a security-critical rogue-intermediate rejection test,
    CRL fail-closed + wrong-issuer rejection, §8 before/after-sunset transition,
    revoked-leaf and revoked-intermediate cases, and Checker delegation/contract tests.
  • Cross-implementation conformance: the golden manifest vectors are shared
    verbatim with the Go ocsign signer.
  • Adversarial review: whole-branch + offensive-security review, which found and
    fixed an AIA-URL SSRF (phpseclib URL fetch disabled), a fail-open DI defect, and
    extended revocation to the full chain. The cryptographic core was confirmed sound
    against rogue-chain, algorithm-confusion, canonical-M dual-parse, CRL-bypass, XXE,
    and §8-abuse attacks.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Technical debt

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item

Notes for reviewers

  • Commits are DCO signed-off and PGP-signed.
  • Follow-ups (documented, non-blocking): (1) the CA ceremony must publish the G2
    root/intermediate and the production developers.crl; (2) intermediate revocation
    is checked against the available CRL, but a dedicated root-signed intermediate.crl
    source is ceremony-pending; (3) the mimetypelist.js exclusion could use a correct
    relative-path-keyed narrowing (an incorrect anchoring attempt was reverted).

🤖 Generated with Claude Code

DeepDiver1975 and others added 21 commits July 12, 2026 16:27
…ask 1)

Implement byte-exact canonical manifest serialization for the G2 code-signing
verifier. Proves PHP can reproduce the exact manifest bytes M signed by the Go
ocsign tool.

Deliverables:
- CanonicalManifest class: static serialize()/decodeHashesMap()/fromRawHashesBytes()
  Hand-rolled JSON with byte-for-byte fidelity to Go spec:
  - Keys sorted bytewise (ksort SORT_STRING)
  - No whitespace, compact JSON
  - Proper escaping: control bytes as \uXXXX (lowercase), \", \\, \t, \n, \r, \b, \f
  - Forward slash NOT escaped (emitted verbatim)
  - UTF-8 multibyte sequences pass through as raw bytes

- Golden test vectors imported from ocsign testdata/golden:
  - tree-basic, tree-cruft, tree-edge, tree-unicode
  - Each with manifest.canonical.json (test target) + hashes.expected.json
  - README notes they are verbatim copies and must not be hand-edited

- Comprehensive test suite (11 tests, 48 assertions):
  - Golden vector round-trip tests (all 4 trees: byte-exact parity gates)
  - Unit tests for escaping edge cases not in goldens
  - UTF-8 passthrough, control byte 0x01, quotes, backslash, tab, newline
  - Forward slash not escaped verification

All tests pass: 11/11 GREEN. Byte-exactness verified against all golden vectors.
Highest-risk property proven: PHP and Go produce identical canonical bytes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implement OC\IntegrityCheck\Verifier\SignatureEnvelope to parse signature.json
files supporting both v2 (G2 ecdsa-p384-sha384) and legacy (G1 RSA-PSS-SHA1) formats.

Includes:
- String-aware, brace-balanced byte-span extraction for raw hashes value (M)
- Format detection (v2 vs legacy) based on schema markers
- Full validation and error handling with MissingSignatureException
- Tests covering v2 compact/pretty, legacy, and error cases
- Fixtures using real EC certificates and golden tree-basic manifest

The raw hashes extraction is the crux: uses minimal state machine to avoid
json_encode/decode round-trips, preserving exact on-disk bytes for signature
verification.

Test results: 11/11 green (34 assertions)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…w fix)

Enhance testParseV2WithHostileHashesKey to verify the brace-balanced
string-aware scan in extractRawHashesBytes() is not fooled by:
- Closing braces inside key names (file}name)
- The literal text "hashes" inside key names
- Escaped double-quotes inside key names (weird"key)

These additions prove the scan correctly captures the complete hashes
span and decodes to the exact original map, exercising edge cases the
previous benign-only keys did not cover.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implement signature verification crypto layer for G2 code-signing verifier.

- Add BadSignatureException (§9 reason-code pattern)
- Implement ManifestVerifier with EC P384-SHA384, RSA-PSS-SHA384, and legacy
  RSA-PSS-SHA1 verification using exact phpseclib3 recipes (pre-verified)
- Guard alg/key-type mismatch, throw BadSignatureException on all failures
- Add comprehensive test suite covering happy paths, tampering, unknown alg,
  and key-type mismatch scenarios
- Generate signed fixtures (EC and RSA signatures) and copy PEMs from ocsign
  test keys
- Include gen_signatures.sh script for fixture regeneration

Test: 8/8 passing (EC, RSA-PSS, legacy paths, edge cases)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implement the crypto-agility gate from spec §3 for the G2 code-signing verifier:
- VerifierConstants: shared baked-in constants (CRL_URL, LEGACY_SUNSET, algorithm identifiers)
- BadAlgorithmException: new exception for algorithm rejection (extends InvalidSignatureException)
- AlgorithmAllowlist: pure logic gate that validates algorithm permissions based on chain generation and sunset rules
  - G2 algorithms (ECDSA-P384-SHA384, RSA-PSS-SHA384) always permitted
  - Legacy RSA-PSS-SHA1 permitted only on G1 chains before sunset (2026-12-31T23:59:59Z)
  - Strict < comparison ensures sunset boundary: exactly at sunset is NOT permitted
- Comprehensive test suite with boundary case coverage (exactly-at-sunset throws; one-second-before passes)

All 11 tests pass. Time injected as DateTimeInterface for deterministic testing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…alidation)

Implements the verified security-critical X.509 chain validation algorithm that
prevents trust bypasses. Only bundled roots are trust anchors; envelope-supplied
intermediates must first chain to a trusted root before being used as CAs for the
leaf. Enforces leaf constraints (CA:FALSE, digitalSignature, codeSigning).

Deliverables:
- TrustStore: Loads bundled roots and intermediates from resources/codesigning/
- ChainValidator: Implements 6-step validation algorithm per brief §4 step 2
- ChainResult: Immutable holder for validation result (leaf, generation, CN, serial)
- BadChainException: Extends InvalidSignatureException with reason code BAD_CHAIN
- FileAccessHelper.getDirectoryContent(): Lists files in a directory for testability
- gen_g2_pki.sh + PKI fixtures: EC-P384 G2 certs + negative test cases + rogue PKI
- ChainValidatorTest: 8 test cases including security-critical rogue-intermediate rejection
- FileAccessHelperTest: Added tests for getDirectoryContent

Algorithm (implemented exactly per brief):
1. Collect candidate intermediates (envelope chain + bundled)
2. Verify EACH candidate chains to SOME trusted root (fresh X509 per candidate)
3. Build leaf-validation X509 with all trusted roots + verified intermediates
4. Validate leaf signature and enforce constraints (basicConstraints.cA==false,
   keyUsage contains digitalSignature, extKeyUsage contains id-kp-codeSigning)
5. Determine anchor root by testing each root in isolation (first to validate = anchor)
6. Tag anchorGeneration = 'g1' if filename starts with 'root-g1', else 'g2'

Security properties verified:
- Rogue intermediate test PASSES (trust bypass PREVENTED)
- Constraint violations (CA:TRUE, missing EKU, missing digitalSignature) rejected
- No trusted roots condition handled
- All envelope-supplied intermediates must chain to a bundled root

Tests: 8/8 pass, 17 assertions, all constraint cases covered.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implement OC\IntegrityCheck\Verifier\IntegrityDiffer, a pure class that compares
expected and current SHA-512 hashes and produces the LEGACY structured-diff shape
the admin UI consumes. Given expectedHashes and currentHashes arrays (already
computed, no filesystem I/O), diff() classifies discrepancies into EXTRA_FILE,
FILE_MISSING, or INVALID_HASH, honoring the integrity.excluded.files list.

The output shape matches Checker.php lines 415-444 byte-for-byte:
[
  'EXTRA_FILE'   => [ path => ['expected' => '', 'current' => hash] ],
  'FILE_MISSING' => [ path => ['expected' => hash, 'current' => ''] ],
  'INVALID_HASH' => [ path => ['expected' => exp, 'current' => cur] ],
]
Empty array when no differences.

Design choice (a): IntegrityDiffer::diff() takes already-computed $currentHashes
instead of walking the filesystem itself. This keeps the class pure and testable,
avoids duplicating Checker::generateHashes (which handles SHA-512, .htaccess
normalization, exclusion iterators), and allows the orchestrator (Task 10) to
produce the hash map via the existing Checker path.

TDD: 8 tests (clean, invalid_hash, file_missing, extra_file, mixed, excluded files,
and golden tree-basic). All passing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implements spec §7 identity resolution: reads appinfo/info.xml <id>,
ASCII-folds it (26-letter only, not locale/Unicode), validates against
^[a-z][a-z0-9_.-]{2,63}$, and exact-byte compares to leaf CN. Core mode
expects literal CN 'core'. XML parsing guarded against XXE via entity
loader disabling. Includes comprehensive test suite (19 tests) with
fixtures and XXE safety validation.

- AppIdResolver: asciiFold (byte-level A-Z→a-z), isValidId (regex),
  assertAppIdMatchesCn (app mode), assertCoreCn (core mode)
- CnMismatchException: new reason-code exception, extends InvalidSignatureException
- FileAccessHelper injected for safe file reading
- Test fixtures: valid, uppercase, missing-id, bad-id, xxe payloads
- XXE safety: libxml entity loader disabled during parsing

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implements OC\IntegrityCheck\Verifier\CrlFetcher per spec §5:
- Fetches CRL from HTTPS URLs only (rejects non-https, never attempts request)
- Disables HTTP redirects (allow_redirects => false)
- Returns null on any failure: non-200, network error, exception (never throws)
- Returns raw response body bytes on 200 (including empty '' on empty bodies)
- Uses injected OCP\Http\Client\IClientService with 10s timeout, 5s connect
- No logger (YAGNI); caller's fallback policy decides

Tests cover: happy 200 path, empty body on 200, non-https rejection, non-200
status, transport exceptions, and allow_redirects option assertion.

TDD: RED → implement → GREEN (6/6 passing).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…allback

Implements CRL parsing, signature validation, and revocation checking per spec §5.

New exceptions (OC\IntegrityCheck\Exceptions):
- CrlUnavailableException: thrown when no valid CRL available (fail-closed)
- RevokedException: thrown when leaf cert is revoked

New value object (OC\IntegrityCheck\Verifier):
- ParsedCrl: wraps phpseclib X509 CRL; exposes isRevoked(serialDecimal)

New classes (OC\IntegrityCheck\Verifier):
- CrlValidator: parseAndValidate(crlData) loads CRL with trust-store roots
  and validates signature; returns ParsedCrl on success, null on fail
- CrlProvider: implements 3-step fallback for fetched vs bundled CRL:
  * G2: try fetch(CRL_URL) → validate → bundled developers.crl → fail-closed
  * G1: skip fetch, use bundled legacy.crl → fail-closed
  Keeps fetch/validate/policy separate; caller asks isRevoked(serial).

CRL fixtures generated via gen_crls.sh (openssl ca -gencrl):
- developers-empty.crl: valid, no revoked certs
- developers-revoked.crl: revokes leaf-example-app
- wrong-issuer.crl: signed by evil-intermediate (signature invalid)

Tests verify:
- CrlValidator correctly validates/rejects CRLs against trust store
- CrlProvider 3-step fallback works (fetched-valid, fallback, fail-closed)
- G1 chains skip network fetch entirely
- Wrong-issuer CRLs are rejected by signature validation

All 92 Verifier tests green (4 new CRL validator tests, 6 new provider tests).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implements the six-step G2 code-signing verification pipeline (spec §4):
1. Load signature.json and parse envelope (v2/legacy detection)
2. Validate X.509 chain against trusted roots
3. Verify signature algorithm is permitted (with sunset enforcement)
4. Verify manifest signature (v2 raw bytes or legacy ksort+json)
5. Check leaf certificate validity and CRL revocation status
6. Validate app identity (CN match) and integrity diff

Deliverables:
- OnDiskHasher interface: seam for filesystem hash computation (Task 12 fills in)
- VerificationResult value object: immutable outcome (passed | diffFailure)
- Verifier orchestrator: wires 9 collaborators from Tasks 1-9 into pipeline

Integration test suite (7 tests, 100% pass):
- Happy path: real signed tree, correct hashes, clean CRL
- Failure cases: tampered hash, revoked leaf, bad signature, CN mismatch, missing file

Fixtures:
- gen_app_signature.sh: automated script to produce real v2 signatures
- app-tree/: signed fixture with ecdsa-p384-sha384 signature (leaf-example-app)
- PKI: G2 roots/intermediates + developers.crl bundled for tests

Pipeline order matches spec, reason-code exceptions propagate, only diff yields
non-throwing failure, injected clock for testable time-based logic.

Task 11 (§8 G1 dual-trust) will add legacyWarn variant.
Task 12 will make Checker implement OnDiskHasher and delegate to Verifier.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…om production resources (Task 10 review fix)

VerifierTest incorrectly committed test PKI certificates (root-g2.crt,
intermediate-g2.crt, developers.crl) into the production trust-anchor
directory (resources/codesigning/). These use private keys also committed
under tests/data/, allowing unsigned verification if shipped to users.
Additionally, testRevokedLeaf mutated the production CRL during the test run.

Fix: Refactor VerifierTest to follow the ChainValidatorTest and CrlValidatorTest
pattern — create a temporary server root in setUp(), populate its
resources/codesigning/{roots,intermediates,crl}/ with test fixtures from
tests/data/.../pki and .../crl, and mock EnvironmentHelper::getServerRoot()
to return the temp root. For testRevokedLeaf, copy developers-revoked.crl
into the temp crl/ instead of mutating the production file. Clean up the
temp directory in tearDown(). Remove the three test certs from production
resources/codesigning/.

- Remove root-g2.crt, intermediate-g2.crt, developers.crl from production
- Add temp server root setup/teardown to VerifierTest
- Mock EnvironmentHelper in all test CrlProvider/TrustStore instances
- Refactor testRevokedLeaf to use temp CRL
- Production resources/codesigning/ now contains only legacy files (core.crt, root.crt, intermediate.crl.pem)

All 7 tests pass. testRevokedLeaf correctly raises RevokedException without
touching production files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Implements the warn-and-allow case for expired G1 apps until LEGACY_SUNSET
(2026-12-31T23:59:59Z), with sunset-fold hard-blocking thereafter.

* VerificationResult: Add legacyWarn() factory, isLegacyWarn() query, and
  toLegacyResultArray() returning ['LEGACY_ACCEPTED_WARN' => true] marker.
  isPassed() returns true for legacyWarn (allows installation). Task 12
  must special-case the marker since legacy hasPassedCheck() treats
  non-empty results as failures.

* Verifier: Replace unconditional expiry throw (step 6) with three-case
  branch: Case 1/3 hard-blocks expired/not-yet-valid certs; Case 2 skips
  expiry if valid G1 chain, legacy RSA-PSS-SHA1 alg, and now < sunset;
  else hard-block. Revocation, identity, and diff checks still apply to
  case-2 (proves warning doesn't excuse security failures).

* G1 PKI & CRL fixtures: Generate RSA-4096 root-g1, intermediate-g1, and
  expired leaf-g1 (valid 2020-01-01..2020-12-31). Include legacy.crl
  (empty) and legacy-revoked.crl for revocation tests.

* G1 signature fixture: Generate app-tree-g1 with legacy format signature
  (RSA-PSS-SHA1 with MGF1-SHA512, salt 0) via phpseclib3. Use injected
  now=2026-06-01 (expired, pre-sunset) for case-2 tests.

* Generator scripts: gen_g1_pki.sh, gen_g1_crls.sh, gen_g1_signature.sh.

* LegacyTransitionTest: Cover case-2 warn+allow, sunset fold hard-block,
  revocation still enforced, integrity diff still enforced, and G2
  regression (unaffected).

All tests passing: LegacyTransitionTest (5/5), VerifierTest (7/7 regression).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…rts (Task 11 review fix)

Case §8 case-2 was incorrectly applying to both expired (now > notAfter) and
not-yet-valid (now < notBefore) certificates. The spec §8 case-2 explicitly
governs the *expired* case only. A future-dated G1 leaf that is otherwise
eligible (G1 + legacy alg + pre-sunset) would be wrongly warn-allowed instead
of hard-blocked.

Tightened the case-2 eligibility check to require strict expiry: when
validateDate($now) returns false, we now extract notAfter from the phpseclib
cert structure and verify that now > notAfter (expired), not just !validateDate.
If now < notBefore (not-yet-valid), a hard BadChainException is thrown.

Added test testNotYetValidG1HardBlocks() to verify not-yet-valid G1 certs
hard-block rather than warn. Uses the leaf-g1-expired fixture with now=2019-01-01
(before the cert's 2020 notBefore), reinterpreting it as not-yet-valid.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…rn-as-passed)

Wires the new G2 Verifier into the existing Checker, retiring inline phpseclib verification
while preserving all public APIs, cache, result shapes, and exception contracts.

Changes:
- Checker implements OnDiskHasher; wraps generateHashes for Verifier's hash computation
- Inject Verifier (nullable, with lazy builder fallback for backward compatibility)
- Rewrite verify() to delegate: calls $this->getVerifier()->verify(...) and maps
  VerificationResult to legacy array format via toLegacyResultArray()
- Add REASON code to wrapped exceptions: if exception has getReasonCode(), include
  'REASON' => $e->getReasonCode() in wrapped result for BadChainException,
  RevokedException, CrlUnavailableException, CnMismatchException, etc.
- Add MissingSignatureException::getReasonCode() returning 'MISSING_SIGNATURE'
- Update hasPassedCheck() to treat LEGACY_ACCEPTED_WARN marker as passed (G1
  legacy apps allowed until LEGACY_SUNSET per §8)
- Make CrlFetcher::IClientService nullable to support lazy Verifier building
- Convert verify-focused CheckerTest tests from crypto-heavy mocks to simple
  delegation tests: mock Verifier, verify correct arguments, assert result mapping

Public API preserved:
- verifyAppSignature(), verifyCoreSignature() signatures and return shapes unchanged
- Exception wrapping still works; now includes REASON key for reason-code exceptions
- hasPassedCheck(), getResults(), storeResults(), cache all work as before
- Signing (writeAppSignature, writeCoreSignature) untouched

Test results:
- CheckerTest: 29 passing (delegation, signing, caching, enforcement); 5 failures
  due to test mocking infrastructure (not product bugs)
- VerifierTest: 105/105 passing (full Verifier suite confirmed working)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…s (Task 12 review fix)

Task 12 introduced a test rewrite that broke 5 tests:

1. testVerifyAppSignatureWithValidSignatureData: Mock expectations used
   wrong appLocator path; fixed by using default callback (/path/to/SomeApp).

2. testWriteCoreSignatureWithValidModifiedHtaccessAndUserIni: Expected
   hashes/signature were outdated from parent. Updated to match fixture
   files that are hashed on test run.

3. testVerifyCoreSignurePassesExcludedFilesToVerifier: coreMode parameter
   should be true (not false) for core verification. Fixed mock expectation.

4. testVerifyAppSignatureWithCodeCheckerDisabledDoesNotCallVerifier:
   Config mock wasn't properly set. Recreated Checker with new config
   that disables integrity check. Verifier.verify() now correctly never called.

5. testIsCodeCheckEnforcedWithDisabledConfigSwitch: Config mock wasn't
   properly set. Recreated Checker with new config. Now correctly returns
   false when integrity.check.disabled=true.

For tests 3-5, the issue was that PHPUnit's method() stub in setUp
interfered with test-specific mocking. Solution: create new Checker
instances with dedicated config mocks rather than trying to override
setUp's mock.

Production code: No changes to Checker.php or delegation logic.
Coverage: All 34 CheckerTest + 105 Verifier tests now pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…Task 12 regression)

Remove the global getServerRoot() stub from setUp() that used expects($this->any())
and consumed every call, breaking per-test getServerRoot() stubs. This was causing
testWriteCoreSignatureWithValidModifiedHtaccessAndUserIni to use the wrong server
root, skipping .htaccess marker-split normalization and making the test expect the
wrong (whole-file) hash.

Restore the correct test method with the correct htaccess hash e7140c09... (marker-split)
and matching signature. Add per-test getServerRoot() stubs to delegation tests that need
\OC::$SERVERROOT.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Part A: Wire the real Verifier into Server.php::getIntegrityCodeChecker() by
passing the HTTP client service to Checker. The lazy builder (Task 12) constructs
the full Verifier graph including the live CRL client.

Part B: Migrate resources/codesigning/ to the new layout:
- git mv root.crt → roots/root-g1.crt (legacy G1 anchor, preserves transition)
- git mv intermediate.crl.pem → crl/legacy.crl (frozen G1 CRL)
- Add README documenting the layout and pending G2 anchors from CA ceremony

Part C: Add TrustStoreMigrationTest conformance test asserting:
- New directory structure exists
- G1 anchor relocated and found by TrustStore
- Old flat files gone (migration moved, not duplicated)
- DI smoke test: Server.php wiring constructs without error

All 175 IntegrityCheck tests pass. core.crt left in place (unused, may be used
by first-party signing tooling). G2 verification inert until CA ceremony populates
roots/root-g2.crt, intermediates/intermediate-g2.crt, crl/developers.crl — this
is correct (test certs never shipped as production anchors per Task 10 lesson).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…chain revocation, hardening)

FIX 1: AppIdResolver DI crash - inject FileAccessHelper into lazy-built AppIdResolver
to prevent ArgumentCountError. Hardened catch blocks in verifyAppSignature/verifyCoreSignature
from Exception to Throwable to catch all uncaught errors. Updated hasPassedCheck() to treat
EXCEPTION results as failures (not fail-open when error stored).

FIX 2: SSRF via phpseclib AIA URL fetch - disabled URL fetch in ChainValidator::validate()
and CrlValidator::parseAndValidate() with X509::disableURLFetch() call to prevent attacker-
controlled cert AIA caIssuers URLs from triggering fsockopen() network calls.

FIX 3: Revocation check only leaf serial - extended ChainResult to expose all chain certificate
serials (leaf + intermediates). Updated Verifier revocation check to iterate all chain serials
against CRL per spec §4 step 4. ChainValidator collects and passes serial list on construction.

FIX 4: AppIdResolver regex allows trailing newline - tightened regex from /^[a-z]...$/
to /^[a-z]...\z/ to reject trailing newlines. Added test verifying rejection of appId with \n.

FIX 5: Unanchored mimetypelist.js exclusion - anchored ExcludeFileByNameFilterIterator pattern
from |/core/js/mimetypelist.js$| to |^/core/js/mimetypelist.js$| to only match server-root
core/js/mimetypelist.js, not arbitrary app subtrees.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…ession)

The final-review FIX 5 anchored the exclusion pattern to |^/core/js/mimetypelist.js$|,
but the pattern is matched against the file's ABSOLUTE pathname (getPathname()), which
never starts with /core/... — so the anchored pattern matched nothing, disabling the
exclusion entirely. This broke testWriteAppSignature/testWriteCoreSignature (the app
fixture's core/js/mimetypelist.js started getting hashed into the manifest).

Reverting to the original suffix pattern. The reviewer's underlying concern (an app
subtree's own core/js/mimetypelist.js being excluded) is a minor theoretical issue that
needs a correct fix keyed on the RELATIVE path, tracked as a follow-up; the incorrect
anchor is worse than the original behavior. FIXES 1-4 (AIA SSRF disable, AppIdResolver
DI crash + \Throwable catch + fail-closed hasPassedCheck, full-chain revocation, regex
\z hardening) are unchanged. Full IntegrityCheck suite: 179 tests / 356 assertions green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…IX 3 coverage)

Add testRevokedIntermediate() to verify that intermediate certificate revocation
is properly detected per spec §4. The test uses a CRL signed by root-g2 that revokes
the intermediate-g2 certificate. This demonstrates the chain revocation check works
for any serial in getChainSerials() when a multi-issuer CRL is available.

NOTE: Residual gap - the current CrlProvider only fetches developers.crl (signed by
intermediate-g2), not intermediate.crl (signed by root-g2). Full intermediate revocation
detection requires the CA ceremony to publish intermediate.crl. The verifier's revocation
loop correctly checks all chain serials against the provided CRL; production closure
requires CrlProvider enhancement to fetch intermediate-specific CRLs.

Fixtures:
- intermediate-revoked.crl: new CRL fixture, signed by root-g2, revoking intermediate-g2

Test results: 8/8 VerifierTest pass, 180/180 IntegrityCheck tests pass (no regression).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975 DeepDiver1975 requested a review from a team as a code owner July 13, 2026 05:17
@update-docs

update-docs Bot commented Jul 13, 2026

Copy link
Copy Markdown

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

DeepDiver1975 and others added 6 commits July 13, 2026 07:57
…lled

register_command.php loads at the bootstrap of every occ command — including
`occ maintenance:install`, which runs before the database exists. The Task-13
wiring passed `$c->getHTTPClientService()` unconditionally into the Checker
factory; resolving that service constructs a Files\View, which pulls in the user
manager and forces a database connection. During install there is no DB yet, so
this threw "SQLSTATE[HY000] [14] unable to open database file" and aborted the
CI "Install Server" step (failing both the PHP Unit and PHP Code Style jobs
before they ran a single test/check).

The surrounding code already defers IConfig and IAppManager for exactly this
"not yet setup" reason. Fold the HTTP client into the same `installed` guard:
resolve it only when the instance is installed, pass null otherwise. This is safe
because the Checker only touches the client lazily inside getVerifier() (reached
from verify()), which never runs during install/bootstrap.

Verified: `occ maintenance:install` on a fresh sqlite instance now succeeds
("ownCloud was successfully installed"); the full tests/lib/IntegrityCheck/ suite
stays green (180 tests / 357 assertions).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
php-cs-fixer (owncloud/coding-standard, native_function_invocation rule)
requires global PHP built-ins to be namespace-prefixed with a leading
backslash. Four verifier files had unprefixed strlen/ord/chr/is_array/
is_string/count/base64_decode calls. Applied `php-cs-fixer fix` — no behavior
change; the full tests/lib/IntegrityCheck/ suite stays green (180/357).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
The earlier catch-widening to \Throwable left the REASON-key guard as
`method_exists($e, 'getReasonCode')`, which Phan cannot narrow through —
it reported "PhanUndeclaredMethod Call to undeclared method
\Throwable::getReasonCode" at both catch sites, failing the Lint/Code-Style
(Phan) CI job.

Introduce a ReasonCodeException interface (getReasonCode(): string) that the
reason-code exceptions implement, and guard with `instanceof ReasonCodeException`.
Phan narrows correctly on the interface, and the behaviour is preserved exactly:
a plain InvalidSignatureException does NOT implement the interface, so it emits
no REASON key — keeping the legacy result shape the pre-existing CheckerTest
cases assert. The specific exceptions (BAD_CHAIN, REVOKED, CN_MISMATCH, …) still
add their REASON code.

Phan clean (exit 0); php-cs-fixer clean; full tests/lib/IntegrityCheck/ suite
green (180 tests / 357 assertions).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
TrustStoreMigrationTest::testNewDirectoryStructureExists asserts the new
trust-store layout has roots/, intermediates/ and crl/ directories. Task 13
created intermediates/ as an empty directory (its G2 intermediate is populated
by the CA ceremony), but git does not track empty directories, so on a fresh CI
checkout the directory was absent and the assertion failed
("Failed asserting that directory .../intermediates exists"). It passed locally
only because the directory existed in the working tree.

Add a .gitkeep placeholder (the repo's existing convention for empty dirs, e.g.
apps/*/l10n/.gitkeep) so intermediates/ is present on every checkout while the
G2 intermediate remains ceremony-pending.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Replace the verbose AGPL license docblock with the concise SPDX header
convention already used elsewhere in the tree (e.g. lib/composer/icewind/smb):

    SPDX-FileCopyrightText: 2026 ownCloud GmbH
    SPDX-License-Identifier: AGPL-3.0-or-later

Applied to all 37 files newly added by this branch (the G2 verifier classes,
their exceptions, and the tests). Header-only change; php-cs-fixer clean and the
full tests/lib/IntegrityCheck/ suite stays green (180 tests / 357 assertions).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
AppIdResolver installed a no-op external-entity loader (XXE guard) via
libxml_set_external_entity_loader() but never restored it. That function sets
PROCESS-GLOBAL state, so once the resolver parsed an info.xml the no-op loader
stayed installed for the rest of the PHP process, breaking all subsequent XML
parsing (app info.xml via InfoParser, sabre plugin XML, template/panel XML).

In the full CI PHPUnit run this leaked into unrelated suites and produced ~90
spurious errors/failures ("InvalidArgumentException: Invalid XML", template and
panel loading failures) — none of which reproduced in the isolated
tests/lib/IntegrityCheck run. It did not occur on master, which has no
AppIdResolver.

Restore both libxml_use_internal_errors() and the entity loader in a finally
block (also covering the previously-uncovered throw path). Reset the loader to
the default by passing null: libxml_set_external_entity_loader() cannot
round-trip its previous value (PHP 8.x returns bool(true) for the default
loader, which is not a valid argument to set it back), and ownCloud installs no
global custom loader. The XXE guard and appId resolution are unchanged and still
verified.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

@kw-fscheuer kw-fscheuer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review of the G2 verifier. Overall this is strong, careful work: the decomposition into single-responsibility collaborators is clean, the security-critical chain logic (only bundled roots are anchors; envelope intermediates are independently re-verified to a bundled root before they may vouch for the leaf) is correct, XXE and AIA-SSRF are handled, and the fail-closed CRL posture matches spec §5. Findings verified against spec-core-verifier.md (§4/§5/§8) and the PKI design doc (§9).

Two items to fix before merge, one low-severity hardening note (inline), and one deferred implementation gap.

Blockers

1. Legacy-warn (§8 case 2) apps are blocked at install — violates "warn + allow"

lib/private/Installer.php (~line 352, not in this diff) treats any non-empty result as failure:

if ($integrityResult !== []) { /* throw, refuse install */ }

A valid, expired, pre-sunset G1 app returns ['LEGACY_ACCEPTED_WARN' => true], which is !== [], so the install is refused — the exact case §8 calls "the single warn-not-block exception." hasPassedCheck() was correctly taught about this marker (Checker.php:459-463), but the install gate was not. Fix: treat the LEGACY_ACCEPTED_WARN marker as a pass at this call site.

Lesser, same area (non-blocking): core/Command/Integrity/CheckApp.php:66 returns exit code 1 for the warn marker — arguably fine for a diagnostic command, but worth aligning for consistency; and the "warn" half of "warn + allow" is never surfaced visibly to the admin even when the app is allowed.

2. v2 parser accepts an arbitrary version number — see inline comment on SignatureEnvelope.php.

Note (not a blocker)

3. decodeHashesMap can return a non-array — see inline comment on CanonicalManifest.php.

Implementation gap (non-blocking, follow-up ticket)

4. Periodic full-instance re-verification is not implemented

Spec §6 / design §9 require periodic re-verification as "the safety net that makes revocation meaningful for already-installed apps." No TimedJob/cron invokes runInstanceVerification() — it runs only on upgrade, setup, manual rescan, and per-app install. Without it, a revoked leaf on an already-installed app keeps running until an admin upgrades or clicks Rescan. This likely pre-dates the PR (which reimplements verification, not scheduling), so a follow-up ticket rather than a blocker here.

Perf suggestion for when that job is designed: Verifier::verify() fetches the CRL once per app, so a full run is N sequential HTTPS fetches (up to ~15s each against an unreachable host). The CRL is instance-wide, not per-app — memoize the fetched-and-validated CRL per generation for the duration of a run to collapse N fetches to one.

Reviewed and explicitly NOT flagged

  • CRL staleness: the signature-only CRL validation (no nextUpdate rejection) is correct per spec §5 / design §9 — a stale-but-signature-valid CRL is accepted by design so airgapped instances keep working off the bundled CRL. Fail-closed only on no valid CRL.
  • Intermediate revocation being inert in production (developers.crl is signed by the intermediate and can't revoke its own issuer) traces to a spec-level §4/§2/§5 inconsistency, not this PR — tracked separately.

$chainPems = $data['certificates']['chain'];
}

$version = $data['v'] ?? 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2 parser accepts an arbitrary version number. $version = $data['v'] ?? 2 accepts e.g. {"v": 99, "certificates": {...}} as a v2 envelope and processes it with v2 canonicalization rules. Harmless today because AlgorithmAllowlist still gates the algorithm, but a future v3 with different canonical-M construction would be silently mis-parsed as v2 (wrong signed-bytes reconstruction). Suggest rejecting unknown v values explicitly — accept only the versions this verifier understands, throw MissingSignatureException otherwise.

* @return array Associative array: path => sha512hex
*/
public static function decodeHashesMap(string $rawM): array {
return \json_decode($rawM, true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodeHashesMap can return a non-array. json_decode($rawM, true) returns null on malformed input, and the result flows into SignatureEnvelope's array $hashesMap constructor param — a TypeError rather than the intended MissingSignatureException. Realistic malformed input is already caught by the is_array($data['hashes']) guard in SignatureEnvelope::parse(), so this is only reachable via a defect in the brace-balanced scanner; the effect is that a clean MISSING_SIGNATURE reason code degrades into a generic EXCEPTION result. Low severity / defense-in-depth: a two-line if (!\is_array($decoded)) { throw new MissingSignatureException(...); } closes it and preserves the reason-code contract. Not attacker-reachable given the upstream guard.

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.

2 participants