http, utility: Fix unicode URL parsing #12324

dio · 2020-07-28T11:18:40Z

Commit Message: Using the current version of googleurl and ICU, in combination with how we build it, we fail on initializing GURL with a unicode URL. This patch temporarily uses a mirror of https://quiche.googlesource.com/googleurl for googleurl and update ICU to 67.1 to enable unicode URL parsing.

This also introduces tools/generate_data.sh to facilitate generating a different set of ICU data, via editing tools/icu/filters.json.

Signed-off-by: Dhi Aurrahman dio@tetrate.io

Additional Description: This requires a "manual" step to generate ICU data since I'm not sure on how to do proper setup (genrule) for Windows.
Risk Level: Low
Testing: Added.
Docs Changes: N/A
Release Notes: N/A
Fixes: #12015

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-07-28T11:19:26Z

bazel/repository_locations.bzl

-        urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_6dafefa72cba2ab2ba4922d17a30618e9617c7cf.tar.gz"],
+        sha256 = "b8ca343feb9aedf29259f25169deb70f93a151a2e4713be97d0da2c5e3737e18",
+        strip_prefix = "quiche-googleurl-1bd6372fe3d267b2790ba76a49290b8a0b2df46c",
+        urls = ["https://github.com/dio/quiche-googleurl/archive/1bd6372fe3d267b2790ba76a49290b8a0b2df46c.tar.gz"],


Will update this when we have an agreement on how we do snapshot for googleurl release.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

alyssawilk

@lizan would you be up for reviewing the build magic? I'm happy to do cross-company sign-off as long as it lands by tomorrow :-)

alyssawilk · 2020-07-29T12:38:49Z

test/common/http/utility_test.cc

@@ -1289,6 +1289,9 @@ TEST(Url, ParsingTest) {
  // Test url with non-default ports.
  validateUrl("https://www.host.com:8443", "https", "www.host.com:8443", "/", 8443);
  validateUrl("http://www.host.com:8080", "http", "www.host.com:8080", "/", 8080);
+
+  // Make sure we survive parsing unicode URL.
+  validateUrl("https://\xe5\x85\x89.example/", "https", "xn--54q.example", "/", 443);


So what was the prior behavior? Would all unicode URLs be deemed invalid? If so I think this merits guarding in case upstreams can't handle unicode and the change in Envoy validation causes problems. If some unicode URLS passed through previously maybe it doesn't make enough difference to merit the guard. WDYT?

Either way probably worth a release note.

Would all unicode URLs be deemed invalid?

Unicode host name segments of URI's are invalid, at the transport level.

Unicode -> PunyCode. The resulting URI is a valid hostname by the DNS spec.

@dio In what circumstances we would get unicode URL in non-punycode format? I'm confused why we need handling that.

Sorry for the confusion. This is motivated by porting the test case from https://quiche.googlesource.com/googleurl/+/refs/heads/master/test/basic_test.cc#31.

I think It is possible if we want to process input from config or logging. However, if this is not a valid use case. I can skip checking for this one.

wrowe · 2020-07-29T16:19:27Z

This is sort of confusing to me. Why would envoy be handling UI-representations, and not transport URI in these particular cases (punycode, for example)? What's the use case?

dio · 2020-07-29T17:58:40Z

@wrowe I'm moving the test from https://quiche.googlesource.com/googleurl/+/refs/heads/master/test/basic_test.cc#31 do you think it is not necessary? Or impossible to happen?

wrowe · 2020-07-29T18:06:41Z

I'm asking, what's the use case? URI at the transport layer aren't presentation elements, they are opaque octets, except as bound by specific RFC's on an element-by-element basis. For example, scheme is very well understood and constrained by familiar ASCII tokens. Hostname must confirm to the DNS spec. We aren't a user agent, so don't care to transform a URI bar, such as in chromium, via punycode, into a valid DNS hostname... Or do we? And when?

lizan · 2020-07-29T20:07:00Z

test/common/http/utility_test.cc

@@ -1289,6 +1289,9 @@ TEST(Url, ParsingTest) {
  // Test url with non-default ports.
  validateUrl("https://www.host.com:8443", "https", "www.host.com:8443", "/", 8443);
  validateUrl("http://www.host.com:8080", "http", "www.host.com:8080", "/", 8080);
+
+  // Make sure we survive parsing unicode URL.
+  validateUrl("https://\xe5\x85\x89.example/", "https", "xn--54q.example", "/", 443);


@dio In what circumstances we would get unicode URL in non-punycode format? I'm confused why we need handling that.

lizan · 2020-07-29T20:07:59Z

bazel/repository_locations.bzl

-        urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"],
+        strip_prefix = "icu",
+        sha256 = "94a80cd6f251a53bd2a997f6f1b5ac6653fe791dfab66e1eb0227740fb86d5dc",
+        # 2020-04-23


no need date for versioned release.

lizan · 2020-07-29T20:14:22Z

bazel/external/icu/data/udata.patch

@@ -0,0 +1,60 @@
+diff --git a/icu4c/source/common/udata.cpp b/icu4c/source/common/udata.cpp


Add comment what this patch do and why it is needed.

Comment added.

lizan

GH doesn't allow me to comment on binary file, but I'm not happy with committing bazel/external/icu/data/data.c.gz.aa in the repo as well.

dio · 2020-07-30T03:24:17Z

@lizan let me try to make it work for windows to generate ICU data. The generated gzip file was added since I wasn't sure how to do that in windows. Otherwise, probably I can just submit the *.c code.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-07-30T12:21:43Z

Seems like we don't really need to embed ICU data for now. I'm closing this and opening #12376 as a PR for updating ICU (to 67.1) and googleurl versions.

url: Fix unicode URL parsing

94a5779

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio commented Jul 28, 2020

View reviewed changes

alyssawilk added the waiting label Jul 28, 2020

Add icu data and tools

2279973

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

repokitteh-read-only bot removed the waiting label Jul 29, 2020

dio added 6 commits July 29, 2020 09:00

Fix format

ef5ae8b

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Move icuuc BUILD file

05ae4e6

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix msvc warning C4067

2cf0f11

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Use local filters.json

5e59876

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

20e6da1

Revert wrong change

22c8b03

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio changed the title ~~WIP: url: Fix unicode URL parsing~~ url: Fix unicode URL parsing Jul 29, 2020

dio changed the title ~~url: Fix unicode URL parsing~~ http, utility: Fix unicode URL parsing Jul 29, 2020

dio marked this pull request as ready for review July 29, 2020 10:54

alyssawilk reviewed Jul 29, 2020

View reviewed changes

alyssawilk assigned lizan Jul 29, 2020

lizan suggested changes Jul 29, 2020

View reviewed changes

lizan reviewed Jul 29, 2020

View reviewed changes

dio added 2 commits July 30, 2020 14:07

Submit data.c instead

d35475d

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Missing popd

4aafd73

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio closed this Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http, utility: Fix unicode URL parsing #12324

http, utility: Fix unicode URL parsing #12324

dio commented Jul 28, 2020 •

edited

Loading

dio Jul 28, 2020 •

edited

Loading

alyssawilk left a comment

alyssawilk Jul 29, 2020

wrowe Jul 29, 2020

lizan Jul 29, 2020

dio Jul 30, 2020

wrowe commented Jul 29, 2020

dio commented Jul 29, 2020 •

edited

Loading

wrowe commented Jul 29, 2020

lizan Jul 29, 2020

lizan Jul 29, 2020

dio Jul 30, 2020

lizan Jul 29, 2020

dio Jul 30, 2020

lizan left a comment

dio commented Jul 30, 2020 •

edited

Loading

dio commented Jul 30, 2020

		@@ -0,0 +1,60 @@
		diff --git a/icu4c/source/common/udata.cpp b/icu4c/source/common/udata.cpp

http, utility: Fix unicode URL parsing #12324

http, utility: Fix unicode URL parsing #12324

Conversation

dio commented Jul 28, 2020 • edited Loading

dio Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrowe commented Jul 29, 2020

dio commented Jul 29, 2020 • edited Loading

wrowe commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

dio commented Jul 30, 2020 • edited Loading

dio commented Jul 30, 2020

dio commented Jul 28, 2020 •

edited

Loading

dio Jul 28, 2020 •

edited

Loading

dio commented Jul 29, 2020 •

edited

Loading

dio commented Jul 30, 2020 •

edited

Loading