Skip to content

fix(secret): do not include secret values in aws and vault error messages - #14049

Open
nic-6443 wants to merge 3 commits into
apache:masterfrom
nic-6443:fix/secret-error-no-value
Open

nic-6443 wants to merge 3 commits into
apache:masterfrom
nic-6443:fix/secret-error-no-value

Conversation

@nic-6443

@nic-6443 nic-6443 commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Description

When a $secret:// reference can't be resolved, apisix/secret.lua logs the provider's error at error level. Two providers put the fetched content into that error:

  • aws: if the reference has a field ($secret://aws/<id>/<secret>/<field>) but the stored secret string isn't JSON, the decode error included the whole secret string, i.e. the plaintext secret ended up in error.log.
  • vault: if the response has no data field, the error included the raw response body.

Both errors now carry only identifiers: the AWS secret name and field, and the vault path. Nothing else changes, the lookup logic and return values are the same. One visible side effect: a vault request rejected with 403 no longer carries vault's {"errors":["permission denied"]} body in the error either, so the existing TEST 8 and TEST 9 assertions in t/secret/vault.t are updated to the new message.

The new cases in t/secret/aws.t and t/secret/vault.t resolve such a reference through fetch_secrets and assert the error is logged without the secret value or response body. Both fail on current master and pass with this change. The aws case uses the existing apisix/full/path localstack fixture; the vault case serves the response from a test location, so it doesn't need a vault server.

Heads-up: #14047 also touches apisix/secret/aws.lua and apisix/secret/vault.lua. This diff is limited to the two error-message lines so the two should merge cleanly; whichever lands second should merge master and re-check.

Which issue(s) this PR fixes:

N/A

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change (not needed, only error log content changes)
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Copilot AI balanced review requested due to automatic review settings October 16, 2026 07:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two existing Vault tests still expect the removed error format and need updated assertions.

1 open finding
What changed in this PR

Sanitizes AWS and Vault secret-resolution errors to avoid logging fetched secret content.

Changes:

  • Replaces secret values and response bodies with lookup identifiers.
  • Adds regression tests checking that sensitive content is absent from error logs.
File Description
t/​secret/​vault.t Adds response-body logging regression coverage.
t/​secret/​aws.t Adds plaintext-secret logging regression coverage.
apisix/​secret/​vault.lua Reports the Vault path instead of the response body.
apisix/​secret/​aws.lua Reports secret and field identifiers instead of the secret value.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apisix/secret/vault.lua
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants