Forward the normalized signing algorithm to the inference provider pool - #1042
lloydmak99 wants to merge 2 commits into
Conversation
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.
Review —
|
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 2m 18s |
There was a problem hiding this comment.
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
PierreLeGuen
left a comment
There was a problem hiding this comment.
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.
|
Thanks — three points raised. Two are addressed in 542aec4; the first does not reproduce. 1. Data-plane default divergence — not reproducibleThe reading of the two source lines is correct:
Confirmed against production: 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 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 — fixedCorrect, and the better test. All four original tests set 3. Comment nit — fixed
Validation on the new head: |
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
Option, with a comment recording why the two defaults must not diverge.
uppercase cases. MockProvider already returns an ECDSA-shaped key when the
algo is absent, so it reproduces the bug without new scaffolding.
Validation
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.