Skip to content

fix: gateway-sign streams with synthesized done markers - #996

Open
hanakannzashi wants to merge 2 commits into
codex/fix-auto-redact-attestation-892from
codex/gateway-sign-synthetic-done-994
Open

fix: gateway-sign streams with synthesized done markers#996
hanakannzashi wants to merge 2 commits into
codex/fix-auto-redact-attestation-892from
codex/gateway-sign-synthetic-done-994

Conversation

@hanakannzashi

@hanakannzashi hanakannzashi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • retain the provider signature for a normal raw stream whose upstream terminates with its own [DONE] frame
  • gateway-sign the exact client-visible SSE stream when Cloud API appends a terminal frame after a clean EOF
  • keep that Gateway signature available before the synthesized terminal frame is sent

This is stacked on #992 because both changes use the same Gateway-signing lifecycle.

Fixes #994

Testing

  • cargo fmt --check
  • cargo test -p services completions:: --lib
  • cargo test -p api --lib
  • cargo test -p api --test e2e_all --no-run

Focused E2E execution is blocked locally by PostgreSQL authentication during test bootstrap.

@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review: gateway-sign streams with synthesized done markers

No prior review threads on this PR, so this is a first pass. The core idea is right: a provider TEE signature covers only the upstream byte stream, so it must not be published once Cloud API appends its own terminator. Pre-seeding public_signature_chat_id from the peeked stream_chat_id and capturing StreamChunk::Text ids (not just Chat) is a genuine fix — text completions previously hit the "no chat_id observed" warning on the gateway path. Ordering is also sound: InterceptStream runs its finalize inline before yielding Poll::Ready(None), so the pin release always precedes the route tail future, with no race against store_chat_signature.

Issues below.


⚠️ 1. Attested streams silently lose all signatures when model metadata is unavailable

crates/api/src/routes/completions.rs:1530

let may_need_synthesized_done_gateway_signature = model_attestation_supported == Some(true);

model_attestation_supported is None whenever get_models_with_pricing() errors or the resolved model is absent from the pricing list (completions.rs:1455-1469) — a transient cache/DB condition, deliberately handled with a warn! rather than a hard failure. But the service layer decides independently, from model.attestation_supported (services/src/completions/mod.rs:1857).

So with metadata None on a genuinely attested model whose upstream ends at EOF without a terminator:

  • services/src/completions/mod.rs:168 -> !saw_upstream_done_marker -> pin released, no provider signature stored
  • may_need_... == false -> hash_client_visible_stream == false -> no gateway signature either

GET /v1/signature/{chat_id} then 404s. On main the provider signature was at least stored. An unrelated metadata read failure now degrades attestation, and because the hashing decision is made up front the tail cannot recover. Suggest failing safe:

// `None` means metadata was unavailable, not that the model is unattested —
// hash so the tail can still mint a signature rather than publishing none.
let may_need_synthesized_done_gateway_signature = model_attestation_supported != Some(false);

⚠️ 2. E2EE streams can now get a Gateway signature, contradicting SIGNATURE_UNSUPPORTED

may_need_synthesized_done_gateway_signature does not exclude e2ee_active, and E2EE + attestation_supported == Some(true) is a supported combination — chat_stream_usage_mode guards !e2ee_active for rewrite/strip precisely because it is reachable. Previously gateway_signature_enabled was never true for E2EE, so lookups fell through to chat_signatures.rs:38-46, which returns: "This model responses are integrity-protected by an end-to-end encrypted channel, not a per-response signature; there is no signature to retrieve."

Now an E2EE stream whose upstream omits the terminator stores a gateway signature over the ciphertext, while the same model and request with an upstream terminator still returns SIGNATURE_UNSUPPORTED. The response now depends on an upstream framing detail the client cannot observe. Add && !e2ee_active unless signing the ciphertext stream is deliberate — in which case it deserves a comment and a test, since it changes a documented API contract.


⚠️ 3. The saw_upstream_done_marker gate is in shared code; only chat completions got the fallback

services/src/completions/mod.rs:168 sits in InterceptStream, which /v1/responses also drives (services/src/responses/service.rs:1473 passes skip_provider_chat_signature: false). A responses-backed stream whose upstream omits the terminator now loses its provider chat signature with no compensating gateway signature on that path — the responses route only stores one keyed by response_id (routes/responses.rs:401,672). Probably acceptable given the resp_-keyed signature exists, but it is outside the stated scope and should be called out in the description or covered by a test.


