Add controls around injected headers

[howardjohn]

This PR adds controls to which headers should be added to
requests/responses. This solves (most of)
istio/istio#17635, a common feature request.

A prototype is implemented in istio/istio#37215.

Open questions:

• Naming bikeshed
• Mesh config or proxy config or something else? Currently its in proxy
  config
• Is a list of ENUMs the best way to represent this? it does allow a
  strange config like [REQUEST_ID, REQUEST_ID] but that can just
  rejected

[howardjohn]

cc @jacob-delgado @nrjpoddar @costinm @hzxuzhonghu @ramaraochavali you all probably have opinions here

[howardjohn]

cc @nrjpoddar any thoughts?

[nrjpoddar]

How does it work if I have XFCC setting configured via GatewayTopology setting?

[nrjpoddar]

This feels like it belongs to VirtualService for more fine grained control.

[howardjohn]

How does it work if I have XFCC setting configured via GatewayTopology setting?

I suppose you mean https://istio.io/latest/docs/ops/configuration/traffic-management/network-topologies/#configuring-x-forwarded-client-cert-headers? There is some overlap for sure but I think it's useful to have config here. This is an on/off switch that applies to sidecar+gateway, then at gateway you can additionally control how it works if it's "on"?

This feels like it belongs to VirtualService for more fine grained control.
About half of these are configurable only at levels higher than Route in Envoy, some I suspect we would need, at best, Proxy Config if we wanted to make it fine grained? Like gw topology

[nrjpoddar]

How does it work if I have XFCC setting configured via GatewayTopology setting?

I suppose you mean https://istio.io/latest/docs/ops/configuration/traffic-management/network-topologies/#configuring-x-forwarded-client-cert-headers? There is some overlap for sure but I think it's useful to have config here. This is an on/off switch that applies to sidecar+gateway, then at gateway you can additionally control how it works if it's "on"?

This feels like it belongs to VirtualService for more fine grained control.
About half of these are configurable only at levels higher than Route in Envoy, some I suspect we would need, at best, Proxy Config if we wanted to make it fine grained? Like gw topology

Currently, for gateway XFCC is default ON IIRC. Are we going to preserve that behvaior?

[howardjohn]

Currently, for gateway XFCC is default ON IIRC. Are we going to preserve that behvaior?

Yes. From the proto If an empty list is provided, all are enabled.. Today, all of these are enabled, so there will be no change to default behavior. This just adds the ability for users to opt out of them

[costinm]

I am more concerned about the default behavior - I don't think users should start having to write configs to get correct and expected behavior.

• inside the mesh we keep all headers ( I don't think there is any use case to configure it )
• for Egress Gateway use case - where this is actually important and not working as intended, we either rely on the gateway class ( new world ) or some env variable to identify the gateway as 'egress'.
• for ServiceEntry MESH_EXTERNAL we strip
• for Original dst - we also strip
• for Ingress Gateway - we respect topology, i.e. strip or preserve headers like XFCC

I don't mind fine tuning this via API - but I don't think this is a MeshConfig/global option, obviously inside the mesh some
of the headers are critical ( XFCC is the only way to know for the app to know the caller ), and the use case is about
not forwarding critical headers to external sites - which is also required by RFC and common practice.

Before adding a 1st class API - I would check what other proxy servers do about this, and how it would fit with the k8s gateway API. But I doubt a lot of users will ask for this if default behaviour is correct.

[howardjohn]

Not sure we can really change the defaults due to backwards compatibility is the main concern

[costinm]

Yes, backward compat is a concern in a lot of areas - we do need some mechanism to migrate to the new behavior, but it doesn't have to be a first class, long-term-support API, it's just intended for safe transition. My personal preference is to use the migration to the new K8S Gateway model to fix all bad behaviors in Istio - this one, various things you discussed in the doc on original dst, automatic mTLS verification, etc. This can be done by creating a Gateway class mesh in a namespace to 'opt-in' to the new behavior ( and replace user-driven namespace annotation as well ). We can also use an env variable ( injection and annotations makes it easy to add this, and they are meant for this kind of migrations). MeshConfig is problematic because it's global ( per-revision actually ), and set on install/upgrade. Also once we add it there - will be hard to take out.

…

On Tue, Apr 26, 2022 at 10:27 AM John Howard ***@***.***> wrote: Not sure we can really change the defaults due to backwards compatibility is the main concern — Reply to this email directly, view it on GitHub <#2240 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2RNEZDJ3LIPZFTOBGTVHARPVANCNFSM5N3ZZMHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[costinm]

@nrjpoddar for gateway, I think the correct default behavior is to filter out XFCC from incoming requests, unless gateway topology is used. And to generate XFCC if the gateway terminates mTLS.

Gateway topology will determine if XFCC is forwarded or not, like Forwarded and other internal headers.

I can't think any good reason to have a different behavior in a gateway. Working on a doc to formalize this (identity delegation), should have it for next week.

[howardjohn]

My personal preference is to use the migration to the new K8S Gateway model
to fix all bad behaviors in Istio - this one, various things you
discussed in the doc on original dst, automatic mTLS verification, etc.
This can be done by creating a Gateway class mesh in a namespace
to 'opt-in' to the new behavior ( and replace user-driven namespace
annotation as well ).

This is actually largely my goal of this PR :-) I want to get to the point where someone turns on some option ("use new stuff", for a lack of a better name), we also turn all of this on. But I don't think we otherwise can just automatically do it because:

