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

backport 1.13: http: fixing a bug with IPv6 hosts #14464

Merged

Conversation

Shikugawa
Copy link
Member

@Shikugawa Shikugawa commented Dec 17, 2020

Fixing a bug where HTTP parser offsets for IPv6 hosts did not include [] and Envoy assumed it did.
This results in mis-parsing addresses for IPv6 CONNECT requests and IPv6 hosts in fully URLs over HTTP/1.1

Risk Level: low
Testing: new unit, integration tests
Docs Changes: n/a
Release Notes: inline

alyssawilk and others added 2 commits December 17, 2020 16:02
Signed-off-by: alyssawilk <alyssar@chromium.org>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa changed the title Backport 1.13/13798 backport 1.13: http: fixing a bug with IPv6 hosts Dec 17, 2020
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@antoniovicente
Copy link
Contributor

It would be good to understand the test failures seen on both the 1.13 and 1.14 backports. The changes in this PR don't seem to be related to those failures.

@cpakulski
Copy link
Contributor

I think that the test failures are related to expired certificates. That was the reason for failures on 1.15 branch. I am investigating it.

@antoniovicente
Copy link
Contributor

I think that the test failures are related to expired certificates. That was the reason for failures on 1.15 branch. I am investigating it.

interesting. What was the fix on 1.15?

@cpakulski
Copy link
Contributor

The fix was to re-run a script which generates new set of certs valid until 2022: test/config/integration/certs/certs.sh.

I think the same problem may exists in test/extensions/transport_socket/tls.

@antoniovicente
Copy link
Contributor

The fix was to re-run a script which generates new set of certs valid until 2022: test/config/integration/certs/certs.sh.

I think the same problem may exists in test/extensions/transport_socket/tls.

I assume we should backport the certs generated to 1.14 and 1.13 also.

@cpakulski
Copy link
Contributor

Correct. PRs for certs in releases 1.13 and 1.14 are ready: #14503 and #14505.

…port-1.13/13798

Signed-off-by: Shikugawa <Shikugawa@gmail.com>
Signed-off-by: Shikugawa <Shikugawa@gmail.com>
@alyssawilk
Copy link
Contributor

Do you know what's up with CI here? I kicked off a second run and it failed too.

@cpakulski
Copy link
Contributor

It fails in IpVersions/ProxyFilterIntegrationTest.UpstreamTlsWithIpHost/IPv4. I am checking why it happens.

@cpakulski
Copy link
Contributor

@Shikugawa I figured out why the test fails. fake_upstream was converted to use loopback address. To fix it modify
makeTcpListenSocket method in test/integration/fake_upstream.cc around line 368
to look as this:

 static Network::SocketPtr makeTcpListenSocket(uint32_t port, Network::Address::IpVersion version) {
  return makeTcpListenSocket(
      Network::Utility::parseInternetAddress(Network::Test::getLoopbackAddressString(version), port));
}

@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14464 (comment) was created by @Shikugawa.

see: more, trace.

@Shikugawa Shikugawa force-pushed the backport-1.13/13798 branch from 2469c3b to b32c71b Compare January 7, 2021 09:53
@Shikugawa
Copy link
Member Author

Shikugawa commented Jan 7, 2021

@cpakulski I found that the creation of the new cert chain is not quite. So it will be caused some strange SSL_ERROR_SYSCALL. This error can be derived from system call related errors. But have the possibility to be derived from invalid creation strategy of cert. openssl/openssl#9566. It had resolved with rewriting test/config/integration/certs/upstreamcert.cfg

Signed-off-by: Shikugawa <Shikugawa@gmail.com>
@Shikugawa Shikugawa force-pushed the backport-1.13/13798 branch from b32c71b to 2612bd9 Compare January 7, 2021 12:49
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

I think I said on the last PR it'd be good to split out TLS changes. Can we do that here and elsewhere? Sorry/thanks!

@cpakulski
Copy link
Contributor

Created #14594 to bring TLS changes and make fake_upstream to use localhost. @Shikugawa Once #14594 is merged to 1.13 you can bring it here.

Signed-off-by: Shikugawa <Shikugawa@gmail.com>
@cpakulski
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14464 (comment) was created by @cpakulski.

see: more, trace.

Signed-off-by: Shikugawa <Shikugawa@gmail.com>
@antoniovicente antoniovicente merged commit 5eeef5b into envoyproxy:release/v1.13 Jan 8, 2021
@Shikugawa Shikugawa deleted the backport-1.13/13798 branch January 9, 2021 08:30
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.

4 participants