envoy icon indicating copy to clipboard operation
envoy copied to clipboard

Support allowlisting request headers for both gRPC and HTTP authorization servers

Open rulex123 opened this issue 2 years ago • 14 comments

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

rulex123 avatar Aug 29 '22 17:08 rulex123

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!

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/22877 was opened by rulex123.

see: more, trace.

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/).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/22877 was opened by rulex123.

see: more, trace.

/retest

rulex123 avatar Sep 01 '22 09:09 rulex123

Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/22877#issuecomment-1234000165 was created by @rulex123.

see: more, trace.

CI fails with some unrelated error (HTTP 404 on downloading some gz file), not sure what to do about that 🤷‍♀️

rulex123 avatar Sep 01 '22 09:09 rulex123

/retest

rulex123 avatar Sep 06 '22 12:09 rulex123

Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/22877#issuecomment-1238063709 was created by @rulex123.

see: more, trace.

/assign @envoyproxy/api-shepherds

ggreenway avatar Sep 06 '22 16:09 ggreenway

@envoyproxy/api-shepherds cannot be assigned to this issue.

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/22877#issuecomment-1238354792 was created by @ggreenway.

see: more, trace.

/assign-from @envoyproxy/api-shepherds

ggreenway avatar Sep 06 '22 16:09 ggreenway

@envoyproxy/api-shepherds assignee is @markdroth

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/22877#issuecomment-1238355350 was created by @ggreenway.

see: more, trace.

OK, I can rework the code to preserve the current behaviour.

rulex123 avatar Sep 12 '22 07:09 rulex123

@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?

rulex123 avatar Sep 18 '22 08:09 rulex123

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 avatar Sep 22 '22 20:09 ggreenway

@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!

rulex123 avatar Oct 16 '22 17:10 rulex123

/lgtm api

markdroth avatar Oct 17 '22 15:10 markdroth

/assign @pradeepcrao

Pradeep can you take a look as extension owner as Greg is OOO (sick). Thank you!

KBaichoo avatar Oct 26 '22 11:10 KBaichoo

@pradeepcrao would you mind giving this another pass

phlax avatar Nov 07 '22 14:11 phlax

Will take a look tomorrow. Sorry for the delay.

pradeepcrao avatar Nov 08 '22 03:11 pradeepcrao

Apologies, but I haven't had time to review this yet, and I'll be out until 11/28.

ggreenway avatar Nov 17 '22 22:11 ggreenway