Skip to content

http/1.1: correctly handle connection header for for incoming requests#239

Merged
mattklein123 merged 2 commits intomasterfrom
conn_close
Nov 23, 2016
Merged

http/1.1: correctly handle connection header for for incoming requests#239
mattklein123 merged 2 commits intomasterfrom
conn_close

Conversation

@mattklein123
Copy link
Copy Markdown
Member

We didn't previously handle the Connection: close header for incoming
HTTP/1.1 requests. This commit fixes that.

There are also some perf related changes/cleanup in this commit also:

  1. Remove synthetic :version header and replace with an explicit codec
    protocol method.
  2. Remove the x-envoy-protocol-version response header. This was used
    for debugging when we first rolled out HTTP/2 support and no longer
    has any real value.

We didn't previously handle the Connection: close header for incoming
HTTP/1.1 requests. This commit fixes that.

There are also some perf related changes/cleanup in this commit also:
1) Remove synthetic :version header and replace with an explicit codec
   protocol method.
2) Remove the x-envoy-protocol-version response header. This was used
   for debugging when we first rolled out HTTP/2 support and no longer
   has any real value.
@mattklein123
Copy link
Copy Markdown
Member Author

@lyft/network-team

@@ -175,7 +174,7 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data) {
// The HTTP/1.1 codec will pause dispatch after a single message is complete. We want to
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's also the case for HTTP/1.0, something like non HTTP/2 codec ..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

const HeaderString& codec_version = request_headers_->Version()->value();
if (!(codec_version == "HTTP/1.1" || codec_version == "HTTP/2")) {
Protocol protocol = connection_manager_.codec_->protocol();
if (!(protocol == Protocol::Http11 || protocol == Protocol::Http2)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not directly, protocol == Protocol::Http10 ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case we add something else in the future, but the chance of that happening is basicaly zero so I will fix it.

@mattklein123 mattklein123 merged commit 70a24c3 into master Nov 23, 2016
@mattklein123 mattklein123 deleted the conn_close branch November 23, 2016 20:16
PiotrSikora added a commit to PiotrSikora/envoy that referenced this pull request Oct 10, 2019
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this pull request Aug 2, 2020
mattklein123 pushed a commit that referenced this pull request Apr 15, 2021
Fix for CVE-2021-28683 (crash when peer sends an SSL Alert with an unknown code)

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Tony Allen <tony@allen.gg>
douglas-reid referenced this pull request in douglas-reid/envoy Apr 19, 2021
Fix for CVE-2021-28683 (crash when peer sends an SSL Alert with an unknown code)

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Tony Allen <tony@allen.gg>

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
…oyproxy#239)

Fix for CVE-2021-28683 (crash when peer sends an SSL Alert with an unknown code)

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Christoph Pakulski <christoph@tetrate.io>
Signed-off-by: Tony Allen <tony@allen.gg>
Signed-off-by: Gokul Nair <gnair@twitter.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Restricting subclassing and visibility of these types.

Risk Level: None
Testing: CI

Signed-off-by: Michael Rebello <mrebello@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Restricting subclassing and visibility of these types.

Risk Level: None
Testing: CI

Signed-off-by: Michael Rebello <mrebello@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024
mathetake added a commit that referenced this pull request Mar 3, 2026
**Commit Message**
Added glossary.md to the docs directory referencing [this
draft](https://docs.google.com/document/d/1Mcsl-N2qwBOTThgxbCioUG-bOWiBM_5HpoxXFLE9OYw/edit?usp=sharing).
This glossary makes the docs more accessible to beginners and users that
may need refreshers on key terms.

**Related Issues/PRs (if applicable)**
Fixes #239

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Erica Hughberg <erica.sundberg.90@gmail.com>
Signed-off-by: melsal13 <mmvsal13@gmail.com>
Signed-off-by: ericmariasis <ericmariasis829@gmail.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: soma00333 <soma03432303@gmail.com>
Signed-off-by: Loong <long0dai@foxmail.com>
Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Co-authored-by: Eric Mariasis <ericmariasis829@gmail.com>
Co-authored-by: Sébastien Han <seb@redhat.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Soma Utsumi <53121322+soma00333@users.noreply.github.com>
Co-authored-by: Loong Dai <long0dai@foxmail.com>
Co-authored-by: Dan Sun <dsun20@bloomberg.net>
Co-authored-by: Erica Hughberg <erica.sundberg.90@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants