opendev/gerrit
Code Issues Proposed changes

Enable specific AccessPaths based on authentication

Browse Source 
If the authentication method permits an AccessPath, add it to the
WebSession as a permitted path. Downstream from authentication a
CurrentUser can change its access path based on the entry point of
the request, allowing RefControl to make decisions around this as
expected, without running into the race condition of making user
before the real access method can be determined.

This allows authentication systems to decide on their own if the
REST_API was sufficiently protected from a potentially evil script.

Change-Id: Iefbe6745421f5f438bc06e2e4578a7207718b9a5
This commit is contained in:
Shawn O. Pearce
2012-11-14 10:55:26 -08:00
parent f7d96cbb03
commit 0185d428db
18 changed files with 107 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
gerrit-httpd/src/main/java/com/google/gerrit/httpd/CacheBasedWebSession.java
View File

@@ -35,6 +35,8 @@ import com.google.inject.servlet.RequestScoped;
import org.eclipse.jgit.http.server.GitSmartHttpTools;
import java.util.EnumSet;
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@@ -66,7 +68,7 @@ public final class CacheBasedWebSession implements WebSession {
private final AuthConfig authConfig;
private final Provider<AnonymousUser> anonymousProvider;
private final IdentifiedUser.RequestFactory identified;
private AccessPath accessPath;
private final EnumSet<AccessPath> okPaths = EnumSet.of(AccessPath.UNKNOWN);
private Cookie outCookie;
private Key key;
@@ -85,36 +87,31 @@ public final class CacheBasedWebSession implements WebSession {
this.anonymousProvider = anonymousProvider;
this.identified = identified;
String cookie = request.getHeader("Authorization");
if (cookie != null && cookie.startsWith("Bearer ")) {
cookie = cookie.substring("Bearer ".length());
accessPath = AccessPath.REST_API;
} else if (cookie != null && GitSmartHttpTools.isGitClient(request)) {
accessPath = AccessPath.GIT;
if (!GitSmartHttpTools.isGitClient(request)) {
String cookie = readCookie();
String token = request.getHeader("Authorization");
if (token != null && token.startsWith("Bearer ")) {
token = token.substring("Bearer ".length());
okPaths.add(AccessPath.REST_API);
} else {
cookie = readCookie();
accessPath = AccessPath.WEB_BROWSER;
token = cookie;
}
if (cookie != null) {
key = new Key(cookie);
if (token != null) {
key = new Key(token);
val = manager.get(key);
} else {
key = null;
val = null;
}
if (isSignedIn() && val.needsCookieRefresh()) {
// Cookie is more than half old. Send the cookie again to the
// client with an updated expiration date. We don't dare to
// change the key token here because there may be other RPCs
// queued up in the browser whose xsrfKey would not get updated
// with the new token, causing them to fail.
//
// client with an updated expiration date.
val = manager.createVal(key, val);
if (cookie != null) {
saveCookie();
}
}
}
}
}
private String readCookie() {
final Cookie[] all = request.getCookies();
@@ -129,10 +126,12 @@ public final class CacheBasedWebSession implements WebSession {
return null;
}
@Override
public boolean isSignedIn() {
return val != null;
}
@Override
public String getAuthorization() {
return isSignedIn() ? "Bearer " + key.getToken() : null;
}
@@ -142,17 +141,34 @@ public final class CacheBasedWebSession implements WebSession {
return keyIn.equals(getAuthorization());
}
@Override
public boolean isAccessPathOk(AccessPath path) {
return okPaths.contains(path);
}
@Override
public void setAccessPathOk(AccessPath path, boolean ok) {
if (ok) {
okPaths.add(path);
} else {
okPaths.remove(path);
}
}
@Override
public AccountExternalId.Key getLastLoginExternalId() {
return val != null ? val.getExternalId() : null;
}
@Override
public CurrentUser getCurrentUser() {
if (isSignedIn()) {
return identified.create(accessPath, val.getAccountId());
return identified.create(val.getAccountId());
}
return anonymousProvider.get();
}
@Override
public void login(final AuthResult res, final boolean rememberMe) {
final Account.Id id = res.getAccountId();
final AccountExternalId.Key identity = res.getExternalId();
@@ -167,11 +183,13 @@ public final class CacheBasedWebSession implements WebSession {
}
/** Set the user account for this current request only. */
@Override
public void setUserAccountId(Account.Id id) {
key = new Key("id:" + id);
val = new Val(id, 0, false, null, 0, null);
}
@Override
public void logout() {
if (val != null) {
manager.destroy(key);

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/ContainerAuthFilter.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.httpd;
import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -99,7 +100,10 @@ class ContainerAuthFilter implements Filter {
rsp.sendError(SC_UNAUTHORIZED);
return false;
}
session.get().setUserAccountId(who.getAccount().getId());
WebSession ws = session.get();
ws.setUserAccountId(who.getAccount().getId());
ws.setAccessPathOk(AccessPath.GIT, true);
ws.setAccessPathOk(AccessPath.REST_API, true);
return true;
}
}

8
gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java
View File

@@ -18,7 +18,9 @@ import com.google.common.cache.Cache;
import com.google.gerrit.common.data.Capable;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.git.AsyncReceiveCommits;
@@ -157,8 +159,12 @@ public class GitOverHttpServlet extends GitServlet {
} catch (NoSuchProjectException err) {
throw new RepositoryNotFoundException(projectName);
}
CurrentUser user = pc.getCurrentUser();
user.setAccessPath(AccessPath.GIT);
if (!pc.isVisible()) {
if (pc.getCurrentUser() instanceof AnonymousUser) {
if (user instanceof AnonymousUser) {
throw new ServiceNotAuthorizedException();
} else {
throw new ServiceNotEnabledException();

9
gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java
View File

@@ -18,6 +18,7 @@ import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import com.google.common.base.Objects;
import com.google.common.base.Strings;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountException;
import com.google.gerrit.server.account.AccountManager;
@@ -103,10 +104,9 @@ class ProjectBasicAuthFilter implements Filter {
private boolean verify(HttpServletRequest req, Response rsp)
throws IOException {
final String hdr = req.getHeader(AUTHORIZATION);
if (hdr == null) {
if (hdr == null || !hdr.startsWith(LIT_BASIC)) {
// Allow an anonymous connection through, or it might be using a
// session cookie instead of basic authentication.
//
return true;
}
@@ -142,7 +142,10 @@ class ProjectBasicAuthFilter implements Filter {
try {
AuthResult whoAuthResult = accountManager.authenticate(whoAuth);
session.get().setUserAccountId(whoAuthResult.getAccountId());
WebSession ws = session.get();
ws.setUserAccountId(whoAuthResult.getAccountId());
ws.setAccessPathOk(AccessPath.GIT, true);
ws.setAccessPathOk(AccessPath.REST_API, true);
return true;
} catch (AccountException e) {
log.warn("Authentication failed for " + username, e);

15
gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectDigestFilter.java
View File

@@ -20,6 +20,7 @@ import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -109,10 +110,9 @@ class ProjectDigestFilter implements Filter {
private boolean verify(HttpServletRequest req, Response rsp)
throws IOException {
final String hdr = req.getHeader(AUTHORIZATION);
if (hdr == null) {
if (hdr == null || !hdr.startsWith("Digest ")) {
// Allow an anonymous connection through, or it might be using a
// session cookie instead of digest authentication.
//
return true;
}
@@ -164,7 +164,10 @@ class ProjectDigestFilter implements Filter {
if (expect.equals(response)) {
try {
if (tokens.checkToken(nonce, "") != null) {
session.get().setUserAccountId(who.getAccount().getId());
WebSession ws = session.get();
ws.setUserAccountId(who.getAccount().getId());
ws.setAccessPathOk(AccessPath.GIT, true);
ws.setAccessPathOk(AccessPath.REST_API, true);
return true;
} else {
@@ -229,12 +232,6 @@ class ProjectDigestFilter implements Filter {
}
private Map<String, String> parseAuthorization(String auth) {
if (!auth.startsWith("Digest ")) {
// We only support Digest authentication scheme, deny the rest.
//
return Collections.emptyMap();
}
Map<String, String> p = new HashMap<String, String>();
int next = "Digest ".length();
while (next < auth.length()) {

9
gerrit-httpd/src/main/java/com/google/gerrit/httpd/WebSession.java
View File

@@ -16,26 +16,23 @@ package com.google.gerrit.httpd;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AuthResult;
public interface WebSession {
public boolean isSignedIn();
public String getAuthorization();
public boolean isValidAuthorization(String keyIn);
public AccountExternalId.Key getLastLoginExternalId();
public CurrentUser getCurrentUser();
public void login(AuthResult res, boolean rememberMe);
/** Set the user account for this current request only. */
public void setUserAccountId(Account.Id id);
public boolean isAccessPathOk(AccessPath path);
public void setAccessPathOk(AccessPath path, boolean ok);
public void logout();
public String getSessionId();
}

2
gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java
View File

@@ -30,7 +30,7 @@ import java.util.Set;
public class AnonymousUser extends CurrentUser {
@Inject
AnonymousUser(CapabilityControl.Factory capabilityControlFactory) {
super(capabilityControlFactory, AccessPath.UNKNOWN);
super(capabilityControlFactory);
}
@Override

11
gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
View File

@@ -33,15 +33,12 @@ import java.util.Set;
*/
public abstract class CurrentUser {
private final CapabilityControl.Factory capabilityControlFactory;
private final AccessPath accessPath;
private AccessPath accessPath = AccessPath.UNKNOWN;
private CapabilityControl capabilities;
protected CurrentUser(
CapabilityControl.Factory capabilityControlFactory,
AccessPath accessPath) {
protected CurrentUser(CapabilityControl.Factory capabilityControlFactory) {
this.capabilityControlFactory = capabilityControlFactory;
this.accessPath = accessPath;
}
/** How this user is accessing the Gerrit Code Review application. */
@@ -49,6 +46,10 @@ public abstract class CurrentUser {
return accessPath;
}
public void setAccessPath(AccessPath path) {
accessPath = path;
}
/**
* Get the set of groups the user is currently a member of.
* <p>

20
gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
View File

@@ -38,6 +38,7 @@ import com.google.inject.Inject;
import com.google.inject.OutOfScopeException;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.google.inject.util.Providers;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.util.SystemReader;
@@ -90,20 +91,19 @@ public class IdentifiedUser extends CurrentUser {
}
public IdentifiedUser create(final Account.Id id) {
return create(AccessPath.UNKNOWN, null, id);
return create((SocketAddress) null, id);
}
public IdentifiedUser create(Provider<ReviewDb> db, Account.Id id) {
return new IdentifiedUser(capabilityControlFactory, AccessPath.UNKNOWN,
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
groupBackend, null, db, id);
}
public IdentifiedUser create(AccessPath accessPath,
Provider<SocketAddress> remotePeerProvider, Account.Id id) {
return new IdentifiedUser(capabilityControlFactory, accessPath,
public IdentifiedUser create(SocketAddress remotePeer, Account.Id id) {
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
groupBackend, remotePeerProvider, null, id);
groupBackend, Providers.of(remotePeer), null, id);
}
}
@@ -149,9 +149,8 @@ public class IdentifiedUser extends CurrentUser {
this.dbProvider = dbProvider;
}
public IdentifiedUser create(final AccessPath accessPath,
final Account.Id id) {
return new IdentifiedUser(capabilityControlFactory, accessPath,
public IdentifiedUser create(Account.Id id) {
return new IdentifiedUser(capabilityControlFactory,
authConfig, anonymousCowardName, canonicalUrl, realm, accountCache,
groupBackend, remotePeerProvider, dbProvider, id);
}
@@ -187,7 +186,6 @@ public class IdentifiedUser extends CurrentUser {
private IdentifiedUser(
CapabilityControl.Factory capabilityControlFactory,
final AccessPath accessPath,
final AuthConfig authConfig,
final String anonymousCowardName,
final Provider<String> canonicalUrl,
@@ -195,7 +193,7 @@ public class IdentifiedUser extends CurrentUser {
final GroupBackend groupBackend,
@Nullable final Provider<SocketAddress> remotePeerProvider,
@Nullable final Provider<ReviewDb> dbProvider, final Account.Id id) {
super(capabilityControlFactory, accessPath);
super(capabilityControlFactory);
this.canonicalUrl = canonicalUrl;
this.accountCache = accountCache;
this.groupBackend = groupBackend;

2
gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
View File

@@ -39,7 +39,7 @@ public class InternalUser extends CurrentUser {
@Inject
protected InternalUser(CapabilityControl.Factory capabilityControlFactory) {
super(capabilityControlFactory, AccessPath.UNKNOWN);
super(capabilityControlFactory);
}
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java
View File

@@ -40,7 +40,7 @@ public class PeerDaemonUser extends CurrentUser {
@Inject
protected PeerDaemonUser(CapabilityControl.Factory capabilityControlFactory,
@Assisted SocketAddress peer) {
super(capabilityControlFactory, AccessPath.SSH_COMMAND);
super(capabilityControlFactory);
this.peer = peer;
}

3
gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.query.change;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
@@ -37,7 +36,7 @@ public final class SingleGroupUser extends CurrentUser {
public SingleGroupUser(CapabilityControl.Factory capabilityControlFactory,
Set<AccountGroup.UUID> groups) {
super(capabilityControlFactory, AccessPath.UNKNOWN);
super(capabilityControlFactory);
this.groups = new ListGroupMembership(groups);
}

3
gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
View File

@@ -33,7 +33,6 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.RulesCache;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
@@ -415,7 +414,7 @@ public class RefControlTest extends TestCase {
private final GroupMembership groups;
MockUser(String name, AccountGroup.UUID[] groupId) {
super(RefControlTest.this.capabilityControlFactory, AccessPath.UNKNOWN);
super(RefControlTest.this.capabilityControlFactory);
username = name;
ArrayList<AccountGroup.UUID> groupIds = Lists.newArrayList(groupId);
groupIds.add(registered);

13
gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java
View File

@@ -21,7 +21,6 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.sshd.SshScope.Context;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.apache.sshd.server.Environment;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -29,7 +28,6 @@ import org.eclipse.jgit.lib.Repository;
import org.kohsuke.args4j.Argument;
import java.io.IOException;
import java.net.SocketAddress;
public abstract class AbstractGitCommand extends BaseCommand {
@Argument(index = 0, metaVar = "PROJECT.git", required = true, usage = "project name")
@@ -84,13 +82,10 @@ public abstract class AbstractGitCommand extends BaseCommand {
}
private SshSession newSession() {
return new SshSession(session, session.getRemoteAddress(), userFactory
.create(AccessPath.GIT, new Provider<SocketAddress>() {
@Override
public SocketAddress get() {
return session.getRemoteAddress();
}
}, user.getAccountId()));
SshSession n = new SshSession(session, session.getRemoteAddress(),
userFactory.create(session.getRemoteAddress(), user.getAccountId()));
n.setAccessPath(AccessPath.GIT);
return n;
}
private void service() throws IOException, Failure {

11
gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.sshd;
import com.google.gerrit.reviewdb.client.AccountSshKey;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PeerDaemonUser;
@@ -23,7 +22,6 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.sshd.SshScope.Context;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.apache.commons.codec.binary.Base64;
@@ -43,7 +41,6 @@ import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.net.SocketAddress;
import java.security.KeyPair;
import java.security.PublicKey;
import java.util.Collection;
@@ -201,13 +198,7 @@ class DatabasePubKeyAuth implements PublickeyAuthenticator {
private IdentifiedUser createUser(final SshSession sd,
final SshKeyCacheEntry key) {
return userFactory.create(AccessPath.SSH_COMMAND,
new Provider<SocketAddress>() {
@Override
public SocketAddress get() {
return sd.getRemoteAddress();
}
}, key.getAccount());
return userFactory.create(sd.getRemoteAddress(), key.getAccount());
}
private SshKeyCacheEntry find(final Iterable<SshKeyCacheEntry> keyList,

3
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshScope.java
View File

@@ -73,8 +73,7 @@ class SshScope {
public CurrentUser getCurrentUser() {
final CurrentUser user = session.getCurrentUser();
if (user instanceof IdentifiedUser) {
return userFactory.create(user.getAccessPath(), //
((IdentifiedUser) user).getAccountId());
return userFactory.create(((IdentifiedUser) user).getAccountId());
}
return user;
}

6
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshSession.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.sshd;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import org.apache.sshd.common.Session.AttributeKey;
@@ -43,6 +44,7 @@ public class SshSession {
}
SshSession(SshSession parent, SocketAddress peer, CurrentUser user) {
user.setAccessPath(AccessPath.SSH_COMMAND);
this.sessionId = parent.sessionId;
this.remoteAddress = peer;
if (parent.remoteAddress == peer) {
@@ -83,6 +85,10 @@ public class SshSession {
authError = error;
}
void setAccessPath(AccessPath path) {
identity.setAccessPath(path);
}
/** @return {@code true} if the authentication did not succeed. */
boolean isAuthenticationError() {
return authError != null;

11
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
View File

@@ -16,7 +16,6 @@ package com.google.gerrit.sshd;
import com.google.common.util.concurrent.Atomics;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PeerDaemonUser;
@@ -116,14 +115,8 @@ public final class SuExec extends BaseCommand {
} else {
peer = peerAddress;
}
return new SshSession(session.get(), peer, userFactory.create(
AccessPath.SSH_COMMAND, new Provider<SocketAddress>() {
@Override
public SocketAddress get() {
return peer;
}
}, accountId));
return new SshSession(session.get(), peer,
userFactory.create(peer, accountId));
}
private static String join(List<String> args) {