Skip to content

Store output API key secrets in .fleet-secrets instead of .fleet-agents - #7416

Merged
ycombinator merged 12 commits into
elastic:mainfrom
ycombinator:secure-output-api-key-storage
Jul 29, 2026
Merged

Store output API key secrets in .fleet-secrets instead of .fleet-agents#7416
ycombinator merged 12 commits into
elastic:mainfrom
ycombinator:secure-output-api-key-storage

Conversation

@ycombinator

@ycombinator ycombinator commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What is the problem this PR solves?

The kibana_system built-in role has broad read (actually all) access to .fleet-agents*. Fleet Server currently writes output API key secrets as plaintext id:secret strings into outputs.{name}.api_key on agent documents in .fleet-agents-7. Any principal with kibana_system credentials can therefore extract these secrets and use them to write to data streams that kibana_system itself cannot access — a privilege escalation.

How does this PR solve the problem?

Instead of storing the raw id:secret string in .fleet-agents-7, Fleet Server now:

  1. On new key generation (policy_output.go): writes the key material to .fleet-secrets via POST /_fleet/secret and stores only a $co.elastic.secret{<secretId>} reference in outputs.{name}.api_key.
  2. At agent checkin (policy_output.go): detects the reference format and resolves it via GET /_fleet/secret/{id} before injecting api_key into the policy payload sent to the agent.
  3. On ACK / key rotation (handleAck.go): deletes the retired secret from .fleet-secrets via DELETE /_fleet/secret/{id}. The secret ID is carried in a new secret_id field on ToRetireAPIKeyIdsItems (schema change in model/schema.json).

Backward compatibility: existing plaintext values already stored in .fleet-agents-7 continue to work unchanged. The check secret.ParseSecretReference(output.APIKey) returns false for plain id:secret strings, so they are used directly. Agents migrate to the new format naturally on the next key rotation.

Dependency: requires elastic/elasticsearch#154498 (granting write_fleet_secrets cluster privilege to the elastic/fleet-server service account) to be merged and deployed first.

How to test this PR locally

Run the integration test suite against a stack with elastic/elasticsearch#154498 applied:

mage integration:test

Verify that:

  • Newly enrolled agents receive a valid api_key in checkin responses
  • .fleet-agents-7 documents show $co.elastic.secret{...} in outputs.*.api_key rather than a plaintext key
  • .fleet-secrets contains the corresponding entries
  • After a policy change that rotates the key, the old secret is deleted from .fleet-secrets

Design Checklist

  • I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer. (Secrets are resolved per-request from ES; no local state.)
  • I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected. (Each checkin that needs to rotate a key adds one POST /_fleet/secret write; reads are already part of the checkin path via ReadSecrets.)
  • I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc. (Secret writes/deletes are only on key rotation, not every checkin.)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool

Related issues

@github-actions

This comment has been minimized.

@mergify

mergify Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cmacknz
cmacknz requested a review from blakerouse July 21, 2026 20:43
@ycombinator
ycombinator force-pushed the secure-output-api-key-storage branch from 81d725e to c083c9b Compare July 22, 2026 15:31
@ycombinator ycombinator added backport-skip Skip notification from the automated backport with mergify enhancement New feature or request labels Jul 22, 2026
Comment thread internal/pkg/policy/policy_output.go Outdated
Comment thread internal/pkg/bulk/secret.go
Comment thread internal/pkg/policy/policy_output.go
Comment thread internal/pkg/policy/policy_output.go Outdated
Comment thread internal/pkg/api/handleAck.go
@ycombinator
ycombinator requested review from lorienhu and samuelvl July 23, 2026 13:36
samuelvl
samuelvl previously approved these changes Jul 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

TL;DR

Both cloud E2E jobs fail before the E2E suite starts because the provisioned deployment has no Fleet endpoint: Terraform outputs fleet_url = "", and .buildkite/scripts/cloud_e2e_test.sh exits with status 1 when FLEET_SERVER_URL is empty. This is a shared deployment-startup failure, not a FIPS-specific failure.

Remediation

  • Inspect the failed deployment’s Integrations Server / Elastic Agent startup logs to determine why the Fleet endpoint was not published, then fix that startup error and rerun the cloud E2E jobs.
  • Verify the 9.6.0-SNAPSHOT stack used by the deployment includes the Elasticsearch write_fleet_secrets change required by this PR; the PR explicitly depends on that change and the new Fleet Server path calls POST /_fleet/secret.
  • Confirm Terraform outputs a non-empty fleet_url before mage test:cloudE2ERun is invoked.
Investigation details

Root Cause

