Skip to content

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

Merged
antoniovicente merged 9 commits intoenvoyproxy:release/v1.13from
Shikugawa:backport-1.13/13798
Jan 8, 2021
Merged

backport 1.13: http: fixing a bug with IPv6 hosts #14464
antoniovicente merged 9 commits intoenvoyproxy:release/v1.13from
Shikugawa:backport-1.13/13798

Conversation

@Shikugawa
Copy link
Copy Markdown
Member

@Shikugawa Shikugawa commented Dec 17, 2020 •
edited by alyssawilk
Loading

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
@alyssawilk @Shikugawa
backport 1.13: http: fixing a bug with IPv6 hosts
223b959 
Signed-off-by: alyssawilk <alyssar@chromium.org>
@Shikugawa
backport 1.13: http: fixing a bug with IPv6 hosts
001cace 
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
@Shikugawa
fix
bcdb5db 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
fix
ae71526 
Signed-off-by: Shikugawa <rei@tetrate.io>
@alyssawilk alyssawilk self-assigned this Dec 17, 2020
@Shikugawa Shikugawa mentioned this pull request Dec 18, 2020
Merged
@antoniovicente
Copy link
Copy Markdown
Contributor

antoniovicente commented Dec 21, 2020

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
Copy Markdown
Contributor

cpakulski commented Dec 21, 2020

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
Copy Markdown
Contributor

antoniovicente commented Dec 21, 2020

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
Copy Markdown
Contributor

cpakulski commented Dec 21, 2020

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
Copy Markdown
Contributor

antoniovicente commented Dec 22, 2020

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
Copy Markdown
Contributor

cpakulski commented Dec 22, 2020

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

@Shikugawa
Merge branch 'release/v1.13' of github.com:envoyproxy/envoy into back…
71257a0 
…port-1.13/13798

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

alyssawilk commented Jan 6, 2021

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

@cpakulski
Copy link
Copy Markdown
Contributor

cpakulski commented Jan 6, 2021

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

@cpakulski
Copy link
Copy Markdown
Contributor

cpakulski commented Jan 6, 2021

@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
Copy Markdown
Member Author

Shikugawa commented Jan 7, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jan 7, 2021

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
Copy Markdown
Member Author

Shikugawa commented Jan 7, 2021 •
edited
Loading

@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

@Shikugawa
fix cert create
2612bd9 
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
alyssawilk
alyssawilk reviewed Jan 7, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

cpakulski commented Jan 7, 2021

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.

@Shikugawa
resolve conflict
4547bff 
Signed-off-by: Shikugawa <Shikugawa@gmail.com>
@cpakulski
Copy link
Copy Markdown
Contributor

cpakulski commented Jan 8, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jan 8, 2021

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

🐱

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

see: more, trace.

@Shikugawa
delete unused file
c1f8fef 
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

Reviewers

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

2 more reviewers

@alyssawilk alyssawilk alyssawilk approved these changes

@antoniovicente antoniovicente antoniovicente approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alyssawilk alyssawilk

@antoniovicente antoniovicente

@cpakulski cpakulski

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Shikugawa @antoniovicente @cpakulski @alyssawilk