Skip to content

Commit 43beeb4

Browse files
jkoskelajkoskela
committed
Add redirect guards
The driver will currently follow any redirects from the server. This can cause problems. For example when running on AWS there is a service running on a link-local address which provides IAM credentials (IMDS). If the server redirects to this address, the driver will return an error message with the IAM credentials.
1 parent dbe02cb commit 43beeb4

File tree

6 files changed

+270
-0
lines changed

6 files changed

+270
-0
lines changed

‎client/trino-client/src/main/java/io/trino/client/SafeRedirector.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package io.trino.client;
15+
16+
import okhttp3.Interceptor;
17+
import okhttp3.Response;
18+
19+
import java.io.IOException;
20+
import java.net.InetAddress;
21+
import java.net.URI;
22+
import java.net.URISyntaxException;
23+
import java.net.UnknownHostException;
24+
import java.util.logging.Level;
25+
import java.util.logging.Logger;
26+
27+
public class SafeRedirector
28+
implements Interceptor
29+
{
30+
private static final Logger log = Logger.getLogger(SafeRedirector.class.getName());
31+
32+
public SafeRedirector() {}
33+
34+
static boolean deny(InetAddress addr)
35+
{
36+
return addr.isAnyLocalAddress() ||
37+
addr.isLoopbackAddress() ||
38+
addr.isLinkLocalAddress() ||
39+
addr.isSiteLocalAddress();
40+
}
41+
42+
@Override
43+
public Response intercept(Chain chain)
44+
throws IOException
45+
{
46+
Response response = chain.proceed(chain.request());
47+
48+
if (response.isRedirect()) {
49+
String location = response.header("Location");
50+
if (location != null) {
51+
validateRedirect(location);
52+
}
53+
}
54+
return response;
55+
}
56+
57+
void validateRedirect(String location)
58+
{
59+
URI redirectUri;
60+
try {
61+
redirectUri = new URI(location);
62+
}
63+
catch (URISyntaxException e) {
64+
log.log(Level.SEVERE, "Failed to parse redirect location " + location, e);
65+
// This should fail later, let it fail in the normal location.
66+
return;
67+
}
68+
69+
String host = redirectUri.getHost();
70+
if (host == null) {
71+
log.warning("Redirect location " + location + " has no host");
72+
return;
73+
}
74+
75+
try {
76+
InetAddress[] addresses = InetAddress.getAllByName(host);
77+
for (InetAddress address : addresses) {
78+
if (deny(address)) {
79+
log.warning("Blocked redirect to " + location + " (resolved to " + address + ")");
80+
throw new ClientException("Redirect to location is not allowed: " + location);
81+
}
82+
}
83+
}
84+
catch (UnknownHostException e) {
85+
log.log(Level.SEVERE, "Failed to resolve redirect host " + host, e);
86+
// This should fail later, let it fail in the normal location.
87+
}
88+
}
89+
}

‎client/trino-client/src/main/java/io/trino/client/uri/ConnectionProperties.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ enum SslVerificationMode
116116
public static final ConnectionProperty<String, Map<String, String>> RESOURCE_ESTIMATES = new ResourceEstimates();
117117
public static final ConnectionProperty<String, List<String>> SQL_PATH = new SqlPath();
118118
public static final ConnectionProperty<String, Boolean> VALIDATE_CONNECTION = new ValidateConnection();
119+
public static final ConnectionProperty<String, Boolean> USE_SAFE_REDIRECT = new UseSafeRedirect();
119120

120121
private static final Set<ConnectionProperty<?, ?>> ALL_PROPERTIES = ImmutableSet.<ConnectionProperty<?, ?>>builder()
121122
// Keep sorted
@@ -173,6 +174,7 @@ enum SslVerificationMode
173174
.add(TIMEZONE)
174175
.add(TRACE_TOKEN)
175176
.add(USER)
177+
.add(USE_SAFE_REDIRECT)
176178
.add(VALIDATE_CONNECTION)
177179
.build();
178180

@@ -958,6 +960,15 @@ public AssumeNullCatalogMeansCurrentCatalog()
958960
}
959961
}
960962

963+
private static class UseSafeRedirect
964+
extends AbstractConnectionProperty<String, Boolean>
965+
{
966+
public UseSafeRedirect()
967+
{
968+
super(PropertyName.USE_SAFE_REDIRECT, Optional.of(false), NOT_REQUIRED, ALLOWED, BOOLEAN_CONVERTER);
969+
}
970+
}
971+
961972
private static class MapPropertyParser
962973
{
963974
private static final CharMatcher PRINTABLE_ASCII = CharMatcher.inRange((char) 0x21, (char) 0x7E);

‎client/trino-client/src/main/java/io/trino/client/uri/HttpClientFactory.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import io.trino.client.ClientException;
1717
import io.trino.client.DnsResolver;
18+
import io.trino.client.SafeRedirector;
1819
import io.trino.client.auth.external.CompositeRedirectHandler;
1920
import io.trino.client.auth.external.ExternalAuthenticator;
2021
import io.trino.client.auth.external.HttpTokenPoller;
@@ -109,6 +110,9 @@ public static OkHttpClient.Builder toHttpClientBuilder(TrinoUri uri, String user
109110
ExternalAuthenticator authenticator = new ExternalAuthenticator(
110111
redirectHandler, poller, knownTokenCache.create(), timeout);
111112

113+
if (uri.useSafeRedirect()) {
114+
builder.addNetworkInterceptor(new SafeRedirector());
115+
}
112116
builder.authenticator(authenticator);
113117
builder.addNetworkInterceptor(authenticator);
114118
}

‎client/trino-client/src/main/java/io/trino/client/uri/PropertyName.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ public enum PropertyName
7676
TIMEZONE("timezone"),
7777
TRACE_TOKEN("traceToken"),
7878
USER("user"),
79+
USE_SAFE_REDIRECT("useSafeRedirect"),
7980
VALIDATE_CONNECTION("validateConnection");
8081

8182
private final String key;

‎client/trino-client/src/main/java/io/trino/client/uri/TrinoUri.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@
9898
import static io.trino.client.uri.ConnectionProperties.TIMEZONE;
9999
import static io.trino.client.uri.ConnectionProperties.TRACE_TOKEN;
100100
import static io.trino.client.uri.ConnectionProperties.USER;
101+
import static io.trino.client.uri.ConnectionProperties.USE_SAFE_REDIRECT;
101102
import static io.trino.client.uri.ConnectionProperties.VALIDATE_CONNECTION;
102103
import static io.trino.client.uri.LoggingLevel.NONE;
103104
import static java.lang.String.CASE_INSENSITIVE_ORDER;
@@ -423,6 +424,11 @@ public boolean isCompressionDisabled()
423424
return resolveWithDefault(DISABLE_COMPRESSION, false);
424425
}
425426

427+
public boolean useSafeRedirect()
428+
{
429+
return resolveWithDefault(USE_SAFE_REDIRECT, false);
430+
}
431+
426432
public Optional<String> getEncoding()
427433
{
428434
Optional<String> encodings = resolveOptional(ENCODING);
@@ -1058,6 +1064,11 @@ public Builder setValidateConnection(boolean value)
10581064
return setProperty(VALIDATE_CONNECTION, value);
10591065
}
10601066

1067+
public Builder setUseSafeRedirect(boolean value)
1068+
{
1069+
return setProperty(USE_SAFE_REDIRECT, value);
1070+
}
1071+
10611072
<V, T> Builder setProperty(ConnectionProperty<V, T> connectionProperty, T value)
10621073
{
10631074
properties.put(connectionProperty.getKey(), connectionProperty.encodeValue(value));

‎client/trino-client/src/test/java/io/trino/client/TestSafeRedirector.java‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package io.trino.client;
15+
16+
import okhttp3.Interceptor;
17+
import okhttp3.Protocol;
18+
import okhttp3.Request;
19+
import okhttp3.Response;
20+
import org.junit.jupiter.api.Test;
21+
22+
import static org.assertj.core.api.Assertions.assertThatCode;
23+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
24+
25+
public class TestSafeRedirector
26+
{
27+
private static final String BASE_URL = "https://example.com";
28+
29+
@Test
30+
public void testRedirectValidation()
31+
{
32+
SafeRedirector redirector = new SafeRedirector();
33+
34+
// Test valid external redirects
35+
assertThatCode(() ->
36+
redirector.intercept(createChainWithRedirect("https://www.example.com")))
37+
.doesNotThrowAnyException();
38+
assertThatCode(() ->
39+
redirector.intercept(createChainWithRedirect("https://api.github.com")))
40+
.doesNotThrowAnyException();
41+
42+
// Test invalid URI
43+
assertThatCode(() ->
44+
redirector.intercept(createChainWithRedirect("not a valid uri")))
45+
.doesNotThrowAnyException();
46+
// Test unresolvable host
47+
assertThatCode(() ->
48+
redirector.intercept(createChainWithRedirect("https://nonexistent.example.invalid")))
49+
.doesNotThrowAnyException();
50+
51+
// Test denied uri's
52+
assertThatThrownBy(() ->
53+
redirector.intercept(createChainWithRedirect("https://127.0.0.1")))
54+
.isInstanceOf(ClientException.class);
55+
assertThatThrownBy(() ->
56+
redirector.intercept(createChainWithRedirect("https://localhost")))
57+
.isInstanceOf(ClientException.class);
58+
assertThatThrownBy(() ->
59+
redirector.intercept(createChainWithRedirect("http://192.168.1.1")))
60+
.isInstanceOf(ClientException.class);
61+
assertThatThrownBy(() ->
62+
redirector.intercept(createChainWithRedirect("https://0.0.0.0")))
63+
.isInstanceOf(ClientException.class);
64+
assertThatThrownBy(() ->
65+
redirector.intercept(createChainWithRedirect("https://172.16.0.1")))
66+
.isInstanceOf(ClientException.class);
67+
assertThatThrownBy(() ->
68+
redirector.intercept(createChainWithRedirect("https://169.254.169.254")))
69+
.isInstanceOf(ClientException.class);
70+
}
71+
72+
private static Interceptor.Chain createChainWithRedirect(String location)
73+
{
74+
return new TestInterceptorChain(new Response.Builder()
75+
.request(new Request.Builder().url(BASE_URL).build())
76+
.protocol(Protocol.HTTP_1_1)
77+
.code(302)
78+
.message("Found")
79+
.header("Location", location)
80+
.build());
81+
}
82+
83+
private static class TestInterceptorChain
84+
implements Interceptor.Chain
85+
{
86+
private final Response response;
87+
88+
TestInterceptorChain(Response response)
89+
{
90+
this.response = response;
91+
}
92+
93+
@Override
94+
public Request request()
95+
{
96+
return response.request();
97+
}
98+
99+
@Override
100+
public Response proceed(Request request)
101+
{
102+
return response;
103+
}
104+
105+
// Implement other Chain methods with default values
106+
@Override
107+
public int connectTimeoutMillis()
108+
{
109+
return 0;
110+
}
111+
112+
@Override
113+
public Interceptor.Chain withConnectTimeout(int timeout, java.util.concurrent.TimeUnit unit)
114+
{
115+
return this;
116+
}
117+
118+
@Override
119+
public int readTimeoutMillis()
120+
{
121+
return 0;
122+
}
123+
124+
@Override
125+
public Interceptor.Chain withReadTimeout(int timeout, java.util.concurrent.TimeUnit unit)
126+
{
127+
return this;
128+
}
129+
130+
@Override
131+
public int writeTimeoutMillis()
132+
{
133+
return 0;
134+
}
135+
136+
@Override
137+
public Interceptor.Chain withWriteTimeout(int timeout, java.util.concurrent.TimeUnit unit)
138+
{
139+
return this;
140+
}
141+
142+
@Override
143+
public okhttp3.Connection connection()
144+
{
145+
return null;
146+
}
147+
148+
@Override
149+
public okhttp3.Call call()
150+
{
151+
return null;
152+
}
153+
}
154+
}

0 commit comments

Comments
 (0)