Add client option to disable redirects

Presto clients can attempt to follow a redirect from an untrusted server, adding option to disable redirect as a security improvement.
prestodb · Jan 10, 2024 · 5a5044b · 5a5044b
1 parent 1d96475
commit 5a5044b
Show file tree

Hide file tree

Showing 11 changed files with 50 additions and 4 deletions.
diff --git a/presto-cli/src/main/java/com/facebook/presto/cli/ClientOptions.java b/presto-cli/src/main/java/com/facebook/presto/cli/ClientOptions.java
@@ -146,6 +146,9 @@ public class ClientOptions
     @Option(name = "--validate-nexturi-source", title = "validate nextUri source", description = "Validate nextUri server host and port does not change during query execution")
     public boolean validateNextUriSource;
 
+    @Option(name = "--disable-redirects", title = "disable redirects", description = "Disable client following redirects from server")
+    public boolean disableRedirects;
+
     public enum OutputFormat
     {
         ALIGNED,

diff --git a/presto-cli/src/main/java/com/facebook/presto/cli/Console.java b/presto-cli/src/main/java/com/facebook/presto/cli/Console.java
@@ -138,7 +138,8 @@ public boolean run()
                 Optional.ofNullable(clientOptions.krb5ConfigPath),
                 Optional.ofNullable(clientOptions.krb5KeytabPath),
                 Optional.ofNullable(clientOptions.krb5CredentialCachePath),
-                !clientOptions.krb5DisableRemoteServiceHostnameCanonicalization)) {
+                !clientOptions.krb5DisableRemoteServiceHostnameCanonicalization,
+                !clientOptions.disableRedirects)) {
             if (hasQuery) {
                 return executeCommand(queryRunner, query, clientOptions.outputFormat, clientOptions.ignoreErrors);
             }

diff --git a/presto-cli/src/main/java/com/facebook/presto/cli/QueryRunner.java b/presto-cli/src/main/java/com/facebook/presto/cli/QueryRunner.java
@@ -66,7 +66,8 @@ public QueryRunner(
             Optional<String> kerberosConfigPath,
             Optional<String> kerberosKeytabPath,
             Optional<String> kerberosCredentialCachePath,
-            boolean kerberosUseCanonicalHostname)
+            boolean kerberosUseCanonicalHostname,
+            boolean followRedirects)
     {
         this.session = new AtomicReference<>(requireNonNull(session, "session is null"));
         this.debug = debug;
@@ -98,6 +99,7 @@ public QueryRunner(
         Optional.ofNullable(session.getExtraCredentials().get(GCS_CREDENTIALS_PATH_KEY))
                 .ifPresent(credentialPath -> setupGCSOauth(builder, credentialPath, Optional.ofNullable(session.getExtraCredentials().get(GCS_OAUTH_SCOPES_KEY))));
 
+        builder.followRedirects(followRedirects);
         this.httpClient = builder.build();
     }
 

diff --git a/presto-cli/src/test/java/com/facebook/presto/cli/AbstractCliTest.java b/presto-cli/src/test/java/com/facebook/presto/cli/AbstractCliTest.java
@@ -140,7 +140,8 @@ protected static QueryRunner createQueryRunner(ClientSession clientSession)
                 Optional.empty(),
                 Optional.empty(),
                 Optional.empty(),
-                false);
+                false,
+                true);
     }
 
     protected static void assertHeaders(String headerName, Headers headers, Set<String> expectedSessionHeaderValues)

diff --git a/presto-cli/src/test/java/com/facebook/presto/cli/TestClientOptions.java b/presto-cli/src/test/java/com/facebook/presto/cli/TestClientOptions.java
@@ -134,6 +134,13 @@ public void testDisableCompression()
         assertTrue(console.clientOptions.toClientSession().isCompressionDisabled());
     }
 
+    @Test
+    public void testDisableFollowingRedirects()
+    {
+        Console console = singleCommand(Console.class).parse("--disable-redirects");
+        assertTrue(console.clientOptions.disableRedirects);
+    }
+
     @Test(expectedExceptions = IllegalArgumentException.class)
     public void testThreePartPropertyName()
     {

diff --git a/presto-client/src/main/java/com/facebook/presto/client/JsonResponse.java b/presto-client/src/main/java/com/facebook/presto/client/JsonResponse.java
@@ -130,7 +130,7 @@ public static <T> JsonResponse<T> execute(JsonCodec<T> codec, OkHttpClient clien
     {
         try (Response response = client.newCall(request).execute()) {
             // TODO: fix in OkHttp: https://github.com/square/okhttp/issues/3111
-            if ((response.code() == 307) || (response.code() == 308)) {
+            if (((response.code() == 307) || (response.code() == 308)) && client.followRedirects()) {
                 String location = response.header(LOCATION);
                 if (location != null) {
                     request = request.newBuilder().url(location).build();

diff --git a/presto-docs/src/main/sphinx/installation/jdbc.rst b/presto-docs/src/main/sphinx/installation/jdbc.rst
@@ -120,4 +120,5 @@ Name                              Description
                                   ``testHeaderKey:testHeaderValue`` will inject the header ``testHeaderKey``
                                   with value ``testHeaderValue``. Values should be percent encoded.
 ``validateNextUriSource``         Validates that host and port in next URI does not change during query execution.
+``followRedirects``               Disable Presto client to follow a redirect as a security measure.
 ================================= =======================================================================
diff --git a/presto-jdbc/src/main/java/com/facebook/presto/jdbc/ConnectionProperties.java b/presto-jdbc/src/main/java/com/facebook/presto/jdbc/ConnectionProperties.java
@@ -63,6 +63,7 @@ final class ConnectionProperties
     public static final ConnectionProperty<List<Protocol>> HTTP_PROTOCOLS = new HttpProtocols();
     public static final ConnectionProperty<List<QueryInterceptor>> QUERY_INTERCEPTORS = new QueryInterceptors();
     public static final ConnectionProperty<Boolean> VALIDATE_NEXTURI_SOURCE = new ValidateNextUriSource();
+    public static final ConnectionProperty<Boolean> FOLLOW_REDIRECTS = new FollowRedirects();
 
     private static final Set<ConnectionProperty<?>> ALL_PROPERTIES = ImmutableSet.<ConnectionProperty<?>>builder()
             .add(USER)
@@ -91,6 +92,7 @@ final class ConnectionProperties
             .add(HTTP_PROTOCOLS)
             .add(QUERY_INTERCEPTORS)
             .add(VALIDATE_NEXTURI_SOURCE)
+            .add(FOLLOW_REDIRECTS)
             .build();
 
     private static final Map<String, ConnectionProperty<?>> KEY_LOOKUP = unmodifiableMap(ALL_PROPERTIES.stream()
@@ -378,4 +380,13 @@ public ValidateNextUriSource()
             super("validateNextUriSource", Optional.of("false"), NOT_REQUIRED, ALLOWED, BOOLEAN_CONVERTER);
         }
     }
+
+    private static class FollowRedirects
+            extends AbstractConnectionProperty<Boolean>
+    {
+        public FollowRedirects()
+        {
+            super("followRedirects", Optional.of("true"), NOT_REQUIRED, ALLOWED, BOOLEAN_CONVERTER);
+        }
+    }
 }
diff --git a/presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriver.java b/presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriver.java
@@ -85,6 +85,7 @@ public Connection connect(String url, Properties info)
         PrestoDriverUri uri = new PrestoDriverUri(url, info);
 
         OkHttpClient.Builder builder = httpClient.newBuilder();
+        builder.followRedirects(uri.followRedirects());
         uri.setupClient(builder);
         QueryExecutor executor = new QueryExecutor(builder.build());
 

diff --git a/presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriverUri.java b/presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoDriverUri.java
@@ -52,6 +52,7 @@
 import static com.facebook.presto.jdbc.ConnectionProperties.CUSTOM_HEADERS;
 import static com.facebook.presto.jdbc.ConnectionProperties.DISABLE_COMPRESSION;
 import static com.facebook.presto.jdbc.ConnectionProperties.EXTRA_CREDENTIALS;
+import static com.facebook.presto.jdbc.ConnectionProperties.FOLLOW_REDIRECTS;
 import static com.facebook.presto.jdbc.ConnectionProperties.HTTP_PROTOCOLS;
 import static com.facebook.presto.jdbc.ConnectionProperties.HTTP_PROXY;
 import static com.facebook.presto.jdbc.ConnectionProperties.KERBEROS_CONFIG_PATH;
@@ -217,6 +218,12 @@ public boolean validateNextUriSource()
         return VALIDATE_NEXTURI_SOURCE.getValue(properties).orElse(false);
     }
 
+    public boolean followRedirects()
+            throws SQLException
+    {
+        return FOLLOW_REDIRECTS.getValue(properties).orElse(true);
+    }
+
     public void setupClient(OkHttpClient.Builder builder)
             throws SQLException
     {

diff --git a/presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestPrestoDriverUri.java b/presto-jdbc/src/test/java/com/facebook/presto/jdbc/TestPrestoDriverUri.java
@@ -26,6 +26,7 @@
 import static com.facebook.presto.jdbc.ConnectionProperties.CUSTOM_HEADERS;
 import static com.facebook.presto.jdbc.ConnectionProperties.DISABLE_COMPRESSION;
 import static com.facebook.presto.jdbc.ConnectionProperties.EXTRA_CREDENTIALS;
+import static com.facebook.presto.jdbc.ConnectionProperties.FOLLOW_REDIRECTS;
 import static com.facebook.presto.jdbc.ConnectionProperties.HTTP_PROTOCOLS;
 import static com.facebook.presto.jdbc.ConnectionProperties.HTTP_PROXY;
 import static com.facebook.presto.jdbc.ConnectionProperties.QUERY_INTERCEPTORS;
@@ -181,6 +182,17 @@ public void testUriWithoutCompression()
         assertEquals(parameters.getProperties().getProperty(DISABLE_COMPRESSION.getKey()), "true");
     }
 
+    @Test
+    public void testUriWithoutFollowingRedirects()
+            throws SQLException
+    {
+        PrestoDriverUri parameters = createDriverUri("presto://localhost:8080/blackhole?followRedirects=false");
+        assertFalse(parameters.followRedirects());
+        assertEquals(parameters.getProperties().getProperty(FOLLOW_REDIRECTS.getKey()), "false");
+
+        assertInvalid("presto://localhost:8080/blackhole?followRedirects=ANOTHERVALUE", "Connection property 'followRedirects' value is invalid: ANOTHERVALUE");
+    }
+
     @Test
     public void testUriWithoutSsl()
             throws SQLException