Skip to content

Forward the normalized signing algorithm to the inference provider pool - #1042

Open
lloydmak99 wants to merge 2 commits into
mainfrom
fix/attestation-forward-normalized-signing-algo
Open

lloydmak99 wants to merge 2 commits into
mainfrom
fix/attestation-forward-normalized-signing-algo

Conversation

@lloydmak99

Copy link
Copy Markdown
Contributor

Problem

GET /v1/attestation/report?signing_algo=ed25519 intermittently returned the
model's ECDSA signing key (128 hex chars) instead of its Ed25519 key (64 hex
chars), breaking E2EE downstream with "Invalid curve public key".

Two services defaulted a missing signing_algo to opposite values. This service
defaults to ed25519 in normalize_signing_algo, in report_cache_key and in the
gateway quote; inference-proxy defaults a missing algo to ecdsa. The build
closure forwarded the caller's raw Option rather than the normalized value, so
a nonce-less request with no signing_algo was cached under an a=ed25519 key
while holding the ECDSA report. Every explicit signing_algo=ed25519 request
inside the 10s cache TTL then read that poisoned entry. The collision is
bidirectional.

Reproduced live against production: one request omitting signing_algo followed
by three explicit ed25519 requests returned the ECDSA key 8 times out of 9, and
the wrong value was byte-identical to the same model's ECDSA key. This is the
cause of the rotating ed25519 failures in nearai/infra-tests.

Changes

  • Forward the normalized algo to pool.get_attestation_report instead of the raw
    Option, with a comment recording why the two defaults must not diverge.
  • Add report_signing_algo_tests covering the omitted, ed25519, ecdsa and
    uppercase cases. MockProvider already returns an ECDSA-shaped key when the
    algo is absent, so it reproduces the bug without new scaffolding.
  • Widen two test helpers to pub(super) so the new module reuses them.

Validation

  • cargo test -p services attestation: 112 passed, 0 failed.
  • cargo clippy --workspace --all-targets -- -D warnings: clean.
  • cargo fmt --all --check: clean.
  • Regression proven: reverting only the production line makes
    omitted_signing_algo_returns_the_ed25519_model_key fail 128 vs 64, the exact
    production symptom.

Rollout Notes

Behavior change: a request that omits signing_algo now receives an Ed25519
model attestation where it previously received an ECDSA one. This is the
correct outcome. The gateway half of that same response was already built with
the ed25519 default, so today's response is internally inconsistent. Callers
that omit the parameter include a few infra-tests call sites.

Follow-up, not included here: chat_signatures.rs defaults to ecdsa for chat
signature lookup. That matches inference-proxy's signature route, so it is
consistent, but the pair is worth an audit. inference-proxy itself is
deliberately unchanged; aligning its default would alter the contract for
direct callers of *.completions.near.ai.

Problem

GET /v1/attestation/report?signing_algo=ed25519 intermittently returned the
model's ECDSA signing key (128 hex chars) instead of its Ed25519 key (64 hex
chars), breaking E2EE downstream with "Invalid curve public key".

Two services defaulted a missing signing_algo to opposite values. This service
defaults to ed25519 in normalize_signing_algo, in report_cache_key and in the
gateway quote; inference-proxy defaults a missing algo to ecdsa. The build
closure forwarded the caller's raw Option rather than the normalized value, so
a nonce-less request with no signing_algo was cached under an a=ed25519 key
while holding the ECDSA report. Every explicit signing_algo=ed25519 request
inside the 10s cache TTL then read that poisoned entry. The collision is
bidirectional.

Reproduced live against production: one request omitting signing_algo followed
by three explicit ed25519 requests returned the ECDSA key 8 times out of 9, and
the wrong value was byte-identical to the same model's ECDSA key. This is the
cause of the rotating ed25519 failures in nearai/infra-tests.

Changes

- Forward the normalized algo to pool.get_attestation_report instead of the raw
  Option, with a comment recording why the two defaults must not diverge.
- Add report_signing_algo_tests covering the omitted, ed25519, ecdsa and
  uppercase cases. MockProvider already returns an ECDSA-shaped key when the
  algo is absent, so it reproduces the bug without new scaffolding.
- Widen two test helpers to pub(super) so the new module reuses them.

Validation

- cargo test -p services attestation: 112 passed, 0 failed.
- cargo clippy --workspace --all-targets -- -D warnings: clean.
- cargo fmt --all --check: clean.
- Regression proven: reverting only the production line makes
  omitted_signing_algo_returns_the_ed25519_model_key fail 128 vs 64, the exact
  production symptom.

Rollout Notes

Behavior change: a request that omits signing_algo now receives an Ed25519
model attestation where it previously received an ECDSA one. This is the
correct outcome. The gateway half of that same response was already built with
the ed25519 default, so today's response is internally inconsistent. Callers
that omit the parameter include a few infra-tests call sites.

