Fix forwarding of auth-response-headers to gRPC backends #7331
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @kd7lxl. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -448,7 +448,7 @@ func TestBuildAuthResponseHeaders(t *testing.T) { | |||
| "proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;", | |||
| } | |||
|
|
|||
| headers := buildAuthResponseHeaders(externalAuthResponseHeaders) | |||
| headers := buildAuthResponseHeaders(proxySetHeader(nil), externalAuthResponseHeaders) | |||
There was a problem hiding this comment.
@kd7lxl is that possible to add some unit_test for this specific case?
There was a problem hiding this comment.
just need add a case to TestProxySetHeader
|
/label tide/merge-method-squash @tao12345666333 any additional thoughts? |
k8s-ci-robot
added
do-not-merge/hold
|
/approve Will leave final review for Jintao, bot overall looks good to me, just want an answer about the unit_test (if it's possible or not) |
|
let me take a look, thanks |
| @@ -448,7 +448,7 @@ func TestBuildAuthResponseHeaders(t *testing.T) { | |||
| "proxy_set_header 'H-With-Caps-And-Dashes' $authHeader1;", | |||
| } | |||
|
|
|||
| headers := buildAuthResponseHeaders(externalAuthResponseHeaders) | |||
| headers := buildAuthResponseHeaders(proxySetHeader(nil), externalAuthResponseHeaders) | |||
There was a problem hiding this comment.
just need add a case to TestProxySetHeader
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
There was a problem hiding this comment.
@kd7lxl has added unit tests and I think it can be merged
/lgtm
/hold cancel
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kd7lxl, rikatz, tao12345666333 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…7331) * add e2e test for auth-response-headers annotation * add e2e test for grpc with auth-response-headers * fix forwarding of auth header to GRPC backends * add test case for proxySetHeader(nil)
* add e2e test for auth-response-headers annotation * add e2e test for grpc with auth-response-headers * fix forwarding of auth header to GRPC backends * add test case for proxySetHeader(nil)
…7331) * add e2e test for auth-response-headers annotation * add e2e test for grpc with auth-response-headers * fix forwarding of auth header to GRPC backends * add test case for proxySetHeader(nil)
Redefining headers to gRPC backends requires the directive
grpc_set_header, not the standardproxy_set_header. nginx.tmpl mostly does a good job of parameterizing this directive name and swapping in the correct name for the backend except for a few places fixed in this PR.What this PR does / why we need it:
When using a gRPC backend, external auth, and
auth-response-headers, the specified header is not rewritten with the one from the auth response. The original request header is passed through unmodified. This is bad because it could allow privilege escalation through spoofed authorization headers.Fixes #3914.
Types of changes
Which issue/s this PR fixes
Fixes #3914
How Has This Been Tested?
Added e2e test cases to exercise the
auth-response-headersannotation with http and grpc backends. The test asserts that the header from the external auth response is the one received by the upstream service, not the original value from the request.Checklist: