Skip to content

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

Merged
Revolyssup merged 22 commits into
apache: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 into
apache:masterfrom
Revolyssup:revolyssup/forward-auth-fix-2

Conversation

@Revolyssup

@Revolyssup Revolyssup commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

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

@kayx23 kayx23 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docs comments

Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md
Comment thread apisix/plugins/forward-auth.lua Outdated
Comment thread apisix/plugins/forward-auth.lua Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
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
Copy Markdown
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

Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
Comment thread docs/en/latest/plugins/forward-auth.md Outdated
Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
Comment thread docs/zh/latest/plugins/forward-auth.md Outdated
nic-6443
nic-6443 previously approved these changes Jul 9, 2025
Comment thread apisix/plugins/forward-auth.lua Outdated
@kayx23

kayx23 commented Jul 11, 2025

Copy link
Copy Markdown
Member

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 thread apisix/plugins/forward-auth.lua Outdated
Comment on lines +128 to +130
if not err and n_resolved > 0 then
auth_headers[header] = resolve_value
end

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.

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

@Revolyssup Revolyssup Jul 14, 2025

Copy link
Copy Markdown
Contributor Author

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

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, one minor issue

Comment thread apisix/plugins/forward-auth.lua Outdated
auth_headers[header] = resolve_value
end
if err then
core.log.error("failed to resolve variable in extra header '", header, "': ", err)

Copy link
Copy Markdown
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
Copy Markdown
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

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

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

@Revolyssup

Revolyssup commented Jul 14, 2025

Copy link
Copy Markdown
Contributor Author

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