Skip to content

fix: use last tenant header value to prevent ext_authz header injection#87

Merged
yizhaodev merged 2 commits intollm-d-incubation:mainfrom
pierDipi:fix/tenant-header-last-value-workaround
Mar 6, 2026
Merged

fix: use last tenant header value to prevent ext_authz header injection#87
yizhaodev merged 2 commits intollm-d-incubation:mainfrom
pierDipi:fix/tenant-header-last-value-workaround

Conversation

@pierDipi
Copy link
Copy Markdown
Contributor

@pierDipi pierDipi commented Mar 4, 2026

The external auth service may append response headers instead of overwriting them. A client can send a spoofed tenant header which gets concatenated with the auth-injected value. This takes the last value from multi-value tenant headers, which is always the trusted auth-injected one per Envoy ext_authz ordering guarantees.

Adds tests for tenant header extraction including the multi-value workaround, request ID handling, and metrics/health path skipping.

Copilot AI review requested due to automatic review settings March 4, 2026 13:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the API server request middleware against tenant header injection when Envoy ext_authz appends (rather than overwrites) tenant headers, and adds focused unit tests around tenant extraction, request ID behavior, and skip-path handling.

Changes:

  • Update tenant ID extraction to prefer the last value when multiple tenant header values are present.
  • Add unit tests covering tenant header extraction scenarios (including multi-value), request ID propagation/generation, and skipping middleware for metrics/health endpoints.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
internal/apiserver/middleware/request_middleware.go Adjusts tenant header extraction logic to use the last header value to mitigate spoofing via header appends.
internal/apiserver/middleware/request_middleware_test.go Adds unit tests for tenant extraction, request ID behavior, and metrics/health skip-path behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@pierDipi pierDipi force-pushed the fix/tenant-header-last-value-workaround branch from 41d9df9 to 01f4d2f Compare March 4, 2026 13:59
@pierDipi pierDipi requested a review from Copilot March 4, 2026 14:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@lioraron lioraron requested a review from yizhaodev March 4, 2026 14:29
The external auth service may append response headers instead of
overwriting them. A client can send a spoofed tenant header which
gets concatenated with the auth-injected value. This takes the last
value from multi-value tenant headers, which is always the trusted
auth-injected one per Envoy ext_authz ordering guarantees.

Adds tests for tenant header extraction including the multi-value
workaround, request ID handling, and metrics/health path skipping.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@pierDipi pierDipi force-pushed the fix/tenant-header-last-value-workaround branch from 01f4d2f to bd3fa76 Compare March 4, 2026 14:33
@pierDipi pierDipi requested a review from Copilot March 4, 2026 14:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Copy Markdown
Contributor

@yizhaodev yizhaodev left a comment

Choose a reason for hiding this comment

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

LGTM

@pierDipi
Copy link
Copy Markdown
Contributor Author

pierDipi commented Mar 6, 2026

@yizhaodev can we merge?

@yizhaodev yizhaodev merged commit c9b3a9b into llm-d-incubation:main Mar 6, 2026
8 checks passed
@yizhaodev
Copy link
Copy Markdown
Contributor

@pierDipi merged, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants