From ac35ab67f23065b44bc404628258ef53d2179615 Mon Sep 17 00:00:00 2001 From: Anirudh Ramachandra Date: Fri, 14 Jul 2023 12:54:18 -0700 Subject: [PATCH] xds: Encode the service authority in XdsNameResolver (#10207) Encode the service authority before passing it into gRPC util in the xDS name resolver to handle xDS requests which might contain multiple slashes. Example: xds:///path/to/service:port. As currently the underlying Java URI library does not break the encoded authority into host/port correctly simplify the check to just look for '@' as we are only interested in checking for user info to validate the authority for HTTP. This change also leads to few changes in unit tests that relied on this check for invalid authorities which now will be considered valid. Just like #9376, depending on Guava packages such as URLEscapers or PercentEscapers leads to internal failures(Ex: Unresolvable reference to com.google.common.escape.Escaper from io.grpc.internal.GrpcUtil). To avoid these issues create an in house version that is heavily inspired by grpc-go/grpc. --- .../main/java/io/grpc/internal/GrpcUtil.java | 93 ++++++++++++++++++- .../java/io/grpc/internal/GrpcUtilTest.java | 43 +++++++-- .../ManagedChannelImplBuilderTest.java | 2 +- .../java/io/grpc/xds/XdsNameResolver.java | 6 +- .../grpc/xds/XdsNameResolverProviderTest.java | 21 +++-- 5 files changed, 146 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/GrpcUtil.java b/core/src/main/java/io/grpc/internal/GrpcUtil.java index 257b6b5f11..cb64f178b5 100644 --- a/core/src/main/java/io/grpc/internal/GrpcUtil.java +++ b/core/src/main/java/io/grpc/internal/GrpcUtil.java @@ -55,9 +55,11 @@ import java.net.SocketAddress; import java.net.URI; import java.net.URISyntaxException; import java.nio.charset.Charset; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; @@ -526,8 +528,8 @@ public final class GrpcUtil { */ public static String checkAuthority(String authority) { URI uri = authorityToUri(authority); - checkArgument(uri.getHost() != null, "No host in authority '%s'", authority); - checkArgument(uri.getUserInfo() == null, + // Verify that the user Info is not provided. + checkArgument(uri.getAuthority().indexOf('@') == -1, "Userinfo must not be present on authority: '%s'", authority); return authority; } @@ -859,5 +861,92 @@ public final class GrpcUtil { return false; } + /** + * Percent encode the {@code authority} based on + * https://datatracker.ietf.org/doc/html/rfc3986#section-3.2. + * + *

When escaping a String, the following rules apply: + * + *

+ * + *

This section does not use URLEscapers from Guava Net as its not Android-friendly thus core + * can't depend on it. + */ + public static class AuthorityEscaper { + // Escapers should output upper case hex digits. + private static final char[] UPPER_HEX_DIGITS = "0123456789ABCDEF".toCharArray(); + private static final Set UNRESERVED_CHARACTERS = Collections + .unmodifiableSet(new HashSet<>(Arrays.asList('-', '_', '.', '~'))); + private static final Set SUB_DELIMS = Collections + .unmodifiableSet(new HashSet<>( + Arrays.asList('!', '$', '&', '\'', '(', ')', '*', '+', ',', ';', '='))); + private static final Set AUTHORITY_DELIMS = Collections + .unmodifiableSet(new HashSet<>(Arrays.asList(':', '[', ']', '@'))); + + private static boolean shouldEscape(char c) { + // Only encode ASCII. + if (c > 127) { + return false; + } + // Letters don't need an escape. + if (((c >= 'a') && (c <= 'z')) || ((c >= 'A') && (c <= 'Z'))) { + return false; + } + // Numbers don't need to be escaped. + if ((c >= '0' && c <= '9')) { + return false; + } + // Don't escape allowed characters. + if (UNRESERVED_CHARACTERS.contains(c) + || SUB_DELIMS.contains(c) + || AUTHORITY_DELIMS.contains(c)) { + return false; + } + return true; + } + + public static String encodeAuthority(String authority) { + Preconditions.checkNotNull(authority, "authority"); + int authorityLength = authority.length(); + int hexCount = 0; + // Calculate how many characters actually need escaping. + for (int index = 0; index < authorityLength; index++) { + char c = authority.charAt(index); + if (shouldEscape(c)) { + hexCount++; + } + } + // If no char need escaping, just return the original string back. + if (hexCount == 0) { + return authority; + } + + // Allocate enough space as encoded characters need 2 extra chars. + StringBuilder encoded_authority = new StringBuilder((2 * hexCount) + authorityLength); + for (int index = 0; index < authorityLength; index++) { + char c = authority.charAt(index); + if (shouldEscape(c)) { + encoded_authority.append('%'); + encoded_authority.append(UPPER_HEX_DIGITS[c >>> 4]); + encoded_authority.append(UPPER_HEX_DIGITS[c & 0xF]); + } else { + encoded_authority.append(c); + } + } + return encoded_authority.toString(); + } + } + private GrpcUtil() {} } diff --git a/core/src/test/java/io/grpc/internal/GrpcUtilTest.java b/core/src/test/java/io/grpc/internal/GrpcUtilTest.java index bd2864ecc9..39acb582d2 100644 --- a/core/src/test/java/io/grpc/internal/GrpcUtilTest.java +++ b/core/src/test/java/io/grpc/internal/GrpcUtilTest.java @@ -163,6 +163,42 @@ public class GrpcUtilTest { assertFalse(GrpcUtil.isGrpcContentType("application/bad")); } + @Test + public void urlAuthorityEscape_ipv6Address() { + assertEquals("[::1]", GrpcUtil.AuthorityEscaper.encodeAuthority("[::1]")); + } + + @Test + public void urlAuthorityEscape_userInAuthority() { + assertEquals("user@host", GrpcUtil.AuthorityEscaper.encodeAuthority("user@host")); + } + + @Test + public void urlAuthorityEscape_slashesAreEncoded() { + assertEquals( + "project%2F123%2Fnetwork%2Fabc%2Fservice", + GrpcUtil.AuthorityEscaper.encodeAuthority("project/123/network/abc/service")); + } + + @Test + public void urlAuthorityEscape_allowedCharsAreNotEncoded() { + assertEquals( + "-._~!$&'()*+,;=@:[]", GrpcUtil.AuthorityEscaper.encodeAuthority("-._~!$&'()*+,;=@:[]")); + } + + @Test + public void urlAuthorityEscape_allLettersAndNumbers() { + assertEquals( + "abcdefghijklmnopqrstuvwxyz0123456789", + GrpcUtil.AuthorityEscaper.encodeAuthority("abcdefghijklmnopqrstuvwxyz0123456789")); + } + + @Test + public void urlAuthorityEscape_unicodeAreNotEncoded() { + assertEquals( + "ö®", GrpcUtil.AuthorityEscaper.encodeAuthority("ö®")); + } + @Test public void checkAuthority_failsOnNull() { thrown.expect(NullPointerException.class); @@ -199,13 +235,6 @@ public class GrpcUtilTest { GrpcUtil.checkAuthority("[ : : 1]"); } - @Test - public void checkAuthority_failsOnInvalidHost() { - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("No host in authority"); - - GrpcUtil.checkAuthority("bad_host"); - } @Test public void checkAuthority_userInfoNotAllowed() { diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 51f21ffc87..dae8b9b375 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -368,7 +368,7 @@ public class ManagedChannelImplBuilderTest { @Test(expected = IllegalArgumentException.class) public void overrideAuthority_invalid() { - builder.overrideAuthority("not_allowed"); + builder.overrideAuthority("user@not_allowed"); } @Test diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 7f853dcf1e..29043b177d 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -147,7 +147,11 @@ final class XdsNameResolver extends NameResolver { XdsClientPoolFactory xdsClientPoolFactory, ThreadSafeRandom random, FilterRegistry filterRegistry, @Nullable Map bootstrapOverride) { this.targetAuthority = targetAuthority; - serviceAuthority = GrpcUtil.checkAuthority(checkNotNull(name, "name")); + + // The name might have multiple slashes so encode it before verifying. + String authority = GrpcUtil.AuthorityEscaper.encodeAuthority(checkNotNull(name, "name")); + serviceAuthority = GrpcUtil.checkAuthority(authority); + this.overrideAuthority = overrideAuthority; this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser"); this.syncContext = checkNotNull(syncContext, "syncContext"); diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java index 95e3f2f997..a216c3de02 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java @@ -110,14 +110,19 @@ public class XdsNameResolverProviderTest { } @Test - public void invalidName_hostnameContainsUnderscore() { - URI uri = URI.create("xds:///foo_bar.googleapis.com"); - try { - provider.newNameResolver(uri, args); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException e) { - // Expected - } + public void validName_urlExtractedAuthorityInvalidWithoutEncoding() { + XdsNameResolver resolver = + provider.newNameResolver(URI.create("xds:///1234/path/foo.googleapis.com:8080"), args); + assertThat(resolver).isNotNull(); + assertThat(resolver.getServiceAuthority()).isEqualTo("1234%2Fpath%2Ffoo.googleapis.com:8080"); + } + + @Test + public void validName_urlwithTargetAuthorityAndExtractedAuthorityInvalidWithoutEncoding() { + XdsNameResolver resolver = provider.newNameResolver(URI.create( + "xds://trafficdirector.google.com/1234/path/foo.googleapis.com:8080"), args); + assertThat(resolver).isNotNull(); + assertThat(resolver.getServiceAuthority()).isEqualTo("1234%2Fpath%2Ffoo.googleapis.com:8080"); } @Test