From 7ac1bfc834fe65b7e86e8f54f1f5025df90f8a92 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 16 Jun 2017 10:25:53 +0200 Subject: [PATCH] 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 Signed-off-by: Matthias Sohn --- .../META-INF/MANIFEST.MF | 2 + .../http/test/SmartClientSmartServerTest.java | 261 +++++++++++++++++- .../eclipse/jgit/junit/http/AppServer.java | 39 ++- .../eclipse/jgit/internal/JGitText.properties | 5 +- .../org/eclipse/jgit/internal/JGitText.java | 1 + .../eclipse/jgit/transport/TransportHttp.java | 142 ++++++++-- 6 files changed, 407 insertions(+), 43 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 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); @@ -668,6 +702,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 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 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 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 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 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. diff --git a/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java b/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java index 69e2cd5957..e257cf65b6 100644 --- a/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java +++ b/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java @@ -55,6 +55,7 @@ import java.net.UnknownHostException; import java.nio.file.Files; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -97,6 +98,9 @@ public class AppServer { /** SSL keystore password; must have at least 6 characters. */ private static final String keyPassword = "mykeys"; + /** Role for authentication. */ + private static final String authRole = "can-access"; + static { // Install a logger that throws warning messages. // @@ -136,10 +140,10 @@ public class AppServer { /** * @param port - * for https, may be zero to allocate a port dynamically + * for http, may be zero to allocate a port dynamically * @param sslPort * for https,may be zero to allocate a port dynamically. If - * negative, the server will be set up without https support.. + * negative, the server will be set up without https support. * @since 4.9 */ public AppServer(int port, int sslPort) { @@ -264,9 +268,10 @@ public class AppServer { return ctx; } - public ServletContextHandler authBasic(ServletContextHandler ctx) { + public ServletContextHandler authBasic(ServletContextHandler ctx, + String... methods) { assertNotYetSetUp(); - auth(ctx, new BasicAuthenticator()); + auth(ctx, new BasicAuthenticator(), methods); return ctx; } @@ -301,22 +306,36 @@ public class AppServer { } } - private void auth(ServletContextHandler ctx, Authenticator authType) { - final String role = "can-access"; - - AbstractLoginService users = new TestMappedLoginService(role); + private ConstraintMapping createConstraintMapping() { ConstraintMapping cm = new ConstraintMapping(); cm.setConstraint(new Constraint()); cm.getConstraint().setAuthenticate(true); cm.getConstraint().setDataConstraint(Constraint.DC_NONE); - cm.getConstraint().setRoles(new String[] { role }); + cm.getConstraint().setRoles(new String[] { authRole }); cm.setPathSpec("/*"); + return cm; + } + + private void auth(ServletContextHandler ctx, Authenticator authType, + String... methods) { + AbstractLoginService users = new TestMappedLoginService(authRole); + List mappings = new ArrayList<>(); + if (methods == null || methods.length == 0) { + mappings.add(createConstraintMapping()); + } else { + for (String method : methods) { + ConstraintMapping cm = createConstraintMapping(); + cm.setMethod(method.toUpperCase(Locale.ROOT)); + mappings.add(cm); + } + } ConstraintSecurityHandler sec = new ConstraintSecurityHandler(); sec.setRealmName(realm); sec.setAuthenticator(authType); sec.setLoginService(users); - sec.setConstraintMappings(new ConstraintMapping[] { cm }); + sec.setConstraintMappings( + mappings.toArray(new ConstraintMapping[mappings.size()])); sec.setHandler(ctx); contexts.removeHandler(ctx); diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index bf2a4a2e0f..c532b328db 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -365,7 +365,7 @@ invalidPathContainsSeparator=Invalid path (contains separator ''{0}''): {1} invalidPathPeriodAtEndWindows=Invalid path (period at end is ignored by Windows): {0} invalidPathSpaceAtEndWindows=Invalid path (space at end is ignored by Windows): {0} invalidPathReservedOnWindows=Invalid path (''{0}'' is reserved on Windows): {1} -invalidRedirectLocation=Redirect or URI ''{0}'': invalid redirect location {1} -> {2} +invalidRedirectLocation=Invalid redirect location {1} -> {2} invalidReflogRevision=Invalid reflog revision: {0} invalidRefName=Invalid ref name: {0} invalidReftableBlock=Invalid reftable block @@ -587,7 +587,7 @@ secondsAgo={0} seconds ago selectingCommits=Selecting commits sequenceTooLargeForDiffAlgorithm=Sequence too large for difference algorithm. serviceNotEnabledNoName=Service not enabled -serviceNotPermitted={0} not permitted +serviceNotPermitted={1} not permitted on ''{0}'' sha1CollisionDetected1=SHA-1 collision detected on {0} shallowCommitsAlreadyInitialized=Shallow commits have already been initialized shallowPacksRequireDepthWalk=Shallow packs require a DepthWalk @@ -636,6 +636,7 @@ timeIsUncertain=Time is uncertain timerAlreadyTerminated=Timer already terminated tooManyCommands=Too many commands tooManyIncludeRecursions=Too many recursions; circular includes in config file(s)? +tooManyRedirects=Too many redirects; stopped after {0} redirects at ''{1}'' topologicalSortRequired=Topological sort required. transactionAborted=transaction aborted transportExceptionBadRef=Empty ref: {0}: {1} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 07666eb934..5acf058764 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -696,6 +696,7 @@ public class JGitText extends TranslationBundle { /***/ public String timerAlreadyTerminated; /***/ public String tooManyCommands; /***/ public String tooManyIncludeRecursions; + /***/ public String tooManyRedirects; /***/ public String topologicalSortRequired; /***/ public String transportExceptionBadRef; /***/ public String transportExceptionEmptyRef; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java index 327dc39c7f..1647200e65 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java @@ -323,6 +323,13 @@ public class TransportHttp extends HttpTransport implements WalkTransport, } } + /** + * The current URI we're talking to. The inherited (final) field + * {@link #uri} stores the original URI; {@code currentUri} may be different + * after redirects. + */ + private URIish currentUri; + private URL baseUrl; private URL objectsUrl; @@ -360,6 +367,7 @@ public class TransportHttp extends HttpTransport implements WalkTransport, */ protected void setURI(final URIish uri) throws NotSupportedException { try { + currentUri = uri; baseUrl = toURL(uri); objectsUrl = new URL(baseUrl, "objects/"); //$NON-NLS-1$ } catch (MalformedURLException e) { @@ -584,9 +592,10 @@ public class TransportHttp extends HttpTransport implements WalkTransport, throw new TransportException(uri, JGitText.get().noCredentialsProvider); if (authAttempts > 1) - credentialsProvider.reset(uri); + credentialsProvider.reset(currentUri); if (3 < authAttempts - || !authMethod.authorize(uri, credentialsProvider)) { + || !authMethod.authorize(currentUri, + credentialsProvider)) { throw new TransportException(uri, JGitText.get().notAuthorized); } @@ -1096,8 +1105,17 @@ public class TransportHttp extends HttpTransport implements WalkTransport, buf = out; } + HttpAuthMethod authenticator = null; + Collection ignoreTypes = EnumSet.noneOf(Type.class); + // Counts number of repeated authentication attempts using the same + // authentication scheme + int authAttempts = 1; int redirects = 0; for (;;) { + // The very first time we will try with the authentication + // method used on the initial GET request. This is a hint only; + // it may fail. If so, we'll then re-try with proper 401 + // handling, going through the available authentication schemes. openStream(); if (buf != out) { conn.setRequestProperty(HDR_CONTENT_ENCODING, ENCODING_GZIP); @@ -1107,31 +1125,111 @@ public class TransportHttp extends HttpTransport implements WalkTransport, buf.writeTo(httpOut, null); } - if (http.followRedirects == HttpRedirectMode.TRUE) { - final int status = HttpSupport.response(conn); - switch (status) { - case HttpConnection.HTTP_MOVED_PERM: - case HttpConnection.HTTP_MOVED_TEMP: - case HttpConnection.HTTP_11_MOVED_TEMP: - // SEE_OTHER after a POST doesn't make sense for a git - // server, so we don't handle it here and thus we'll - // report an error in openResponse() later on. - URIish newUri = redirect( - conn.getHeaderField(HDR_LOCATION), - '/' + serviceName, redirects++); - try { - baseUrl = toURL(newUri); - } catch (MalformedURLException e) { - throw new TransportException(MessageFormat.format( - JGitText.get().invalidRedirectLocation, - uri, baseUrl, newUri), e); + final int status = HttpSupport.response(conn); + switch (status) { + case HttpConnection.HTTP_OK: + // We're done. + return; + + case HttpConnection.HTTP_NOT_FOUND: + throw new NoRemoteRepositoryException(uri, MessageFormat + .format(JGitText.get().uriNotFound, conn.getURL())); + + case HttpConnection.HTTP_FORBIDDEN: + throw new TransportException(uri, + MessageFormat.format( + JGitText.get().serviceNotPermitted, + baseUrl, serviceName)); + + case HttpConnection.HTTP_MOVED_PERM: + case HttpConnection.HTTP_MOVED_TEMP: + case HttpConnection.HTTP_11_MOVED_TEMP: + // SEE_OTHER after a POST doesn't make sense for a git + // server, so we don't handle it here and thus we'll + // report an error in openResponse() later on. + if (http.followRedirects != HttpRedirectMode.TRUE) { + // Let openResponse() issue an error + return; + } + currentUri = redirect( + conn.getHeaderField(HDR_LOCATION), + '/' + serviceName, redirects++); + try { + baseUrl = toURL(currentUri); + } catch (MalformedURLException e) { + throw new TransportException(uri, MessageFormat.format( + JGitText.get().invalidRedirectLocation, + baseUrl, currentUri), e); + } + continue; + + case HttpConnection.HTTP_UNAUTHORIZED: + HttpAuthMethod nextMethod = HttpAuthMethod + .scanResponse(conn, ignoreTypes); + switch (nextMethod.getType()) { + case NONE: + throw new TransportException(uri, + MessageFormat.format( + JGitText.get().authenticationNotSupported, + conn.getURL())); + case NEGOTIATE: + // RFC 4559 states "When using the SPNEGO [...] with + // [...] POST, the authentication should be complete + // [...] before sending the user data." So in theory + // the initial GET should have been authenticated + // already. (Unless there was a redirect?) + // + // We try this only once: + ignoreTypes.add(HttpAuthMethod.Type.NEGOTIATE); + if (authenticator != null) { + ignoreTypes.add(authenticator.getType()); } - continue; + authAttempts = 1; + // We only do the Kerberos part of SPNEGO, which + // requires only one attempt. We do *not* to the + // NTLM part of SPNEGO; it's a multi-round + // negotiation and among other problems it would + // be unclear when to stop if no HTTP_OK is + // forthcoming. In theory a malicious server + // could keep sending requests for another NTLM + // round, keeping a client stuck here. + break; default: + // DIGEST or BASIC. Let's be sure we ignore NEGOTIATE; + // if it was available, we have tried it before. + ignoreTypes.add(HttpAuthMethod.Type.NEGOTIATE); + if (authenticator == null || authenticator + .getType() != nextMethod.getType()) { + if (authenticator != null) { + ignoreTypes.add(authenticator.getType()); + } + authAttempts = 1; + } break; } + authMethod = nextMethod; + authenticator = nextMethod; + CredentialsProvider credentialsProvider = getCredentialsProvider(); + if (credentialsProvider == null) { + throw new TransportException(uri, + JGitText.get().noCredentialsProvider); + } + if (authAttempts > 1) { + credentialsProvider.reset(currentUri); + } + if (3 < authAttempts || !authMethod.authorize(currentUri, + credentialsProvider)) { + throw new TransportException(uri, + JGitText.get().notAuthorized); + } + authAttempts++; + continue; + + default: + // Just return here; openResponse() will report an appropriate + // error. + return; } - break; } } -- 2.39.5