opendev
/
gerrit
Code Issues Proposed changes

Revert "Fix the missing DB entry in Gerrit DB" 

This reverts commit fcc90699fd.

Reason for revert: Security vulnerability for OAuth and OpenID auth
schemes.

OAuth and OpenID authentication schemes support multiple identity
providers, e.g.: CAS-OAuth2 and GitHub-OAuth2. An attacker can easily
impersonate existing Gerrit user by creating account on a different
provider with exactly the same username as the existing Gerrit account.
Instead of creating a fresh new user, the new account is erroneously
linked to the existing Gerrit account, even though, account linking
feature was not triggered from the Gerrit UI.

The original commit tried to fix intermittent database corruption
problem, with missing record in the database, in the context of single
identity provider (LDAP) where such problem doesn't exist, as there is
no way that one single username can belong to physical different users.
Nevertheless, there should be found another workaround, as trying to
recover on the fly and introducing severe security breach for other auth
schemes supported in Gerrit.

If all else fails, the missing database record has to be inserted
manually and the corresponding account must be re-indexed.

Bug: Issue 7652
Bug: Issue 10242
Change-Id: Icba3452c153b2ae3cc1a4ebc569342641f38c07c
This commit is contained in:
David Ostrovsky 2019-01-05 17:04:59 +01:00 committed by David Pursehouse
parent 0b61aa0aa1
commit 2afce52141
1 changed files with 0 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
View File

@ -14,7 +14,6 @@
package com.google.gerrit.server.account;
import static com.google.gerrit.server.account.ExternalId.SCHEME_USERNAME;
import static java.util.stream.Collectors.toSet;
import com.google.common.base.Strings;
@ -108,23 +107,8 @@ public class AccountManager {
try (ReviewDb db = schema.open()) {
ExternalId id = findExternalId(db, who.getExternalIdKey());
if (id == null) {
if (who.getUserName() != null) {
ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName());
ExternalId existingId = findExternalId(db, key);
if (existingId != null) {
// An inconsistency is detected in the database, having a record for scheme
// "username:"
// but no record for scheme "gerrit:". Try to recover by linking
// "gerrit:" identity to the existing account.
log.warn(
"User {} already has an account; link new identity to the existing account.",
who.getUserName());
return link(existingId.accountId(), who);
}
}
// New account, automatically create and return.
//
log.debug("External ID not found. Attempting to create new account.");
return create(db, who);
}
@ -367,16 +351,13 @@ public class AccountManager {
public AuthResult link(Account.Id to, AuthRequest who)
throws AccountException, OrmException, IOException {
try (ReviewDb db = schema.open()) {
log.debug("Link another authentication identity to an existing account");
ExternalId extId = findExternalId(db, who.getExternalIdKey());
if (extId != null) {
if (!extId.accountId().equals(to)) {
throw new AccountException("Identity in use by another account");
}
log.debug("Updating existing external ID data");
update(db, who, extId);
} else {
log.debug("Linking new external ID to the existing account");
externalIdsUpdateFactory
.create()
.insert(