envoy
envoy copied to clipboard
Support allowlisting request headers for both gRPC and HTTP authorization servers
Commit Message: add support for allowlisting request headers included in the check request to the authorization server, be it HTTP or gRPC (currently, this is supported for HTTP only). This patch causes a change in the current behaviour, which is to allow all client request headers through to a gRPC authorization server; moving forward, explicit configuration is needed to allowlist any necessary headers (for both HTTP and gRPC).
Additional Description: n/a
Risk Level: medium
Testing: updated unit and integration tests
Docs Changes: updated API protos
Release Notes: updated release notes
As a reminder, PRs marked as draft will not be automatically assigned reviewers, or be handled by maintainer-oncall triage.
Please mark your PR as ready when you want it to be reviewed!
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/)
.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/)
.
/retest
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
CI fails with some unrelated error (HTTP 404 on downloading some gz file), not sure what to do about that 🤷♀️
/retest
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
/assign @envoyproxy/api-shepherds
@envoyproxy/api-shepherds cannot be assigned to this issue.
/assign-from @envoyproxy/api-shepherds
@envoyproxy/api-shepherds assignee is @markdroth
OK, I can rework the code to preserve the current behaviour.
@ggreenway Before I change the code, I'd like to make sure we agree on the new API. To recap, the current default behaviour is that all client request headers are relayed to a gRPC auth server, but only a handful (i.e. Authorization, Host, Path and Method) to an HTTP auth server. For HTTP, one can add to these default headers by configuring allowed_headers
on the authorization request message.
Moving forward, we want to support configuring which headers are relayed to the auth server (no matter the protocol): the (new) default behaviour will be to relay all client request headers, and a newly introduced field on the ext_authz message - called headers_to_relay
- is used to control this behaviour. In addition, the pre-existing allowed_headers
is deprecated.
One implication of these changes is that HTTP auth servers that rely on the (previous) default behaviour, will have to configure the new headers_to_relay
field if they wish to mimic it. Otherwise, with these changes they will start receiving all client request headers.
WDYT?
If you want to change existing behavior, you're going to need to go through a deprecation cycle of the existing behavior, which roughly looks like adding a warning of using the existing behavior, wait a few releases, flip to the new behavior by default but allow a runtime override, wait a few releases, then delete code for the old behavior.
It will be simpler/easier to just maintain the current behavior, even though it doesn't really make sense to have different defaults between grpc and http modes.
@ggreenway I reworked the code to preserve backwards compatibility, as you suggested. I think this patch might be ready for a first pass of code review. Could you take a look, or loop in someone that can help with that? Thanks!
/lgtm api
/assign @pradeepcrao
Pradeep can you take a look as extension owner as Greg is OOO (sick). Thank you!
@pradeepcrao would you mind giving this another pass
Will take a look tomorrow. Sorry for the delay.
Apologies, but I haven't had time to review this yet, and I'll be out until 11/28.