From 850b1c312ce8c283b2bfd503d7a142f7ca599e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Mon, 9 Dec 2019 17:24:27 +0100 Subject: [PATCH 1/2] Fix handling of interactive/batch users in the QoS filter For the git-over-http requests this filter didn't work properly because the basic authentication happened later in the filter chain and at the moment when the ProjectQoSFilter was invoked the current user was not yet set. Therefore, the current user was always AnonymousUser in this filter and, by default, it always used the interactive queue type. The sshd.batchThreads [1] setting didn't work as documented i.e. to define the max number of Git requests for batch users over SSH and HTTP together. Bind ProjectQoSFilter after the ProjectBasicAuthFilter which is bound from the GitOverHttpModule so that the current user is authenticated when the ProjectQoSFilter is reached. [1] https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#sshd.batchThreads Change-Id: I0a5a70a5ee2d6416b69aa3ab4e4c55640bca8670 --- java/com/google/gerrit/pgm/Daemon.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index b08842ed57..591297335a 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -579,14 +579,14 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList<>(); - if (sshd) { - modules.add(new ProjectQoSFilter.Module()); - } modules.add(RequestContextFilter.module()); modules.add(RequestMetricsFilter.module()); modules.add(H2CacheBasedWebSession.module()); modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); + if (sshd) { + modules.add(new ProjectQoSFilter.Module()); + } modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class)); From 8899c99d9f20dec70154d59ff23faa5bc197d79c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 10 Dec 2019 10:13:41 +0000 Subject: [PATCH 2/2] Validate individual fetch on git protocol v2 Add additional end-to-end test for making sure the fetch of an individual patch-set refs on a secure repository is correctly processed. Bug: Issue 11986 Change-Id: I31057bef73f77ef1f068e9c488b45208d82036a3 --- .../integration/git/GitProtocolV2IT.java | 86 ++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java index 9c4bdef407..8577c16faa 100644 --- a/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java +++ b/javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java @@ -35,6 +35,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.common.ChangeInput; +import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.inject.Inject; @@ -47,12 +48,15 @@ import org.junit.Test; @UseSsh public class GitProtocolV2IT extends StandaloneSiteTest { + private static final String ADMIN_PASSWORD = "secret"; private final String[] SSH_KEYGEN_CMD = new String[] {"ssh-keygen", "-t", "rsa", "-q", "-P", "", "-f"}; private final String[] GIT_LS_REMOTE = new String[] {"git", "-c", "protocol.version=2", "ls-remote", "-o", "trace=12345"}; private final String[] GIT_CLONE_MIRROR = new String[] {"git", "-c", "protocol.version=2", "clone", "--mirror"}; + private final String[] GIT_FETCH = new String[] {"git", "-c", "protocol.version=2", "fetch"}; + private final String[] GIT_INIT = new String[] {"git", "init"}; private final String GIT_SSH_COMMAND = "ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i"; @@ -61,6 +65,7 @@ public class GitProtocolV2IT extends StandaloneSiteTest { @Inject private ProjectOperations projectOperations; @Inject private @TestSshServerAddress InetSocketAddress sshAddress; @Inject private @GerritServerConfig Config config; + @Inject private AllProjectsName allProjectsName; @BeforeClass public static void assertGitClientVersion() throws Exception { @@ -131,7 +136,7 @@ public class GitProtocolV2IT extends StandaloneSiteTest { .commit; // Prepare new change on secret branch - in = new ChangeInput(project.get(), "secret", "Test secret change"); + in = new ChangeInput(project.get(), ADMIN_PASSWORD, "Test secret change"); in.newBranch = true; // Create new change and retrieve SHA1 for the created patch set @@ -257,9 +262,82 @@ public class GitProtocolV2IT extends StandaloneSiteTest { } } + @Test + public void testGitWireProtocolV2FetchIndividualRef() throws Exception { + try (ServerContext ctx = startServer()) { + ctx.getInjector().injectMembers(this); + + // Setup admin password + gApi.accounts().id(admin.username()).setHttpPassword(ADMIN_PASSWORD); + + // Get authenticated Git/HTTP URL + String urlWithCredentials = + config + .getString("gerrit", null, "canonicalweburl") + .replace("http://", "http://" + admin.username() + ":" + ADMIN_PASSWORD + "@"); + + // Create project + Project.NameKey privateProject = Project.nameKey("private-project"); + gApi.projects().create(privateProject.get()); + + // Set protocol.version=2 in target repository + execute( + ImmutableList.of("git", "config", "protocol.version", "2"), + sitePaths + .site_path + .resolve("git") + .resolve(privateProject.get() + Constants.DOT_GIT) + .toFile()); + + // Disallow general read permissions for anonymous users + projectOperations + .project(allProjectsName) + .forUpdate() + .add(deny(Permission.READ).ref("refs/*").group(SystemGroupBackend.ANONYMOUS_USERS)) + .add( + allow(Permission.READ) + .ref("refs/heads/master") + .group(SystemGroupBackend.REGISTERED_USERS)) + .update(); + + // Set up project permission to allow registered users fetching changes/* + projectOperations + .project(privateProject) + .forUpdate() + .add( + allow(Permission.READ) + .ref("refs/changes/*") + .group(SystemGroupBackend.REGISTERED_USERS)) + .update(); + + // Create new change and retrieve refs for the created patch set + ChangeInput visibleChangeIn = + new ChangeInput(privateProject.get(), "master", "Test private change"); + visibleChangeIn.newBranch = true; + int visibleChangeNumber = gApi.changes().create(visibleChangeIn).info()._number; + Change.Id changeId = Change.id(visibleChangeNumber); + String visibleChangeNumberRef = RefNames.patchSetRef(PatchSet.id(changeId, 1)); + + // Fetch a single ref using git wire protocol v2 over HTTP with authentication + execute(GIT_INIT); + + String outFetchRef = + execute( + ImmutableList.builder() + .add(GIT_FETCH) + .add(urlWithCredentials + "/" + privateProject.get()) + .add(visibleChangeNumberRef) + .build(), + ImmutableMap.of("GIT_TRACE_PACKET", "1")); + + assertThat(outFetchRef).contains("git< version 2"); + assertThat(outFetchRef).contains(visibleChangeNumberRef); + } + } + private void setUpUserAuthentication(String username) throws Exception { // Assign HTTP password to user - gApi.accounts().id(username).setHttpPassword("secret"); + gApi.accounts().id(username).setHttpPassword(ADMIN_PASSWORD); // Generate private/public key for user execute( @@ -285,6 +363,10 @@ public class GitProtocolV2IT extends StandaloneSiteTest { assertThat(out).contains(commit); } + private String execute(String... cmds) throws Exception { + return execute(ImmutableList.builder().add(cmds).build()); + } + private String execute(ImmutableList cmd) throws Exception { return execute(cmd, sitePaths.data_dir.toFile(), ImmutableMap.of()); }