[elastic_security] Migrate Alert Data Stream Authentication From required_vars to var_groups - #20366
[elastic_security] Migrate Alert Data Stream Authentication From required_vars to var_groups#20366mohitjha-elastic wants to merge 2 commits into
Conversation
mohitjha-elastic
added
breaking change
Team:Security-Service Integrations
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
✅ 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. |
|
✅ All changelog entries have the correct PR link. |
💚 Build Succeeded
|
andrewkroh
added
the
documentation
|
@vera-review-bot review |
![vera-review-bot[bot]](https://avatars.githubusercontent.com/in/3297022?s=60&v=4){kind=small}
| "Content-Type": ["application/json"], | ||
| "Authorization": [ | ||
| state.auth_type == 'api_auth' ? | ||
| has(state.api_key) && state.api_key != "" ? |
There was a problem hiding this comment.
Severity: 🔴 Critical confidence: medium path: packages/elastic_security/data_stream/alert/agent/stream/cel.yml.hbs:55
The new presence-based auth detection cannot distinguish auth types because the state block still emits all four credential keys unconditionally; wrap each key in a {{#if}} guard so unselected credentials are absent from state.
Details
The auth_type selector was removed from state, and the Authorization header now infers the auth method from the credential values themselves. But the state: block (lines 18-21) still emits api_key, user, password and bearer_token on every render, regardless of which var_group option the user picked.
Two consequences:
-
has(state.api_key)/has(state.user)/has(state.password)are always true — the key is always present in the map — so those guards do no work and the whole decision rests on the!= ""comparisons. -
When a var is not set (basic_auth or bearer_auth selected, so
api_keyis unset), handlebars renders the line asapi_key:with no value, which YAML-loads asnull, not as an empty string.state.api_key != ""is therefore true for a null value, the first branch is taken, and"Apikey " + state.api_keyfails with a no-such-overload evaluation error. Basic Auth and Bearer Auth users get no events at all.
The rest of this template already guards optional vars this way (proxy_url, ssl, http_client_timeout on lines 7-15) precisely because unset vars render as empty/null. The auth vars did not need guards before, because the old code selected on state.auth_type and never read the unused keys; now that the code reads them, they do.
CI does not catch this: the system test only exercises the API-key path.
Recommendation:
Emit each credential only when it is set, so has() is a genuine presence check:
state:
{{#if api_key}}
api_key: {{api_key}}
{{/if}}
{{#if username}}
user: {{username}}
{{/if}}
{{#if password}}
password: {{password}}
{{/if}}
{{#if bearer_token}}
bearer_token: {{bearer_token}}
{{/if}}
index: .alerts-security.alerts-{{space_id}}
batch_size: {{batch_size}}
initial_interval: {{initial_interval}}Then the header selection can rely on presence alone:
"Authorization": [
has(state.api_key) ?
"Apikey " + state.api_key
:
has(state.user) && has(state.password) ?
"Basic " + base64(state.user + ":" + state.password)
:
"Bearer " + state.bearer_token
],
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
There was a problem hiding this comment.
I don't think this is a correctness issue, but I do think the suggested approach is cleaner.
| Processor '{{{ _ingest.on_failure_processor_type }}}' | ||
| {{{#_ingest.on_failure_processor_tag}}}with tag '{{{ _ingest.on_failure_processor_tag }}}' | ||
| {{{/_ingest.on_failure_processor_tag}}}failed with message '{{{ _ingest.on_failure_message }}}' | ||
| {{{/_ingest.on_failure_processor_tag}}}failed in pipeline '{{{ _ingest.pipeline }}}' with message '{{{ _ingest.on_failure_message }}}' |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: high path: packages/elastic_security/data_stream/alert/elasticsearch/ingest_pipeline/default.yml:150
The pipeline-level on_failure message interpolates _ingest.pipeline, which does not exist in an on_failure context and always renders empty; use _ingest.on_failure_pipeline instead.
Details
Elasticsearch exposes only four ingest metadata fields inside an on_failure block: on_failure_message, on_failure_processor_type, on_failure_processor_tag and on_failure_pipeline. _ingest.pipeline is not one of them, so the new text always renders as failed in pipeline '' with message '...' — the addition adds noise instead of the intended context.
The json processor's own on_failure in this same file (line 22) already uses the correct {{{_ingest.on_failure_pipeline}}}, so the two error messages are also inconsistent with each other.
Recommendation:
Use the on_failure-scoped metadata field:
on_failure:
- append:
field: error.message
tag: append_pipeline_error_message
value: >-
Processor '{{{ _ingest.on_failure_processor_type }}}'
{{{#_ingest.on_failure_processor_tag}}}with tag '{{{ _ingest.on_failure_processor_tag }}}'
{{{/_ingest.on_failure_processor_tag}}}failed in pipeline '{{{ _ingest.on_failure_pipeline }}}' with message '{{{ _ingest.on_failure_message }}}'🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
There was a problem hiding this comment.
This is not correct for single pipeline ingestions. The syntactic consistency may be worth addressing though.
Details
PUT _ingest/pipeline/test-on-failure-fields
{
"processors": [
{
"convert": {
"field": "not_a_number",
"type": "integer",
"tag": "convert_test"
}
}
],
"on_failure": [
{
"set": {
"field": "debug.ingest_pipeline",
"value": "{{{_ingest.pipeline}}}"
}
},
{
"set": {
"field": "debug.on_failure_pipeline",
"value": "{{{_ingest.on_failure_pipeline}}}"
}
},
{
"set": {
"field": "debug.on_failure_processor_type",
"value": "{{{_ingest.on_failure_processor_type}}}"
}
},
{
"set": {
"field": "debug.on_failure_processor_tag",
"value": "{{{_ingest.on_failure_processor_tag}}}"
}
},
{
"set": {
"field": "debug.on_failure_message",
"value": "{{{_ingest.on_failure_message}}}"
}
}
]
}
POST _ingest/pipeline/test-on-failure-fields/_simulate
{
"docs": [
{
"_source": {
"not_a_number": "hello"
}
}
]
}
DELETE _ingest/pipeline/test-on-failure-fields
{
"docs": [
{
"doc": {
"_index": "_index",
"_version": "-3",
"_id": "_id",
"_source": {
"not_a_number": "hello",
"debug": {
"ingest_pipeline": "test-on-failure-fields",
"on_failure_message": "For input string: \"hello\"",
"on_failure_processor_tag": "convert_test",
"on_failure_pipeline": "test-on-failure-fields",
"on_failure_processor_type": "convert"
}
},
"_ingest": {
"timestamp": "2026-07-28T01:04:51.588036835Z"
}
}
}
]
}
| - version: "0.6.0" | ||
| changes: | ||
| - description: | | ||
| Migrate alert data stream authentication from required_vars to var_groups for improved UX and cleaner configuration management. |
There was a problem hiding this comment.
Severity: 🟡 Medium confidence: medium path: packages/elastic_security/changelog.yml:5
Moving the credential vars from stream level to input level drops their stored values on upgrade, but neither the changelog entry nor the README tells users they must re-enter credentials; add that upgrade note.
Details
api_key, username, password and bearer_token previously lived in data_stream/alert/manifest.yml and were stored on the policy under the stream's vars. In 0.6.0 they are declared under policy_templates[].inputs[].vars instead. Fleet matches stored var values by name at the same level when it upgrades a package policy, so a var that disappears from the stream level and reappears at the input level does not carry its value forward — existing 0.5.0 policies come out of the upgrade with empty credentials and stop collecting until an operator re-enters them.
The entry is correctly typed breaking-change, but its text ("improved UX and cleaner configuration management") gives the operator no indication that action is required. The README setup section is likewise unchanged.
Recommendation:
Spell out the required action in the changelog entry, and mirror it in _dev/build/docs/README.md:
- version: "0.6.0"
changes:
- description: |
Migrate alert data stream authentication from required_vars to var_groups for improved UX and cleaner configuration management.
Bump Kibana and Elastic Agent requirement to ^9.4.0 to support this feature.
Breaking change: the authentication settings moved from the data stream to the integration policy level.
After upgrading, edit each existing Elastic Security integration policy and re-enter the credentials
(API key, username/password, or bearer token) for the selected authentication type.
type: breaking-change
link: https://github.com/elastic/integrations/pull/20366🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| on_failure: | ||
| - append: | ||
| field: error.message | ||
| tag: append_pipeline_error_message |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/elastic_security/data_stream/alert/elasticsearch/ingest_pipeline/default.yml:146
The tag backfill in this on_failure block is incomplete — the third processor (append to tags) still has no tag; add one so the package is consistent with the format_version 3.6.1 bump.
Details
This PR adds tag to three processors and bumps format_version to 3.6.1, the spec version at which processor tags are checked. The final processor of the same pipeline-level on_failure block — the append to tags at line 155 — was missed, so the block is now half-tagged and the untagged failure is the one that will be hardest to trace, since it is what runs when the pipeline itself blows up.
Recommendation:
Tag the remaining on_failure processor:
- append:
field: tags
tag: append_pipeline_error_preserve_original_event_tag
value: preserve_original_event
allow_duplicates: false🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
| title: Collect Elastic Security events via API | ||
| description: Collect events from Elastic instance via API. | ||
| vars: | ||
| - name: username |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: low path: packages/elastic_security/manifest.yml:47
The four relocated credential vars lost their show_user: true attribute in the move to the policy template; restore it so they stay in the primary configuration form rather than behind Advanced options.
Details
At the data stream level each of username, password, api_key and bearer_token carried show_user: true. The copies added under policy_templates[].inputs[].vars carry only type, title, description, multi and secret. Dropping required: false is correct here (the spec forbids required: true on vars owned by a var_group, and requiredness is inferred from var_groups[].required), but show_user is an independent display hint and is not implied by var_group membership.
Recommendation:
Keep the display hint on the relocated vars:
vars:
- name: username
type: text
title: Username
description: The username of Elasticsearch Instance to be used with Basic Auth headers.
multi: false
show_user: true
- name: password
type: password
title: Password
description: The password of Elasticsearch Instance to be used with Basic Auth headers.
multi: false
show_user: true
secret: true🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
Review summaryIssues found across the latest commits 7b66c5c — 1 critical, 2 medium, 2 low
🤖 AI-Generated Review | Vera Review Bot | 📚 Knowledge base: integration-skills
|
| "Content-Type": ["application/json"], | ||
| "Authorization": [ | ||
| state.auth_type == 'api_auth' ? | ||
| has(state.api_key) && state.api_key != "" ? |
There was a problem hiding this comment.
I don't think this is a correctness issue, but I do think the suggested approach is cleaner.
| - type: cel | ||
| title: Collect Elastic Security events via API | ||
| description: Collect events from Elastic instance via API. | ||
| vars: |
There was a problem hiding this comment.
Is there a reason to move this to the package level?
| Processor '{{{ _ingest.on_failure_processor_type }}}' | ||
| {{{#_ingest.on_failure_processor_tag}}}with tag '{{{ _ingest.on_failure_processor_tag }}}' | ||
| {{{/_ingest.on_failure_processor_tag}}}failed with message '{{{ _ingest.on_failure_message }}}' | ||
| {{{/_ingest.on_failure_processor_tag}}}failed in pipeline '{{{ _ingest.pipeline }}}' with message '{{{ _ingest.on_failure_message }}}' |
There was a problem hiding this comment.
This is not correct for single pipeline ingestions. The syntactic consistency may be worth addressing though.
Details
PUT _ingest/pipeline/test-on-failure-fields
{
"processors": [
{
"convert": {
"field": "not_a_number",
"type": "integer",
"tag": "convert_test"
}
}
],
"on_failure": [
{
"set": {
"field": "debug.ingest_pipeline",
"value": "{{{_ingest.pipeline}}}"
}
},
{
"set": {
"field": "debug.on_failure_pipeline",
"value": "{{{_ingest.on_failure_pipeline}}}"
}
},
{
"set": {
"field": "debug.on_failure_processor_type",
"value": "{{{_ingest.on_failure_processor_type}}}"
}
},
{
"set": {
"field": "debug.on_failure_processor_tag",
"value": "{{{_ingest.on_failure_processor_tag}}}"
}
},
{
"set": {
"field": "debug.on_failure_message",
"value": "{{{_ingest.on_failure_message}}}"
}
}
]
}
POST _ingest/pipeline/test-on-failure-fields/_simulate
{
"docs": [
{
"_source": {
"not_a_number": "hello"
}
}
]
}
DELETE _ingest/pipeline/test-on-failure-fields
{
"docs": [
{
"doc": {
"_index": "_index",
"_version": "-3",
"_id": "_id",
"_source": {
"not_a_number": "hello",
"debug": {
"ingest_pipeline": "test-on-failure-fields",
"on_failure_message": "For input string: \"hello\"",
"on_failure_processor_tag": "convert_test",
"on_failure_pipeline": "test-on-failure-fields",
"on_failure_processor_type": "convert"
}
},
"_ingest": {
"timestamp": "2026-07-28T01:04:51.588036835Z"
}
}
}
]
}
4 participants
Proposed commit message
Checklist
changelog.ymlfile.How to test this PR locally
Related Issues