Skip to content

feat: add support for extra_headers in forward-auth plugin#12405

Merged
Revolyssup merged 22 commits intoapache:masterfrom
Revolyssup:revolyssup/forward-auth-fix-2
Jul 14, 2025
Merged

feat: add support for extra_headers in forward-auth plugin#12405
Revolyssup merged 22 commits intoapache:masterfrom
Revolyssup:revolyssup/forward-auth-fix-2

Conversation

@Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Jul 7, 2025

Description

A follow up for - #12404. The root cause is explained in the revert PR.

With the new solution:
Do not need to pass the whole request body to the auth service and extract data from body into headers and then forward to the auth service. The headers to be extracted will be specified with a new field: extra_headers

Fixes #11200

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
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request plugin labels Jul 7, 2025
Copy link
Member

@kayx23 kayx23 left a comment

Choose a reason for hiding this comment

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

Docs comments

When the decision is to be made on the basis of POST body, then it is recommended to use `$post_arg.xyz` with `extra_headers` field and make the decision on Authorization service on basis of headers rather than using POST `request_method` to pass the entire request body to Authorization service.
:::

Create a serverless function on the `/auth` route that checks for the presence of the `tenant_id` header. If present, the route responds with HTTP 200 and sets the `X-User-ID` header to a fixed value `i-am-an-user`. If `tenant_id` is missing, it returns HTTP 400 with an error message.
Copy link
Member

Choose a reason for hiding this comment

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

Untranslated English content in chinese doc

nic-6443
nic-6443 previously approved these changes Jul 9, 2025
@kayx23
Copy link
Member

kayx23 commented Jul 11, 2025

No additional comments for docs.

@nic-6443 nic-6443 requested review from bzp2010 and membphis July 11, 2025 07:24
nic-6443
nic-6443 previously approved these changes Jul 11, 2025
Comment on lines 128 to 130
if not err and n_resolved > 0 then
auth_headers[header] = resolve_value
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This basically says that the value itself is only rewritten when the variable substitution succeeds, which may not be necessary?

Copy link
Contributor Author

@Revolyssup Revolyssup Jul 14, 2025

Choose a reason for hiding this comment

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

I have modified to only skip in case of error.
When the value is not of type variable(doesn't contain $), it will still be added to auth headers.
Added one more test case to confirm it.

membphis
membphis previously approved these changes Jul 14, 2025
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM, one minor issue

auth_headers[header] = resolve_value
end
if err then
core.log.error("failed to resolve variable in extra header '", header, "': ", err)
Copy link
Member

Choose a reason for hiding this comment

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

another question: I think we should print the value, it is important information for user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@Revolyssup Revolyssup dismissed stale reviews from membphis and nic-6443 via ab3bc84 July 14, 2025 04:59
Copy link
Contributor

@bzp2010 bzp2010 left a comment

Choose a reason for hiding this comment

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

Your checklist mentions that it is not compatible? Can you reconfirm this?

@Revolyssup
Copy link
Contributor Author

Revolyssup commented Jul 14, 2025

Your checklist mentions that it is not compatible? Can you reconfirm this?

Oh this is compatible. Missed the last check. Fixed it.

@Revolyssup Revolyssup merged commit eed93f3 into apache:master Jul 14, 2025
30 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request plugin size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: After the forward-auth verification is passed, the upstream cannot obtain the request body (payload) with 504 Timeout

5 participants