The confirmed failure is the cloud E2E precondition. In .buildkite/scripts/cloud_e2e_test.sh, FLEET_SERVER_URL is read from the Terraform state and the script exits 1 when it is empty. Both failed artifacts show fleet_url = "" in the Terraform output immediately before the deployment is destroyed. dev-tools/cloud/terraform/outputs.tf derives this value from ec_deployment.deployment.integrations_server.endpoints.fleet, so the endpoint was unavailable from the provisioned deployment.

The captured artifacts do not include the earlier Elastic Agent/Integrations Server startup error, so they cannot establish whether the endpoint was missing because the image failed to start, the snapshot lacked the required Elasticsearch privilege, or another deployment issue occurred. The PR changes no cloud Terraform or E2E orchestration code; both failures use the same shared script.

Evidence

  • Build: https://buildkite.com/elastic/fleet-server/builds/15692
  • Jobs: Cloud e2e Test and Cloud e2e FIPS Test
  • Key log excerpt from both artifacts: fleet_url = "" -> null (Terraform output), followed by Error: The command exited with status 1 after cleanup.
  • .buildkite/scripts/cloud_e2e_test.sh: explicitly reports FLEET_SERVER_URL is empty, cloud e2e tests cannot be executed and exits 1.
  • No matching flaky-test issue was found.

Verification

Not run locally: the failure depends on the external Elastic Cloud deployment, and the provided logs omit the deployment startup log that caused the Fleet endpoint to be unavailable.


What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

@github-actions

Copy link
Copy Markdown
Contributor

TL;DR

Both Cloud e2E jobs fail before the Go tests start because Terraform returns an empty fleet_url; .buildkite/scripts/cloud_e2e_test.sh then exits with status 1. This is a cloud deployment/output issue, not evidence of a failure in the PR’s secret-handling code.

Remediation

  • Inspect the complete test:cloudE2EUp output and the resulting ECH deployment to determine why integrations_server.endpoints.fleet is absent; rerun after a Fleet endpoint is provisioned and terraform output fleet_url is non-empty.
  • Preserve the full pre-cleanup log on the next run; the supplied artifacts contain only the teardown and the generic exit-status line.
  • Once the endpoint is available, rerun both standard and FIPS cloud e2E jobs. If the tests then fail on POST /_fleet/secret, verify the required Elasticsearch change elastic/elasticsearch#154498 is in the selected snapshot (dev-tools/integration/.env still points to 9.6.0-b07e0453-SNAPSHOT).
Investigation details

Root Cause

The captured teardown plan for both jobs reports fleet_url = "" while the Elasticsearch, Kibana, and integrations-server HTTPS endpoints are populated. In .buildkite/scripts/cloud_e2e_test.sh:31-35, that output is assigned to FLEET_SERVER_URL and an empty value causes the script to exit at line 44, before mage test:cloudE2ERun at line 48. The Terraform output is explicitly empty when integrations_server.endpoints is null (dev-tools/cloud/terraform/outputs.tf:28-30).

Evidence

Verification

  • Not run locally: the cloud E2E path requires the provisioned ECH environment, and the provided logs do not contain the deployment command’s failure details or the FLEET_SERVER_URL is empty line.

Follow-up

If a full rerun reaches the Go tests, separately validate the PR’s documented Elasticsearch dependency before attributing any later / _fleet/secret error to this build.


What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

lorienhu
lorienhu previously approved these changes Jul 23, 2026
@ycombinator
ycombinator force-pushed the secure-output-api-key-storage branch from 020d99e to 7c22d8f Compare July 23, 2026 23:23
ycombinator and others added 2 commits July 28, 2026 21:42
Fixes a privilege escalation (elastic/security#12225) where the kibana_system
role's read access to .fleet-agents-7 could be used to extract plaintext
output API key secrets and escalate to SIEM write access.

Changes:
- On new key generation, write the raw key material to .fleet-secrets via
  POST /_fleet/secret and store only a $co.elastic.secret{id} reference in
  outputs.{name}.api_key in .fleet-agents-7
- At agent checkin, resolve the reference via GET /_fleet/secret/{id} before
  injecting api_key into the policy payload delivered to the agent
- On ACK (key rotation), delete the retired secret from .fleet-secrets via
  DELETE /_fleet/secret/{id}; the secret ID is carried in a new secret_id
  field on ToRetireAPIKeyIdsItems
- Plaintext values already stored in .fleet-agents-7 continue to work
  transparently (backward compat); migration to the new format happens
  naturally on the next key rotation

Requires elastic/elasticsearch#154498 (write_fleet_secrets privilege for the
elastic/fleet-server service account) to be merged and deployed first.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ycombinator and others added 4 commits July 28, 2026 21:42
When an output is removed from the policy, the retired API key entry
was missing SecretID, so deleteRetiredSecrets would skip cleaning up
the corresponding .fleet-secrets entry.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
c.Perform only errors on network failures, not 4xx/5xx responses.
Without this check, a 403 (missing write_fleet_secrets privilege) or
500 would cause Write() to return an empty secret ID, producing a
broken $co.elastic.secret{} reference silently stored in the agent doc.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ReadSecrets returns no error when a secret ID is absent from the
response (e.g. deleted between write and read). The zero-value map
lookup would silently inject an empty api_key into the agent policy,
causing the agent to fail connecting to Elasticsearch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
handleUnenroll was invalidating all output API keys but never deleting
their corresponding secrets from .fleet-secrets. Add a deleteRetiredSecrets
call that covers both retired keys (SecretID already set in ToRetireAPIKeyIds
items) and the current active output key (secret ID parsed from the
$co.elastic.secret{} reference in output.APIKey).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ycombinator
ycombinator force-pushed the secure-output-api-key-storage branch from 7c22d8f to 0af4d41 Compare July 29, 2026 04:42
The golangci-lint config has check-type-assertions: true and
--whole-files is passed, so touching bulk.go surfaces all pre-existing
single-value type assertions. Convert them to v, _ := args.Get(0).(T)
throughout.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
--whole-files causes golangci-lint to flag pre-existing issues in any
file touched by the PR. Fix prealloc warnings in secret.go by adding
capacity hints to slice literals, and replace WriteString(fmt.Sprintf)
with fmt.Fprintf in policy_output.go as suggested by staticcheck QF1012.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ycombinator
ycombinator enabled auto-merge (squash) July 29, 2026 13:48
@ycombinator
ycombinator requested a review from lorienhu July 29, 2026 13:49
@ycombinator ycombinator added backport-9.4 Automated backport to 9.4 branch. backport-9.5 Automated backport to the 9.5 branch backport-skip Skip notification from the automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify backport-9.4 Automated backport to 9.4 branch. backport-9.5 Automated backport to the 9.5 branch labels Jul 29, 2026

@blakerouse blakerouse 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.

Overall the code looks good, but I do have a worry. Correct me if I am off base here.

Being that the API key is now in secrets and this is not running like a transactional database across two tables there is a case that might not be handled correctly.

Write the secret to the secret store, but then writing the reference into .fleet-agents fails. I don't see where if that fails the secret is then deleted. I feel like it would need to be deleted then, because it is just left in there with no reference.

Also what happens if writing into the secret store fails? Does that just error back to the caller and they make the API call again? Seems like it, I believe that is correct.

…Secret

If WriteSecret succeeds but the subsequent bulker.Update to .fleet-agents
fails, the written secret would previously be left in .fleet-secrets with
no reference. Now a compensating DeleteSecret is issued on the update
failure path; if that cleanup also fails it logs a warning and still
returns the original update error.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ycombinator

Copy link
Copy Markdown
Contributor Author

Write the secret to the secret store, but then writing the reference into .fleet-agents fails. I don't see where if that fails the secret is then deleted. I feel like it would need to be deleted then, because it is just left in there with no reference.

Yep, good catch. This is a gap. Fixed now in 0818c0e.

Also what happens if writing into the secret store fails? Does that just error back to the caller and they make the API call again? Seems like it, I believe that is correct.

Yes, that's what happens. This early return when bulker.WriteSecret fails means no secret is written and the Agent doc is not updated either. When Agent makes the API call again, the whole flow will be retried.

@ycombinator
ycombinator requested a review from blakerouse July 29, 2026 16:38

@blakerouse blakerouse 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.

Nice! Thanks for the fixes.

@ycombinator
ycombinator merged commit bc1c737 into elastic:main Jul 29, 2026
12 checks passed
@ycombinator
ycombinator deleted the secure-output-api-key-storage branch July 29, 2026 20:57
ycombinator added a commit that referenced this pull request Aug 3, 2026
maps.Copy was introduced in the conflict resolution but the maps
package was not imported on this branch (it landed on main via #7416
which was not backported).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ycombinator added a commit that referenced this pull request Aug 3, 2026
maps.Copy was introduced in the conflict resolution but the maps
package was not imported on this branch (it landed on main via #7416
which was not backported).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ycombinator added a commit that referenced this pull request Aug 3, 2026
maps.Copy was introduced in the conflict resolution but the maps
package was not imported on this branch (it landed on main via #7416
which was not backported).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-skip Skip notification from the automated backport with mergify enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants