Only proxy some headers from the reply in MTBroker filter

[aliok]

Fixes: #6276

Related: knative-extensions/eventing-kafka-broker#2122

Multiple Content-Length headers were set when there's a reply used.

Specifically, we added support for proxying headers from replies here: #5946

However, not every header is good for proxying.
One example is Content-Length. Having multiple Content-Length headers somehow exploitable (not a security expert here).

This PR prevents having multiple Content-Length headers. There might be other headers that we shouldn't proxy. I will ask this in a PR comment.

This is very hard to test because when I tell the mock http server to send a specific Content-Length with the reply for checking, it responds with 502. However, we tested it at eventing-kafka-broker repository and it fixes the issue there.

Proposed Changes

• 🧹 MTChannelBroker filter does not proxy Content-Length headers from replies anymore. Instead, it computes on its own.

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

- :broom: MTChannelBroker filter does not proxy headers from replies anymore other than the headers specified in its hardcoded allow-list. The list so far only contains `Retry-After`. 

Docs

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [aliok]

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

[aliok]

/hold

@knative/security-wg-leads @evankanderson @rhuss Can you think of any other headers that we shouldn't proxy?

[aliok]

/cherrypick release-1.4

[knative-prow-robot]

@aliok: once the present PR merges, I will cherry-pick it on top of release-1.4 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.4

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.

[aliok]

/cherrypick release-1.3

[knative-prow-robot]

@aliok: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.3

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.

[aliok]

/cherrypick release-1.2

[knative-prow-robot]

@aliok: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-1.2

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.

[codecov]

Codecov Report

Merging #6357 (1e261aa) into main (01f5612) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #6357   +/-   ##
=======================================
  Coverage   82.39%   82.40%           
=======================================
  Files         233      233           
  Lines       11567    11572    +5     
=======================================
+ Hits         9531     9536    +5     
  Misses       1573     1573           
  Partials      463      463           

Impacted Files	Coverage Δ
pkg/broker/filter/filter_handler.go	75.42% <100.00%> (+0.42%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01f5612...1e261aa. Read the comment docs.

[aliok]

/test upgrade-tests_eventing_main
Infra

[aliok]

/test upgrade-tests_eventing_main

Flake?

[aliok]

cc @evankanderson
cc @rhuss

[evankanderson]

I'd suggest an opposite approach of extracting a CloudEvent from the HTTP response, rather than attempting to handle HTTP headers being the CloudEvents spec.

(Will look at the PR contents shortly.)

In general, attempting a blocklist approach to validation is a pattern that's associated with surprising security vulnerabilities. The Host header would be another interesting example of an improper header to repeat.

[aliok]

Thanks for the feedback @evankanderson

I'd suggest an opposite approach of extracting a CloudEvent from the HTTP response, rather than attempting to handle HTTP headers being the CloudEvents spec.

Any headers with ce- prefix or specified in the spec is already proxied - should be handled by CloudEvents SDK.
Other headers are the ones that should be additionally proxied. On example is Retry-After header. It is required for #5811 for example.

In general, attempting a blocklist approach to validation is a pattern that's associated with surprising security vulnerabilities. The Host header would be another interesting example of an improper header to repeat.

We could have an allow-list then :)

I don't want to go into making this allow-list configurable (e.g. via configmap or something similar) for now.

Any feedback would be appreciated before I go and modify my PR.

[evankanderson]

I 100% agree on not making the allow-list configurable. This is part of the "delivery contract", so any additional headers that we interpret outside of CloudEvents should be documented somewhere, and not just a willy-nilly repetition.

[lionelvillard]

In this particular case I think we want a block-list to allow custom headers to pass-through, and we should audit which standardized HTTP headers need to be filtered out.

[evankanderson]

Question: why do we need / want a block-list here?

If I introduce a new header, or include a header in my reply which is only specified for requests (for example Host or Cookie), reflecting that header to a new endpoint feels like surprising behavior.

[aliok]