4. Outer timeout can leave a half-written signature pair

completions.rs:2081-2090 wraps store_chat_signature in tokio::time::timeout, but store_chat_signature_and_unpin_impl documents the opposite pattern deliberately (attestation/chat_signatures.rs:158-164): "an outer timeout would drop the future". The pin-leak hazard does not apply here, since the service already released it — but store_gateway_signature loops ["ecdsa", "ed25519"] with an await per algorithm, so a timeout firing between them commits ecdsa and drops ed25519, and ?signing_algo=ed25519 404s while ecdsa succeeds. Prefer a service method with the bound inside, reusing the existing STREAM_SIGNATURE_STORE_TIMEOUT (already 5s) instead of the new duplicate STREAM_SIGNATURE_STORE_TIMEOUT_SECS constant.


5. Per-chunk String allocation plus async mutex on the hot path

completions.rs:1699-1713 and 1773-1786 build candidate (a String clone of the chat id) on every chunk, then discard it because chat_id is already Some — which it now almost always is, since it is pre-seeded from the peek at line 1648. Move the clone inside the check:

let mut chat_id = public_signature_chat_id.lock().await;
if chat_id.is_none() {
    *chat_id = Some(match chunk {
        inference_providers::StreamChunk::Chat(c) => c.id.clone(),
        inference_providers::StreamChunk::Text(c) => c.id.clone(),
    });
}

That match now appears three times and is worth extracting into a small stream_chunk_id helper. Separately, hash_client_visible_stream is now true for every attested stream, so continuous-usage, E2EE and multimodal streams pay a SHA-256 update plus a tokio::sync::Mutex round trip per chunk whose result is discarded whenever the upstream does send its own terminator. Cheap, but it is new work on the common NEAR AI client path (continuous_usage_stats: true).


6. Dead branch

completions.rs:1716-1718, inside the raw-passthrough arm:

if event.is_done_marker() {
    if gateway_signature_enabled { return None; }

That arm is guarded by !auto_redact_enabled && !rewrite_public_stream_usage && !strip_intermediate_usage, and gateway_signature_enabled is true only when one of those three holds — so it is unreachable. Harmless as defense in depth, but a debug_assert! or a one-line comment would stop the next reader from inferring the combination is live.


7. Test readability

with_disconnect_after(usize::MAX) to mean "emit everything but suppress the terminator" leans on chunks.truncate(usize::MAX) being a no-op plus the disconnect_after_chunks.is_none() check at mock.rs:1144. It reads as "disconnect", the opposite of what is being tested. A with_no_done_marker() builder on ResponseTemplate would say what it means.

Not blocking, but there is no coverage for the two behaviour changes above: an E2EE stream without an upstream terminator, and an attested stream with model_attestation_supported == None. Unlike auto_redact_requires_gateway_signature, the new gate is an inline expression rather than a function, so it cannot join the existing table of chat_stream_usage_mode unit tests; extracting it would fix that.


⚠️ Issues found — items 1 and 2 are behavioural regressions worth resolving before merge.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

OpenCodeReview: Review failed: 0 finding(s); 4 of 4 selected item(s) failed.

@think-in-universe think-in-universe 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.

Found one incorrect signature-finalization branch.

Comment thread crates/services/src/completions/mod.rs
@think-in-universe

Copy link
Copy Markdown
Contributor

@ironloopai review

@ironloopai

ironloopai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review · Status

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details
  • Run: 15a8bdcf-733f-43dd-9cc7-b015eabb1c1b
  • Base: codex/fix-auto-redact-attestation-892 at bb50741
  • Head: codex/gateway-sign-synthetic-done-994 at fc0113b
  • Created: 2026-09-03 06:40 UTC
  • Updated: 2026-09-03 07:04 UTC

Manual command by think-in-universe · attempt 1 of 3 · completed in 23m 22s

@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 beyond the existing review feedback.

Validation
  • e2e_all test target — The changed end-to-end test target compiles successfully.
Review details
  • Run: 15a8bdcf-733f-43dd-9cc7-b015eabb1c1b
  • Attempts: 1

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