Follow-up, not included here: chat_signatures.rs defaults to ecdsa for chat
signature lookup. That matches inference-proxy's signature route, so it is
consistent, but the pair is worth an audit. inference-proxy itself is
deliberately unchanged; aligning its default would alter the contract for
direct callers of *.completions.near.ai.
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review — fix/attestation-forward-normalized-signing-algo

The core fix is correct: report_cache_key, normalize_signing_algo, and GatewayQuoteInput all default a missing algo to ed25519, so forwarding the raw Option was the only place the report could disagree with its own cache key. Verified the other get_attestation_report call sites are already safe — ita/service.rs:236 passes Some(signing_algo.to_string()) (already normalized) and inference_provider_pool/mod.rs:1512 iterates both algos explicitly. Test scaffolding matches the current shapes of ModelWithPricing (all 30 fields), DstackCpuQuote, and AttestationRepository.

One issue that should be resolved (or at least explicitly accepted) before merge.

⚠️ The default divergence isn't closed — it moves to the report ↔ completions pair

The PR normalizes the control plane to ed25519 but leaves the data plane passing through:

  • crates/api/src/routes/completions.rs:60insert_encryption_headers only inserts x_signing_algo when the caller sent X-Signing-Algo.
  • crates/inference_providers/src/attested/nearai/mod.rs:747 — the header is only set if let Some(algo) = extra.remove(...), so an absent value means no header reaches inference-proxy, which (per this PR's own premise, and mirrored by MockProvider) defaults to ecdsa.

Failure scenario for a client that omits the algorithm on both calls — which works today:

  1. GET /v1/attestation/report?model=X (no signing_algo) → pre-PR returns the model's ECDSA key deterministically (raw None → proxy default); post-PR returns the Ed25519 key.
  2. POST /v1/chat/completions with X-Client-Pub-Key but no X-Signing-Algo → proxy still decrypts with ECDSA.
  3. Payload was sealed to the Ed25519 key → E2EE fails. model_pub_key routing still resolves the provider (the pool registers keys for both algos), so this surfaces as a decrypt error, not a clean 4xx.

So the class of bug being fixed (two defaults disagreeing across a boundary) is re-created for omit-everywhere clients. The rollout note covers "you now get Ed25519 instead of ECDSA" but not that the completions half doesn't move with it.

Smallest in-repo fix that makes cloud-api internally consistent end to end, without touching inference-proxy's contract for direct *.completions.near.ai callers:

// crates/api/src/routes/completions.rs — insert_encryption_headers
// Default to ed25519 to match normalize_signing_algo / report_cache_key on the
// attestation endpoint. inference-proxy defaults a missing algo to ecdsa, so
// omitting the header here would pair an ed25519 model key with an ecdsa decrypt.
extra.insert(
    service_encryption_headers::SIGNING_ALGO.to_string(),
    serde_json::Value::String(
        encryption_headers.signing_algo.clone().unwrap_or_else(|| "ed25519".to_string()),
    ),
);

This only matters when E2EE is actually in play (x-client-pub-key present), so it could be scoped to that condition. If the intent is to ship the report fix first and align the data plane separately, please say so in the rollout notes and land them together — the intermediate state is worse for omit-everywhere clients than what is in production now. The same reasoning applies to crates/api/src/routes/anthropic.rs:773 and responses/service.rs:1436.

Test coverage: the production symptom isn't the thing under test

service_with_mock_provider sets report_cache: None, so all four tests exercise the cache-bypass path. They prove the algo is forwarded, but not the reported symptom — that a nonce-less omitted-algo request poisons the a=ed25519 entry and a subsequent explicit signing_algo=ed25519 request reads it. Worth one test with the cache enabled:

// omitted first (populates a=ed25519), then explicit ed25519 must hit that entry
let svc = service_with_mock_provider_cached().await;
let a = report_via(&svc, None).await;
let b = report_via(&svc, Some("ed25519")).await;
assert_eq!(model_signing_public_key(&a).len(), ED25519_PUBKEY_HEX_LEN);
assert_eq!(model_signing_public_key(&b).len(), ED25519_PUBKEY_HEX_LEN);

That fails pre-fix for the exact reason production failed, and it also pins the (correct) invariant that omitted and explicit-ed25519 requests are allowed to share one cache entry.

Nit

report.rs:200 — the comment says the clone is needed because algo "is still borrowed above (get_signing_address_hex)". That borrow ends at that call; the only real reason is the move into gateway_fut below. Trim it to that so the next reader doesn't go looking for a live borrow.

⚠️

@ironloopai

ironloopai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details
  • Run: b4430e19-32f8-44fc-8602-36ea06f1d012
  • Base: main at 45d11e3
  • Head: fix/attestation-forward-normalized-signing-algo at 4df9952
  • Created: 2026-09-10 21:52 UTC
  • Updated: 2026-09-10 21:54 UTC

Automatic trigger · attempt 1 of 3 · completed in 2m 18s

@ironloopai ironloopai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review · Summary

🟢 No actionable findings

No additional actionable findings in the reviewed change.

Validation
  • Static review — The normalized signing algorithm is forwarded to the provider request, and the added regression tests cover omitted, explicit, and uppercase algorithm inputs.
  • Captured CI — The captured PR evidence reports passing release build, lint, and unit-test checks.
Review details
  • Run: b4430e19-32f8-44fc-8602-36ea06f1d012
  • Attempts: 1

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ 1 posted as inline comment(s)
  • 📝 0 posted as summary

Comment thread crates/services/src/attestation/report_signing_algo_tests.rs Outdated

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No confirmed issues found.

Checks: cargo +1.92.0 test -p services --lib attestation:: — 112 passed, 0 failed, 1 ignored, including all four new signing-algorithm tests.; cargo +1.92.0 check -p services --tests — passed.; cargo +1.92.0 clippy -p services --all-targets -- -D warnings — passed.

Problem

Review noted that all four tests in report_signing_algo_tests built the service
with report_cache: None, so every one took the cache-bypass arm. They proved
the normalized algo is forwarded, but not the reported production symptom: a
nonce-less request omitting signing_algo populated the a=ed25519 cache entry
with an ECDSA report, and explicit signing_algo=ed25519 requests inside the TTL
then read that poisoned entry.

A comment in report.rs also justified the clone with a borrow that has already
ended, rather than the later move into gateway_fut.

Changes

- Add service_with_mock_provider_cached, mirroring the production cache from
  lifecycle.rs, and a test that calls the report path twice on one service:
  first with the algo omitted, then explicitly ed25519, asserting both keys are
  64 hex chars.
- Assert the two reports share a gateway request_nonce first. A nonce-less build
  generates a random nonce, so equality proves the second call was a cache hit
  rather than a fresh build, and pins the correct invariant that omitted and
  explicit-ed25519 requests share one entry.
- Update the module docs, which previously disclaimed cache coverage.
- Trim the report.rs comment to cite only the move.

Validation

- cargo test -p services attestation: 113 passed, 0 failed.
- cargo clippy --workspace --all-targets -- -D warnings: clean.
- cargo fmt --all --check: clean.
- Reverting only the production line makes the new cached test fail 128 vs 64 on
  the poisoning assertion, while the shared-entry assertion still passes, so the
  failure is a poisoned cache hit and not a mis-built report.

No production behavior change in this commit; report.rs is comment-only.
@lloydmak99
lloydmak99 deployed to Cloud API test env September 10, 2026 23:20 — with GitHub Actions Active
@lloydmak99

Copy link
Copy Markdown
Contributor Author

Thanks — three points raised. Two are addressed in 542aec4; the first does not reproduce.

1. Data-plane default divergence — not reproducible

The reading of the two source lines is correct: insert_encryption_headers (completions.rs:60) only inserts x_signing_algo when the caller sent it, and prepare_encryption_headers (nearai/mod.rs:747) only forwards the header when present. But the failure scenario needs step 2 — a request carrying X-Client-Pub-Key with no X-Signing-Algo reaching the proxy and silently decrypting with ECDSA — and that request is rejected.

inference-proxy/src/encryption.rs:38-53, extract_encryption_context, requires the pair to be present together or not at all. (Some(_), None) and (None, Some(_)) both return 400. There is no (None, Some) arm that defaults to ecdsa; the proxy only defaults a missing algo on the attestation and signature GET routes, never on the E2EE data plane.

Confirmed against production:

POST https://cloud-api.near.ai/v1/chat/completions
  X-Client-Pub-Key: <32 bytes hex>     (no X-Signing-Algo)
→ HTTP 400
  "Both X-Signing-Algo and X-Client-Pub-Key headers must be present together"

So an omit-everywhere E2EE client cannot exist today — it gets a 400 before and after this change. The class of client the finding describes has no working state to regress from, and the intermediate state is not worse than production. Defaulting the header in completions.rs would change a currently-rejected request into an accepted one, which is a separate behavior change and not a fix for this PR.

Happy to be shown wrong if there is a path that reaches the proxy with a client key and no algo header.

2. Cache-path test coverage — fixed

Correct, and the better test. All four original tests set report_cache: None and took the bypass arm. 542aec4 adds a cached test that reproduces the poisoning across two calls on one service. Detail in the inline reply.

3. Comment nit — fixed

report.rs:200 now cites only the move into gateway_fut. The borrow had indeed already ended.

Validation on the new head: cargo test -p services attestation 113 passed / 0 failed, clippy -D warnings clean, fmt --check clean.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants