Skip to content

OpenTelemetry: metrics for signature verification duration#769

Open
dahlia wants to merge 14 commits into
fedify-dev:mainfrom
dahlia:otel/sig-metrics
Open

OpenTelemetry: metrics for signature verification duration#769
dahlia wants to merge 14 commits into
fedify-dev:mainfrom
dahlia:otel/sig-metrics

Conversation

@dahlia
Copy link
Copy Markdown
Member

@dahlia dahlia commented May 15, 2026

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 the meterProvider option and the FederationMetrics scaffolding for histograms; this PR fills in the verification-side instrumentation.

activitypub.signature.verification.duration (unit ms) records the time the verifier spent end to end, including local key lookup and any remote key fetch. It carries activitypub.signature.kind (http, linked_data, or object_integrity) and activitypub.signature.result (verified, rejected, missing, or error). OIP never produces missing, 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), and http_signatures.failure_reason on rejected HTTP rows (invalidSignature or keyFetchError).

activitypub.signature.key_fetch.duration (unit ms) 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 is hit (served by the configured KeyCache), fetched (loaded through the document loader with a usable key), or error.

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 the tracerProvider they had been silently dropping, alongside the new meterProvider.

Avoiding double-counting required two small refactors. The cached-key retry inside verifyRequestDraft and verifyRequestRfc9421 now calls the inner spec helper directly, reusing the outer span and a new mutable metricsContext, so each public verifyRequestDetailed call emits exactly one span and one verification measurement. The OIP cached-key retry inside verifyProofInternal does the same. The RFC 9421 multi-signature loop uses a setFailure(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/measureOipKeyFetch helpers. They start performance.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 as error and 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/fedify with [#316] and [#737] references.

Test plan

  • mise run check
  • mise run test:deno (31,505 tests, 447 sub-steps)
  • mise run test:node
  • mise run docs:build
  • New tests in packages/fedify/src/sig/http.test.ts, packages/fedify/src/sig/ld.test.ts, and packages/fedify/src/sig/proof.test.ts cover the verified, rejected, missing, error, cardinality, no-double-count, and cache-hit vs fetched paths for each signature kind.

dahlia added 9 commits May 16, 2026 00:41
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 dahlia added this to the Fedify 2.3 milestone May 15, 2026
@dahlia dahlia self-assigned this May 15, 2026
@dahlia dahlia added type/enhancement Improvements to existing features component/federation Federation object related component/signatures OIP or HTTP/LD Signatures related component/otel OpenTelemetry integration labels May 15, 2026
@dahlia
Copy link
Copy Markdown
Member Author

dahlia commented May 15, 2026

@codex review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9e4dc698-d5f7-4b0d-bce4-2bace3e357a8

📥 Commits

Reviewing files that changed from the base of the PR and between b3d2090 and 1549d0b.

📒 Files selected for processing (2)
  • packages/fedify/src/sig/ld.ts
  • packages/fedify/src/sig/proof.test.ts

📝 Walkthrough

Walkthrough

Adds OpenTelemetry histograms and recording helpers for signature verification duration and public-key fetch duration across HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs; threads a MeterProvider through verification call paths, updates retry/span behavior to avoid double measurements, and adds tests and documentation.

Changes

Signature Verification Metrics Instrumentation

Layer / File(s) Summary
Metric type contract and recording methods
packages/fedify/src/federation/metrics.ts
Adds exported types SignatureVerificationKind, SignatureVerificationResult, SignatureKeyFetchResult, and SignatureVerificationExtraAttributes; two Histogram instruments (activitypub.signature.verification.duration, activitypub.signature.key_fetch.duration) and recording methods on FederationMetrics.
HTTP signature verification instrumentation
packages/fedify/src/sig/http.ts
Threads meterProvider through HTTP verification, adds HttpSignatureMetricsContext, measureKeyFetch() to record single key-fetch durations, bounded DRAFT_KNOWN_ALGORITHMS, classification helpers, and records verification-duration in finally while preserving single-call metrics across cached-key retries.
HTTP signature metrics tests
packages/fedify/src/sig/http.test.ts
Adds tests using createTestMeterProvider() and assertGreaterOrEqual to assert histogram measurements and attributes across verified, missing, invalid, key-fetch error, cache/retry, and unknown-algorithm scenarios.
Linked Data signature verification instrumentation
packages/fedify/src/sig/ld.ts
Adds meter imports, measureLdKeyFetch wrapper, threads meterProvider through VerifySignatureOptions, bounds LD_KNOWN_SIGNATURE_TYPES, refactors verifyJsonLd to classify outcomes and record verification/key-fetch durations with bounded ldType.
Linked Data signature metrics tests
packages/fedify/src/sig/ld.test.ts
Adds verifyJsonLd() tests using a test MeterProvider to validate verification and key-fetch histogram emissions for verified/missing/rejected/malformed, cold-cache, cached-key retry, and unknown-type cases.
Object Integrity Proof verification instrumentation
packages/fedify/src/sig/proof.ts
Adds meter imports, OIP_KNOWN_CRYPTOSUITES, measureOipKeyFetch, extends VerifyProofOptions with meterProvider, records verification-duration in finally with bounded cryptosuite, and changes retries to recurse into verifyProofInternal() to reuse the outer span/measurement.
Object Integrity Proof metrics tests
packages/fedify/src/sig/proof.test.ts
Adds tests using createTestMeterProvider() asserting activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration emissions, classification, cached-key retry behavior, and cryptosuite attribute omission when unknown.
Metrics provider wiring
packages/fedify/src/federation/handler.ts, packages/fedify/src/federation/middleware.ts
handleInboxInternal now extracts meterProvider and forwards it into verifyJsonLd, verifyObject, and verifyRequestDetailed; middleware verification calls also thread meterProvider.
Documentation and changelog
docs/manual/opentelemetry.md, CHANGES.md
Documents activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration, their required/optional bounded attributes and result buckets, retry/key-fetch measurement semantics, and adds Version 2.3.0 changelog entry.

Sequence Diagram

sequenceDiagram
  participant Caller as verifyRequestDetailed / verifyJsonLd / verifyProof
  participant MetricsCtx as MetricsContext
  participant SpecVerifier as SpecVerifier (draft|rfc|ld|oip)
  participant KeyFetch as measureKeyFetch / measureLdKeyFetch / measureOipKeyFetch
  participant FM as FederationMetrics

  Caller->>MetricsCtx: init timing state (start, kind, candidate algorithm/type)
  Caller->>SpecVerifier: call with metricsContext & meterProvider
  SpecVerifier->>KeyFetch: call wrapped fetchKey (may be cached or remote)
  KeyFetch->>FM: recordSignatureKeyFetchDuration(kind, result)
  KeyFetch-->>SpecVerifier: key (or throw)
  SpecVerifier-->>Caller: verification result (verified/rejected/error/missing)
  Caller->>FM: recordSignatureVerificationDuration(kind, result, bounded algorithm/type/failure)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • #737: Adds a histogram for signature verification duration and bounded attributes — this PR implements that instrumentation across signature types.
  • #738: Introduces key-fetch result buckets and key-fetch timing instrumentation which this PR implements for public-key fetches.

Possibly related PRs

  • fedify-dev/fedify#755: Related prior work threading MeterProvider and adding federation metrics plumbing.

Suggested reviewers

  • 2chanhaeng
  • sij411
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding OpenTelemetry metrics for signature verification duration, which is the core objective of this PR.
Description check ✅ Passed The description comprehensively explains the PR's purpose, implementation approach, and testing strategy, directly relating to the changeset.
Linked Issues check ✅ Passed The PR fully implements all coding requirements from issue #737: histogram metrics for signature verification duration with specified attributes (kind, result, bounded optional attributes), separate key-fetch duration metric, proper scoping to avoid double-counting, and comprehensive tests.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the scope of issue #737: instrumentation of the three verification paths (HTTP, LD, OIP), propagation of meterProvider through call sites, and documentation/test updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread packages/fedify/src/sig/proof.ts Outdated
Comment thread packages/fedify/src/sig/proof.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between fafcfa7 and 99a15a3.

📒 Files selected for processing (11)
  • CHANGES.md
  • docs/manual/opentelemetry.md
  • packages/fedify/src/federation/handler.ts
  • packages/fedify/src/federation/metrics.ts
  • packages/fedify/src/federation/middleware.ts
  • packages/fedify/src/sig/http.test.ts
  • packages/fedify/src/sig/http.ts
  • packages/fedify/src/sig/ld.test.ts
  • packages/fedify/src/sig/ld.ts
  • packages/fedify/src/sig/proof.test.ts
  • packages/fedify/src/sig/proof.ts

Comment thread packages/fedify/src/sig/ld.test.ts
Comment thread packages/fedify/src/sig/proof.test.ts
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

❌ Patch coverage is 87.28324% with 44 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/fedify/src/sig/http.ts 81.88% 22 Missing and 3 partials ⚠️
packages/fedify/src/sig/proof.ts 80.35% 8 Missing and 3 partials ⚠️
packages/fedify/src/sig/ld.ts 91.48% 6 Missing and 2 partials ⚠️
Files with missing lines Coverage Δ
packages/fedify/src/federation/handler.ts 79.72% <100.00%> (+0.06%) ⬆️
packages/fedify/src/federation/metrics.ts 99.71% <100.00%> (+0.05%) ⬆️
packages/fedify/src/federation/middleware.ts 96.21% <100.00%> (+<0.01%) ⬆️
packages/fedify/src/sig/ld.ts 89.10% <91.48%> (-0.34%) ⬇️
packages/fedify/src/sig/proof.ts 76.10% <80.35%> (+0.50%) ⬆️
packages/fedify/src/sig/http.ts 79.65% <81.88%> (+0.47%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dahlia added 3 commits May 16, 2026 04:58
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
@dahlia
Copy link
Copy Markdown
Member Author

dahlia commented May 15, 2026

/gemini review

@dahlia
Copy link
Copy Markdown
Member Author

dahlia commented May 15, 2026

@codex review

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 99a15a3 and b3d2090.

📒 Files selected for processing (3)
  • packages/fedify/src/sig/ld.test.ts
  • packages/fedify/src/sig/proof.test.ts
  • packages/fedify/src/sig/proof.ts

Comment thread packages/fedify/src/sig/proof.test.ts
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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 thread packages/fedify/src/sig/ld.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

dahlia added 2 commits May 16, 2026 08:54
The "cached-key retry emits one measurement, not two" step had two
weaknesses.  First, it primed the cache with rsaPublicKey2, a
CryptographicKey rather than a Multikey, so `fetchKey()` skipped the
cache entirely and went straight to a fresh fetch.  The retry path
inside verifyProofInternal was never triggered; the test passed
because verification.duration count was 1 either way.

Swap the cached value to ed25519Multikey (a valid Ed25519 Multikey
for a different key) so `fetchKey()` returns it as `cached=true`,
the Ed25519 algorithm check passes, and the "signature failed with
cached key" recursive retry actually runs.

Second, the step only checked verification.duration; the per-fetch
behavior on the same path was unchecked.  Add assertions for the
two `activitypub.signature.key_fetch.duration` measurements the
retry produces: `result=hit` for the stale cached attempt followed
by `result=fetched` for the refetch, mirroring the LD cached-key
retry test added earlier in this PR.

fedify-dev#769 (comment)

Assisted-by: Claude Code:claude-opus-4-7
Destructure `signature` from `jsonLd` once after the initial type
guard, instead of repeating `jsonLd as { signature?: unknown }` for
each of the three follow-up checks.  Two early returns also replace
the original chained conditions, which reads more naturally.

Keep the return type as `Record<string, unknown> | undefined`.  The
caller in `verifyJsonLd` reads `sig.creator`, `sig.signatureValue`,
and `sig.type` as string properties; switching to `object | undefined`
would push the cast burden into the caller at every property access,
trading one local cast for three remote ones.

fedify-dev#769 (comment)

Assisted-by: Claude Code:claude-opus-4-7
@dahlia
Copy link
Copy Markdown
Member Author

dahlia commented May 15, 2026

/gemini review

@dahlia
Copy link
Copy Markdown
Member Author

dahlia commented May 15, 2026

@codex review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces two new OpenTelemetry histograms—activitypub.signature.verification.duration and activitypub.signature.key_fetch.duration—to track ActivityPub signature verification performance across HTTP Signatures, Linked Data Signatures, and Object Integrity Proofs. The implementation propagates the meterProvider through the federation handler and middleware, integrates metric recording into the core verification logic, and adds comprehensive tests. These metrics capture end-to-end verification time and isolate public key lookup duration with attributes for signature kind, result, and specific algorithms. I have no feedback to provide.

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/federation Federation object related component/otel OpenTelemetry integration component/signatures OIP or HTTP/LD Signatures related type/enhancement Improvements to existing features

Development

Successfully merging this pull request may close these issues.

OpenTelemetry metrics for signature verification duration

1 participant