Add AtprotoTypesVerify: CAR/MST/CID repo-proof verification - #54
Merged
Conversation
Belongs here, not downstream in a consumer package: this is generic com.atproto.repo protocol machinery (CAR framing, DAG-CBOR, MST proof walking, CID recomputation, commit-signature verification) with zero domain-specific coupling, sitting beside Atproto.CID/DID/DIDDocument the same way Atproto.Repo already reads. Two additive products so a space-constrained consumer (e.g. an App Clip target) can link only the light seam (RecordPath/Proof/ ProofVerifying/ProofUnavailable, now in AtprotoTypes core) and never pay for AtprotoTypesVerify existing. AtprotoTypesVerifyMocks promotes the fixture-building test support into a real library product, mirroring AtprotoTypesMocks, since a different package's tests can't reach @testable-only internals across a package boundary. secp256k1 lands separately; a k256 account fails closed with unsupportedCurve rather than being waved through.
🦋 Changeset detectedLatest commit: 1359c2e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Adversarial review found a real gap: RepoSigningKey.init(atprotoKeyIn:did:) skipped its document-to-DID binding check entirely whenever a verification method's controller field was empty — the common case for real documents, not an edge case. Nothing else in the verifier compares document.id to the did being checked, so a caller-side bug pairing the wrong document with a did would have passed silently. Fixed by falling back to document.id (DID convention: absent controller means self-controlled) rather than skipping the check. Mutation-verified: reverting the fix makes the new test fail, exactly on this scenario. Also: p256Order/isLowS/varint were public with no cross-package consumer (AtprotoTypesVerifyMocks is in the same package) — tightened to package. ContentIdentifier.atprotoCID round-tripped through the base32 string form with a comment claiming Atproto.CID's byte init was package-scoped to a different package; it's the same package now, so it calls the package init directly and the property is no longer throws. Changeset corrected — it claimed the core AtprotoTypes target was untouched, but the Atproto.Repo seam landed there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI invokes `xcodebuild -scheme AtprotoTypes ... test`, and the shared scheme's TestAction only listed AtprotoTypesTests. The 46 verify tests this branch added have never run in CI — only locally. Found by adversarial review; confirmed by running the exact CI command before and after (33 tests only, then 33 + 46 across both targets, TEST SUCCEEDED). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ycheproof gate From-scratch, verify-only Swift port (no UInt128, this package floors below its availability) — most Bluesky accounts sign with this curve and swift-crypto has no k256 support at all. Checked against Wycheproof's 252-vector secp256k1 test suite (exact bucket census, not just "no crash") and a P256K-backed differential oracle in AtprotoTypesVerifyTests only — nothing new ships in the product. RepoFixture's signer is now a protocol (RepoFixtureSigningKey) instead of concrete P256, so AtprotoTypesVerifyMocks can build synthetic k256 repos without depending on a signing library itself; the actual k256 test signer lives in AtprotoTypesVerifyTests, backed by P256K.
…dor license - ECDSA.verify now rejects a non-32-byte digest explicitly. Scalar's reducingBigEndian init silently folds any other length to zero, and z = 0 lets anyone forge a signature over an arbitrary (r, s) without the private key. Not reachable today (RepoSigningKey.verify always passes a SHA-256 output), but ECDSA.verify is the boundary where that stops being true by construction rather than by caller discipline. - Secp256k1FieldTests' ">p" case was missing a hex digit, so it exercised the length check rather than the range check it claims to. - Vendor Wycheproof's LICENSE alongside the JSON fixture (Apache-2.0 requires it accompany redistribution). - Correct two reduction-convergence doc comments that overstated how fast the fold shrinks per pass — conclusions were right, the sketched numbers weren't.
Adds docs/dependency-choices.md covering DAG-CBOR, CID, CAR, MST and secp256k1: which libraries were evaluated, the specific property each one can't express, and what would change the answer. The CBOR case in particular is easy to get wrong from stale information — swift-cbor does enforce DAG-CBOR strictness now; the blocker is that it and its alternatives are Codable-only, and re-encoding a sig-stripped commit preimage needs a schemaless tree or unknown fields vanish. Each implementation file gets a one-line pointer rather than a copy of the reasoning. Also drops two references to a private tracker ID from this public repo's source, which read as dead links to anyone outside the org.
The k256 differential-oracle dependency carries a `.buildTool()` plugin that copies its shared sources into the build directory. xcodebuild refuses to run an unvalidated plugin non-interactively, so every Apple job failed at "Plugin SharedSourcesPlugin must be enabled before it can be used" while Linux and Android stayed green — `swift test` doesn't gate plugins, which is also why local runs missed it. The workflow already passes -skipMacroValidation for the same class of non-interactive gate; this adds the plugin equivalent.
2 tasks
…nto llm/atproto-repo-verify
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relocates GER-2254's CAR/DAG-CBOR/MST/CID/commit-signature verifier from
AtprotoGerm(germDM-ios-refresh) to here, where it belongs: it's genericcom.atproto.repoprotocol machinery with zero Germ-specific coupling, sitting besideAtproto.CID/DID/DIDDocumentrather than beside Germ's declaration/anchoring logic.Two additive products, no changes to the existing
AtprotoTypestarget's public API:AtprotoTypesVerify— the verifier itself.Atproto.Repo.RecordPath/Proof/ProofVerifying/ProofUnavailablemove intoAtprotoTypescore (the Clip-safe seam); CAR/DAG-CBOR/MST/CID/RepoSigningKey/Verifierland in the new product. No new third-party dependencies —swift-cryptoandswift-basesare already package-level dependencies here.AtprotoTypesVerifyMocks— promotes the fixture-building test support (RepoFixture: builds a real signed commit over an MST over record blocks) into a real library product, mirroringAtprotoTypesMocks. Needed because a consuming package's tests can't reach@testable-only internals across a package boundary — two members (RepoSigningKey.p256Order/.isLowS) went frominternaltopublicfor this reason. Its signer is now a protocol (RepoFixtureSigningKey) rather than concrete P-256, so a consumer's tests can build synthetic k256 repos too.secp256k1 is supported, not just P-256: a from-scratch, verify-only Swift port (field/scalar arithmetic on
UInt64limbs — noUInt128, this package floors below its availability; Jacobian point operations; SEC1 decompression; ECDSA), since most real Bluesky accounts sign with this curve and swift-crypto has no k256 support at all. Checked against Wycheproof's 252-vector secp256k1 suite (exact bucket census: 95 accept / 116 refused-non-canonical / 18 refused-bad-length / 23 refused-other, not just "no crash") and a P256K-backed differential oracle (100 keypairs × 3 messages, plus mutation-agreement tests) — both test-only,swift-secp256k1never ships in a product. A genuine k256 commit signature verifies end-to-end through the whole CAR/MST/commit-signature pipeline.Why these aren't imported
docs/dependency-choices.mdrecords the evaluation for all five primitives — which libraries were considered, the specific property each can't express, and what would change the answer.Worth flagging for review, since it's counterintuitive: strictness is no longer the reason for the DAG-CBOR decision.
swift-cbor0.1.0 does enforce a real DAG-CBOR profile (and is already in the wider dependency graph, so it'd be free), as doesthecoolwinter/CBOR. The blocker is that both areCodable-only with no public value tree — and verifying a commit signature means re-encoding the commit minus itssig, so any field a struct doesn't model would vanish from the preimage and break every proof the first time a PDS writes something unanticipated.Testing
81 tests in 10 suites (
AtprotoTypesVerifyTests), existingAtprotoTypesTestsunaffected. DAG-CBOR is pinned against the RFC 8949 vector table; fixtures build real repos so a test can forge one field and watch the proof fail for that reason.🤖 Generated with Claude Code