Do authentication re-tries on HTTP POST

There is at least one git server out there (GOGS) that does
not require authentication on the initial GET for
info/refs?service=git-receive-pack but that _does_ require
authentication for the subsequent POST to actually do the push.

This occurs on GOGS with public repositories; for private
repositories it wants authentication up front.

Handle this behavior by adding 401 handling to our POST request.
Note that this is suboptimal; we'll re-send the push data at
least twice if an authentication failure on POST occurs. It
would be much better if the server required authentication
up-front in the GET request.

Added authentication unit tests (using BASIC auth) to the
SmartClientSmartServerTest:

- clone with authentication
- clone with authentication but lacking CredentialsProvider
- clone with authentication and wrong password
- clone with authentication after redirect
- clone with authentication only on POST, but not on GET

Also tested manually in the wild using repositories at try.gogs.io.
That server offers only BASIC auth, so the other paths
(DIGEST, NEGOTIATE, fall back from DIGEST to BASIC) are untested
and I have no way to test them.

* public repository: GET unauthenticated, POST authenticated
  Also tested after clearing the credentials and then entering a
  wrong password: correctly asks three times during the HTTP
  POST for user name and password, then gives up.
* private repository: authentication already on GET; then gets
  applied correctly initially to the POST request, which succeeds.

Also fix the authentication to use the credentials for the redirected
URI if redirects had occurred. We must not present the credentials
for the original URI in that case. Consider a malicious redirect A->B:
this would allow server B to harvest the user credentials for server
A. The unit test for authentication after a redirect also tests for
this.

Bug: 513043
Change-Id: I97ee5058569efa1545a6c6f6edfd2b357c40592a
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 254 insertions, 9 deletions

diff --git a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
index 421fa8ad61..08bd0ef14f 100644
--- a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
+++ b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF
@@ -8,6 +8,8 @@ Bundle-Localization: plugin
 Bundle-RequiredExecutionEnvironment: JavaSE-1.8
 Import-Package: javax.servlet;version="[2.5.0,3.2.0)",
  javax.servlet.http;version="[2.5.0,3.2.0)",
+ org.apache.commons.codec;version="1.6.0",
+ org.apache.commons.codec.binary;version="1.6.0",
  org.eclipse.jetty.continuation;version="[9.4.5,10.0.0)",
  org.eclipse.jetty.http;version="[9.4.5,10.0.0)",
  org.eclipse.jetty.io;version="[9.4.5,10.0.0)",
diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index 8cadca5235..1b0c6949a9 100644
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -83,6 +83,7 @@ import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jgit.errors.RemoteRepositoryException;
 import org.eclipse.jgit.errors.TransportException;
+import org.eclipse.jgit.errors.UnsupportedCredentialItem;
 import org.eclipse.jgit.http.server.GitServlet;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
@@ -105,12 +106,15 @@ import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.transport.CredentialItem;
+import org.eclipse.jgit.transport.CredentialsProvider;
 import org.eclipse.jgit.transport.FetchConnection;
 import org.eclipse.jgit.transport.HttpTransport;
 import org.eclipse.jgit.transport.RemoteRefUpdate;
 import org.eclipse.jgit.transport.Transport;
 import org.eclipse.jgit.transport.TransportHttp;
 import org.eclipse.jgit.transport.URIish;
+import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider;
 import org.eclipse.jgit.transport.http.HttpConnectionFactory;
 import org.eclipse.jgit.transport.http.JDKHttpConnectionFactory;
 import org.eclipse.jgit.transport.http.apache.HttpClientConnectionFactory;
@@ -129,12 +133,19 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 
 	private Repository remoteRepository;
 
+	private CredentialsProvider testCredentials = new UsernamePasswordCredentialsProvider(
+			AppServer.username, AppServer.password);
+
 	private URIish remoteURI;
 
 	private URIish brokenURI;
 
 	private URIish redirectURI;
 
+	private URIish authURI;
+
+	private URIish authOnPostURI;
+
 	private RevBlob A_txt;
 
 	private RevCommit A, B;
@@ -169,7 +180,11 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 
 		ServletContextHandler broken = addBrokenContext(gs, src, srcName);
 
-		ServletContextHandler redirect = addRedirectContext(gs, src, srcName);
+		ServletContextHandler redirect = addRedirectContext(gs);
+
+		ServletContextHandler auth = addAuthContext(gs, "auth");
+
+		ServletContextHandler authOnPost = addAuthContext(gs, "pauth", "POST");
 
 		server.setUp();
 
@@ -177,6 +192,8 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 		remoteURI = toURIish(app, srcName);
 		brokenURI = toURIish(broken, srcName);
 		redirectURI = toURIish(redirect, srcName);
+		authURI = toURIish(auth, srcName);
+		authOnPostURI = toURIish(authOnPost, srcName);
 
 		A_txt = src.blob("A");
 		A = src.commit().add("A_txt", A_txt).create();
@@ -271,9 +288,14 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 		return broken;
 	}
 
-	@SuppressWarnings("unused")
-	private ServletContextHandler addRedirectContext(GitServlet gs,
-			TestRepository<Repository> src, String srcName) {
+	private ServletContextHandler addAuthContext(GitServlet gs,
+			String contextPath, String... methods) {
+		ServletContextHandler auth = server.addContext('/' + contextPath);
+		auth.addServlet(new ServletHolder(gs), "/*");
+		return server.authBasic(auth, methods);
+	}
+
+	private ServletContextHandler addRedirectContext(GitServlet gs) {
 		ServletContextHandler redirect = server.addContext("/redirect");
 		redirect.addFilter(new FilterHolder(new Filter() {
 
@@ -283,6 +305,11 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 			private Pattern responsePattern = Pattern
 					.compile("/response/(\\d+)/(30[1237])/");
 
+			// Enables tests to specify the context that the request should be
+			// redirected to in the end. If not present, redirects got to the
+			// normal /git context.
+			private Pattern targetPattern = Pattern.compile("/target(/\\w+)/");
+
 			@Override
 			public void init(FilterConfig filterConfig)
 					throws ServletException {
@@ -322,18 +349,25 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 							.parseUnsignedInt(matcher.group(1));
 					responseCode = Integer.parseUnsignedInt(matcher.group(2));
 					if (--nofRedirects <= 0) {
-						urlString = fullUrl.substring(0, matcher.start()) + '/'
-								+ fullUrl.substring(matcher.end());
+						urlString = urlString.substring(0, matcher.start())
+								+ '/' + urlString.substring(matcher.end());
 					} else {
-						urlString = fullUrl.substring(0, matcher.start())
+						urlString = urlString.substring(0, matcher.start())
 								+ "/response/" + nofRedirects + "/"
 								+ responseCode + '/'
-								+ fullUrl.substring(matcher.end());
+								+ urlString.substring(matcher.end());
 					}
 				}
 				httpServletResponse.setStatus(responseCode);
 				if (nofRedirects <= 0) {
-					urlString = urlString.replace("/redirect", "/git");
+					String targetContext = "/git";
+					matcher = targetPattern.matcher(urlString);
+					if (matcher.find()) {
+						urlString = urlString.substring(0, matcher.start())
+								+ '/' + urlString.substring(matcher.end());
+						targetContext = matcher.group(1);
+					}
+					urlString = urlString.replace("/redirect", targetContext);
 				}
 				httpServletResponse.setHeader(HttpSupport.HDR_LOCATION,
 						urlString);
@@ -669,6 +703,215 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 	}
 
 	@Test
+	public void testInitialClone_WithAuthentication() throws Exception {
+		Repository dst = createBareRepository();
+		assertFalse(dst.hasObject(A_txt));
+
+		try (Transport t = Transport.open(dst, authURI)) {
+			t.setCredentialsProvider(testCredentials);
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+		}
+
+		assertTrue(dst.hasObject(A_txt));
+		assertEquals(B, dst.exactRef(master).getObjectId());
+		fsck(dst, B);
+
+		List<AccessEvent> requests = getRequests();
+		assertEquals(3, requests.size());
+
+		AccessEvent info = requests.get(0);
+		assertEquals("GET", info.getMethod());
+		assertEquals(401, info.getStatus());
+
+		info = requests.get(1);
+		assertEquals("GET", info.getMethod());
+		assertEquals(join(authURI, "info/refs"), info.getPath());
+		assertEquals(1, info.getParameters().size());
+		assertEquals("git-upload-pack", info.getParameter("service"));
+		assertEquals(200, info.getStatus());
+		assertEquals("application/x-git-upload-pack-advertisement",
+				info.getResponseHeader(HDR_CONTENT_TYPE));
+		assertEquals("gzip", info.getResponseHeader(HDR_CONTENT_ENCODING));
+
+		AccessEvent service = requests.get(2);
+		assertEquals("POST", service.getMethod());
+		assertEquals(join(authURI, "git-upload-pack"), service.getPath());
+		assertEquals(0, service.getParameters().size());
+		assertNotNull("has content-length",
+				service.getRequestHeader(HDR_CONTENT_LENGTH));
+		assertNull("not chunked",
+				service.getRequestHeader(HDR_TRANSFER_ENCODING));
+
+		assertEquals(200, service.getStatus());
+		assertEquals("application/x-git-upload-pack-result",
+				service.getResponseHeader(HDR_CONTENT_TYPE));
+	}
+
+	@Test
+	public void testInitialClone_WithAuthenticationNoCredentials()
+			throws Exception {
+		Repository dst = createBareRepository();
+		assertFalse(dst.hasObject(A_txt));
+
+		try (Transport t = Transport.open(dst, authURI)) {
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+			fail("Should not have succeeded -- no authentication");
+		} catch (TransportException e) {
+			String msg = e.getMessage();
+			assertTrue("Unexpected exception message: " + msg,
+					msg.contains("no CredentialsProvider"));
+		}
+		List<AccessEvent> requests = getRequests();
+		assertEquals(1, requests.size());
+
+		AccessEvent info = requests.get(0);
+		assertEquals("GET", info.getMethod());
+		assertEquals(401, info.getStatus());
+	}
+
+	@Test
+	public void testInitialClone_WithAuthenticationWrongCredentials()
+			throws Exception {
+		Repository dst = createBareRepository();
+		assertFalse(dst.hasObject(A_txt));
+
+		try (Transport t = Transport.open(dst, authURI)) {
+			t.setCredentialsProvider(new UsernamePasswordCredentialsProvider(
+					AppServer.username, "wrongpassword"));
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+			fail("Should not have succeeded -- wrong password");
+		} catch (TransportException e) {
+			String msg = e.getMessage();
+			assertTrue("Unexpected exception message: " + msg,
+					msg.contains("auth"));
+		}
+		List<AccessEvent> requests = getRequests();
+		// Once without authentication plus three re-tries with authentication
+		assertEquals(4, requests.size());
+
+		for (AccessEvent event : requests) {
+			assertEquals("GET", event.getMethod());
+			assertEquals(401, event.getStatus());
+		}
+	}
+
+	@Test
+	public void testInitialClone_WithAuthenticationAfterRedirect()
+			throws Exception {
+		Repository dst = createBareRepository();
+		assertFalse(dst.hasObject(A_txt));
+
+		URIish cloneFrom = extendPath(redirectURI, "/target/auth");
+		CredentialsProvider uriSpecificCredentialsProvider = new UsernamePasswordCredentialsProvider(
+				"unknown", "none") {
+			@Override
+			public boolean get(URIish uri, CredentialItem... items)
+					throws UnsupportedCredentialItem {
+				// Only return the true credentials if the uri path starts with
+				// /auth. This ensures that we do provide the correct
+				// credentials only for the URi after the redirect, making the
+				// test fail if we should be asked for the credentials for the
+				// original URI.
+				if (uri.getPath().startsWith("/auth")) {
+					return testCredentials.get(uri, items);
+				}
+				return super.get(uri, items);
+			}
+		};
+		try (Transport t = Transport.open(dst, cloneFrom)) {
+			t.setCredentialsProvider(uriSpecificCredentialsProvider);
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+		}
+
+		assertTrue(dst.hasObject(A_txt));
+		assertEquals(B, dst.exactRef(master).getObjectId());
+		fsck(dst, B);
+
+		List<AccessEvent> requests = getRequests();
+		assertEquals(4, requests.size());
+
+		AccessEvent redirect = requests.get(0);
+		assertEquals("GET", redirect.getMethod());
+		assertEquals(join(cloneFrom, "info/refs"), redirect.getPath());
+		assertEquals(301, redirect.getStatus());
+
+		AccessEvent info = requests.get(1);
+		assertEquals("GET", info.getMethod());
+		assertEquals(join(authURI, "info/refs"), info.getPath());
+		assertEquals(401, info.getStatus());
+
+		info = requests.get(2);
+		assertEquals("GET", info.getMethod());
+		assertEquals(join(authURI, "info/refs"), info.getPath());
+		assertEquals(1, info.getParameters().size());
+		assertEquals("git-upload-pack", info.getParameter("service"));
+		assertEquals(200, info.getStatus());
+		assertEquals("application/x-git-upload-pack-advertisement",
+				info.getResponseHeader(HDR_CONTENT_TYPE));
+		assertEquals("gzip", info.getResponseHeader(HDR_CONTENT_ENCODING));
+
+		AccessEvent service = requests.get(3);
+		assertEquals("POST", service.getMethod());
+		assertEquals(join(authURI, "git-upload-pack"), service.getPath());
+		assertEquals(0, service.getParameters().size());
+		assertNotNull("has content-length",
+				service.getRequestHeader(HDR_CONTENT_LENGTH));
+		assertNull("not chunked",
+				service.getRequestHeader(HDR_TRANSFER_ENCODING));
+
+		assertEquals(200, service.getStatus());
+		assertEquals("application/x-git-upload-pack-result",
+				service.getResponseHeader(HDR_CONTENT_TYPE));
+	}
+
+	@Test
+	public void testInitialClone_WithAuthenticationOnPostOnly()
+			throws Exception {
+		Repository dst = createBareRepository();
+		assertFalse(dst.hasObject(A_txt));
+
+		try (Transport t = Transport.open(dst, authOnPostURI)) {
+			t.setCredentialsProvider(testCredentials);
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+		}
+
+		assertTrue(dst.hasObject(A_txt));
+		assertEquals(B, dst.exactRef(master).getObjectId());
+		fsck(dst, B);
+
+		List<AccessEvent> requests = getRequests();
+		assertEquals(3, requests.size());
+
+		AccessEvent info = requests.get(0);
+		assertEquals("GET", info.getMethod());
+		assertEquals(join(authOnPostURI, "info/refs"), info.getPath());
+		assertEquals(1, info.getParameters().size());
+		assertEquals("git-upload-pack", info.getParameter("service"));
+		assertEquals(200, info.getStatus());
+		assertEquals("application/x-git-upload-pack-advertisement",
+				info.getResponseHeader(HDR_CONTENT_TYPE));
+		assertEquals("gzip", info.getResponseHeader(HDR_CONTENT_ENCODING));
+
+		AccessEvent service = requests.get(1);
+		assertEquals("POST", service.getMethod());
+		assertEquals(join(authOnPostURI, "git-upload-pack"), service.getPath());
+		assertEquals(401, service.getStatus());
+
+		service = requests.get(2);
+		assertEquals("POST", service.getMethod());
+		assertEquals(join(authOnPostURI, "git-upload-pack"), service.getPath());
+		assertEquals(0, service.getParameters().size());
+		assertNotNull("has content-length",
+				service.getRequestHeader(HDR_CONTENT_LENGTH));
+		assertNull("not chunked",
+				service.getRequestHeader(HDR_TRANSFER_ENCODING));
+
+		assertEquals(200, service.getStatus());
+		assertEquals("application/x-git-upload-pack-result",
+				service.getResponseHeader(HDR_CONTENT_TYPE));
+	}
+
+	@Test
 	public void testFetch_FewLocalCommits() throws Exception {
 		// Bootstrap by doing the clone.
 		//