Resolve did:key verification locally#915
Conversation
Resolve Ed25519 did:key verification methods locally so Object Integrity Proof verification can work for portable ActivityPub objects without fetching the verification method as a remote JSON-LD document. Add vocab-runtime helpers for exporting and importing Ed25519 did:key DIDs, parsing did:key verification method DID URLs, and rejecting unsupported or malformed DID keys before they reach remote lookup paths. Document the new proof verification behavior and update the changelog for PR 915. Fixes fedify-dev#827 fedify-dev#915 Assisted-by: Codex:gpt-5.5
dahlia
added
component/vocab
✅ Deploy Preview for fedify-json-schema canceled.
|
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changesdid:key support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant fetchKeyWithResult as key.ts
participant resolveDidKey as key.ts
participant parseDidKeyVerificationMethod as vocab-runtime
participant keyCache
Caller->>fetchKeyWithResult: fetchKey(did:key URL, Multikey)
fetchKeyWithResult->>resolveDidKey: resolveDidKey(url, cls)
resolveDidKey->>keyCache: check cache
alt cache miss
resolveDidKey->>parseDidKeyVerificationMethod: parseDidKeyVerificationMethod(url)
parseDidKeyVerificationMethod-->>resolveDidKey: DidKeyVerificationMethod
resolveDidKey->>keyCache: cache Multikey
end
resolveDidKey-->>fetchKeyWithResult: Multikey / invalid result
fetchKeyWithResult-->>Caller: return early without documentLoader
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces local resolution for Ed25519 did:key verification methods, allowing Object Integrity Proofs (FEP-8b32) and portable objects (FEP-ef61) to be verified without fetching remote JSON-LD documents. It adds several helper functions (exportDidKey, importDidKey, and parseDidKeyVerificationMethod) to @fedify/vocab-runtime, integrates them into fetchKey and verifyProof, and updates OpenTelemetry metrics and documentation accordingly. The review feedback highlights two important improvements in packages/fedify/src/sig/key.ts: first, handling cached negative entries (cachedKey === null) in resolveDidKey to avoid redundant parsing, and second, correcting the metric outcome logic so that negative cache hits are properly recorded as "hit" instead of "invalid".
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fedify/src/sig/key.ts (1)
396-419: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCache-hit check duplicated between
getCachedFetchKeyandresolveDidKey.Both functions independently re-implement
cachedKey instanceof cls && cachedKey.publicKey != null, butresolveDidKeydoesn't handle the explicitcachedKey === null("entry cached but unavailable") branch thatgetCachedFetchKeydoes. This divergence is easy to miss on future edits to either function.♻️ Suggested extraction
+function checkCachedKeyHit<T extends CryptographicKey | Multikey>( + cachedKey: CryptographicKey | Multikey | null | undefined, + cls: FetchableKeyClass<T>, +): (T & { publicKey: CryptoKey }) | undefined { + if (cachedKey instanceof cls && cachedKey.publicKey != null) { + return cachedKey as T & { publicKey: CryptoKey }; + } + return undefined; +}Then reuse
checkCachedKeyHitin bothgetCachedFetchKeyandresolveDidKey.Also applies to: 431-483
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/fedify/src/sig/key.ts` around lines 396 - 419, The cache-hit logic is duplicated between getCachedFetchKey and resolveDidKey and the null-cache case is handled only in getCachedFetchKey. Extract the shared check into a helper such as checkCachedKeyHit that encapsulates the instanceof cls plus publicKey presence and the explicit cachedKey === null unavailable branch, then have both getCachedFetchKey and resolveDidKey call it so the behavior stays consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/fedify/src/sig/key.ts`:
- Around line 396-419: The cache-hit logic is duplicated between
getCachedFetchKey and resolveDidKey and the null-cache case is handled only in
getCachedFetchKey. Extract the shared check into a helper such as
checkCachedKeyHit that encapsulates the instanceof cls plus publicKey presence
and the explicit cachedKey === null unavailable branch, then have both
getCachedFetchKey and resolveDidKey call it so the behavior stays consistent in
one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b961de55-cf73-48d4-8691-886369e3cda1
📒 Files selected for processing (9)
CHANGES.mddocs/manual/opentelemetry.mddocs/manual/send.mdpackages/fedify/src/sig/key.test.tspackages/fedify/src/sig/key.tspackages/fedify/src/sig/proof.test.tspackages/vocab-runtime/src/key.test.tspackages/vocab-runtime/src/key.tspackages/vocab-runtime/src/mod.ts
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Keep positive key cache hits in one helper so regular remote key lookups and local did:key resolution use the same class and public key checks. The did:key path still ignores stale negative cache entries because it can resolve supported keys locally and replace the old unavailable entry. fedify-dev#915 (review) Assisted-by: Codex:gpt-5.5
dahlia
moved this to In review
in STF 2025–2026 Milestone 7: Advanced Features and Standards (FEP-ef61, new FEPs)
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces local resolution for Ed25519 did:key verification methods, allowing verifyProof() to verify Object Integrity Proofs without fetching remote JSON-LD documents. It adds helper functions (exportDidKey, importDidKey, and parseDidKeyVerificationMethod) to @fedify/vocab-runtime, updates OpenTelemetry metrics to handle hostless key identifiers, and includes comprehensive tests. The reviewer's feedback points out an important improvement in resolveDidKey to correctly respect negative cache entries when a key is cached as unavailable, preventing redundant resolution attempts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Object Integrity Proofs on portable ActivityPub objects cannot rely on the usual remote key lookup path. In the FEP-ef61 shape, the object can be identified under an
ap://did:key:...authority, and the proof can point at adid:key:z...#z...verification method. Treating that verification method as a JSON-LD document would make the proof depend on a fetch that has no useful remote document to fetch.This PR handles that case inside key resolution instead. packages/fedify/src/sig/key.ts now recognizes supported
did:keyDID URLs before the document loader path, parses only the Ed25519did:key:z...#z...form, and turns it into aMultikeydirectly. Unsupported DID keys and malformed verification methods fail locally, without poisoning the key cache and without falling back to a remote lookup. That keeps the implementation narrow: it is not a general DID resolver, and it does not add policy checks about whether a portable object ID is authorized by a proof. It only supplies the public key needed by proof verification.The parsing and key conversion live in packages/vocab-runtime/src/key.ts so the low-level rules are shared and testable. The new helpers are:
exportDidKey()importDidKey()parseDidKeyVerificationMethod()They deliberately accept Ed25519
did:keyvalues only. RSA multicodec keys, non-base58btc multibase values, malformed multicodec prefixes, short Ed25519 keys, bare DIDs where a verification method is required, and mismatched fragments are rejected before aCryptoKeyis returned.The proof tests exercise the FEP-ef61/FEP-8b32 path with a portable
ap://object ID and adid:keyverification method. The document loader throws in that test, so a passing test proves that verification used the local key resolution path. The same test also covers tampering, fragment mismatch, and an unsupported RSAdid:keyproof that must not fall back to document loading.I also updated the Object Integrity Proof documentation in docs/manual/send.md and the key lookup metric description in docs/manual/opentelemetry.md. The metric result remains
fetchedfor successful cold lookups, but the docs now spell out that a cold lookup may be local key resolution rather than an HTTP document fetch.Fixes #827.