OpenTelemetry: metrics for signature verification duration#769
Open
dahlia wants to merge 12 commits into
Open
Conversation
Add an optional meterProvider?: MeterProvider field to the three
OpenTelemetry-aware verifier option types, mirroring the existing
tracerProvider:
- VerifyRequestOptions in sig/http.ts
- VerifySignatureOptions in sig/ld.ts (inherited by
VerifyJsonLdOptions)
- VerifyProofOptions in sig/proof.ts (inherited by
VerifyObjectOptions)
Thread the option from the existing inbox/outbox call sites:
- handler.ts's handleInboxInternal forwards parameters.meterProvider
to verifyJsonLd, verifyObject, and verifyRequestDetailed.
- middleware.ts's InboxContext.handleObject and getSignedKey forward
this.meterProvider.
verifyRequestDraft and verifyRequestRfc9421 now also destructure
meterProvider, and the recursive verifyRequestDetailed calls inside
the HTTP double-knock fallback thread both meterProvider and
tracerProvider through (previously they silently fell back to the
global tracer provider on retry, which would have split spans across
providers; this fixes that drop alongside the new meterProvider).
No metrics are recorded yet. Subsequent commits will add the
activitypub.signature.verification.duration and
activitypub.signature.key_fetch.duration histograms that consume this
option.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Add the `activitypub.signature.verification.duration` OpenTelemetry
histogram (unit `ms`) and instrument the HTTP Signatures verification
path so each public call to `verifyRequestDetailed()` (and its
`verifyRequest()` wrapper) emits exactly one measurement.
Each measurement carries:
- `activitypub.signature.kind` = `http`
- `activitypub.signature.result` = `verified`, `rejected`, `missing`,
or `error`
- `http_signatures.algorithm`, only when the parsed algorithm value
comes from a bounded set. For draft-cavage signatures the value
is gated by a `DRAFT_KNOWN_ALGORITHMS` whitelist; for RFC 9421 it
is recorded only after the algorithm matches
`rfc9421AlgorithmMap`. Both gates keep the metric attribute on a
small spec-bounded set so attacker-supplied algorithm strings
cannot inflate cardinality.
- `http_signatures.failure_reason`, only on `rejected` rows, taken
from the `VerifyRequestFailureReason.type` discriminator.
The new `FederationMetrics.recordSignatureVerificationDuration()`
helper writes the measurement through the existing
`getFederationMetrics(meterProvider)` cache, mirroring the shape of
the existing delivery and queue-task metric helpers.
Implementation notes:
- `verifyRequestDraft()` and `verifyRequestRfc9421()` now accept a
mutable `HttpSignatureMetricsContext` so the outer
`verifyRequestDetailed()` can read back the bounded algorithm of
the candidate whose result was actually returned, without
surfacing it on the public result type.
- `verifyRequestRfc9421()` tracks the bounded algorithm per
candidate via a `setFailure(result, algorithm?)` helper, so a
later candidate cannot inherit an earlier candidate's algorithm
when iterating multi-signature inputs.
- The cached-key retry paths in both spec implementations now call
`verifyRequestDraft()` / `verifyRequestRfc9421()` directly with
the outer span and metrics context, instead of recursing through
`verifyRequestDetailed()`. This collapses the retry into a
single observed verification (one `http_signatures.verify` span
and one histogram measurement per public call), and is regression-
guarded by a new `cached-key retry emits one measurement, not
two` test step.
New tests cover the verified, missing, invalid-signature, key-fetch
failure, double-count (cached-key retry and `verifyRequest()`
wrapper), and unknown-algorithm cardinality paths.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Extend the `activitypub.signature.verification.duration` histogram to
cover Linked Data Signatures. `verifyJsonLd()` now emits exactly one
measurement per call, carrying:
- `activitypub.signature.kind` = `linked_data`
- `activitypub.signature.result` = `verified`, `rejected`, `missing`,
or `error`
- `ld_signatures.type`, gated by a new `LD_KNOWN_SIGNATURE_TYPES`
whitelist (currently `RsaSignature2017`). Unknown type strings,
including attacker-supplied ones, keep their span attribute but are
dropped from the metric so cardinality stays bounded.
Result classification:
- thrown error → `error`
- returned `true` → `verified`
- returned `false` with a `signature` property present (even when it
is `null` or otherwise malformed) → `rejected`
- returned `false` with no `signature` property at all → `missing`
Two small helpers tidy up the existing inline checks:
- `hasLdSignatureProperty()` does the loose presence check used for
classification, so malformed signatures are still reported as
`rejected` rather than `missing`.
- `getLdSignatureObject()` extracts the inner signature record only
when it is a plain object, driving both the existing span
attributes and the new bounded-type lookup.
New test cases in *packages/fedify/src/sig/ld.test.ts* cover verified,
missing, invalid, malformed-null-signature, and unknown-type cardinality
paths.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Extend the `activitypub.signature.verification.duration` histogram to
cover Object Integrity Proofs. `verifyProof()` now emits exactly one
measurement per public call, carrying:
- `activitypub.signature.kind` = `object_integrity`
- `activitypub.signature.result` = `verified`, `rejected`, or
`error`. OIP intentionally has no `missing` value, because
`verifyObject()` decides whether to call `verifyProof()` at all.
- `object_integrity_proofs.cryptosuite`, gated by a new
`OIP_KNOWN_CRYPTOSUITES` whitelist (currently `eddsa-jcs-2022`).
Unknown cryptosuite strings keep their span attribute but are
dropped from the metric so cardinality stays bounded.
Result classification:
- thrown error → `error`
- `verifyProofInternal()` returns a non-null Multikey → `verified`
- `verifyProofInternal()` returns null → `rejected`
The two cached-key retry paths inside `verifyProofInternal()` now
recurse via `verifyProofInternal()` instead of `verifyProof()`, so the
retry stays inside the outer `verifyProof()`'s span and timing context.
Each public `verifyProof()` call therefore emits exactly one
`object_integrity_proofs.verify` span and one duration measurement.
New tests in *packages/fedify/src/sig/proof.test.ts* cover verified,
rejected, cached-key retry no-double-count, and `verifyObject()`
wrapper no-double-count paths.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Add the `activitypub.signature.key_fetch.duration` OpenTelemetry
histogram (unit `ms`), recorded around every public key lookup that
runs inside one of Fedify's three signature verification paths. The
new metric lets operators subtract key fetch time from total
verification time to isolate crypto-only latency.
Each measurement carries:
- `activitypub.signature.kind`: `http`, `linked_data`, or
`object_integrity`
- `activitypub.signature.key_fetch.result`: `hit` (served from the
in-process key cache), `fetched` (network roundtrip returned a
usable key), or `error` (no usable key, including throws)
The metric is recorded one-per-fetch-attempt, deliberately distinct
from `activitypub.signature.verification.duration`, which is
one-per-public-call. A verification that retries after a cache
mismatch therefore emits two key fetch measurements (one `hit` for the
stale attempt, one `fetched` for the freshly fetched retry) plus a
single verification measurement that covers both.
Each verifier wraps its `fetchKey` / `fetchKeyDetailed` calls in a
small per-file helper (`measureKeyFetch` in http.ts, `measureLdKeyFetch`
in ld.ts, `measureOipKeyFetch` in proof.ts) so:
- the timer starts immediately before the fetch and ends as soon as
the promise settles, even when concurrent local work (JCS
canonicalization, SHA-256 digest, etc.) runs alongside the fetch;
- thrown errors are recorded as `error` and rethrown, so verifier
behavior is unchanged;
- no key IDs, URLs, hostnames, or other unbounded values reach the
metric attributes.
Tests cover `hit`, `fetched`, and `error` paths for HTTP, plus
`fetched` for LD and OIP, plus `hit` for OIP, plus a negative test
asserting that an unsigned document under `verifyJsonLd()` emits no
key fetch measurement at all.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Extend docs/manual/opentelemetry.md to cover the two new histograms:
activitypub.signature.verification.duration and
activitypub.signature.key_fetch.duration.
The metric attribute reference now explains:
- that activitypub.signature.kind takes one of http, linked_data, or
object_integrity, and activitypub.signature.result takes one of
verified, rejected, missing, or error (with a note that
object_integrity never produces missing);
- that the verification.duration histogram covers the full
verification path including local key lookup and remote key fetches,
so operators can subtract key_fetch.duration to isolate the
non-fetch verification work (canonicalization, hashing, attribution
and owner checks, cryptographic verification);
- that direct calls to verifyRequest() / verifyRequestDetailed(),
verifyJsonLd(), and verifyProof() emit exactly one measurement per
public call, while wrappers such as verifyObject() emit one
measurement per inner verifyProof() call;
- that http_signatures.algorithm, ld_signatures.type, and
object_integrity_proofs.cryptosuite are recorded only when the
parsed value matches a small spec-bounded set, listing the exact
set for each kind;
- that http_signatures.failure_reason on rejected rows is one of
invalidSignature or keyFetchError, and that missing HTTP
signatures use activitypub.signature.result=missing without a
failure_reason attribute;
- that key IDs, actor IDs, URLs, and object IDs are deliberately
excluded from both histograms;
- that key_fetch.duration is recorded per fetch attempt, so a
cache-mismatch retry emits two key fetch measurements alongside the
single verification measurement that covers both;
- that hit corresponds to a result served by the configured KeyCache
(which may itself be backed by Redis or a database), and fetched
corresponds to a key loaded through the document loader, regardless
of whether the document loader hits the network or a local store.
fedify-dev#316
fedify-dev#737
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5.5
Add a 2.3.0 changelog entry for the new activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration histograms, listing the always-present and bounded conditional attributes, and the corresponding issue link reference. fedify-dev#316 fedify-dev#737 Assisted-by: Claude Code:claude-opus-4-7
The exported type's JSDoc still described `hit` as "served from the in-process key cache" and `fetched` as "required a network round trip," which no longer matches the implementation or docs/manual/opentelemetry.md. `KeyCache` can be backed by a remote store such as Redis or a database, and `fetched` only requires loading through the document loader (typically but not exclusively a network roundtrip). Update the JSDoc to mirror the manual. fedify-dev#316 fedify-dev#737 Assisted-by: Codex:gpt-5.5
dahlia
added
type/enhancement
Member
Author
|
@codex review |
📝 WalkthroughWalkthroughThis PR adds OpenTelemetry instrumentation for signature verification and public key fetch duration across three signature types (HTTP Signatures, Linked Data Signatures, Object Integrity Proofs) with bounded cardinality attributes, threads meterProvider through verification call paths, adds histogram recording helpers, tests, and documentation. ChangesSignature Verification Metrics Instrumentation
Sequence DiagramsequenceDiagram
participant verifyRequestDetailed
participant metricsContext as MetricsContext
participant draft_or_rfc as SpecVerifier
participant measureKeyFetch
participant FederationMetrics
verifyRequestDetailed->>metricsContext: init timing state (start, algorithm)
verifyRequestDetailed->>draft_or_rfc: call with metricsContext & meterProvider
draft_or_rfc->>measureKeyFetch: wrap fetchKeyDetailed
measureKeyFetch->>FederationMetrics: recordSignatureKeyFetchDuration(kind, result)
draft_or_rfc->>verifyRequestDetailed: return result
verifyRequestDetailed->>FederationMetrics: recordSignatureVerificationDuration(kind, result, algorithm/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces two new OpenTelemetry histograms, activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration, to monitor the performance of ActivityPub signature verification and public key lookups. The changes include updates to the core federation metrics, documentation, and verification logic for HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs. Review feedback suggests refactoring the measureOipKeyFetch helper to use async/await for consistency with other helpers in the PR and adding a null check for verificationMethodId in the proof verification logic to improve robustness against malformed data.
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/fedify/src/sig/ld.test.ts`:
- Around line 346-527: Add a new test step to verify the cached-key retry path:
call verifyJsonLd with a custom keyCache whose get returns a valid but "stale"
CryptographicKey (and set is a noop) so the retry path in verifyJsonLd is hit;
use createTestMeterProvider to capture metrics, assert the overall verification
result, then call
recorder.getMeasurements("activitypub.signature.key_fetch.duration") and assert
there are exactly 2 measurements with the first metric having attribute
"activitypub.signature.key_fetch.result" === "hit" and the second === "fetched";
reference verifyJsonLd, keyCache.get/keyCache.set, CryptographicKey and
generateCryptoKeyPair to construct the cached key and recorder.getMeasurements
to assert the two entries.
In `@packages/fedify/src/sig/proof.test.ts`:
- Around line 571-793: Add a test step to assert that an unrecognized
cryptosuite is omitted from metric attributes: create a cloned proof with
cryptosuite "ecdsa-rdfc-2019" (use proof.clone or construct a DataIntegrityProof
with that cryptosuite), call verifyProof(jsonLd, unknownProof, { documentLoader:
mockDocumentLoader, contextLoader: mockDocumentLoader, meterProvider }) using
createTestMeterProvider, then check the
"activitypub.signature.verification.duration" measurement has result "rejected"
and that "object_integrity_proofs.cryptosuite" is not present in
measurements[0].attributes; this locks the bounding behavior tied to
OIP_KNOWN_CRYPTOSUITES.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdf94651-712f-4968-ab5d-11a9aa1ce215
📒 Files selected for processing (11)
CHANGES.mddocs/manual/opentelemetry.mdpackages/fedify/src/federation/handler.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/sig/http.test.tspackages/fedify/src/sig/http.tspackages/fedify/src/sig/ld.test.tspackages/fedify/src/sig/ld.tspackages/fedify/src/sig/proof.test.tspackages/fedify/src/sig/proof.ts
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The OIP key-fetch timing helper used `Promise.prototype.then()` with success and error handlers, while the matching helpers in *http.ts* (`measureKeyFetch`) and *ld.ts* (`measureLdKeyFetch`) use async/await with try/catch. Rewriting `measureOipKeyFetch` the same way removes that local inconsistency. The function is still invoked synchronously up to its first `await`, so `verifyProofInternal()`'s concurrent overlap of the key fetch with JCS canonicalization and SHA-256 digest work is preserved. fedify-dev#769 (comment) Assisted-by: Claude Code:claude-opus-4-7
The LD path (`verifySignature()`) retries with a fresh fetch when the cached key fails to verify, and the surrounding documentation states that each retry emits its own `activitypub.signature.key_fetch.duration` measurement. HTTP and OIP already had regression tests for this; LD did not. Add a step that primes the keyCache with a wrong-but-valid RSA key for the test vector's signer, runs `verifyJsonLd()`, and asserts the two key-fetch measurements appear in order: `result=hit` for the stale cached attempt, then `result=fetched` for the fresh retry that finally verifies. fedify-dev#769 (comment) Assisted-by: Claude Code:claude-opus-4-7
Add a regression test that verifies an unrecognized cryptosuite does not flow through to the `object_integrity_proofs.cryptosuite` metric attribute. `DataIntegrityProof`'s constructor and `clone()` reject any cryptosuite other than `eddsa-jcs-2022`, so the exotic proof is built through `DataIntegrityProof.fromJsonLd()`, which is permissive enough to accept arbitrary field values. `verifyProof()` then records `result=rejected` (the internal validator returns null for any cryptosuite other than `eddsa-jcs-2022`), and the test asserts the histogram measurement does not carry an `object_integrity_proofs.cryptosuite` attribute. This locks in the `OIP_KNOWN_CRYPTOSUITES` whitelist as a defense against attacker-controlled cryptosuite strings inflating metric cardinality. fedify-dev#769 (comment) Assisted-by: Claude Code:claude-opus-4-7
Member
Author
|
/gemini review |
Member
Author
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/fedify/src/sig/proof.test.ts`:
- Around line 663-693: Add assertions that the key-fetch sequence produced two
measurements: call
recorder.getMeasurements("activitypub.signature.key_fetch.duration"), assert its
length is 2, and assert the first measurement's attributes indicate the cache
"hit" and the second indicates "fetched" (e.g.,
measurements[0].attributes.result === "hit" and
measurements[1].attributes.result === "fetched") so the verifyProof retry path
(verifyProof / proof.ts logic) is validated to emit hit then fetched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5aca9a40-b389-4d98-960c-379b5ddca482
📒 Files selected for processing (3)
packages/fedify/src/sig/ld.test.tspackages/fedify/src/sig/proof.test.tspackages/fedify/src/sig/proof.ts
Comment on lines
+663
to
+693
| await t.step("cached-key retry emits one measurement, not two", async () => { | ||
| const [meterProvider, recorder] = createTestMeterProvider(); | ||
| const keyId = "https://server.example/users/alice#ed25519-key"; | ||
| // Prime the cache with a wrong-algorithm key (the rsaPublicKey2 is RSA, | ||
| // not Ed25519) so that verifyProofInternal falls through to the | ||
| // fresh-fetch retry path. | ||
| const cache: Record<string, CryptographicKey | Multikey | null> = { | ||
| [keyId]: rsaPublicKey2, | ||
| }; | ||
| const key = await verifyProof(jsonLd, proof, { | ||
| documentLoader: mockDocumentLoader, | ||
| contextLoader: mockDocumentLoader, | ||
| meterProvider, | ||
| keyCache: { | ||
| get(id) { | ||
| return Promise.resolve(cache[id.href]); | ||
| }, | ||
| set(id, k) { | ||
| cache[id.href] = k; | ||
| return Promise.resolve(); | ||
| }, | ||
| } satisfies KeyCache, | ||
| }); | ||
| assert(key != null); | ||
| assertEquals( | ||
| recorder.getMeasurements( | ||
| "activitypub.signature.verification.duration", | ||
| ).length, | ||
| 1, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Strengthen the cached-retry test by asserting key-fetch sequence.
This step already validates the no-double-count contract for activitypub.signature.verification.duration, but it leaves the companion key-fetch behavior for the same path unchecked. Per the PR objective ("a verification that retries emits multiple key-fetch measurements and a single verification measurement"), the recursion at proof.ts Lines 505-515 should produce two activitypub.signature.key_fetch.duration measurements in order: hit (the primed RSA key) followed by fetched (the forced refetch). This mirrors the LD cached-key retry test added in this PR.
🧪 Proposed additional assertions
assert(key != null);
assertEquals(
recorder.getMeasurements(
"activitypub.signature.verification.duration",
).length,
1,
);
+ const keyFetches = recorder.getMeasurements(
+ "activitypub.signature.key_fetch.duration",
+ );
+ assertEquals(keyFetches.length, 2);
+ assertEquals(
+ keyFetches[0].attributes["activitypub.signature.key_fetch.result"],
+ "hit",
+ );
+ assertEquals(
+ keyFetches[1].attributes["activitypub.signature.key_fetch.result"],
+ "fetched",
+ );
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await t.step("cached-key retry emits one measurement, not two", async () => { | |
| const [meterProvider, recorder] = createTestMeterProvider(); | |
| const keyId = "https://server.example/users/alice#ed25519-key"; | |
| // Prime the cache with a wrong-algorithm key (the rsaPublicKey2 is RSA, | |
| // not Ed25519) so that verifyProofInternal falls through to the | |
| // fresh-fetch retry path. | |
| const cache: Record<string, CryptographicKey | Multikey | null> = { | |
| [keyId]: rsaPublicKey2, | |
| }; | |
| const key = await verifyProof(jsonLd, proof, { | |
| documentLoader: mockDocumentLoader, | |
| contextLoader: mockDocumentLoader, | |
| meterProvider, | |
| keyCache: { | |
| get(id) { | |
| return Promise.resolve(cache[id.href]); | |
| }, | |
| set(id, k) { | |
| cache[id.href] = k; | |
| return Promise.resolve(); | |
| }, | |
| } satisfies KeyCache, | |
| }); | |
| assert(key != null); | |
| assertEquals( | |
| recorder.getMeasurements( | |
| "activitypub.signature.verification.duration", | |
| ).length, | |
| 1, | |
| ); | |
| }); | |
| await t.step("cached-key retry emits one measurement, not two", async () => { | |
| const [meterProvider, recorder] = createTestMeterProvider(); | |
| const keyId = "https://server.example/users/alice#ed25519-key"; | |
| // Prime the cache with a wrong-algorithm key (the rsaPublicKey2 is RSA, | |
| // not Ed25519) so that verifyProofInternal falls through to the | |
| // fresh-fetch retry path. | |
| const cache: Record<string, CryptographicKey | Multikey | null> = { | |
| [keyId]: rsaPublicKey2, | |
| }; | |
| const key = await verifyProof(jsonLd, proof, { | |
| documentLoader: mockDocumentLoader, | |
| contextLoader: mockDocumentLoader, | |
| meterProvider, | |
| keyCache: { | |
| get(id) { | |
| return Promise.resolve(cache[id.href]); | |
| }, | |
| set(id, k) { | |
| cache[id.href] = k; | |
| return Promise.resolve(); | |
| }, | |
| } satisfies KeyCache, | |
| }); | |
| assert(key != null); | |
| assertEquals( | |
| recorder.getMeasurements( | |
| "activitypub.signature.verification.duration", | |
| ).length, | |
| 1, | |
| ); | |
| const keyFetches = recorder.getMeasurements( | |
| "activitypub.signature.key_fetch.duration", | |
| ); | |
| assertEquals(keyFetches.length, 2); | |
| assertEquals( | |
| keyFetches[0].attributes["activitypub.signature.key_fetch.result"], | |
| "hit", | |
| ); | |
| assertEquals( | |
| keyFetches[1].attributes["activitypub.signature.key_fetch.result"], | |
| "fetched", | |
| ); | |
| }); |
🤖 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/proof.test.ts` around lines 663 - 693, Add assertions
that the key-fetch sequence produced two measurements: call
recorder.getMeasurements("activitypub.signature.key_fetch.duration"), assert its
length is 2, and assert the first measurement's attributes indicate the cache
"hit" and the second indicates "fetched" (e.g.,
measurements[0].attributes.result === "hit" and
measurements[1].attributes.result === "fetched") so the verifyProof retry path
(verifyProof / proof.ts logic) is validated to emit hit then fetched.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces two new OpenTelemetry histograms, activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration, to monitor ActivityPub signature verification and public key lookup performance. The implementation spans HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs, including updates to documentation and the addition of comprehensive test suites. The reviewer suggested an improvement to the getLdSignatureObject function in packages/fedify/src/sig/ld.ts to simplify type assertions and enhance readability by utilizing the object type for dynamic properties.
Comment on lines
+462
to
+474
| function getLdSignatureObject( | ||
| jsonLd: unknown, | ||
| ): Record<string, unknown> | undefined { | ||
| if ( | ||
| typeof jsonLd === "object" && jsonLd != null && "signature" in jsonLd && | ||
| typeof (jsonLd as { signature?: unknown }).signature === "object" && | ||
| (jsonLd as { signature?: unknown }).signature != null && | ||
| !Array.isArray((jsonLd as { signature?: unknown }).signature) | ||
| ) { | ||
| return (jsonLd as { signature: Record<string, unknown> }).signature; | ||
| } | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
The getLdSignatureObject function can be simplified to improve readability and avoid repetitive type assertions. Additionally, use the object type instead of Record for dynamic properties to align with repository practices and reduce unnecessary type assertions.
function getLdSignatureObject(
jsonLd: unknown,
): object | undefined {
if (
typeof jsonLd === "object" && jsonLd != null && "signature" in jsonLd
) {
const { signature } = jsonLd as { signature: unknown };
if (
typeof signature === "object" &&
signature != null &&
!Array.isArray(signature)
) {
return signature;
}
}
return undefined;
}References
- Avoid using Record types for dynamic properties where the structure can vary; prefer 'object' or more flexible types to prevent type errors and unnecessary assertions.
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
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.
Resolves #737, a sub-issue of #316.
Summary
Fedify already wraps each signature verification in a span (
http_signatures.verify,ld_signatures.verify,object_integrity_proofs.verify), but spans answer individual-request questions. Operators couldn't easily compute p95 verification latency, see whether key-fetch failures were inflating inbox latency, or compare OIP verification cost to HTTP Signature cost. PR #755 (issue #619) already shipped themeterProvideroption and theFederationMetricsscaffolding for histograms; this PR fills in the verification-side instrumentation.activitypub.signature.verification.duration(unitms) records the time the verifier spent end to end, including local key lookup and any remote key fetch. It carriesactivitypub.signature.kind(http,linked_data, orobject_integrity) andactivitypub.signature.result(verified,rejected,missing, orerror). OIP never producesmissing, because the caller decides whether to invoke proof verification at all. Spec-bounded optional attributes are recorded only when the parsed value matches a small whitelist, so attacker-supplied JSON-LD or signature headers cannot inflate cardinality:http_signatures.algorithm(draft-cavage names plus the RFC 9421 algorithm map),ld_signatures.type(RsaSignature2017),object_integrity_proofs.cryptosuite(eddsa-jcs-2022), andhttp_signatures.failure_reasonon rejected HTTP rows (invalidSignatureorkeyFetchError).activitypub.signature.key_fetch.duration(unitms) measures the public key lookup separately, so operators can subtract fetch time from the total to isolate the rest of the verification work: canonicalization, hashing, attribution and owner checks, cryptographic verification. Each fetch attempt is its own measurement; a verification that retries after a cache mismatch emits two key-fetch measurements alongside the single verification measurement that covers both. The result attribute ishit(served by the configuredKeyCache),fetched(loaded through the document loader with a usable key), orerror.The new option is optional and defaults to the global
MeterProvider, so existing callers see no behavior change beyond the new metric emissions.Implementation notes
The three verify-options interfaces in packages/fedify/src/sig/http.ts, packages/fedify/src/sig/ld.ts, and packages/fedify/src/sig/proof.ts now accept an optional
meterProvider. The option is threaded from the existing call sites in packages/fedify/src/federation/handler.ts and packages/fedify/src/federation/middleware.ts. The HTTP double-knock recursive calls also pick up thetracerProviderthey had been silently dropping, alongside the newmeterProvider.Avoiding double-counting required two small refactors. The cached-key retry inside
verifyRequestDraftandverifyRequestRfc9421now calls the inner spec helper directly, reusing the outer span and a new mutablemetricsContext, so each publicverifyRequestDetailedcall emits exactly one span and one verification measurement. The OIP cached-key retry insideverifyProofInternaldoes the same. The RFC 9421 multi-signature loop uses asetFailure(result, algorithm?)helper so an earlier candidate's bounded algorithm cannot stick to a later candidate's failure.The key-fetch instrumentation lives in per-file
measureKeyFetch/measureLdKeyFetch/measureOipKeyFetchhelpers. They startperformance.now()immediately before invoking the fetch closure and record the result as soon as the promise settles, so the canonicalization and SHA-256 work running concurrently in the OIP path does not pollute the measurement. Throws are recorded aserrorand rethrown, so verifier behavior is unchanged.Documentation in docs/manual/opentelemetry.md now lists both histograms in the Instrumented metrics table and documents the full attribute set, including the per-call vs per-fetch granularity and the exact spec-bounded value lists. CHANGES.md has a 2.3.0 bullet under
@fedify/fedifywith[#316]and[#737]references.Test plan
mise run checkmise run test:deno(31,505 tests, 447 sub-steps)mise run test:nodemise run docs:build