feat(agent-vault): session logs - #407
saifsmailbox98 wants to merge 42 commits into
Conversation
|
💬 Discussion in Slack: #pr-review-cli-407-feat-agent-vault-ship-session-activity-from-the-proxy Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 3 · PR risk: 0/10 |
Every request that reaches forwardHTTP becomes a record, a blocked one included, since under the default any-host policy an agent reaching somewhere nobody configured is ordinary passthrough traffic and logging only brokered calls would make the one request worth catching invisible. Records go into a bounded per-session ring; a flusher drains it every 60 seconds or every 1000 records, seals each slice with AES-256-GCM, POSTs the metadata for a presigned URL and PUTs the ciphertext to the customer's bucket. The hot path costs one append under a mutex. Chunk ids are ULIDs rather than a counter: a counter resets whenever the session cache evicts an entry, which happens at nine ordinary sites, and would then collide with the server's unique index for the rest of the session's life. Resolve carries the key exactly once per session. The proxy reports that it holds one and the backend skips the unwrap, which is a KMS round trip, on every poll after the first. Nothing is persisted to disk. A failed upload is retried with the same chunk id, which the server replays idempotently; an outage pauses rather than discards; and a per-tick breaker keeps a hundred spools against a dead bucket or an unreachable control plane from costing a hundred serial timeouts.
…cts or the signed url in logs
87aebb9 to
5f92a4e
Compare
saifsmailbox98
changed the title
…f ULID uuid.NewV7 is monotonic within the process, so chunks split from one flush keep their seal order, and a minting failure now drops the batch with an error instead of panicking. oklog/ulid goes back to an indirect dependency. The pinned crypto vector is regenerated for the new fixture id.
… log key Every way a chunk leaves the proxy returns its bytes to the pending cap, and resolve asks for the key until the proxy holds it.
…tdown shipping once, and what a sealed chunk holds The old shutdown race test only caught its bug by luck; the new one blocks the run loop's upload so a second post fails deterministically. The wiring test now decrypts the uploaded chunk and checks its records.
…er test that shares it The backend's copy of the vector tested no backend code and was removed, so these point at the browser instead.
…nd the zero-capacity ring branch
…om its earliest to latest record, and retry a 408
…s a session log upload
…and reset the upload breakers only on the minute tick
saifsmailbox98
changed the title
|
@greptile review |
…l list drops the oldest without a scan
scott-ray-wilson
left a comment
scott-ray-wilson
left a comment
There was a problem hiding this comment.
Re-reviewed at 37633ba. Verified live against the dev stack: uploads to an Object Lock bucket with the signed checksum, the new AAD and UUIDv7 ids decrypting in the browser, the gone-session drop on Infisical's NotFound, and the shutdown flush on a quiet proxy. go test -race is clean over 20 runs. One Medium below, plus a follow-up on the 404/401 thread.
| r.forgetIdleSpoolsLocked(started) | ||
| r.mu.Unlock() | ||
| } | ||
| for _, spool := range r.dueSpools(pass, started) { |
There was a problem hiding this comment.
Medium: the final flush ships sessions one at a time, so a restart loses most sessions' last minute
Each due session costs a POST and then a PUT in series, so the 5s close budget covers roughly 5s divided by two round trips. A probe with 60 active sessions at 100ms per call lost 36 of them (720 records) on shutdown; at 50ms it lost 13. The only trace is the "session log records were not shipped before shutdown" warning, and a rolling deploy restarts every proxy this way.
This also bears on the earlier flushMu thread. The run loop doesn't check stop first, so when a wake or tick is ready at the same time, select can start a new pass on context.Background() after stop is closed (113 of 200 trials in a probe), and close() then waits for it. That reproduces the overrun (close took 11.5s against the 5s budget in one run, losing the last session's records) without SIGTERM landing during a slow upload.
Suggest sealing every ring first, then shipping with bounded parallelism (as refreshParallelism does for refreshes), checking stop with a non-blocking select at the top of the loop, and running passes on a context that stop cancels.
There was a problem hiding this comment.
Good catch. Stop is checked first now and passes run on a context stop cancels, so close() takes over at once. Sessions also ship 8 at a time now, network calls in parallel and state changes still in one goroutine, so the final flush gets through a lot more in 5s.
| log.Warn().Err(err).Msg("agent-vault: Infisical rejected this proxy's token, holding activity") | ||
| return false | ||
|
|
||
| case isSessionGone(err): |
There was a problem hiding this comment.
The 404 half works: I checked it live against the real backend, and a gone session now logs "Infisical no longer accepts session logs for this session, dropping what was held". Two things are left:
- Any 401 that isn't
ProxyTokenRejectedstill counts as a gone session inisSessionGone, so a 401 from a middlebox or an auth proxy in front of Infisical drops the session's buffer as if the session had ended. Infisical's own 401s carry a name (UnauthorizedError), so the same name check as the 404 would work. - The 403 case is concretely
TokenError. An invalid or expired proxy JWT (say, after anAUTH_SECRETrotation) gets a 403TokenErrorfrom the error handler, whichclassifyChunkErrortreats as a refused chunk and drops, logging an error each time, while resolve treats the same 403 as an outage. ClassifyingTokenErrorlikeProxyTokenRejectedwould hold the data until the proxy logs in again.
…ions in parallel, and hold chunks on foreign 401s and TokenError
…lthy pass never evicts chunks it just sealed
…requests arriving mid-upload wait for the next pass instead of shipping one per round
…ject, when the proxy drops what it held
2 participants
Description 📣
Adds the proxy side of Agent Vault session logs. The proxy records each request an agent makes through a session that has session logs on, encrypts the records with the session's key, and uploads them to the customer's S3 bucket every 60 seconds or every 1,000 records. Records are held in memory within fixed limits, and nothing is written to disk. Backend PR: Infisical/infisical#8275
Changes to existing behaviour:
hasSessionLogKeyand reads a newsessionLogsfield. With an older backend the field is missing, so the proxy doesn't record anything.NotFound. A 404 from a load balancer or a missing route is treated like an outage, so cached credentials keep working through the grace period.Type ✨
Tests 🛠️
Run this proxy build and follow the steps to verify in Infisical/infisical#8275.