diff --git a/jib-core/CHANGELOG.md b/jib-core/CHANGELOG.md index 00150da1c3..0c0535c0d3 100644 --- a/jib-core/CHANGELOG.md +++ b/jib-core/CHANGELOG.md @@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file. ### Fixed - `Containerizer#setAllowInsecureRegistries(boolean)` and the `sendCredentialsOverHttp` system property are now effective for authentication service server connections. ([#2074](https://github.com/GoogleContainerTools/jib/pull/2074) +- Fixed inefficient communications when interacting with insecure registries and servers (when `Containerizer#setAllowInsecureRegistries(boolean)` is set). ([#946](https://github.com/GoogleContainerTools/jib/issues/946)) ## 0.12.0 diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/configuration/BuildContext.java b/jib-core/src/main/java/com/google/cloud/tools/jib/configuration/BuildContext.java index 186138f527..f252e1a379 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/configuration/BuildContext.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/configuration/BuildContext.java @@ -417,10 +417,6 @@ public EventHandlers getEventHandlers() { return eventHandlers; } - public FailoverHttpClient getHttpClient() { - return httpClient; - } - public ExecutorService getExecutorService() { return executorService; } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java b/jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java index 3f73847437..e86abdfdb9 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java @@ -26,12 +26,15 @@ import com.google.api.client.util.SslUtils; import com.google.cloud.tools.jib.api.LogEvent; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import java.io.IOException; import java.net.ConnectException; import java.net.URL; import java.security.GeneralSecurityException; import java.util.ArrayDeque; import java.util.Deque; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Consumer; import java.util.function.Supplier; import javax.net.ssl.SSLException; @@ -71,6 +74,13 @@ */ public class FailoverHttpClient { + /** Represents failover actions taken. To be recorded in the failover history. */ + private static enum Failover { + NONE, // no failover (secure HTTPS) + INSECURE_HTTPS, // HTTPS with certificate validation disabled + HTTP // plain HTTP + } + private static boolean isHttpsProtocol(URL url) { return "https".equals(url.getProtocol()); } @@ -110,6 +120,8 @@ private static HttpTransport getInsecureHttpTransport() { private final Supplier secureHttpTransportFactory; private final Supplier insecureHttpTransportFactory; + private final ConcurrentHashMap failoverHistory = new ConcurrentHashMap<>(); + private final Deque transportsCreated = new ArrayDeque<>(); private final Deque responsesCreated = new ArrayDeque<>(); @@ -209,10 +221,18 @@ public Response put(URL url, Request request) throws IOException { * @throws IOException if building the HTTP request fails. */ public Response call(String httpMethod, URL url, Request request) throws IOException { - if (!isHttpsProtocol(url) && !enableHttpAndInsecureFailover) { + if (!isHttpsProtocol(url)) { + if (enableHttpAndInsecureFailover) { // HTTP requested. We only care if HTTP is enabled. + return call(httpMethod, url, request, getHttpTransport(true)); + } throw new SSLException("insecure HTTP connection not allowed: " + url); } + Optional fastPathResponse = followFailoverHistory(httpMethod, url, request); + if (fastPathResponse.isPresent()) { + return fastPathResponse.get(); + } + try { return call(httpMethod, url, request, getHttpTransport(true)); @@ -223,11 +243,15 @@ public Response call(String httpMethod, URL url, Request request) throws IOExcep try { logInsecureHttpsFailover(url); - return call(httpMethod, url, request, getHttpTransport(false)); + Response response = call(httpMethod, url, request, getHttpTransport(false)); + failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.INSECURE_HTTPS); + return response; } catch (SSLException ignored) { // This is usually when the server is plain-HTTP. logHttpFailover(url); - return call(httpMethod, toHttp(url), request, getHttpTransport(true)); + Response response = call(httpMethod, toHttp(url), request, getHttpTransport(true)); + failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.HTTP); + return response; } } catch (ConnectException ex) { @@ -235,20 +259,33 @@ public Response call(String httpMethod, URL url, Request request) throws IOExcep // ConnectException for connection timeout. (Could be a JDK bug.) Note SocketTimeoutException // does not extend ConnectException (or vice versa), and we want to be consistent to error out // on timeouts: https://github.com/GoogleContainerTools/jib/issues/1895#issuecomment-527544094 - if (ex.getMessage() != null && ex.getMessage().contains("timed out")) { - throw ex; - } - - // Fall back to HTTP only if "url" had no port specified (i.e., we tried the default HTTPS - // port 443) and we could not connect to 443. It's worth trying port 80. - if (enableHttpAndInsecureFailover && isHttpsProtocol(url) && url.getPort() == -1) { - logHttpFailover(url); - return call(httpMethod, toHttp(url), request, getHttpTransport(true)); + if (ex.getMessage() == null || !ex.getMessage().contains("timed out")) { + // Fall back to HTTP only if "url" had no port specified (i.e., we tried the default HTTPS + // port 443) and we could not connect to 443. It's worth trying port 80. + if (enableHttpAndInsecureFailover && isHttpsProtocol(url) && url.getPort() == -1) { + logHttpFailover(url); + Response response = call(httpMethod, toHttp(url), request, getHttpTransport(true)); + failoverHistory.put(url.getHost() + ":" + url.getPort(), Failover.HTTP); + return response; + } } throw ex; } } + private Optional followFailoverHistory(String httpMethod, URL url, Request request) + throws IOException { + Preconditions.checkArgument(isHttpsProtocol(url)); + switch (failoverHistory.getOrDefault(url.getHost() + ":" + url.getPort(), Failover.NONE)) { + case HTTP: + return Optional.of(call(httpMethod, toHttp(url), request, getHttpTransport(true))); + case INSECURE_HTTPS: + return Optional.of(call(httpMethod, url, request, getHttpTransport(false))); + default: + return Optional.empty(); // No history found. Should go for normal execution path. + } + } + private Response call(String httpMethod, URL url, Request request, HttpTransport httpTransport) throws IOException { boolean clearAuthorization = !isHttpsProtocol(url) && !sendAuthorizationOverHttp; @@ -271,7 +308,7 @@ private Response call(String httpMethod, URL url, Request request, HttpTransport try { Response response = new Response(httpRequest.execute()); synchronized (responsesCreated) { - responsesCreated.addLast(response); + responsesCreated.add(response); } return response; } catch (HttpResponseException ex) { @@ -283,18 +320,18 @@ private HttpTransport getHttpTransport(boolean secureTransport) { HttpTransport transport = secureTransport ? secureHttpTransportFactory.get() : insecureHttpTransportFactory.get(); synchronized (transportsCreated) { - transportsCreated.addLast(transport); + transportsCreated.add(transport); } return transport; } private void logHttpFailover(URL url) { String log = "Failed to connect to " + url + " over HTTPS. Attempting again with HTTP."; - logger.accept(LogEvent.info(log)); + logger.accept(LogEvent.warn(log)); } private void logInsecureHttpsFailover(URL url) { String log = "Cannot verify server at " + url + ". Attempting again with no TLS verification."; - logger.accept(LogEvent.info(log)); + logger.accept(LogEvent.warn(log)); } } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientTest.java index eb2901ab96..701cb10fbe 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/FailoverHttpClientTest.java @@ -33,8 +33,10 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.List; import java.util.concurrent.atomic.LongAdder; import java.util.function.Consumer; +import java.util.stream.Collectors; import javax.net.ssl.SSLException; import javax.net.ssl.SSLPeerUnverifiedException; import org.junit.Assert; @@ -150,14 +152,9 @@ public void testGet_insecureClientOnUnverifiableServer() throws IOException { Assert.assertEquals("body", new String(bytes, StandardCharsets.UTF_8)); } - Assert.assertEquals(2, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0)); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(1)); - - String log = - "Cannot verify server at https://insecure. Attempting again with no TLS verification."; - Mockito.verify(logger).accept(LogEvent.info(log)); - Mockito.verifyNoMoreInteractions(logger); + verifyCapturedUrls("https://insecure", "https://insecure"); + verifyWarnings( + "Cannot verify server at https://insecure. Attempting again with no TLS verification."); } @Test @@ -180,17 +177,10 @@ public void testGet_insecureClientOnHttpServer() throws IOException { Mockito.verify(mockHttpRequest, Mockito.times(2)).execute(); Mockito.verify(mockInsecureHttpRequest, Mockito.times(1)).execute(); - Assert.assertEquals(3, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0)); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(1)); - Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(2)); - - String log1 = - "Cannot verify server at https://insecure. Attempting again with no TLS verification."; - String log2 = "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP."; - Mockito.verify(logger).accept(LogEvent.info(log1)); - Mockito.verify(logger).accept(LogEvent.info(log2)); - Mockito.verifyNoMoreInteractions(logger); + verifyCapturedUrls("https://insecure", "https://insecure", "http://insecure"); + verifyWarnings( + "Cannot verify server at https://insecure. Attempting again with no TLS verification.", + "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP."); } @Test @@ -211,13 +201,8 @@ public void testGet_insecureClientOnHttpServerAndNoPortSpecified() throws IOExce Mockito.verify(mockHttpRequest, Mockito.times(2)).execute(); Mockito.verifyNoInteractions(mockInsecureHttpRequest); - Assert.assertEquals(2, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0)); - Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(1)); - - String log = "Failed to connect to https://insecure over HTTPS. Attempting again with HTTP."; - Mockito.verify(logger).accept(LogEvent.info(log)); - Mockito.verifyNoMoreInteractions(logger); + verifyCapturedUrls("https://insecure", "http://insecure"); + verifyWarnings("Failed to connect to https://insecure over HTTPS. Attempting again with HTTP."); } @Test @@ -232,8 +217,7 @@ public void testGet_secureClientOnNonListeningServerAndNoPortSpecified() throws } catch (ConnectException ex) { Assert.assertEquals("my exception", ex.getMessage()); - Assert.assertEquals(1, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getValue()); + verifyCapturedUrls("https://insecure"); Mockito.verify(mockHttpRequest, Mockito.times(1)).execute(); Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger); @@ -253,8 +237,7 @@ public void testGet_insecureClientOnNonListeningServerAndPortSpecified() throws } catch (ConnectException ex) { Assert.assertEquals("my exception", ex.getMessage()); - Assert.assertEquals(1, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure:5000"), urlCaptor.getValue()); + verifyCapturedUrls("https://insecure:5000"); Mockito.verify(mockHttpRequest, Mockito.times(1)).execute(); Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger); @@ -273,8 +256,7 @@ public void testGet_timeoutFromConnectException() throws IOException { } catch (ConnectException ex) { Assert.assertEquals("Connection timed out", ex.getMessage()); - Assert.assertEquals(1, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getValue()); + verifyCapturedUrls("https://insecure"); Mockito.verify(mockHttpRequest, Mockito.times(1)).execute(); Mockito.verifyNoInteractions(mockInsecureHttpRequest, logger); @@ -293,9 +275,7 @@ public void testGet_doNotSendCredentialsOverHttp() throws IOException { try (Response response = insecureHttpClient.get(new URL("https://insecure"), fakeRequest(null))) {} - Assert.assertEquals(2, urlCaptor.getAllValues().size()); - Assert.assertEquals(new GenericUrl("https://insecure"), urlCaptor.getAllValues().get(0)); - Assert.assertEquals(new GenericUrl("http://insecure"), urlCaptor.getAllValues().get(1)); + verifyCapturedUrls("https://insecure", "http://insecure"); Assert.assertEquals(2, httpHeadersCaptor.getAllValues().size()); Assert.assertEquals( @@ -345,6 +325,79 @@ public void testShutDown() throws IOException { } } + @Test + public void testFollowFailoverHistory_insecureHttps() throws IOException { + FailoverHttpClient httpClient = newHttpClient(true, false); + + Mockito.when(mockHttpRequest.execute()) + .thenThrow(new SSLException("")) + .thenReturn(mockHttpResponse); + try (Response response1 = httpClient.get(new URL("https://url"), fakeRequest(null)); + Response response2 = httpClient.post(new URL("https://url"), fakeRequest(null))) {} + + Mockito.verify(mockHttpRequest, Mockito.times(1)).execute(); + Mockito.verify(mockInsecureHttpRequest, Mockito.times(2)).execute(); + verifyCapturedUrls("https://url", "https://url", "https://url"); + verifyWarnings( + "Cannot verify server at https://url. Attempting again with no TLS verification."); + } + + @Test + public void testFollowFailoverHistory_httpFailoverByConnectionError() throws IOException { + FailoverHttpClient httpClient = newHttpClient(true, false); + + Mockito.when(mockHttpRequest.execute()) + .thenThrow(new ConnectException()) + .thenReturn(mockHttpResponse); + + try (Response response1 = httpClient.get(new URL("https://url"), fakeRequest(null)); + Response response2 = httpClient.post(new URL("https://url"), fakeRequest(null))) {} + + Mockito.verify(mockHttpRequest, Mockito.times(3)).execute(); + verifyCapturedUrls("https://url", "http://url", "http://url"); + verifyWarnings("Failed to connect to https://url over HTTPS. Attempting again with HTTP."); + } + + @Test + public void testFollowFailoverHistory_httpFailover() throws IOException { + FailoverHttpClient httpClient = newHttpClient(true, false); + + Mockito.when(mockHttpRequest.execute()) + .thenThrow(new SSLException("")) + .thenReturn(mockHttpResponse); + Mockito.when(mockInsecureHttpRequest.execute()).thenThrow(new SSLException("")); + + try (Response response1 = httpClient.get(new URL("https://url:123"), fakeRequest(null)); + Response response2 = httpClient.post(new URL("https://url:123"), fakeRequest(null))) {} + + Mockito.verify(mockHttpRequest, Mockito.times(3)).execute(); + Mockito.verify(mockInsecureHttpRequest, Mockito.times(1)).execute(); + verifyCapturedUrls("https://url:123", "https://url:123", "http://url:123", "http://url:123"); + verifyWarnings( + "Cannot verify server at https://url:123. Attempting again with no TLS verification.", + "Failed to connect to https://url:123 over HTTPS. Attempting again with HTTP."); + } + + @Test + public void testFollowFailoverHistory_portsDifferent() throws IOException { + FailoverHttpClient httpClient = newHttpClient(true, false); + + Mockito.when(mockHttpRequest.execute()) + .thenThrow(new SSLException("")) + .thenThrow(new SSLException("")) + .thenReturn(mockHttpResponse); + + try (Response response1 = httpClient.get(new URL("https://url:1"), fakeRequest(null)); + Response response2 = httpClient.post(new URL("https://url:2"), fakeRequest(null))) {} + + Mockito.verify(mockHttpRequest, Mockito.times(2)).execute(); + Mockito.verify(mockInsecureHttpRequest, Mockito.times(2)).execute(); + verifyCapturedUrls("https://url:1", "https://url:1", "https://url:2", "https://url:2"); + verifyWarnings( + "Cannot verify server at https://url:1. Attempting again with no TLS verification.", + "Cannot verify server at https://url:2. Attempting again with no TLS verification."); + } + private void setUpMocks( HttpTransport mockHttpTransport, HttpRequestFactory mockHttpRequestFactory, @@ -406,4 +459,17 @@ private void verifyCall(String httpMethod, CallFunction callFunction) throws IOE Assert.assertEquals("crepecake", byteArrayOutputStream.toString(StandardCharsets.UTF_8.name())); Assert.assertEquals("crepecake".length(), totalByteCount.longValue()); } + + private void verifyWarnings(String... logs) { + for (String log : logs) { + Mockito.verify(logger, Mockito.times(1)).accept(LogEvent.warn(log)); + } + Mockito.verifyNoMoreInteractions(logger); + } + + private void verifyCapturedUrls(String... urls) { + List captured = + urlCaptor.getAllValues().stream().map(GenericUrl::toString).collect(Collectors.toList()); + Assert.assertEquals(Arrays.asList(urls), captured); + } } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java index f315e1113e..7fb95628fc 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java @@ -92,7 +92,7 @@ public void testInsecureConnection_insecureHttpsFailover() String endpoint = server.getEndpoint(); String expectedLog = "Cannot verify server at " + endpoint + ". Attempting again with no TLS verification."; - Mockito.verify(logger).accept(LogEvent.info(expectedLog)); + Mockito.verify(logger).accept(LogEvent.warn(expectedLog)); } } @@ -114,8 +114,8 @@ public void testInsecureConnection_plainHttpFailover() "Cannot verify server at " + httpsUrl + ". Attempting again with no TLS verification."; String expectedLog2 = "Failed to connect to " + httpsUrl + " over HTTPS. Attempting again with HTTP."; - Mockito.verify(logger).accept(LogEvent.info(expectedLog1)); - Mockito.verify(logger).accept(LogEvent.info(expectedLog2)); + Mockito.verify(logger).accept(LogEvent.warn(expectedLog1)); + Mockito.verify(logger).accept(LogEvent.warn(expectedLog2)); } } } diff --git a/jib-gradle-plugin/CHANGELOG.md b/jib-gradle-plugin/CHANGELOG.md index 4b346e1db8..4db4b460bb 100644 --- a/jib-gradle-plugin/CHANGELOG.md +++ b/jib-gradle-plugin/CHANGELOG.md @@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file. - Fixed reporting parent build file when `skaffold init` is run on multi-module projects. ([#2091](https://github.com/GoogleContainerTools/jib/pull/2091)) - Now correctly uses the `war` task if it is enabled and the `bootWar` task is disabled for Spring WAR projects. ([#2096](https://github.com/GoogleContainerTools/jib/issues/2096)) - `allowInsecureRegistries` and the `sendCredentialsOverHttp` system property are now effective for authentication service server connections. ([#2074](https://github.com/GoogleContainerTools/jib/pull/2074) +- Fixed inefficient communications when interacting with insecure registries and servers (when `allowInsecureRegistries` is set). ([#946](https://github.com/GoogleContainerTools/jib/issues/946)) ## 1.7.0 diff --git a/jib-maven-plugin/CHANGELOG.md b/jib-maven-plugin/CHANGELOG.md index 0b1bb415a3..3be8af1830 100644 --- a/jib-maven-plugin/CHANGELOG.md +++ b/jib-maven-plugin/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. - Fixed reporting wrong module name when `skaffold init` is run on multi-module projects ([#2088](https://github.com/GoogleContainerTools/jib/issues/2088)). - `` and the `sendCredentialsOverHttp` system property are now effective for authentication service server connections. ([#2074](https://github.com/GoogleContainerTools/jib/pull/2074) +- Fixed inefficient communications when interacting with insecure registries and servers (when `` is set). ([#946](https://github.com/GoogleContainerTools/jib/issues/946)) ## 1.7.0