• Simply using the gateway-api is not a signal, since you can use no APIs at all and get mesh, or use a mix
• Even with gateway-api I think it is still reasonable to want to enable these, they just shouldn't be the default

[ramaraochavali]

Is a list of ENUMs the best way to represent this? it does allow a
strange config like [REQUEST_ID, REQUEST_ID] but that can just
rejected

I think it is better to have to individual fields for the most common headers in the HeaderSpecifier that allows to enable/disable or set value for example server header can have a string and requestid can have an extension like UUID etc....

[howardjohn]

Is a list of ENUMs the best way to represent this? it does allow a
strange config like [REQUEST_ID, REQUEST_ID] but that can just
rejected

I think it is better to have to individual fields for the most common headers in the HeaderSpecifier that allows to enable/disable or set value for example server header can have a string and requestid can have an extension like UUID etc....

I do like making it future-proof but I also can't really see us making them configurable personally... those seem like pretty niche cases?

[ramaraochavali]

message ProxyHeaders {
ForwardedClientCert forwarded_client_certs (APPEND|SANITIZE|APPEND_FORWARD....)
bool enable_envoy_headers
bool include_attempt_count
bool skip_server_header
}

I think a message like above would be more readable and help us expand later if needed instead of an enum.

[costinm]

That's strange - empty list means all, a list with NO_HEADERS means none ?

[howardjohn]

Good point...

[linsun]

agreed - may make sense to ask users to explicitly list the headers they want to remove from the default.

[costinm]

message ProxyHeaders { ForwardedClientCert forwarded_client_certs (APPEND|SANITIZE|APPEND_FORWARD....) bool enable_envoy_headers bool include_attempt_count bool skip_server_header }

I think a message like above would be more readable and help us expand later if needed instead of an enum.

I mostly agree - but no "envoy" in the API please since it should be moved to the vendor-neutral K8S Gateway policy.

Since ambient is L4 and doesn't add headers - would it make sense to just drop this PR, which is on ProxyConfig, and
instead just focus on a Gateway policy for the PEPs ( which would apply to ingress/egress as well ) ?

[kyessenov]

Looks great. I think we just need another pass on the text and maybe align XFCC with gateway topology enum, otherwise, LGTM.

[kyessenov]

@linsun Can you take another look? I think this would be great if it makes it to 1.19.

[linsun]

LGTM left a few clarification qs. Another q - how do I control if it is inbound or outbound?

[howardjohn]

LGTM left a few clarification qs. Another q - how do I control if it is inbound or outbound?

You don't. Most of the headers only apply in one direction and are documented in the comments.

[linsun]

LGTM left a few clarification qs. Another q - how do I control if it is inbound or outbound?

You don't. Most of the headers only apply in one direction and are documented in the comments.

Thanks, I found only 1 place was not clear, otherwise it was clear.

[linsun]

@howardjohn let me know when this is ready to be approved, didn't want to approve it if you have a few minor clarifications in mind.

[linsun]

LGTM

[howardjohn]

/cherrypick release-1.19

[istio-testing]

@howardjohn: new pull request created: #2902

In response to this:

/cherrypick release-1.19

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.

[Stono]

Hello, i've just stumbled across this - because we've noticed envoy headers being sent to external services (MESH_EXTERNAL). I'm struggling to follow the PR to be honest... but in 1.19.1 this still is the case.

Is there a way to stop this, without me adding headers.remove to every virtualservice for every external service we have (a lot?)

[kyessenov]

@Stono The defaults have not changed, you have to manually change the default proxy config to get an effect.

[bagga11]

@howardjohn , I am not understanding how can the customer remove the "server: istio-envoy" from the response header. Can you please help in that?

[howardjohn]

proxyHeaders:
  server:
    disabled: true