Conversation
Migrate the securityhub_findings and securityhub_findings_full_posture data streams from the legacy httpjson input with hand-rolled SigV4 signing to the CEL input using run_as_cel with native AWS authentication (auth.aws). The previous templates signed each request in an httpjson transform and re-evaluated the request body (including now()) on every pagination page. Because the UpdatedAt window and the SigV4 signature were recomputed per page, a paginated GetFindings call could invalidate its NextToken and fail. The CEL programs pin the request body once per collection interval and only append NextToken between pages, so the window and signed request stay stable across pagination. Also add a `filters` variable to both data streams so operators can apply server-side AwsSecurityFindingFilters to GetFindings and collect only matching findings. For Findings the managed UpdatedAt filter takes precedence; for Findings Full Posture the variable defaults to the existing RecordState/WorkflowStatus exclusions. These two data streams now require Elastic Agent 9.4.0+ (or 8.19.18+ on 8.x) for the httpjson-to-CEL redirector; the aws package already requires Kibana ^9.4.0. Changes: - Replace httpjson.yml.hbs with httpjson_as_cel.yml.hbs for both streams - Add filters and max_executions variables to both manifests - Document the Findings Filters setting and version requirement - Bump package version to 7.2.0 and add a changelog entry
Elastic Docs Style Checker (Vale)Summary: 2 suggestions found 💡 Suggestions (2): Optional style improvements. Apply when helpful.
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. |
🚀 Benchmarks reportTo see the full report comment with |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
There was a problem hiding this comment.
Pull request overview
This PR migrates the AWS Security Hub Findings and Findings Full Posture data streams from legacy httpjson (with custom SigV4 signing) to CEL execution via run_as_cel with auth.aws, aiming to stabilize pagination and enable server-side filtering.
Changes:
- Switch both Security Hub findings streams to
httpjson_as_cel.yml.hbstemplates usingrun_as_cel+auth.aws. - Add
filters(AwsSecurityFindingFilters) and hiddenmax_executionsvariables to both streams. - Update Security Hub docs and bump the aws package version + changelog.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/aws/manifest.yml | Bumps the aws package version to 7.3.0. |
| packages/aws/changelog.yml | Adds a 7.3.0 changelog entry describing the CEL migration + filters. |
| packages/aws/docs/securityhub.md | Documents CEL-based GetFindings collection, incremental behavior, and Findings Filters. |
| packages/aws/_dev/build/docs/securityhub.md | Mirrors the Security Hub documentation updates for the built docs. |
| packages/aws/data_stream/securityhub_findings/manifest.yml | Switches stream template to httpjson_as_cel.yml.hbs and adds filters / max_executions. |
| packages/aws/data_stream/securityhub_findings/agent/stream/httpjson.yml.hbs | Removes the legacy httpjson template with hand-rolled SigV4 signing. |
| packages/aws/data_stream/securityhub_findings/agent/stream/httpjson_as_cel.yml.hbs | Adds CEL program for stable UpdatedAt window + pagination with native auth.aws. |
| packages/aws/data_stream/securityhub_findings_full_posture/manifest.yml | Switches stream template to httpjson_as_cel.yml.hbs and adds filters / max_executions. |
| packages/aws/data_stream/securityhub_findings_full_posture/agent/stream/httpjson.yml.hbs | Removes the legacy httpjson template with hand-rolled SigV4 signing. |
| packages/aws/data_stream/securityhub_findings_full_posture/agent/stream/httpjson_as_cel.yml.hbs | Adds CEL program for pagination with native auth.aws and configurable server-side filters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Migrate the securityhub_findings and securityhub_findings_full_posture data streams from the legacy httpjson input with hand-rolled SigV4 signing to the CEL input using run_as_cel with native AWS authentication (auth.aws). The previous templates signed each request in an httpjson transform and re-evaluated the request body (including now()) on every pagination page. Because the UpdatedAt window and the SigV4 signature were recomputed per page, a paginated GetFindings call could invalidate its NextToken and fail. The CEL programs pin the request body once per collection interval and only append NextToken between pages, so the window and signed request stay stable across pagination. Wire the full auth.aws credential set (access keys, session token, shared credentials, and assume role) to match the other migrated AWS streams, so the Assume Role and Shared Credentials setup options the Security Hub policy template still offers are honoured rather than silently falling back to the ambient credential chain. Reset next_token on the non-2xx error path. Without this, a failure part-way through pagination left a stale NextToken in the CEL state that was replayed on every later interval; because Security Hub pagination tokens are short-lived the request kept failing and the stream stayed wedged until the agent restarted. Add a filters variable to both data streams so operators can apply server-side AwsSecurityFindingFilters to GetFindings and collect only matching findings. For Findings the managed UpdatedAt filter takes precedence; for Findings Full Posture the variable defaults to the existing RecordState/WorkflowStatus exclusions. Guard the variable so a cleared value renders an empty object instead of null. Add a max_executions variable to both streams to bound pagination per interval. Findings keeps the input default of 1000 because it resumes incrementally via its cursor; Findings Full Posture defaults to 10000 (about one million findings per cycle) because it re-reads the entire current posture with no cursor. Document the cap and how to raise it in a troubleshooting section. Changes: - Replace httpjson.yml.hbs with httpjson_as_cel.yml.hbs for both streams - Add filters and max_executions variables to both manifests - Document the Findings Filters setting and CEL execution cap - Bump package version and add a changelog entry
There was a problem hiding this comment.
🟡 Changes recommended
The new CEL templates can emit request.tracer.enabled: null when unset and the documented/manifested max_executions constraints are inconsistent with other repo streams, both of which should be reconciled before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The migration and supporting manifest/docs/test updates look consistent, with only minor documentation wording nits noted.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/aws/_dev/build/docs/securityhub.md:32
- Grammar: “data streams collect from the
GetFindingsAPI” reads awkwardly; use “collect data from” to make the sentence clear.
packages/aws/docs/securityhub.md:32 - Grammar: “data streams collect from the
GetFindingsAPI” reads awkwardly; use “collect data from” to make the sentence clear.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The updated Security Hub documentation introduces/retains incorrect guidance about required credentials, minimum Agent version, and interval-based mitigation for the full-posture stream.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
packages/aws/_dev/build/docs/securityhub.md:31
- This new note about using
auth.awsconflicts with (and is undermined by) the earlier documentation that says access keys are compulsory and that the module supports Elastic Agent 8.4.0. Since these streams now userun_as_cel+auth.aws(and the package manifest requires Agent >= 9.4.0), please update the Compatibility/Credentials wording so it reflects the supported credential methods and the actual minimum agent version.
packages/aws/_dev/build/docs/securityhub.md:41 - The troubleshooting advice to "shorten the collection interval so each run has fewer pages to fetch" is true for the incremental Findings stream, but it doesn't reduce paging for Findings Full Posture, which re-reads the full posture each run. This guidance should distinguish between the two streams (and optionally point users to Findings Filters for Full Posture).
packages/aws/docs/securityhub.md:31 - This new note about using
auth.awsconflicts with (and is undermined by) the earlier documentation that says access keys are compulsory and that the module supports Elastic Agent 8.4.0. Since these streams now userun_as_cel+auth.aws(and the package manifest requires Agent >= 9.4.0), please update the Compatibility/Credentials wording so it reflects the supported credential methods and the actual minimum agent version.
packages/aws/docs/securityhub.md:41 - The troubleshooting advice to "shorten the collection interval so each run has fewer pages to fetch" is true for the incremental Findings stream, but it doesn't reduce paging for Findings Full Posture, which re-reads the full posture each run. This guidance should distinguish between the two streams (and optionally point users to Findings Filters for Full Posture).
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
efd6
left a comment
There was a problem hiding this comment.
Some of the copilot comments look valid. After that, LGTM
There was a problem hiding this comment.
🟡 Changes recommended
Both CEL requests omit the required Content-Type: application/json header.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
| ?"NextToken": state.?page.next_token, | ||
| }.encode_json() | ||
| ).do_request().as(resp, (resp.StatusCode == 200) ? |
There was a problem hiding this comment.
Agree. I would recommend using post_request, then you can include the content type in the request call, rather than the approach suggested by Vera.
| ?"NextToken": state.?page.next_token, | ||
| }.encode_json() | ||
| ).do_request().as(resp, (resp.StatusCode == 200) ? |
|
✅ All changelog entries have the correct PR link. |
|
🟢 No issues across the latest commits bdcae55. Review summaryIssues found across earlier commits 7ca81fd — 3 high, 3 medium, 1 low
🤖 AI-Generated Review | Vera Review Bot - v0.4.2 | 📚 Knowledge base: integration-skills
|
|
@efd6 can I get another review on this? |
💚 Build Succeeded
History
cc @kcreddy |
|
|
||
| ## Troubleshooting | ||
|
|
||
| ### "reached maximum number of CEL executions" |
There was a problem hiding this comment.
I was going to say that we can avoid raising this in agent health updates by using remaining_executions conditions in the CEL, but the signal is genuinely useful here, so I don't think using that would be a good idea.
| ?"NextToken": state.?page.next_token, | ||
| }.encode_json() | ||
| ).do_request().as(resp, (resp.StatusCode == 200) ? |
There was a problem hiding this comment.
Agree. I would recommend using post_request, then you can include the content type in the request call, rather than the approach suggested by Vera.
| }.encode_json() | ||
| ).do_request().as(resp, (resp.StatusCode == 200) ? | ||
| resp.Body.decode_json().as(body, | ||
| body.?NextToken.orValue("").as(next, |
There was a problem hiding this comment.
| body.?NextToken.orValue("").as(next, | |
| body.?NextToken.as(next, |
but equally (possibly better), this as binding could be dropped since we're not doing anything expensive to get it.
| body.?NextToken.orValue("").as(next, | ||
| { | ||
| "events": body.?Findings.orValue([]).map(e, {"message": e.encode_json()}), | ||
| "page": (next == "") ? {} : {"next_token": next}, |
There was a problem hiding this comment.
| "page": (next == "") ? {} : {"next_token": next}, | |
| "page": {?"next_token": next}, |
or
| "page": (next == "") ? {} : {"next_token": next}, | |
| "page": {?"next_token": body.?NextToken}, |
if the binding is removed.
| { | ||
| "events": body.?Findings.orValue([]).map(e, {"message": e.encode_json()}), | ||
| "page": (next == "") ? {} : {"next_token": next}, | ||
| "want_more": next != "", |
There was a problem hiding this comment.
| "want_more": next != "", | |
| "want_more": next.hasValue(), |
or
| "want_more": next != "", | |
| "want_more": has(body.NextToken), |
| @@ -0,0 +1,141 @@ | |||
| config_version: 2 | |||
There was a problem hiding this comment.
| config_version: 2 |

Proposed commit message
Note
Note on bugfix/enhancement separation. Per the backport policy agreed in #20040, bugfixes should land separately from enhancements so they can be cherry-picked to backport branches. That isn't possible here: the
Invalid NextTokenfix depends on the migration itself. httpjson re-evaluatesrequest.transformson every page and has no way to pin a value computed on page 1 for later pages, so thenow-derivedStartcan drift mid-pagination. The CEL program fixes this by holdingstartinstateand only recomputing it when neitherwant_morenorpage.next_tokenis set. MakingEndstatic was the furthest httpjson could go and was already in place.So this PR intentionally keeps both changelog entries under one version. If the
NextTokenfix ever needs backporting, it means backporting the migration, which would be an exception to the bugfix-only rule and would needrun_as_celsupport on the target line's minimum agent version.Checklist
changelog.ymlfile.How to test this PR locally