diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java index 17925987a45b4b..724a357a872fea 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java @@ -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; @@ -53,6 +52,12 @@ @ThreadSafe class HttpConnector { + /** Supplies request headers, including credentials, before a connection is opened. */ + @FunctionalInterface + interface RequestHeadersProvider { + ImmutableMap> 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; @@ -107,9 +112,7 @@ private int scale(int unscaled) { return Math.round(unscaled * timeoutScaling); } - URLConnection connect( - URI originalUrl, Function>> requestHeaders) - throws IOException { + URLConnection connect(URI originalUrl, RequestHeadersProvider requestHeaders) throws IOException { if (Thread.interrupted()) { throw new InterruptedIOException(); @@ -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> headers = requestHeaders.get(url); HttpURLConnection connection = null; try { ProxyInfo proxyInfo = proxyHelper.createProxyIfNeeded(url); @@ -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> entry : requestHeaders.apply(url).entrySet()) { + for (Map.Entry> 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. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java index 3ea2705e1ca242..951dfd3adc3b2e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java @@ -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; @@ -111,8 +110,8 @@ public HttpStream connect( // REQUEST_HEADERS should not be overridable by user provided headers baseHeaders.putAll(REQUEST_HEADERS); - Function>> 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, @@ -125,7 +124,7 @@ public HttpStream connect( HttpUtils.toUri(connection), newUrl -> new ImmutableMap.Builder>() - .putAll(headerFunction.apply(newUrl)) + .putAll(headerFunction.get(newUrl)) .putAll(extraHeaders) .buildOrThrow()); }, @@ -133,11 +132,8 @@ public HttpStream connect( } @VisibleForTesting - static Function>> getHeaderFunction( - URI originalUrl, - Map> baseHeaders, - Credentials credentials, - EventHandler eventHandler) { + static HttpConnector.RequestHeadersProvider getHeaderFunction( + URI originalUrl, Map> baseHeaders, Credentials credentials) { Preconditions.checkNotNull(originalUrl); Preconditions.checkNotNull(baseHeaders); Preconditions.checkNotNull(credentials); @@ -161,14 +157,7 @@ static Function>> 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(); }; } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java index 0df1dcfa01ae84..bad150076f8199 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java @@ -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; @@ -81,7 +81,8 @@ private static Optional 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), @@ -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), @@ -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> baseHeaders = @@ -162,12 +179,12 @@ public void testHeaderComputationFunction() throws Exception { originalUrl, ImmutableMap.of("Authentication", ImmutableList.of("Zm9vOmZvb3NlY3JldA=="))); - Function>> 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"), @@ -175,7 +192,7 @@ public void testHeaderComputationFunction() throws Exception { 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"), @@ -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", @@ -215,12 +232,12 @@ public void testHeaderComputationFunction() throws Exception { // Verify that URL-specific headers overwrite ImmutableMap> annonAuth = ImmutableMap.of("Authentication", ImmutableList.of("YW5vbnltb3VzOmZvb0BleGFtcGxlLm9yZw==")); - Function>> 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==")); } @@ -237,13 +254,13 @@ public void testHeaderComputationFunction_crossOriginStripsSensitiveHeaders() th URI originalUrl = URI.create("http://EXAMPLE.COM/file.txt"); - Function>> 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"), @@ -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")); } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java index 28da13409f814a..f1f16ed7db0031 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorTest.java @@ -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; @@ -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> headers = new ConcurrentHashMap<>(); @@ -698,17 +720,14 @@ public Object call() throws Exception { }); // Header function that provides different auth headers for // the two servers. - Function>> authHeaders = - new Function<>() { - @Override - public ImmutableMap> 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 =