[http] Handle faulty filters from removing required request headers

[asraa]

Signed-off-by: Asra Ali asraa@google.com

Commit Message: Remove RELEASE_ASSERT around required headers, which could be triggered with a faulty filter.

• This rejects the request in the router filter and sends back a local reply with status 503.

Docs: Added docs for new response code missing_headers_after_filter_chain and release note.
Release Note: Added
Testing: Added protocol_integration_test that tests a faulty filter removing method/host/path
Partial fix for #13756

[asraa]

/cc @yanavlasov @htuch @antoniovicente @mattklein123 @alyssawilk What do you think? I kind of dislike this solution now that I do it, because missing these headers from downstream traffic acts differently than missing these headers from filters. On the other hand, it's not a Bad Request, it is a bad proxy config.

Maybe ENVOY_BUG plus checking nullptrs and adding defaults like H/2 codec is better?

envoy/source/common/http/http2/codec_impl.cc

Line 193 in dca5fff

} else if (headers.Method() && headers.Method()->value() == "CONNECT") {

[mattklein123]

I kind of dislike this solution now that I do it, because missing these headers from downstream traffic acts differently than missing these headers from filters. On the other hand, it's not a Bad Request, it is a bad proxy config.

IMO the way you implemented this is correct, in the sense that if these headers get removed by the filter chain it should be a 5xx.

I'm fine with this solution if this makes you all feel more comfortable. I'm also fine with ENVOY_BUG + defaults.

[htuch]

I think in practice this never gets hit unless there is a really problematic misconfiguration (so bad that regular users aren't likely to stumble on it), so it's not a huge issue that there is some inconsistency here. So, 5xx seems legit, but I don't feel so strongly vs. ENVOY_BUG + safe defaults; both options are consistent with where you are heading with the error handling policy doc.

[asraa]

All set, description updated, and ready for review.

[alyssawilk]

So good to have this cleaned up! Thank you!

[alyssawilk]

Should we just assert checkHeaderMap?
Also maybe update encodeHeaders include comments about this requirement?

[alyssawilk]

Also are we comfortable with the router being the terminal filter? I know it is today but I thought once we add upstream filters that won't be the case any more. I wonder if we should just handle errors here and return a status or something? cc @snowp

[snowp]

I think it would still be the terminal filter as far as the HCM is concerned, but it definitely wouldn't be the last extension point that could be modifying the headers before we hit the upstream codec. I haven't looked at this PR in detail, but conceptually it seems like doing this validation makes more sense in the codec?

[asraa]

Oh! Yes. I could change encodeHeaders to return a status that could be handled in the caller's code.

Doing it in the codec would be a lot cleaner in terms of extensibility and not worrying about the problem happening later. What worried me was how large-scale the change would be if encodeHeaders returned a status. I'll give it a try now, and see if I hit bad trouble.

[yanavlasov]

I wonder if we may run into the problem where a buggy filter removes a required header and then subsequent filter in the chain crashes because it expects the header to be in the map. Would it make sense to check the integrity of the header map after calling decodeHeaders() for each filter and abort with 500 if a filter messes up the header map?

[asraa]

Resolution: Fixed with per-iteration check in the filter manager. Avoid the issue of relying on router filter being terminal. FM sends back direct response and debug logs point to which filter made the error.

@snowp: I don't think there's a way to get the codec to do these checks. The codec has no way of handling the error at that point. I think future extensions that may end up manipulating need to harden against causing errors as well.

re original comment: encodeHeaders doc string updated, ASSERT on required header check.

[alyssawilk]

Hm, I mildly prefer having this function do the checks, and return a failure status, rather than having the caller have to know to do the check beforehand. What do folks think about that?

[mattklein123]

I think the concern about doing the check here is it will make the change much larger. In principle I agree it would be better, but this seems fine to me also. I'm fine either way.

[asraa]

I could build it off as a start, my concern is that every caller of encodeHeaders would need to handle the status (if absl::Status is used, whose result can't be ignored), and I'm not sure if it's worth implementing in every place. I could start with IgnoreError() calls for now though. I do think the change is better done inside, especially if there are other places that will modify headers before encoding.

[asraa]

Tracing through the change now. I'll push the change as soon as I can get tests working, can always revert later.

[asraa]

Updated this to show the change if the return value of encodeHeaders() is changed.

(1) It wasn't actually that bad, although it's a large scale change.
(2) This doesn't address Yan's point of checking filters on return of decodeHeaders() that otherwise solves the problem to prevent a following filter from QoD'ing. I will add that back in if we go this route, I commented it out just to test encodeHeaders e2e. Otherwise it gets caught there earlier and I can only test it in unit tests.
(3) H/2 already handles missing required headers because it will trigger oInvalidFrame, but this is more transparent of what's happening

Personally it makes sense to me to check that filters don't do anything wrong per iteration on them in the FM like the original. It also is nice to do it in encodeHeaders too (in case there is another extension point that modifies them). So both protections are probably the best scenario.

[mattklein123]

Overall this all looks reasonable to me so happy to go this direction if that is the consensus!

/wait

[alyssawilk]

Whoops, started reviewing before realizing this was WIP state.

Yeah, I think this looks good - I think it's totally worth the churn so thumbs up to cleaning it up and landing it!
for error handling I think we could just check status and sendLocalReply from the router pretty easily, and then snow can make sure your regression test continues to work when we add (potentially buggy) upstream filters

[alyssawilk]

Nice clean up! Two more thoughts before I tackle tests, just to make sure we agree on the path.

[alyssawilk]

LGTM modulo that one test nit!

[alyssawilk]

Wohoo!

[mattklein123]

LGTM with a few small questions/comments. Thank you!

/wait

[mattklein123]

Debug log level? Should this local reply block be shared with the FM code in a small utility?

[mattklein123]

Thanks this LGTM but needs a main merge. @alyssawilk any concerns with the latest change to remove the FM check?

/wait

[alyssawilk]

Nope - I'd only really wanted checks at the router/codec but didn't object to double checks. Still LGTM!

…

On Wed, Nov 4, 2020 at 4:20 PM Matt Klein ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks this LGTM but needs a main merge. @alyssawilk <https://github.com/alyssawilk> any concerns with the latest change to remove the FM check? /wait — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13470 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPLNOO6LND6YSUCBWNTSOHATNANCNFSM4SKKIELA> .