Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle removal of critical headers by filters #13756

Closed
yanavlasov opened this issue Oct 26, 2020 · 7 comments · Fixed by #16745
Closed

Gracefully handle removal of critical headers by filters #13756

yanavlasov opened this issue Oct 26, 2020 · 7 comments · Fixed by #16745
Labels
area/http enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently

Comments

@yanavlasov
Copy link
Contributor

It happened on several occasions that buggy filters modify or remove headers critical to Envoy's internals (or other filters in the chain). Envoy should check if required request or response headers are still present and valid after invoking filter's decodeHeaders() or encodeHeaders() method. If a filter has modified or deleted critical header the request should be aborted with 500 response.

Critical request header are:

  • :host

Critical response headers are:

  • :status
@yanavlasov yanavlasov added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Oct 26, 2020
@asraa asraa added area/http and removed triage Issue requires triage labels Oct 26, 2020
@github-actions
Copy link

github-actions bot commented Dec 9, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020
@mattklein123 mattklein123 removed the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020
@github-actions
Copy link

github-actions bot commented Jan 8, 2021

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 8, 2021
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@mathetake
Copy link
Member

mathetake commented Mar 24, 2021

I've started looking into this, and it looks like only the response path is not guarded from misbehaving filters (the request path is guarded by HeaderUtility::checkRequiredHeaders and the requirement check result is gracefully handled in onPoolReady).

After this is fixed, let's revisit to #15487 and think about what we can do at Wasm level. cc @PiotrSikora

cc @asraa

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 24, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 23, 2021
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@mathetake
Copy link
Member

I reworked the PR #16745 to resolve (this issue has not been resolved yet and still causes crashes).

alyssawilk pushed a commit that referenced this issue Jun 2, 2021
… chain. (#16745)

This is the rework of the previous PR (#15658).

Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves #13756 and ref: #15487.
Testing: new integration tests
Risk: low

Signed-off-by: Takeshi Yoneda <[email protected]>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this issue Sep 30, 2021
… chain. (envoyproxy#16745)

This is the rework of the previous PR (envoyproxy#15658).

Additional Description: Previously, Envoy hasn't been guarded from the removal of critical response headers like :status. This happens when there's misbehaving filter chan, and especially this protection is important when users run their own extensions through Wasm. Resolves envoyproxy#13756 and ref: envoyproxy#15487.
Testing: new integration tests
Risk: low

Signed-off-by: Takeshi Yoneda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/http enhancement Feature requests. Not bugs or questions. stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
5 participants