TransportHttp: shared SSLContext during fetch or push

TransportHttp makes several HTTP requests. The SSLContext and socket
factory must be shared over these requests, otherwise authentication
information may not be propagated correctly from one request to the
next. This is important for authentication mechanisms that rely on
client-side state, like NEGOTIATE (either NTLM, if the underlying HTTP
library supports it, or Kerberos). In particular, SPNEGO cannot
authenticate on a POST request; the authentication must come from the
initial GET request, which implies that the POST request must use the
same SSLContext and socket factory that was used for the GET.

Change the way HTTPS connections are configured. Introduce the concept
of a GitSession, which is a client-side HTTP session over several HTTPS
requests. TransportHttp creates such a session and uses it to configure
all HTTP requests during that session (fetch or push). This gives a way
to abstract away the differences between JDK and Apache HTTP connections
and to configure SSL setup outside.

A GitSession can maintain state and thus give all HTTP requests in a
session the same socket factory.

Introduce an extension interface HttpConnectionFactory2 that adds a
method to obtain a new GitSession. Implement this for both existing
HTTP connection factories. Change TransportHttp to use the new
GitSession to configure HTTP connections.

The old methods for disabling SSL verification still exist to support
possibly external connection and connection factory implementations
that do not make use of the new GitSession yet.

Bug: 535850
Change-Id: Iedf67464e4e353c1883447c13c86b5a838e678f1
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.http.apache/resources/org/eclipse/jgit/transport/http/apache/internal/HttpApacheText.properties

@@ -1 +1,2 @@
+httpWrongConnectionType=Wrong connection type: expected {0}, got {1}.
 unexpectedSSLContextException=unexpected exception when searching for the TLS protocol

org.eclipse.jgit.http.apache/src/org/eclipse/jgit/transport/http/apache/HttpClientConnection.java

@@ -57,9 +57,7 @@ import org.apache.http.config.Registry;
 import org.apache.http.config.RegistryBuilder;
 import org.apache.http.conn.socket.ConnectionSocketFactory;
 import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.DefaultHostnameVerifier;
 import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
-import org.apache.http.conn.util.PublicSuffixMatcherLoader;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.impl.client.HttpClients;
 import org.apache.http.impl.client.SystemDefaultCredentialsProvider;
