beyondinsight_password_safe: fix Cookie header and add session sign-out - #21183
beyondinsight_password_safe: fix Cookie header and add session sign-out#21183efd6 wants to merge 1 commit into
Conversation
efd6
added
bugfix
d71ea3b to
46511b1
Compare
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
46511b1 to
0624a0e
Compare
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
🚀 Benchmarks reportPackage
|
| Data stream | Previous EPS | New EPS | Diff (%) | Result |
|---|---|---|---|---|
managedsystem |
7407.41 | 5405.41 | -2002 (-27.03%) | 💔 |
session |
7142.86 | 5714.29 | -1428.57 (-20%) | 💔 |
To see the full report comment with /test benchmark fullreport
![vera-review-bot[bot]](https://avatars.githubusercontent.com/in/3297022?s=60&v=4){kind=small}
| "retries": 0, | ||
| } | ||
| // Sign out to release the server-side session before returning. | ||
| request("POST", state.url.trim_suffix("/") + "/Auth/Signout").with( |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/beyondinsight_password_safe/data_stream/session/agent/stream/cel.yml.hbs:59
A transport-level failure on the chained Signout request fails the whole evaluation and discards the already-fetched page of data. If that trade-off is not acceptable, publish the page first and sign out in a follow-up evaluation.
Details
Every terminal success branch now nests the event output inside request("POST", .../Auth/Signout).do_request().as(_, {...events...}) (session line 59, and the equivalent branches in the other four streams). In mito, a do_request() that fails before a response exists (connection reset, timeout, DNS failure) is a CEL evaluation error rather than a response with a status code, so the evaluation aborts and the page that was already retrieved successfully is never published; the state, including the cookies and any pagination offset, stays as it was before the evaluation. For the paginated streams the next cycle then re-requests the last page with the stale cookies. The PR description names the orphaned-session consequence of a non-HTTP failure between sign-in and Signout, but not this data-side consequence: a data fetch that succeeded has its result gated on a request whose outcome is otherwise ignored. This is a known-limitation call rather than a defect, hence low severity.
Recommendation:
If the trade-off is unacceptable, return the fetched events immediately with a cursor flag that requests one more evaluation, and perform the sign-out at the top of the program on that flag, so a Signout failure can no longer take the data with it:
// terminal success branch: publish events, request one more evaluation for sign-out
{
"events": body.map(e, {"event": {"original": e.encode_json()}}),
"want_more": true,
"retries": 0,
"cursor": {
"cookies": state.cursor.cookies,
"signout": true,
},
}
// at the top of the program, before the data-fetch branch
state.?cursor.signout.orValue(false) ?
request("POST", state.url.trim_suffix("/") + "/Auth/Signout").with({
"Header": {
"Authorization": [sprintf("PS-Auth key=%s; runas=%s;", [state.apikey, state.username]) + (has(state.password) ? sprintf(" pwd=[%s];", [state.password]) : "")],
"Content-Type": ["application/json"],
"Cookie": [state.cursor.cookies.map(c, c.split(";")[0]).join("; ")],
},
}).do_request().as(_, {
"events": [{"cel": true}],
"want_more": false,
"cursor": {},
})
:
...
If the current behaviour is accepted as the known limitation, no code change is needed, but it is worth naming this data-loss consequence alongside the orphaned-session note in the commit message.
Also in: packages/beyondinsight_password_safe/data_stream/managedaccount/agent/stream/cel.yml.hbs:78, packages/beyondinsight_password_safe/data_stream/managedsystem/agent/stream/cel.yml.hbs:78, packages/beyondinsight_password_safe/data_stream/asset/agent/stream/cel.yml.hbs:117, packages/beyondinsight_password_safe/data_stream/useraudit/agent/stream/cel.yml.hbs:105
🤖 AI-Generated Review | Vera Review Bot - v0.4.1 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
There was a problem hiding this comment.
This falls into the broad category of limitations raised in the description and needs discussion.
| enable_request_tracer: true | ||
| assert: | ||
| hit_count: 4 | ||
| hit_count: 3 |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/beyondinsight_password_safe/data_stream/session/_dev/test/system/test-default-config.yml:14
The session system test now finishes its whole scenario in one poll cycle but keeps a 15s interval, so every later cycle re-signs-in and indexes the same 3 sessions again and the exact hit_count of 3 depends on timing. Use a long interval like the managedaccount and managedsystem tests.
Details
The rewritten session mock drives the 401 -> re-auth -> 200 -> Signout sequence within a single periodic execution and no longer uses as_sequence, so it does not run out of responses. After Signout the cursor is {}; at the next tick the program signs in again (the SignAppin rule returns session_cookie2 for every request after the first), matches the unconditional session_cookie2 Sessions rule, and indexes the same 3 sessions again. The old mock needed two cycles and was capped by as_sequence, which is why a 15s interval with an exact count of 4 was stable. elastic-package treats assert.hit_count as an exact match after waiting for at least that many hits, so the new test passes only if the count is checked before the second cycle lands. The margin is usually sufficient, hence low severity, but the sibling managedaccount and managedsystem tests avoid the race with interval: 4h.
Recommendation:
Raise the poll interval so only one cycle runs during the test, matching the sibling streams:
data_stream:
vars:
interval: 4h
preserve_original_event: true
enable_request_tracer: true
assert:
hit_count: 3Also in: packages/beyondinsight_password_safe/data_stream/session/_dev/test/system/test-no-password-config.yml:13
🤖 AI-Generated Review | Vera Review Bot - v0.4.1 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
All five data streams stored raw Set-Cookie response header values in the cursor and replayed them verbatim as the Cookie request header. This sends cookie attribute tokens (path, HttpOnly, SameSite, etc.) as bogus name-value pairs and emits a separate Cookie header line per cookie. RFC 6265 §5.4 forbids multiple Cookie header lines; ASP.NET commonly reads only the first. If the session cookie is not in that first line, the server returns 401. The fix strips attributes and joins all cookies into a single header value at every send site. The integration never called POST /Auth/Signout. Audit log showed numerous sign-in calls with no corresponding sign-outs. Each poll cycle now chains a Signout call at every terminal branch: the success path and non-401 HTTP errors. The 401 path skips sign-out because the server has already invalidated the session. Session cookies are cleared from the cursor at the end of each cycle so the next poll starts with a fresh sign-in. Test mocks are updated to return two Set-Cookie headers with realistic attributes and to assert the correct joined single-value Cookie header, so the mocks reject the pre-fix code. A Signout rule is added to each. Sign-out is only guaranteed for handled HTTP responses. If the agent process is killed or the evaluation crashes due to a non-HTTP failure (network error, CEL runtime fault) after sign-in but before Signout, the server session is left open. The cursor retains the cookies so the next successful poll clears it, but one session may be orphaned per crash. This is a known limitation of the two-phase evaluation structure where sign-in and data fetch run in separate evaluations.
0624a0e to
05b04c0
Compare
|
✅ All changelog entries have the correct PR link. |
|
🟢 No issues across the latest commits 05b04c0.
|
💚 Build Succeeded
History
cc @efd6 |
1 participant
Proposed commit message
Note
Recommend reading the session data stream change first since it's the simplest and will give an indication of the overall shape of the change.
Note
As commented in the commit message, there is an error handling limitation. I have options to deal with this, but they complicate the code further. If it's seen that the limitation in unbearable, I can work one of them into the code, otherwise we can add in a follow-up.
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots