Skip to content

Propagate credential helper failures during downloads #30647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fzlzjerry wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Propagate credential helper failures during downloads #30647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.bazel.repository.downloader;

import com.google.common.base.Ascii;
import com.google.common.base.Function;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -53,6 +52,12 @@
@ThreadSafe
class HttpConnector {

/** Supplies request headers, including credentials, before a connection is opened. */
@FunctionalInterface
interface RequestHeadersProvider {
ImmutableMap<String, List<String>> get(URI url) throws IOException;
}

private static final int MAX_ATTEMPTS = 8;
private static final int MAX_REDIRECTS = 40;
private static final int MIN_RETRY_DELAY_MS = 100;
Expand Down Expand Up @@ -107,9 +112,7 @@ private int scale(int unscaled) {
return Math.round(unscaled * timeoutScaling);
}

URLConnection connect(
URI originalUrl, Function<URI, ImmutableMap<String, List<String>>> requestHeaders)
throws IOException {
URLConnection connect(URI originalUrl, RequestHeadersProvider requestHeaders) throws IOException {

if (Thread.interrupted()) {
throw new InterruptedIOException();
Expand All @@ -123,6 +126,9 @@ URLConnection connect(
int redirects = 0;
int connectTimeout = scale(MIN_CONNECT_TIMEOUT_MS);
while (true) {
// Resolve credentials before opening a connection so failures are not retried as network
// errors or followed by an unauthenticated request.
ImmutableMap<String, List<String>> headers = requestHeaders.get(url);
HttpURLConnection connection = null;
try {
ProxyInfo proxyInfo = proxyHelper.createProxyIfNeeded(url);
Expand All @@ -138,7 +144,7 @@ URLConnection connect(
COMPRESSED_EXTENSIONS.contains(HttpUtils.getExtension(url.getPath()))
|| COMPRESSED_EXTENSIONS.contains(HttpUtils.getExtension(originalUrl.getPath()));
connection.setInstanceFollowRedirects(false);
for (Map.Entry<String, List<String>> entry : requestHeaders.apply(url).entrySet()) {
for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
if (isAlreadyCompressed && Ascii.equalsIgnoreCase(entry.getKey(), "Accept-Encoding")) {
// We're not going to ask for compression if we're downloading a file that already
// appears to be compressed.
Expand Down
23 changes: 6 additions & 17 deletions ...a/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.google.auth.Credentials;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Ascii;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -111,8 +110,8 @@ public HttpStream connect(
// REQUEST_HEADERS should not be overridable by user provided headers
baseHeaders.putAll(REQUEST_HEADERS);

Function<URI, ImmutableMap<String, List<String>>> headerFunction =
getHeaderFunction(url, baseHeaders.buildKeepingLast(), credentials, eventHandler);
HttpConnector.RequestHeadersProvider headerFunction =
getHeaderFunction(url, baseHeaders.buildKeepingLast(), credentials);
URLConnection connection = connector.connect(url, headerFunction);
return httpStreamFactory.create(
connection,
Expand All @@ -125,19 +124,16 @@ public HttpStream connect(
HttpUtils.toUri(connection),
newUrl ->
new ImmutableMap.Builder<String, List<String>>()
.putAll(headerFunction.apply(newUrl))
.putAll(headerFunction.get(newUrl))
.putAll(extraHeaders)
.buildOrThrow());
},
type);
}

@VisibleForTesting
static Function<URI, ImmutableMap<String, List<String>>> getHeaderFunction(
URI originalUrl,
Map<String, List<String>> baseHeaders,
Credentials credentials,
EventHandler eventHandler) {
static HttpConnector.RequestHeadersProvider getHeaderFunction(
URI originalUrl, Map<String, List<String>> baseHeaders, Credentials credentials) {
Preconditions.checkNotNull(originalUrl);
Preconditions.checkNotNull(baseHeaders);
Preconditions.checkNotNull(credentials);
Expand All @@ -161,14 +157,7 @@ static Function<URI, ImmutableMap<String, List<String>>> getHeaderFunction(
}
}
}
try {
headers.putAll(credentials.getRequestMetadata(url));
} catch (IOException e) {
// If fetching credentials fails for any reason, still try to do the connection, not adding
// authentication information as we cannot look it up.
eventHandler.handle(
Event.warn("Error retrieving auth headers, continuing without: " + e.getMessage()));
}
headers.putAll(credentials.getRequestMetadata(url));
return headers.buildKeepingLast();
};
}
Expand Down
63 changes: 40 additions & 23 deletions ...m/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import com.google.common.base.Function;
import com.google.auth.Credentials;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.authandtls.StaticCredentials;
Expand Down Expand Up @@ -81,7 +81,8 @@ private static Optional<Checksum> makeChecksum(String string) {

@Before
public void before() throws Exception {
when(connector.connect(eq(TEST_URL), any(Function.class))).thenReturn(connection);
when(connector.connect(eq(TEST_URL), any(HttpConnector.RequestHeadersProvider.class)))
.thenReturn(connection);
when(streamFactory.create(
same(connection),
any(URI.class),
Expand Down Expand Up @@ -127,7 +128,7 @@ public void run() {
@Test
public void success() throws Exception {
assertThat(toByteArray(multiplexer.connect(TEST_URL, DUMMY_CHECKSUM))).isEqualTo(TEST_DATA);
verify(connector).connect(eq(TEST_URL), any(Function.class));
verify(connector).connect(eq(TEST_URL), any(HttpConnector.RequestHeadersProvider.class));
verify(streamFactory)
.create(
any(URLConnection.class),
Expand All @@ -140,14 +141,30 @@ public void success() throws Exception {

@Test
public void failure() throws Exception {
when(connector.connect(any(URI.class), any(Function.class))).thenThrow(new IOException("oops"));
when(connector.connect(any(URI.class), any(HttpConnector.RequestHeadersProvider.class)))
.thenThrow(new IOException("oops"));
IOException e =
assertThrows(IOException.class, () -> multiplexer.connect(TEST_URL, Optional.empty()));
assertThat(e).hasMessageThat().contains("oops");
verify(connector).connect(any(URI.class), any(Function.class));
verify(connector).connect(any(URI.class), any(HttpConnector.RequestHeadersProvider.class));
verifyNoMoreInteractions(connector, streamFactory);
}

@Test
public void credentialFailureIsPropagated() throws Exception {
IOException credentialFailure = new IOException("credential helper failed");
Credentials credentials = mock(Credentials.class);
when(credentials.getRequestMetadata(TEST_URL)).thenThrow(credentialFailure);

HttpConnector.RequestHeadersProvider headerFunction =
HttpConnectorMultiplexer.getHeaderFunction(TEST_URL, ImmutableMap.of(), credentials);

IOException thrown = assertThrows(IOException.class, () -> headerFunction.get(TEST_URL));

assertThat(thrown).isSameInstanceAs(credentialFailure);
verifyNoInteractions(eventHandler);
}

@Test
public void testHeaderComputationFunction() throws Exception {
ImmutableMap<String, List<String>> baseHeaders =
Expand All @@ -162,20 +179,20 @@ public void testHeaderComputationFunction() throws Exception {
originalUrl,
ImmutableMap.of("Authentication", ImmutableList.of("Zm9vOmZvb3NlY3JldA==")));

Function<URI, ImmutableMap<String, List<String>>> headerFunction =
HttpConnector.RequestHeadersProvider headerFunction =
HttpConnectorMultiplexer.getHeaderFunction(
originalUrl, baseHeaders, new StaticCredentials(additionalHeaders), eventHandler);
originalUrl, baseHeaders, new StaticCredentials(additionalHeaders));

// Unrelated URL
assertThat(headerFunction.apply(URI.create("http://example.org/some/path/file.txt")))
assertThat(headerFunction.get(URI.create("http://example.org/some/path/file.txt")))
.containsExactly(
"Accept-Encoding",
ImmutableList.of("gzip"),
"User-Agent",
ImmutableList.of("Bazel/testing"));

// With auth headers
assertThat(headerFunction.apply(URI.create("http://hosting.example.com/user/foo/file.txt")))
assertThat(headerFunction.get(URI.create("http://hosting.example.com/user/foo/file.txt")))
.containsExactly(
"Accept-Encoding",
ImmutableList.of("gzip"),
Expand All @@ -185,26 +202,26 @@ public void testHeaderComputationFunction() throws Exception {
ImmutableList.of("Zm9vOmZvb3NlY3JldA=="));

// Other hosts
assertThat(headerFunction.apply(URI.create("http://hosting2.example.com/user/foo/file.txt")))
assertThat(headerFunction.get(URI.create("http://hosting2.example.com/user/foo/file.txt")))
.containsExactly(
"Accept-Encoding",
ImmutableList.of("gzip"),
"User-Agent",
ImmutableList.of("Bazel/testing"));
assertThat(headerFunction.apply(URI.create("http://sub.hosting.example.com/user/foo/file.txt")))
assertThat(headerFunction.get(URI.create("http://sub.hosting.example.com/user/foo/file.txt")))
.containsExactly(
"Accept-Encoding",
ImmutableList.of("gzip"),
"User-Agent",
ImmutableList.of("Bazel/testing"));
assertThat(headerFunction.apply(URI.create("http://example.com/user/foo/file.txt")))
assertThat(headerFunction.get(URI.create("http://example.com/user/foo/file.txt")))
.containsExactly(
"Accept-Encoding",
ImmutableList.of("gzip"),
"User-Agent",
ImmutableList.of("Bazel/testing"));
assertThat(
headerFunction.apply(
headerFunction.get(
URI.create("http://hosting.example.com.evil.example/user/foo/file.txt")))
.containsExactly(
"Accept-Encoding",
Expand All @@ -215,12 +232,12 @@ public void testHeaderComputationFunction() throws Exception {
// Verify that URL-specific headers overwrite
ImmutableMap<String, List<String>> annonAuth =
ImmutableMap.of("Authentication", ImmutableList.of("YW5vbnltb3VzOmZvb0BleGFtcGxlLm9yZw=="));
Function<URI, ImmutableMap<String, List<String>>> combinedHeaders =
HttpConnector.RequestHeadersProvider combinedHeaders =
HttpConnectorMultiplexer.getHeaderFunction(
originalUrl, annonAuth, new StaticCredentials(additionalHeaders), eventHandler);
assertThat(combinedHeaders.apply(URI.create("http://hosting.example.com/user/foo/file.txt")))
originalUrl, annonAuth, new StaticCredentials(additionalHeaders));
assertThat(combinedHeaders.get(URI.create("http://hosting.example.com/user/foo/file.txt")))
.containsExactly("Authentication", ImmutableList.of("Zm9vOmZvb3NlY3JldA=="));
assertThat(combinedHeaders.apply(URI.create("http://unreleated.example.org/user/foo/file.txt")))
assertThat(combinedHeaders.get(URI.create("http://unreleated.example.org/user/foo/file.txt")))
.containsExactly(
"Authentication", ImmutableList.of("YW5vbnltb3VzOmZvb0BleGFtcGxlLm9yZw=="));
}
Expand All @@ -237,13 +254,13 @@ public void testHeaderComputationFunction_crossOriginStripsSensitiveHeaders() th

URI originalUrl = URI.create("http://EXAMPLE.COM/file.txt");

Function<URI, ImmutableMap<String, List<String>>> headerFunction =
HttpConnector.RequestHeadersProvider headerFunction =
HttpConnectorMultiplexer.getHeaderFunction(
originalUrl, baseHeaders, StaticCredentials.EMPTY, eventHandler);
originalUrl, baseHeaders, StaticCredentials.EMPTY);

// Same origin (case-insensitive host matching e.g. EXAMPLE.COM vs example.com and default port
// 80 normalization)
assertThat(headerFunction.apply(URI.create("http://example.com:80/other.txt")))
assertThat(headerFunction.get(URI.create("http://example.com:80/other.txt")))
.containsExactly(
"Authorization", ImmutableList.of("Bearer token"),
"Proxy-Authorization", ImmutableList.of("Basic proxy"),
Expand All @@ -252,15 +269,15 @@ public void testHeaderComputationFunction_crossOriginStripsSensitiveHeaders() th
"User-Agent", ImmutableList.of("Bazel/testing"));

// Cross origin (host change)
assertThat(headerFunction.apply(URI.create("http://other.org/file.txt")))
assertThat(headerFunction.get(URI.create("http://other.org/file.txt")))
.containsExactly("User-Agent", ImmutableList.of("Bazel/testing"));

// Cross origin (port change)
assertThat(headerFunction.apply(URI.create("http://example.com:8080/file.txt")))
assertThat(headerFunction.get(URI.create("http://example.com:8080/file.txt")))
.containsExactly("User-Agent", ImmutableList.of("Bazel/testing"));

// Cross origin (scheme change)
assertThat(headerFunction.apply(URI.create("https://example.com/file.txt")))
assertThat(headerFunction.get(URI.create("https://example.com/file.txt")))
.containsExactly("User-Agent", ImmutableList.of("Bazel/testing"));
}
}
43 changes: 31 additions & 12 deletions ...est/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.nio.charset.StandardCharsets.US_ASCII;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;

import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteStreams;
Expand Down Expand Up @@ -121,6 +122,27 @@ public void badHost_throwsIOException() throws Exception {
connector.connect(URI.create("http://bad.example"), url -> ImmutableMap.of());
}

@Test
public void requestHeadersFailure_doesNotConnectOrRetry() throws Exception {
IOException headerFailure = new IOException("credential helper failed");
AtomicInteger calls = new AtomicInteger();

IOException thrown =
assertThrows(
IOException.class,
() ->
connector.connect(
URI.create("http://test.example"),
url -> {
calls.incrementAndGet();
throw headerFailure;
}));

assertThat(thrown).isSameInstanceAs(headerFailure);
assertThat(calls.get()).isEqualTo(1);
verifyNoInteractions(proxyHelper);
}

@Test
public void normalRequest() throws Exception {
final Map<String, List<String>> headers = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -698,17 +720,14 @@ public Object call() throws Exception {
});
// Header function that provides different auth headers for
// the two servers.
Function<URI, ImmutableMap<String, List<String>>> authHeaders =
new Function<>() {
@Override
public ImmutableMap<String, List<String>> apply(URI url) {
if (url.getPort() == server1.getLocalPort()) {
return ImmutableMap.of("Authentication", ImmutableList.of(basic1));
} else if (url.getPort() == server2.getLocalPort()) {
return ImmutableMap.of("Authentication", ImmutableList.of(basic2));
} else {
return ImmutableMap.of();
}
HttpConnector.RequestHeadersProvider authHeaders =
url -> {
if (url.getPort() == server1.getLocalPort()) {
return ImmutableMap.of("Authentication", ImmutableList.of(basic1));
} else if (url.getPort() == server2.getLocalPort()) {
return ImmutableMap.of("Authentication", ImmutableList.of(basic2));
} else {
return ImmutableMap.of();
}
};
URLConnection connection =
Expand Down