Comparing the behaviour previous to #5946, I think having an allow-list for now is the minimal change we can do to make things working without any big impact. I prefer going this route and seeing if there's any feedback from the community around other headers.

cc @lionelvillard

[evankanderson]

This change seems to be an improvement, though I'd still prefer to see an allow-list of pass-through headers.

For clarity / posterity, this is at the following location (**) in the data path, on the response return from the subscriber:

[ channel ] --(subscription)-->** [ MT broker filter ] **--(trigger)--> [subscriber]

The goal is to be able to leverage delivery contract extensions by simply proxying through delivery contract headers to the Channel implementation. I'm presuming that the problem is that we don't have a map/StringSet of the known delivery headers at the moment, so it's hard to build the allow-list. It also means that experimental delivery contract headers which are implemented in a single out-of-core channel need a change in the core in order to work with the MT Broker.

[aliok]

This change seems to be an improvement, though I'd still prefer to see an allow-list of pass-through headers.

This is the allow-list: https://github.com/knative/eventing/pull/6357/files#diff-f2e0f39d7130b2386e41c21acc3e30d1ce6a37f44c11e81388825f6094af0306R62-R65

I'm presuming that the problem is that we don't have a map/StringSet of the known delivery headers at the moment, so it's hard to build the allow-list.

Yes.

It also means that experimental delivery contract headers which are implemented in a single out-of-core channel need a change in the core in order to work with the MT Broker.

This is also true. Thanks for pointing that out. Updating the release note for the PR now.

At the very least, I tested the change with KafkaChannel and it works fine :) I won't be able to test other channel implementations.
I prefer being reactive instead of proactive here. I mean, instead of making a big plan and creating a cross-WG and cross-vendor tasks, I would like to just merge this one and see if there are any channels breaking.

experimental delivery contract headers which are implemented in a single out-of-core channel

Do we know which channel implementations are those?

cc @lionelvillard maybe you know?

[aliok]

Oh, BTW, this proxying feature is added with #5946 in Dec 2021.

So, it hasn't been too much time that would allow channel implementors to rely on the proxy-everything approach to implement experimental delivery contract headers.

cc @evankanderson

[aliok]

/hold

I asked in the dev mailing list if there are any channel implementors who rely on proxying all headers: https://groups.google.com/g/knative-dev/c/R7uUCJuZE50

Even if you think we're all good to merge, I will still wait for a week before I merge this to allow people to shout.

[lionelvillard]

I misunderstood the allow-list is only about HTTP headers, not CloudEvents attributes. Got that mixed up.

@aliok feel free to uphold whenever you are ready.

[pierDipi]

This PR was discussed in the last WG, and we agreed on having the allowlist since the functionality of proxying headers was introduced as part of the (still) experimental Retry-After feature.

[pierDipi]

If we will have more use-cases that want to proxy arbitrary headers we can always introduce a more extensible solution.

[evankanderson]

It also means that experimental delivery contract headers which are implemented in a single out-of-core channel need a change in the core in order to work with the MT Broker.

This is also true. Thanks for pointing that out. Updating the release note for the PR now.

Sorry, I hadn't seen the earlier rewrite. I'm happy with the allow-list for now. I was just trying to understand the risk / danger / problem with the allowlist, and the tradeoff is that extending the delivery contract requires extending the "allowed-to-proxy-headers" list. That seems reasonable on the face of it; if we discover this is a problem, we can discuss how to configure this (global configmap, subscription status, etc) then.

I'm in favor of this.

/lgtm

[evankanderson]

This is fine. As an FYI, Kubernetes has some helpers here: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/sets#String

There will probably be an equivalent for go 1.19 or so with generics, I expect.

[knative-prow-robot]

@aliok: new pull request created: #6386

Details

In response to this:

/cherrypick release-1.4

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.

[knative-prow-robot]

@aliok: new pull request created: #6387

Details

In response to this:

/cherrypick release-1.3

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.

[knative-prow-robot]

@aliok: new pull request created: #6388

Details

In response to this:

/cherrypick release-1.2

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.