@@ -103,7 +101,11 @@ public class HttpClientConnection implements HttpConnection {
 
 	private HostnameVerifier hostnameverifier;
 
-	SSLContext ctx;
+	private SSLContext ctx;
+
+	private SSLConnectionSocketFactory socketFactory;
+
+	private boolean usePooling = true;
 
 	private HttpClient getClient() {
 		if (client == null) {
@@ -125,11 +127,18 @@ public class HttpClientConnection implements HttpConnection {
 				configBuilder
 						.setRedirectsEnabled(followRedirects.booleanValue());
 			}
-			SSLConnectionSocketFactory sslConnectionFactory = getSSLSocketFactory();
+			boolean pooled = true;
+			SSLConnectionSocketFactory sslConnectionFactory;
+			if (socketFactory != null) {
+				pooled = usePooling;
+				sslConnectionFactory = socketFactory;
+			} else {
+				// Legacy implementation.
+				pooled = (hostnameverifier == null);
+				sslConnectionFactory = getSSLSocketFactory();
+			}
 			clientBuilder.setSSLSocketFactory(sslConnectionFactory);
-			if (hostnameverifier != null) {
-				// Using a custom verifier: we don't want pooled connections
-				// with this.
+			if (!pooled) {
 				Registry<ConnectionSocketFactory> registry = RegistryBuilder
 						.<ConnectionSocketFactory> create()
 						.register("https", sslConnectionFactory)
@@ -147,14 +156,19 @@ public class HttpClientConnection implements HttpConnection {
 		return client;
 	}
 
+	void setSSLSocketFactory(@NonNull SSLConnectionSocketFactory factory,
+			boolean isDefault) {
+		socketFactory = factory;
+		usePooling = isDefault;
+	}
+
 	private SSLConnectionSocketFactory getSSLSocketFactory() {
 		HostnameVerifier verifier = hostnameverifier;
 		SSLContext context;
 		if (verifier == null) {
 			// Use defaults
-			context = SSLContexts.createDefault();
-			verifier = new DefaultHostnameVerifier(
-					PublicSuffixMatcherLoader.getDefault());
+			context = SSLContexts.createSystemDefault();
+			verifier = SSLConnectionSocketFactory.getDefaultHostnameVerifier();
 		} else {
 			// Using a custom verifier. Attention: configure() must have been
 			// called already, otherwise one gets a "context not initialized"

org.eclipse.jgit.http.apache/src/org/eclipse/jgit/transport/http/apache/HttpClientConnectionFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Christian Halstrick <christian.halstrick@sap.com> and others
+ * Copyright (C) 2013, 2020 Christian Halstrick <christian.halstrick@sap.com> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -12,27 +12,100 @@ package org.eclipse.jgit.transport.http.apache;
 import java.io.IOException;
 import java.net.Proxy;
 import java.net.URL;
+import java.security.GeneralSecurityException;
+import java.text.MessageFormat;
 
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocket;
+import javax.net.ssl.TrustManager;
+
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
 import org.eclipse.jgit.transport.http.HttpConnection;
-import org.eclipse.jgit.transport.http.HttpConnectionFactory;
+import org.eclipse.jgit.transport.http.HttpConnectionFactory2;
+import org.eclipse.jgit.transport.http.NoCheckX509TrustManager;
+import org.eclipse.jgit.transport.http.apache.internal.HttpApacheText;
+import org.eclipse.jgit.util.HttpSupport;
 
 /**
- * A factory returning instances of
- * {@link org.eclipse.jgit.transport.http.apache.HttpClientConnection}
+ * A factory returning instances of {@link HttpClientConnection}.
  *
  * @since 3.3
  */
-public class HttpClientConnectionFactory implements HttpConnectionFactory {
-	/** {@inheritDoc} */
+public class HttpClientConnectionFactory implements HttpConnectionFactory2 {
+
 	@Override
 	public HttpConnection create(URL url) throws IOException {
 		return new HttpClientConnection(url.toString());
 	}
 
-	/** {@inheritDoc} */
 	@Override
-	public HttpConnection create(URL url, Proxy proxy)
-			throws IOException {
+	public HttpConnection create(URL url, Proxy proxy) throws IOException {
 		return new HttpClientConnection(url.toString(), proxy);
 	}
+
+	@Override
+	public GitSession newSession() {
+		return new HttpClientSession();
+	}
+
+	private static class HttpClientSession implements GitSession {
+
+		private SSLContext securityContext;
+
+		private SSLConnectionSocketFactory socketFactory;
+
+		private boolean isDefault;
+
+		@Override
+		public HttpClientConnection configure(HttpConnection connection,
+				boolean sslVerify)
+				throws IOException, GeneralSecurityException {
+			if (!(connection instanceof HttpClientConnection)) {
+				throw new IllegalArgumentException(MessageFormat.format(
+						HttpApacheText.get().httpWrongConnectionType,
+						HttpClientConnection.class.getName(),
+						connection.getClass().getName()));
+			}
+			HttpClientConnection conn = (HttpClientConnection) connection;
+			String scheme = conn.getURL().getProtocol();
+			if (!"https".equals(scheme)) { //$NON-NLS-1$
+				return conn;
+			}
+			if (securityContext == null || isDefault != sslVerify) {
+				isDefault = sslVerify;
+				HostnameVerifier verifier;
+				if (sslVerify) {
+					securityContext = SSLContext.getDefault();
+					verifier = SSLConnectionSocketFactory
+							.getDefaultHostnameVerifier();
+				} else {
+					securityContext = SSLContext.getInstance("TLS");
+					TrustManager[] trustAllCerts = {
+							new NoCheckX509TrustManager() };
+					securityContext.init(null, trustAllCerts, null);
+					verifier = (name, session) -> true;
+				}
+				socketFactory = new SSLConnectionSocketFactory(securityContext,
+						verifier) {
+
+					@Override
+					protected void prepareSocket(SSLSocket socket)
+							throws IOException {
+						super.prepareSocket(socket);
+						HttpSupport.configureTLS(socket);
+					}
+				};
+			}
+			conn.setSSLSocketFactory(socketFactory, isDefault);
+			return conn;
+		}
+
+		@Override
+		public void close() {
+			securityContext = null;
+			socketFactory = null;
+		}
+
+	}
 }

org.eclipse.jgit.http.apache/src/org/eclipse/jgit/transport/http/apache/internal/HttpApacheText.java

@@ -27,5 +27,6 @@ public class HttpApacheText extends TranslationBundle {
 	}
 
 	// @formatter:off
+	/***/ public String httpWrongConnectionType;
 	/***/ public String unexpectedSSLContextException;
 }

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -312,6 +312,7 @@ hoursAgo={0} hours ago
 httpConfigCannotNormalizeURL=Cannot normalize URL path {0}: too many .. segments
 httpConfigInvalidURL=Cannot parse URL from subsection http.{0} in git config; ignored.
 httpFactoryInUse=Changing the HTTP connection factory after an HTTP connection has already been opened is not allowed.
+httpWrongConnectionType=Wrong connection type: expected {0}, got {1}.
 hugeIndexesAreNotSupportedByJgitYet=Huge indexes are not supported by jgit, yet
 hunkBelongsToAnotherFile=Hunk belongs to another file
 hunkDisconnectedFromFile=Hunk disconnected from file

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -340,6 +340,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String httpConfigCannotNormalizeURL;
 	/***/ public String httpConfigInvalidURL;
 	/***/ public String httpFactoryInUse;
+	/***/ public String httpWrongConnectionType;
 	/***/ public String hugeIndexesAreNotSupportedByJgitYet;
 	/***/ public String hunkBelongsToAnotherFile;
 	/***/ public String hunkDisconnectedFromFile;

org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java

@@ -53,6 +53,7 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.security.GeneralSecurityException;
 import java.security.cert.CertPathBuilderException;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.CertificateException;
@@ -97,6 +98,7 @@ import org.eclipse.jgit.transport.HttpAuthMethod.Type;
 import org.eclipse.jgit.transport.HttpConfig.HttpRedirectMode;
 import org.eclipse.jgit.transport.http.HttpConnection;
 import org.eclipse.jgit.transport.http.HttpConnectionFactory;
+import org.eclipse.jgit.transport.http.HttpConnectionFactory2;
 import org.eclipse.jgit.util.HttpSupport;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
@@ -264,6 +266,8 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 
 	private HttpConnectionFactory factory;
 
+	private HttpConnectionFactory2.GitSession gitSession;
+
 	private boolean factoryUsed;
 
 	/**
@@ -529,7 +533,10 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 	/** {@inheritDoc} */
 	@Override
 	public void close() {
-		// No explicit connections are maintained.
+		if (gitSession != null) {
+			gitSession.close();
+			gitSession = null;
+		}
 	}
 
 	/**
@@ -963,7 +970,17 @@ public class TransportHttp extends HttpTransport implements WalkTransport,
 		factoryUsed = true;
 		HttpConnection conn = factory.create(u, proxy);
 
-		if (!sslVerify && "https".equals(u.getProtocol())) { //$NON-NLS-1$
+		if (gitSession == null && (factory instanceof HttpConnectionFactory2)) {
+			gitSession = ((HttpConnectionFactory2) factory).newSession();
+		}
+		if (gitSession != null) {
+			try {
+				gitSession.configure(conn, sslVerify);
+			} catch (GeneralSecurityException e) {
+				throw new IOException(e.getMessage(), e);
+			}
+		} else if (!sslVerify && "https".equals(u.getProtocol())) { //$NON-NLS-1$
+			// Backwards compatibility
 			HttpSupport.disableSslVerify(conn);
 		}
 

org.eclipse.jgit/src/org/eclipse/jgit/transport/http/HttpConnectionFactory2.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2020 Thomas Wolf <thomas.wolf@paranor.ch> and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+package org.eclipse.jgit.transport.http;
+
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+
+import org.eclipse.jgit.annotations.NonNull;
+
+/**
+ * A {@link HttpConnectionFactory} that supports client-side sessions that can
+ * maintain state and configure connections.
+ *
+ * @since 5.11
+ */
+public interface HttpConnectionFactory2 extends HttpConnectionFactory {
+
+	/**
+	 * Creates a new {@link GitSession} instance that can be used with
+	 * connections created by this {@link HttpConnectionFactory} instance.
+	 *
+	 * @return a new {@link GitSession}
+	 */
+	@NonNull
+	GitSession newSession();
+
+	/**
+	 * A {@code GitSession} groups the multiple HTTP connections
+	 * {@link org.eclipse.jgit.transport.TransportHttp TransportHttp} uses for
+	 * the requests it makes during a git fetch or push. A {@code GitSession}
+	 * can maintain client-side HTTPS state and can configure individual
+	 * connections.
+	 */
+	interface GitSession {
+
+		/**
+		 * Configure a just created {@link HttpConnection}.
+		 *
+		 * @param connection
+		 *            to configure; created by the same
+		 *            {@link HttpConnectionFactory} instance
+		 * @param sslVerify
+		 *            whether SSL is to be verified
+		 * @return the configured {@connection}
+		 * @throws IOException
+		 *             if the connection cannot be configured
+		 * @throws GeneralSecurityException
+		 *             if the connection cannot be configured
+		 */
+		@NonNull
+		HttpConnection configure(@NonNull HttpConnection connection,
+				boolean sslVerify) throws IOException, GeneralSecurityException;
+
+		/**
+		 * Closes the {@link GitSession}, releasing any internal state.
+		 */
+		void close();
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/transport/http/JDKHttpConnectionFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Christian Halstrick <christian.halstrick@sap.com> and others
+ * Copyright (C) 2013, 2020 Christian Halstrick <christian.halstrick@sap.com> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -12,6 +12,18 @@ package org.eclipse.jgit.transport.http;
 import java.io.IOException;
 import java.net.Proxy;
 import java.net.URL;
+import java.security.GeneralSecurityException;
+import java.text.MessageFormat;
+
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocket;
+import javax.net.ssl.SSLSocketFactory;
+import javax.net.ssl.TrustManager;
+
+import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.internal.transport.http.DelegatingSSLSocketFactory;
+import org.eclipse.jgit.util.HttpSupport;
 
 /**
  * A factory returning instances of
@@ -19,17 +31,70 @@ import java.net.URL;
  *
  * @since 3.3
  */
-public class JDKHttpConnectionFactory implements HttpConnectionFactory {
-	/** {@inheritDoc} */
+public class JDKHttpConnectionFactory implements HttpConnectionFactory2 {
+
 	@Override
 	public HttpConnection create(URL url) throws IOException {
 		return new JDKHttpConnection(url);
 	}
 
-	/** {@inheritDoc} */
 	@Override
 	public HttpConnection create(URL url, Proxy proxy)
 			throws IOException {
 		return new JDKHttpConnection(url, proxy);
 	}
+
+	@Override
+	public GitSession newSession() {
+		return new JdkConnectionSession();
+	}
+
+	private static class JdkConnectionSession implements GitSession {
+
+		private SSLContext securityContext;
+
+		private SSLSocketFactory socketFactory;
+
+		@Override
+		public JDKHttpConnection configure(HttpConnection connection,
+				boolean sslVerify) throws GeneralSecurityException {
+			if (!(connection instanceof JDKHttpConnection)) {
+				throw new IllegalArgumentException(MessageFormat.format(
+						JGitText.get().httpWrongConnectionType,
+						JDKHttpConnection.class.getName(),
+						connection.getClass().getName()));
+			}
+			JDKHttpConnection conn = (JDKHttpConnection) connection;
+			String scheme = conn.getURL().getProtocol();
+			if (!"https".equals(scheme) || sslVerify) { //$NON-NLS-1$
+				// sslVerify == true: use the JDK defaults
+				return conn;
+			}
+			if (securityContext == null) {
+				securityContext = SSLContext.getInstance("TLS"); //$NON-NLS-1$
+				TrustManager[] trustAllCerts = {
+						new NoCheckX509TrustManager() };
+				securityContext.init(null, trustAllCerts, null);
+				socketFactory = new DelegatingSSLSocketFactory(
+						securityContext.getSocketFactory()) {
+
+					@Override
+					protected void configure(SSLSocket socket) {
+						HttpSupport.configureTLS(socket);
+					}
+				};
+			}
+			conn.setHostnameVerifier((name, session) -> true);
+			((HttpsURLConnection) conn.wrappedUrlConnection)
+					.setSSLSocketFactory(socketFactory);
+			return conn;
+		}
+
+		@Override
+		public void close() {
+			securityContext = null;
+			socketFactory = null;
+		}
+	}
+
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/http/NoCheckX509TrustManager.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2016, 2020 JGit contributors
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Contributors:
+ *    Saša Živkov 2016 - initial API
+ *    Thomas Wolf 2020 - extracted from HttpSupport
+ */
+package org.eclipse.jgit.transport.http;
+
+import java.security.cert.X509Certificate;
+
+import javax.net.ssl.X509TrustManager;
+
+/**
+ * A {@link X509TrustManager} that doesn't verify anything. Can be used for
+ * skipping SSL checks.
+ *
+ * @since 5.11
+ */
+public class NoCheckX509TrustManager implements X509TrustManager {
+
+	@Override
+	public X509Certificate[] getAcceptedIssuers() {
+		return null;
+	}
+
+	@Override
+	public void checkClientTrusted(X509Certificate[] certs,
+			String authType) {
+		// no check
+	}
+
+	@Override
+	public void checkServerTrusted(X509Certificate[] certs,
+			String authType) {
+		// no check
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/util/HttpSupport.java

@@ -24,21 +24,18 @@ import java.net.URL;
 import java.net.URLEncoder;
 import java.security.KeyManagementException;
 import java.security.NoSuchAlgorithmException;
-import java.security.cert.X509Certificate;
 import java.text.MessageFormat;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.Set;
 
-import javax.net.ssl.HostnameVerifier;
-import javax.net.ssl.SSLSession;
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.TrustManager;
-import javax.net.ssl.X509TrustManager;
 
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.transport.http.HttpConnection;
+import org.eclipse.jgit.transport.http.NoCheckX509TrustManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -301,40 +298,13 @@ public class HttpSupport {
 	 */
 	public static void disableSslVerify(HttpConnection conn)
 			throws IOException {
-		final TrustManager[] trustAllCerts = new TrustManager[] {
-				new DummyX509TrustManager() };
+		TrustManager[] trustAllCerts = {
+				new NoCheckX509TrustManager() };
 		try {
 			conn.configure(null, trustAllCerts, null);
-			conn.setHostnameVerifier(new DummyHostnameVerifier());
+			conn.setHostnameVerifier((name, session) -> true);
 		} catch (KeyManagementException | NoSuchAlgorithmException e) {
-			throw new IOException(e.getMessage());
-		}
-	}
-
-	private static class DummyX509TrustManager implements X509TrustManager {
-		@Override
-		public X509Certificate[] getAcceptedIssuers() {
-			return null;
-		}
-
-		@Override
-		public void checkClientTrusted(X509Certificate[] certs,
-				String authType) {
-			// no check
-		}
-
-		@Override
-		public void checkServerTrusted(X509Certificate[] certs,
-				String authType) {
-			// no check
-		}
-	}
-
-	private static class DummyHostnameVerifier implements HostnameVerifier {
-		@Override
-		public boolean verify(String hostname, SSLSession session) {
-			// always accept
-			return true;
+			throw new IOException(e.getMessage(), e);
 		}
 	}