Use HeaderBlacklist to filter unwated headers in reverse proxy

[akashshinde]

This PR provides a way to delete unwated headers from proxy response.

Proxy.Config already has a field(HeaderBlacklist) to blacklist/filter headers, but It is not currently used anywhere. ref:

console/pkg/proxy/proxy.go

Line 23 in ca7021b

HeaderBlacklist []string

This PR deletes headers provided in the HeaderBlacklist

• Added Unit test

[jhadvig]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akashshinde, jhadvig
To complete the pull request process, please assign spadgett
You can assign the PR to them by writing /assign @spadgett in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[spadgett]

HeaderBlacklist is intended to remove request headers, not response headers.

[spadgett]

/lgtm cancel

[spadgett]

It looks like we have the request headers hard-coded here, so we're not sending anything we shouldn't.

https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/pkg/proxy/proxy.go#L88

Maybe we can remove the HeaderBlacklist in the proxy config?

[spadgett]

@akashshinde Do you have a need to filter additional response headers or did you just notice it was unused?

[akashshinde]

@spadgett
We have a use case where proxied url copies all headers along with response which is correct but origin url/proxied url copies Cache-Control header which tells browser to cache the response.
Therefore when UI makes second request to same proxy url, browser uses the cached response instead of the one sent by server.
So we want a way to filter out that header.
If we can't use HeaderBlacklist then we need to find alternative way to tackle this issue.
Let me know what do you suggest.

[akashshinde]

It looks like we have the request headers hard-coded here, so we're not sending anything we shouldn't.

https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/pkg/proxy/proxy.go#L88

Maybe we can remove the HeaderBlacklist in the proxy config?

Please check https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/cmd/bridge/main.go#L328
We pass Cookie and X-CSRFToken here in HeaderBlacklist (same headers we've hard coded in https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/pkg/proxy/proxy.go#L88)

So I'm guessing we should remove hard coded headers and keep the field HeaderBlacklist, This way we will have control over response headers.

[spadgett]

Please check https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/cmd/bridge/main.go#L328
We pass Cookie and X-CSRFToken here in HeaderBlacklist (same headers we've hard coded in https://github.com/spadgett/console/blob/bc9a54150429b081278e2024fb6cb769f5ca3a86/pkg/proxy/proxy.go#L88)

Right these are definitely request headers, not response headers. X-CSRFToken comes from the console client, and Cookie is a request header.

So I'm guessing we should remove hard coded headers and keep the field HeaderBlacklist, This way we will have control over response headers.

But this was intended to filter request headers. I would rather have the hard-coded list to be honest since we should never send these for security reasons. It would be too easy to forget to set that on one of our proxies.

Do you have a specific need to filter a different response header?

[akashshinde]

Do you have a specific need to filter a different response header?

this is the reason - #6044 (comment)
Let me know if I am not clear.

[spadgett]

I'd make this ReponseHeaderDenylist and remove HeaderBlacklist from the config and the references in main.go.

[spadgett]

The problem though is that removing Cache-Control is not enough. It leaves it up to the browser. You really want to replace it with another value.

[spadgett]

Any chance of fixing the Cache-Control header in the upstream service if it shouldn't be cached?

[akashshinde]

Any chance of fixing the Cache-Control header in the upstream service if it shouldn't be cached?

https://redhat-developer.github.io/redhat-helm-charts/index.yaml this is the URL we are serving through the proxy and it's being served through GitHub pages. I doubt If we could remove/configure http headers in GitHub service @sbose78 wdyt?
Github page adds two headers which are causing this issue.

Cache-Control: 600
Etag: unique-id

The problem though is that removing Cache-Control is not enough. It leaves it up to the browser. You really want to replace it with another value.

Well I was thinking of replacing Cache-Control default value to no-cache, private, this would avoid caching in browser, But to modify the response headers we would need to add ModifyResponse func(r *http.Response) instead of HeaderBlacklist string[] in Config

Config struct would look like

type Config struct {
	ModifyHTTPProxyResponse func(r *http.Response)
	Endpoint        *url.URL
	TLSClientConfig *tls.Config
	Origin          string
}

[spadgett]

@akashshinde If GitHub pages is setting a valid ETag on the response, why is the Cache-Control header a problem? You should get the cached version only when the ETag hasn't changed.

[akashshinde]

@spadgett we are no longer using proxy in #5933, therefore we can close this PR unless you have a reason to keep it open.

[sbose78]

Yea, let's close this PR.