Allow effective permissions only for trusted OpenID providers

Not all OpenID provider operators may be trusted by a Gerrit site
administrator.  The OpenID protocol is open to man-in-the-middle
attacks, and the security of a user's account is only as good as
the claimed id's host, or its delegate provider.  These provide
an attacker many avenues with which to enter a Gerrit instance
and try to abuse an account's privileges.

Gerrit sites relying on OpenID authentication can now require
that an account which has been granted permissions beyond those
that are given to anyone (aka anonymous and registered users)
use only trusted OpenID providers, as configured by the site's
administrator in the database.

Signed-off-by: Shawn O. Pearce <sop@google.com>

17 files changed, 260 insertions, 14 deletions

diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index f1fd347595..5485994447 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -131,6 +131,15 @@ about the system groups) and the maximum range is chosen (so the
 lowest value granted to any group, and the highest value granted
 to any group).
 
+OpenID Authentication
+~~~~~~~~~~~~~~~~~~~~~
+
+If the Gerrit instance is configured to use OpenID authentication,
+an account's effective group membership will be restricted to only
+the `Anonymous Users` and `Registered Users` groups, unless *all*
+of its OpenID identities match one or more of the patterns listed
+in the `trusted_external_ids` table.
+
 All Projects
 ~~~~~~~~~~~~
 
diff --git a/Documentation/config-sso.txt b/Documentation/config-sso.txt
index 57e4b47008..63bb156812 100644
--- a/Documentation/config-sso.txt
+++ b/Documentation/config-sso.txt
@@ -22,6 +22,36 @@ OpenID authentication services.
 
 * http://openid.net/[openid.net]
 
+In order to use permissions beyond those granted to the
+`Anonymous Users` and `Registered Users` groups, an account
+must only have OpenIDs which match at least one pattern from the
+`trusted_external_ids` table.  Patterns may be either a regular
+expression (must start with `^` and end with `$`) or be a simple
+prefix (any other string).
+
+Out of the box Gerrit is configured to trust three patterns:
+
+* `http://` -- trust all OpenID providers using the HTTP protocol
+* `https://` -- trust all OpenID providers using the HTTPS protocol
+* `https://www.google.com/accounts/o8/id?id=` -- trust Google Accounts
+
+The first two patterns trust all OpenID providers on the Internet.
+The Google specific pattern is obviously also implied by the second
+pattern (`https://`), but is inserted by default in order to permit
+securing Gerrit to trust only Google Accounts easier:
+
+====
+  DELETE FROM trusted_external_ids
+  WHERE external_id IN ('http://', 'https://');
+====
+
+After making changes to `trusted_external_ids`, either restart
+Gerrit, or force a cache flush over SSH:
+
+====
+  ssh -p 29418 review.example.com gerrit flush-caches
+====
+
 Database Schema
 ~~~~~~~~~~~~~~~
 
