Upgrade Google HTTP Client to 1.33.0 (Apache v2) (#2120)

* Upgrade google-http-client to 1.33.0 (v2)
* Remove "excludes" from "google-auth-library-oauth2-http"

build.gradle

@@ -46,9 +46,10 @@ subprojects {
     // For Google libraries, check <http-client-bom.version>, <google.auth.version>, <guava.version>,
     // ... in https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/pom.xml
     // for best compatibility.
-    GOOGLE_HTTP_CLIENT: '1.27.0',
-    GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP: '0.16.2',
-    GUAVA: '28.0-jre',
+    GOOGLE_HTTP_CLIENT: '1.33.0',
+    GOOGLE_HTTP_CLIENT_APACHE_V2: '1.33.0',
+    GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP: '0.18.0',
+    GUAVA: '28.1-jre',
 
     COMMONS_COMPRESS: '1.19',
     JACKSON_DATABIND: '2.9.10',
@@ -64,7 +65,10 @@ subprojects {
   // Use this to ensure we correctly override transitive dependencies
   // TODO: There might be a plugin that does this
   task ensureTransitiveDependencyOverrides {
-    def rules = ["google-http-client": dependencyVersions.GOOGLE_HTTP_CLIENT]
+    def rules = [
+        "google-http-client": dependencyVersions.GOOGLE_HTTP_CLIENT,
+        "google-http-client-apache-v2": dependencyVersions.GOOGLE_HTTP_CLIENT_APACHE_V2,
+    ]
     doLast {
       configurations.runtimeClasspath.resolvedConfiguration.resolvedArtifacts.each { artifact ->
         def dependency = artifact.moduleVersion.id

jib-core/build.gradle

@@ -5,9 +5,8 @@ plugins {
 
 dependencies {
   implementation "com.google.http-client:google-http-client:${dependencyVersions.GOOGLE_HTTP_CLIENT}"
-  implementation("com.google.auth:google-auth-library-oauth2-http:${dependencyVersions.GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP}") {
-    exclude group: "com.google.http-client", module: "google-http-client"
-  }
+  implementation "com.google.http-client:google-http-client-apache-v2:${dependencyVersions.GOOGLE_HTTP_CLIENT_APACHE_V2}"
+  implementation "com.google.auth:google-auth-library-oauth2-http:${dependencyVersions.GOOGLE_AUTH_LIBRARY_OAUTH2_HTTP}"
 
   implementation "org.apache.commons:commons-compress:${dependencyVersions.COMMONS_COMPRESS}"
   implementation "com.google.guava:guava:${dependencyVersions.GUAVA}"

jib-core/src/main/java/com/google/cloud/tools/jib/http/FailoverHttpClient.java

@@ -22,20 +22,19 @@
 import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpResponseException;
 import com.google.api.client.http.HttpTransport;
-import com.google.api.client.http.apache.ApacheHttpTransport;
+import com.google.api.client.http.apache.v2.ApacheHttpTransport;
+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.function.Consumer;
 import java.util.function.Supplier;
 import javax.net.ssl.SSLException;
-import org.apache.http.auth.AuthScope;
-import org.apache.http.auth.UsernamePasswordCredentials;
-import org.apache.http.impl.client.DefaultHttpClient;
+import org.apache.http.conn.ssl.NoopHostnameVerifier;
+import org.apache.http.impl.client.HttpClientBuilder;
 
 /**
  * Thread-safe HTTP client that can automatically failover from secure HTTPS to insecure HTTPS or
@@ -86,55 +85,23 @@ private static HttpTransport getSecureHttpTransport() {
     //
     // A new ApacheHttpTransport needs to be created for each connection because otherwise HTTP
     // connection persistence causes the connection to throw NoHttpResponseException.
-    ApacheHttpTransport transport = new ApacheHttpTransport();
-    addProxyCredentials(transport);
-    return transport;
+    return new ApacheHttpTransport();
   }
 
   private static HttpTransport getInsecureHttpTransport() {
     try {
-      ApacheHttpTransport insecureTransport =
-          new ApacheHttpTransport.Builder().doNotValidateCertificate().build();
-      addProxyCredentials(insecureTransport);
-      return insecureTransport;
+      HttpClientBuilder httpClientBuilder =
+          ApacheHttpTransport.newDefaultHttpClientBuilder()
+              .setSSLSocketFactory(null) // creates new factory with the SSLContext given below
+              .setSSLContext(SslUtils.trustAllSSLContext())
+              .setSSLHostnameVerifier(new NoopHostnameVerifier());
+      // Do not use NetHttpTransport. See comments in getConnectionFactory for details.
+      return new ApacheHttpTransport(httpClientBuilder.build());
     } catch (GeneralSecurityException ex) {
       throw new RuntimeException("platform does not support TLS protocol", ex);
     }
   }
 
-  /**
-   * Registers proxy credentials onto transport client, in order to deal with proxies that require
-   * basic authentication.
-   *
-   * @param transport Apache HTTP transport
-   */
-  @VisibleForTesting
-  static void addProxyCredentials(ApacheHttpTransport transport) {
-    addProxyCredentials(transport, "https");
-    addProxyCredentials(transport, "http");
-  }
-
-  private static void addProxyCredentials(ApacheHttpTransport transport, String protocol) {
-    Preconditions.checkArgument(protocol.equals("http") || protocol.equals("https"));
-
-    String proxyHost = System.getProperty(protocol + ".proxyHost");
-    String proxyUser = System.getProperty(protocol + ".proxyUser");
-    String proxyPassword = System.getProperty(protocol + ".proxyPassword");
-    if (proxyHost == null || proxyUser == null || proxyPassword == null) {
-      return;
-    }
-
-    String defaultProxyPort = protocol.equals("http") ? "80" : "443";
-    int proxyPort = Integer.parseInt(System.getProperty(protocol + ".proxyPort", defaultProxyPort));
-
-    DefaultHttpClient httpClient = (DefaultHttpClient) transport.getHttpClient();
-    httpClient
-        .getCredentialsProvider()
-        .setCredentials(
-            new AuthScope(proxyHost, proxyPort),
-            new UsernamePasswordCredentials(proxyUser, proxyPassword));
-  }
-
   private final boolean enableHttpAndInsecureFailover;
   private final boolean sendAuthorizationOverHttp;
   private final Consumer<LogEvent> logger;

jib-core/src/main/java/com/google/cloud/tools/jib/registry/RegistryEndpointCaller.java

@@ -164,10 +164,11 @@ private T call(URL url) throws IOException, RegistryException {
             throw new RegistryUnauthorizedException(serverUrl, imageName, responseException);
           }
 
-        } else if (responseException.getStatusCode()
-                == HttpStatusCodes.STATUS_CODE_TEMPORARY_REDIRECT
-            || responseException.getStatusCode() == HttpStatusCodes.STATUS_CODE_MOVED_PERMANENTLY
-            || responseException.getStatusCode() == STATUS_CODE_PERMANENT_REDIRECT) {
+          // 301 (Moved Permanently), 302 (Found), 303 (See Other), and 307 (Temporary Redirect) are
+          // automatically followed by Google HTTP Client (setFollowRedirects(true)), but 308 isn't.
+          // https://github.com/googleapis/google-http-java-client/issues/873
+          // TODO: remove this when the bug is fixed.
+        } else if (responseException.getStatusCode() == STATUS_CODE_PERMANENT_REDIRECT) {
           // 'Location' header can be relative or absolute.
           URL redirectLocation = new URL(url, responseException.getHeaders().getLocation());
           return call(redirectLocation);

jib-core/src/test/java/com/google/cloud/tools/jib/http/WithServerFailoverHttpClientTest.java

@@ -24,6 +24,7 @@
 import java.nio.charset.StandardCharsets;
 import java.security.GeneralSecurityException;
 import java.util.Arrays;
+import java.util.List;
 import java.util.function.Consumer;
 import javax.net.ssl.SSLException;
 import org.hamcrest.CoreMatchers;
@@ -149,4 +150,45 @@ public void testProxyCredentialProperties()
       }
     }
   }
+
+  @Test
+  public void testRedirectionUrls()
+      throws IOException, InterruptedException, GeneralSecurityException, URISyntaxException {
+    // Sample query strings from
+    // https://github.com/GoogleContainerTools/jib/issues/1986#issuecomment-547610104
+    String url1 = "?id=301&_auth_=exp=1572285389~hmac=f0a387f0";
+    String url2 = "?id=302&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614";
+    String url3 = "?id=303&_auth_=exp=1572285389~hmac=f0a387f0";
+    String url4 = "?id=307&Signature=2wYOD0a%2BDAkK%2F9lQJUOuIpYti8o%3D&Expires=1569997614";
+
+    String redirect301 =
+        "HTTP/1.1 301 Moved Permanently\nLocation: " + url1 + "\nContent-Length: 0\n\n";
+    String redirect302 = "HTTP/1.1 302 Found\nLocation: " + url2 + "\nContent-Length: 0\n\n";
+    String redirect303 = "HTTP/1.1 303 See Other\nLocation: " + url3 + "\nContent-Length: 0\n\n";
+    String redirect307 =
+        "HTTP/1.1 307 Temporary Redirect\nLocation: " + url4 + "\nContent-Length: 0\n\n";
+    String ok200 = "HTTP/1.1 200 OK\nContent-Length:12\n\nHello World!";
+    List<String> responses =
+        Arrays.asList(redirect301, redirect302, redirect303, redirect307, ok200);
+
+    FailoverHttpClient httpClient = new FailoverHttpClient(true /*insecure*/, false, logger);
+    try (TestWebServer server = new TestWebServer(false, responses, 1)) {
+      httpClient.get(new URL(server.getEndpoint()), request);
+
+      Assert.assertThat(
+          server.getInputRead(),
+          CoreMatchers.containsString("GET /?id=301&_auth_=exp%3D1572285389~hmac%3Df0a387f0 "));
+      Assert.assertThat(
+          server.getInputRead(),
+          CoreMatchers.containsString(
+              "GET /?id=302&Signature=2wYOD0a%2BDAkK/9lQJUOuIpYti8o%3D&Expires=1569997614 "));
+      Assert.assertThat(
+          server.getInputRead(),
+          CoreMatchers.containsString("GET /?id=303&_auth_=exp%3D1572285389~hmac%3Df0a387f0 "));
+      Assert.assertThat(
+          server.getInputRead(),
+          CoreMatchers.containsString(
+              "GET /?id=307&Signature=2wYOD0a%2BDAkK/9lQJUOuIpYti8o%3D&Expires=1569997614 "));
+    }
+  }
 }