diff --git a/src/main/java/com/google/gerrit/client/changes/PatchSetPublishDetail.java b/src/main/java/com/google/gerrit/client/changes/PatchSetPublishDetail.java
index 7231c63f59..61d6b7b63b 100644
--- a/src/main/java/com/google/gerrit/client/changes/PatchSetPublishDetail.java
+++ b/src/main/java/com/google/gerrit/client/changes/PatchSetPublishDetail.java
@@ -72,7 +72,7 @@ public class PatchSetPublishDetail {
 
   private void computeAllowed() {
     final Account.Id me = Common.getAccountId();
-    final Set<AccountGroup.Id> am = Common.getGroupCache().getGroups(me);
+    final Set<AccountGroup.Id> am = Common.getGroupCache().getEffectiveGroups(me);
     final ProjectCache.Entry pe =
         Common.getProjectCache().get(change.getDest().getParentKey());
     computeAllowed(am, pe.getRights());
diff --git a/src/main/java/com/google/gerrit/client/data/ApprovalDetail.java b/src/main/java/com/google/gerrit/client/data/ApprovalDetail.java
index 47fe49cf20..f307fefda4 100644
--- a/src/main/java/com/google/gerrit/client/data/ApprovalDetail.java
+++ b/src/main/java/com/google/gerrit/client/data/ApprovalDetail.java
@@ -73,7 +73,7 @@ public class ApprovalDetail {
 
   void applyProjectRights(final GroupCache groupCache,
       final Map<ApprovalCategory.Id, Collection<ProjectRight>> rights) {
-    final Set<AccountGroup.Id> groups = groupCache.getGroups(account);
+    final Set<AccountGroup.Id> groups = groupCache.getEffectiveGroups(account);
     for (final ChangeApproval a : approvals) {
       Collection<ProjectRight> l = rights.get(a.getCategoryId());
       short min = 0, max = 0;
diff --git a/src/main/java/com/google/gerrit/client/data/GroupCache.java b/src/main/java/com/google/gerrit/client/data/GroupCache.java
index 17a569d313..1ff0181e38 100644
--- a/src/main/java/com/google/gerrit/client/data/GroupCache.java
+++ b/src/main/java/com/google/gerrit/client/data/GroupCache.java
@@ -19,12 +19,14 @@ import com.google.gerrit.client.reviewdb.AccountGroup;
 import com.google.gerrit.client.reviewdb.AccountGroupMember;
 import com.google.gerrit.client.reviewdb.ReviewDb;
 import com.google.gerrit.client.reviewdb.SystemConfig;
+import com.google.gerrit.client.reviewdb.TrustedExternalId;
 import com.google.gerrit.client.rpc.Common;
 import com.google.gwtorm.client.OrmException;
 
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -34,7 +36,7 @@ public class GroupCache {
   private AccountGroup.Id anonymousGroupId;
   private AccountGroup.Id registeredGroupId;
   private Set<AccountGroup.Id> anonOnly;
-
+  private List<TrustedExternalId> trustedIds;
 
   private final LinkedHashMap<Account.Id, Set<AccountGroup.Id>> byAccount =
       new LinkedHashMap<Account.Id, Set<AccountGroup.Id>>(16, 0.75f, true) {
@@ -108,7 +110,7 @@ public class GroupCache {
     if (isAnonymousUsers(groupId) || isRegisteredUsers(groupId)) {
       return true;
     }
-    return getGroups(accountId).contains(groupId);
+    return getEffectiveGroups(accountId).contains(groupId);
   }
 
   /**
@@ -162,11 +164,15 @@ public class GroupCache {
 
   /**
    * Get the groups a specific account is a member of.
+   * <p>
+   * A user is only a member of groups beyond {@link #anonymousGroupId} and
+   * {@link #registeredGroupId} if their account is using only
+   * {@link TrustedExternalId}s.
    * 
    * @param accountId the account to obtain the group list for.
    * @return unmodifiable set listing the groups the account is a member of.
    */
-  public Set<AccountGroup.Id> getGroups(final Account.Id accountId) {
+  public Set<AccountGroup.Id> getEffectiveGroups(final Account.Id accountId) {
     if (accountId == null) {
       return anonOnly;
     }
@@ -183,9 +189,11 @@ public class GroupCache {
     try {
       final ReviewDb db = Common.getSchemaFactory().open();
       try {
-        for (final AccountGroupMember g : db.accountGroupMembers().byAccount(
-            accountId)) {
-          m.add(g.getAccountGroupId());
+        if (isIdentityTrustable(db, accountId)) {
+          for (final AccountGroupMember g : db.accountGroupMembers().byAccount(
+              accountId)) {
+            m.add(g.getAccountGroupId());
+          }
         }
       } finally {
         db.close();
@@ -201,10 +209,40 @@ public class GroupCache {
     return m;
   }
 
+  private boolean isIdentityTrustable(final ReviewDb db,
+      final Account.Id accountId) throws OrmException {
+    switch (Common.getGerritConfig().getLoginType()) {
+      case HTTP:
+        // Its safe to assume yes for an HTTP authentication type, as the
+        // only way in is through some external system that the admin trusts
+        //
+        return true;
+
+      case OPENID:
+      default:
+        // Validate against the trusted provider list
+        //
+        return TrustedExternalId.isIdentityTrustable(getTrustedIds(db), db
+            .accountExternalIds().byAccount(accountId));
+    }
+  }
+
+  private synchronized List<TrustedExternalId> getTrustedIds(final ReviewDb db)
+      throws OrmException {
+    if (trustedIds == null) {
+      trustedIds =
+          Collections.unmodifiableList(db.trustedExternalIds().all().toList());
+    }
+    return trustedIds;
+  }
+
   /** Force the entire group cache to flush from memory and recompute. */
   public void flush() {
     synchronized (byAccount) {
       byAccount.clear();
     }
+    synchronized (this) {
+      trustedIds = null;
+    }
   }
 }
diff --git a/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java b/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
index 6e33eb3d97..1bf1e5fff6 100644
--- a/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
+++ b/src/main/java/com/google/gerrit/client/reviewdb/ReviewDb.java
@@ -21,7 +21,7 @@ import com.google.gwtorm.client.Sequence;
 
 /** The review service database schema. */
 public interface ReviewDb extends Schema {
-  public static final int VERSION = 5;
+  public static final int VERSION = 6;
 
   @Relation
   SchemaVersionAccess schemaVersion();
@@ -30,6 +30,9 @@ public interface ReviewDb extends Schema {
   SystemConfigAccess systemConfig();
 
   @Relation
+  TrustedExternalIdAccess trustedExternalIds();
+
+  @Relation
   ApprovalCategoryAccess approvalCategories();
 
   @Relation
diff --git a/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalId.java b/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalId.java
new file mode 100644
index 0000000000..5a6ea57ad7
--- /dev/null
+++ b/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalId.java
@@ -0,0 +1,107 @@
+// Copyright 2009 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.reviewdb;
+
+import com.google.gwtorm.client.Column;
+import com.google.gwtorm.client.StringKey;
+
+import java.util.Collection;
+
+public final class TrustedExternalId {
+  public static class Key extends StringKey<com.google.gwtorm.client.Key<?>> {
+    @Column
+    protected String pattern;
+
+    protected Key() {
+    }
+
+    public Key(final String re) {
+      pattern = re;
+    }
+
+    @Override
+    public String get() {
+      return pattern;
+    }
+
+    @Override
+    protected void set(String newValue) {
+      pattern = newValue;
+    }
+  }
+
+  public static boolean isIdentityTrustable(
+      final Collection<TrustedExternalId> trusted,
+      final Iterable<AccountExternalId> ids) {
+    for (final AccountExternalId e : ids) {
+      if (!isTrusted(e, trusted)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private static boolean isTrusted(final AccountExternalId id,
+      final Collection<TrustedExternalId> trusted) {
+    if (id.getExternalId().startsWith("Google Account ")) {
+      // Assume this is a trusted token, its a legacy import from
+      // a fairly well respected provider and only takes effect if
+      // the administrator has the import still enabled
+      //
+      return true;
+    }
+
+    if (id.getExternalId().startsWith("mailto:")) {
+      // mailto identities are created by sending a unique validation
+      // token to the address and asking them to come back to the site
+      // with that token.
+      //
+      return true;
+    }
+
+    for (final TrustedExternalId t : trusted) {
+      if (t.matches(id)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  @Column
+  protected Key idPattern;
+
+  protected TrustedExternalId() {
+  }
+
+  public TrustedExternalId(final TrustedExternalId.Key k) {
+    idPattern = k;
+  }
+
+  public TrustedExternalId.Key getKey() {
+    return idPattern;
+  }
+
+  public String getIdPattern() {
+    return idPattern.pattern;
+  }
+
+  public boolean matches(final AccountExternalId id) {
+    final String p = getIdPattern();
+    if (p.startsWith("^") && p.endsWith("$")) {
+      return id.getExternalId().matches(p);
+    }
+    return id.getExternalId().startsWith(p);
+  }
+}
diff --git a/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalIdAccess.java b/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalIdAccess.java
new file mode 100644
index 0000000000..9d345af38c
--- /dev/null
+++ b/src/main/java/com/google/gerrit/client/reviewdb/TrustedExternalIdAccess.java
@@ -0,0 +1,30 @@
+// Copyright 2009 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.reviewdb;
+
+import com.google.gwtorm.client.Access;
+import com.google.gwtorm.client.OrmException;
+import com.google.gwtorm.client.PrimaryKey;
+import com.google.gwtorm.client.Query;
+import com.google.gwtorm.client.ResultSet;
+
+public interface TrustedExternalIdAccess extends
+    Access<TrustedExternalId, TrustedExternalId.Key> {
+  @PrimaryKey("idPattern")
+  TrustedExternalId get(TrustedExternalId.Key key) throws OrmException;
+
+  @Query
+  ResultSet<TrustedExternalId> all() throws OrmException;
+}
diff --git a/src/main/java/com/google/gerrit/client/rpc/BaseServiceImplementation.java b/src/main/java/com/google/gerrit/client/rpc/BaseServiceImplementation.java
index 631aec5ef4..a321ae04ba 100644
--- a/src/main/java/com/google/gerrit/client/rpc/BaseServiceImplementation.java
+++ b/src/main/java/com/google/gerrit/client/rpc/BaseServiceImplementation.java
@@ -106,7 +106,7 @@ public class BaseServiceImplementation {
       //
       return false;
     }
-    final Set<AccountGroup.Id> myGroups = Common.getGroupCache().getGroups(who);
+    final Set<AccountGroup.Id> myGroups = Common.getGroupCache().getEffectiveGroups(who);
     return canPerform(myGroups, e, ApprovalCategory.READ, (short) 1, true);
   }
 
diff --git a/src/main/java/com/google/gerrit/client/workflow/FunctionState.java b/src/main/java/com/google/gerrit/client/workflow/FunctionState.java
index fa73399e0d..f6243bf38f 100644
--- a/src/main/java/com/google/gerrit/client/workflow/FunctionState.java
+++ b/src/main/java/com/google/gerrit/client/workflow/FunctionState.java
@@ -177,7 +177,7 @@ public class FunctionState {
   public Set<AccountGroup.Id> getGroups(final Account.Id id) {
     Set<AccountGroup.Id> g = groupCache.get(id);
     if (g == null) {
-      g = Common.getGroupCache().getGroups(id);
+      g = Common.getGroupCache().getEffectiveGroups(id);
       groupCache.put(id, g);
     }
     return g;
diff --git a/src/main/java/com/google/gerrit/server/AccountSecurityImpl.java b/src/main/java/com/google/gerrit/server/AccountSecurityImpl.java
index 52a4808403..058c5a376f 100644
--- a/src/main/java/com/google/gerrit/server/AccountSecurityImpl.java
+++ b/src/main/java/com/google/gerrit/server/AccountSecurityImpl.java
@@ -193,6 +193,7 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
           final Transaction txn = db.beginTransaction();
           db.accountExternalIds().delete(toDelete, txn);
           txn.commit();
+          Common.getGroupCache().invalidate(me);
         }
 
         return removed;
diff --git a/src/main/java/com/google/gerrit/server/GerritServer.java b/src/main/java/com/google/gerrit/server/GerritServer.java
index bbbe78978f..8a055ea7cc 100644
--- a/src/main/java/com/google/gerrit/server/GerritServer.java
+++ b/src/main/java/com/google/gerrit/server/GerritServer.java
@@ -30,6 +30,7 @@ import com.google.gerrit.client.reviewdb.ProjectRight;
 import com.google.gerrit.client.reviewdb.ReviewDb;
 import com.google.gerrit.client.reviewdb.SchemaVersion;
 import com.google.gerrit.client.reviewdb.SystemConfig;
+import com.google.gerrit.client.reviewdb.TrustedExternalId;
 import com.google.gerrit.client.rpc.Common;
 import com.google.gerrit.client.workflow.NoOpFunction;
 import com.google.gerrit.client.workflow.SubmitFunction;
@@ -252,6 +253,19 @@ public class GerritServer {
     s.gerritGitName = "Gerrit Code Review";
     s.setLoginType(SystemConfig.LoginType.OPENID);
     c.systemConfig().insert(Collections.singleton(s));
+
+    // By default with OpenID trust any http:// or https:// provider
+    //
+    initTrustedExternalId(c, "http://");
+    initTrustedExternalId(c, "https://");
+    initTrustedExternalId(c, "https://www.google.com/accounts/o8/id?id=");
+  }
+
+  private void initTrustedExternalId(final ReviewDb c, final String re)
+      throws OrmException {
+    c.trustedExternalIds().insert(
+        Collections.singleton(new TrustedExternalId(new TrustedExternalId.Key(
+            re))));
   }
 
   private void initWildCardProject(final ReviewDb c) throws OrmException {
diff --git a/src/main/java/com/google/gerrit/server/GroupAdminServiceImpl.java b/src/main/java/com/google/gerrit/server/GroupAdminServiceImpl.java
index 982cde66a4..201cd9e942 100644
--- a/src/main/java/com/google/gerrit/server/GroupAdminServiceImpl.java
+++ b/src/main/java/com/google/gerrit/server/GroupAdminServiceImpl.java
@@ -251,7 +251,7 @@ public class GroupAdminServiceImpl extends BaseServiceImplementation implements
       throws OrmException {
     final Account.Id me = Common.getAccountId();
     final List<AccountGroup> own = new ArrayList<AccountGroup>();
-    for (final AccountGroup.Id groupId : Common.getGroupCache().getGroups(me)) {
+    for (final AccountGroup.Id groupId : Common.getGroupCache().getEffectiveGroups(me)) {
       for (final AccountGroup g : db.accountGroups().ownedByGroup(groupId)) {
         own.add(g);
       }
diff --git a/src/main/java/com/google/gerrit/server/OpenIdLoginServlet.java b/src/main/java/com/google/gerrit/server/OpenIdLoginServlet.java
index a035d6ddb3..818423b49a 100644
--- a/src/main/java/com/google/gerrit/server/OpenIdLoginServlet.java
+++ b/src/main/java/com/google/gerrit/server/OpenIdLoginServlet.java
@@ -427,6 +427,7 @@ public class OpenIdLoginServlet extends HttpServlet {
       id.setLastUsedOn();
       id.setEmailAddress(email);
       db.accountExternalIds().insert(Collections.singleton(id));
+      Common.getGroupCache().invalidate(account.getId());
     } else {
       if (email != null && !email.equals(id.getEmailAddress())) {
         id.setEmailAddress(email);
diff --git a/src/main/java/com/google/gerrit/server/ProjectAdminServiceImpl.java b/src/main/java/com/google/gerrit/server/ProjectAdminServiceImpl.java
index 356f1a7990..9a2ab4f55a 100644
--- a/src/main/java/com/google/gerrit/server/ProjectAdminServiceImpl.java
+++ b/src/main/java/com/google/gerrit/server/ProjectAdminServiceImpl.java
@@ -464,7 +464,7 @@ public class ProjectAdminServiceImpl extends BaseServiceImplementation
   private List<Project> myOwnedProjects(final ReviewDb db) throws OrmException {
     final Account.Id me = Common.getAccountId();
     final List<Project> own = new ArrayList<Project>();
-    for (final AccountGroup.Id groupId : Common.getGroupCache().getGroups(me)) {
+    for (final AccountGroup.Id groupId : Common.getGroupCache().getEffectiveGroups(me)) {
       for (final Project g : db.projects().ownedByGroup(groupId)) {
         own.add(g);
       }
diff --git a/src/main/java/com/google/gerrit/server/ssh/AbstractCommand.java b/src/main/java/com/google/gerrit/server/ssh/AbstractCommand.java
index dcb8ac07c7..0448042563 100644
--- a/src/main/java/com/google/gerrit/server/ssh/AbstractCommand.java
+++ b/src/main/java/com/google/gerrit/server/ssh/AbstractCommand.java
@@ -107,7 +107,7 @@ abstract class AbstractCommand implements Command, SessionAware {
 
   protected Set<AccountGroup.Id> getGroups() {
     if (userGroups == null) {
-      userGroups = Common.getGroupCache().getGroups(getAccountId());
+      userGroups = Common.getGroupCache().getEffectiveGroups(getAccountId());
     }
     return userGroups;
   }
diff --git a/src/main/webapp/WEB-INF/sql/upgrade005_006.sql b/src/main/webapp/WEB-INF/sql/upgrade005_006.sql
new file mode 100644
index 0000000000..f8113706c3
--- /dev/null
+++ b/src/main/webapp/WEB-INF/sql/upgrade005_006.sql
@@ -0,0 +1,13 @@
+-- Upgrade: schema_version 5 to 6
+--
+
+CREATE TABLE trusted_external_ids
+(id_pattern VARCHAR(255) NOT NULL
+,PRIMARY KEY (id_pattern)
+) WITHOUT OIDS;
+
+INSERT INTO trusted_external_ids VALUES ('http://');
+INSERT INTO trusted_external_ids VALUES ('https://');
+INSERT INTO trusted_external_ids VALUES ('https://www.google.com/accounts/o8/id?id=');
+
+UPDATE schema_version SET version_nbr = 6;