From 47a462d1aa39ffd562f606ffb42b87388a5a6c74 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 15 Sep 2015 14:56:45 -0400 Subject: [PATCH] Distinguish case where a public key must match a specific user GerritPublicKeyChecker was always checking that the user IDs in the key matched at least one identity from AccountExternalIds for the current user in the request. This was broken when combined with web-of-trust checks, since only the initial key passed to check() matches the current user; all other keys match different users. Add a factory to create two different kinds of checkers, depending on whether we have a specific user in mind that we want to match the initial key against. Expose the depth to checkCustom() so we only use this user on the initial call and not on later recursive calls. Add tests to GerritPublicKeyCheckerTest for web-of-trust checks, which were previously only in PublicKeyCheckerTest. Change-Id: I1af584354f26c80e9ed3b7643ed6e3624820214c --- .../gerrit/gpg/GerritPublicKeyChecker.java | 177 ++++++++--- .../google/gerrit/gpg/PublicKeyChecker.java | 7 +- .../google/gerrit/gpg/SignedPushModule.java | 1 - .../gerrit/gpg/SignedPushPreReceiveHook.java | 12 +- .../google/gerrit/gpg/server/PostGpgKeys.java | 10 +- .../gpg/GerritPublicKeyCheckerTest.java | 277 +++++++++++++++++- 6 files changed, 414 insertions(+), 70 deletions(-) diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java index 54fa6f786c..d942c7576c 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java @@ -14,19 +14,23 @@ package com.google.gerrit.gpg; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString; import static com.google.gerrit.reviewdb.client.AccountExternalId.SCHEME_GPGKEY; import com.google.common.base.CharMatcher; import com.google.common.base.MoreObjects; import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Ordering; import com.google.common.io.BaseEncoding; import com.google.gerrit.common.PageLinks; import com.google.gerrit.reviewdb.client.AccountExternalId; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -53,69 +57,149 @@ import java.util.Set; * For Gerrit, keys must contain a self-signed user ID certification matching a * trusted external ID in the database, or an email address thereof. */ -@Singleton public class GerritPublicKeyChecker extends PublicKeyChecker { private static final Logger log = LoggerFactory.getLogger(GerritPublicKeyChecker.class); + private final Provider db; private final String webUrl; - private final Provider userProvider; + private final IdentifiedUser.GenericFactory userFactory; + private final IdentifiedUser expectedUser; - private static List getTrustedFingerprints(Config cfg) { - String[] strs = cfg.getStringList("receive", null, "trustedKey"); - if (strs == null || strs.length == 0) { - return null; + @Singleton + public static class Factory { + private final Provider db; + private final String webUrl; + private final IdentifiedUser.GenericFactory userFactory; + private final int maxTrustDepth; + private final ImmutableList trusted; + + @Inject + Factory(@GerritServerConfig Config cfg, + Provider db, + IdentifiedUser.GenericFactory userFactory, + @CanonicalWebUrl String webUrl) { + this.db = db; + this.webUrl = webUrl; + this.userFactory = userFactory; + this.maxTrustDepth = cfg.getInt("receive", null, "maxTrustDepth", 0); + + String[] strs = cfg.getStringList("receive", null, "trustedKey"); + if (strs.length != 0) { + List fps = new ArrayList<>(strs.length); + for (String str : strs) { + str = CharMatcher.WHITESPACE.removeFrom(str).toUpperCase(); + fps.add(new Fingerprint(BaseEncoding.base16().decode(str))); + } + trusted = ImmutableList.copyOf(fps); + } else { + trusted = null; + } } - List fps = new ArrayList<>(strs.length); - for (String str : strs) { - str = CharMatcher.WHITESPACE.removeFrom(str).toUpperCase(); - fps.add(new Fingerprint(BaseEncoding.base16().decode(str))); + + /** + * Create a checker that can check arbitrary public keys. + *

+ * Each key is checked against the set of identities in the database + * belonging to the same user as the key. + * + * @return a new checker. + */ + public GerritPublicKeyChecker create() { + return new GerritPublicKeyChecker(this, null); + } + + /** + * Create a checker for checking a single public key against a known user. + *

+ * The top-level key passed to {@link #check(PGPPublicKey, PublicKeyStore)} + * must belong to the given user. (Other keys checked in the course of + * verifying the web of trust are checked against the set of identities in + * the database belonging to the same user as the key.) + * + * @param expectedUser the user + * @return a new checker. + */ + public GerritPublicKeyChecker create(IdentifiedUser expectedUser) { + checkNotNull(expectedUser); + return new GerritPublicKeyChecker(this, expectedUser); } - return fps; } - @Inject - GerritPublicKeyChecker( - @GerritServerConfig Config cfg, - @CanonicalWebUrl String webUrl, - Provider userProvider) { - super(cfg.getInt("receive", null, "maxTrustDepth", 0), - getTrustedFingerprints(cfg)); - this.webUrl = webUrl; - this.userProvider = userProvider; + private GerritPublicKeyChecker(Factory factory, IdentifiedUser expectedUser) { + super(factory.maxTrustDepth, factory.trusted); + this.db = factory.db; + this.webUrl = factory.webUrl; + this.userFactory = factory.userFactory; + this.expectedUser = expectedUser; } @Override - public CheckResult checkCustom(PGPPublicKey key) { + public CheckResult checkCustom(PGPPublicKey key, int depth) { try { - Set allowedUserIds = getAllowedUserIds(); - if (allowedUserIds.isEmpty()) { - return CheckResult.bad("No identities found for user; check " - + webUrl + "#" + PageLinks.SETTINGS_WEBIDENT); + if (depth == 0 && expectedUser != null) { + return checkIdsForExpectedUser(key); + } else { + return checkIdsForArbitraryUser(key); } - - @SuppressWarnings("unchecked") - Iterator userIds = key.getUserIDs(); - while (userIds.hasNext()) { - String userId = userIds.next(); - if (isAllowed(userId, allowedUserIds)) { - Iterator sigs = getSignaturesForId(key, userId); - while (sigs.hasNext()) { - if (isValidCertification(key, sigs.next(), userId)) { - return CheckResult.trusted(); - } - } - } - } - - return CheckResult.bad(missingUserIds(allowedUserIds)); - } catch (PGPException e) { + } catch (PGPException | OrmException e) { String msg = "Error checking user IDs for key"; log.warn(msg + " " + keyIdToString(key.getKeyID()), e); return CheckResult.bad(msg); } } + private CheckResult checkIdsForExpectedUser(PGPPublicKey key) + throws PGPException { + Set allowedUserIds = getAllowedUserIds(expectedUser); + if (allowedUserIds.isEmpty()) { + return CheckResult.bad("No identities found for user; check " + + webUrl + "#" + PageLinks.SETTINGS_WEBIDENT); + } + if (hasAllowedUserId(key, allowedUserIds)) { + return CheckResult.trusted(); + } + return CheckResult.bad(missingUserIds(allowedUserIds)); + } + + private CheckResult checkIdsForArbitraryUser(PGPPublicKey key) + throws PGPException, OrmException { + AccountExternalId extId = db.get().accountExternalIds().get( + toExtIdKey(key)); + if (extId == null) { + return CheckResult.bad("Key is not associated with any users"); + } + IdentifiedUser user = userFactory.create(db, extId.getAccountId()); + Set allowedUserIds = getAllowedUserIds(user); + if (allowedUserIds.isEmpty()) { + return CheckResult.bad("No identities found for user"); + } + if (hasAllowedUserId(key, allowedUserIds)) { + return CheckResult.trusted(); + } + return CheckResult.bad( + "Key does not contain any valid certifications for user's identities"); + } + + private boolean hasAllowedUserId(PGPPublicKey key, Set allowedUserIds) + throws PGPException { + @SuppressWarnings("unchecked") + Iterator userIds = key.getUserIDs(); + while (userIds.hasNext()) { + String userId = userIds.next(); + if (isAllowed(userId, allowedUserIds)) { + Iterator sigs = getSignaturesForId(key, userId); + while (sigs.hasNext()) { + if (isValidCertification(key, sigs.next(), userId)) { + return true; + } + } + } + } + + return false; + } + @SuppressWarnings("unchecked") private Iterator getSignaturesForId(PGPPublicKey key, String userId) { @@ -124,8 +208,7 @@ public class GerritPublicKeyChecker extends PublicKeyChecker { Collections.emptyIterator()); } - private Set getAllowedUserIds() { - IdentifiedUser user = userProvider.get(); + private Set getAllowedUserIds(IdentifiedUser user) { Set result = new HashSet<>(); result.addAll(user.getEmailAddresses()); for (AccountExternalId extId : user.state().getExternalIds()) { @@ -175,4 +258,10 @@ public class GerritPublicKeyChecker extends PublicKeyChecker { } return sb.toString(); } + + static AccountExternalId.Key toExtIdKey(PGPPublicKey key) { + return new AccountExternalId.Key( + SCHEME_GPGKEY, + BaseEncoding.base16().encode(key.getFingerprint())); + } } diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java index a5e14a2aa3..725a6e1036 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java @@ -114,16 +114,19 @@ public class PublicKeyChecker { * subclasses. * * @param key the public key. + * @param depth the depth from the initial key passed to {@link #check( + * PGPPublicKey, PublicKeyStore)}: 0 if this was the initial key, up to a + * maximum of {@code maxTrustDepth}. * @return the result of the custom check. */ - public CheckResult checkCustom(PGPPublicKey key) { + public CheckResult checkCustom(PGPPublicKey key, int depth) { return CheckResult.ok(); } private CheckResult check(PGPPublicKey key, PublicKeyStore store, int depth, boolean expand, Set seen) { CheckResult basicResult = checkBasic(key); - CheckResult customResult = checkCustom(key); + CheckResult customResult = checkCustom(key, depth); CheckResult trustResult = checkWebOfTrust(key, store, depth, seen); if (!expand && !trustResult.isTrusted()) { trustResult = CheckResult.create(trustResult.getStatus(), diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushModule.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushModule.java index 750880682b..c694cb9098 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushModule.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushModule.java @@ -53,7 +53,6 @@ class SignedPushModule extends AbstractModule { if (!BouncyCastleUtil.havePGP()) { throw new ProvisionException("Bouncy Castle PGP not installed"); } - bind(PublicKeyChecker.class).to(GerritPublicKeyChecker.class); bind(PublicKeyStore.class).toProvider(StoreProvider.class); DynamicSet.bind(binder(), ReceivePackInitializer.class) .to(Initializer.class); diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java index b2dca8bf3a..50f0642574 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java @@ -14,9 +14,11 @@ package com.google.gerrit.gpg; +import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import org.eclipse.jgit.lib.Repository; @@ -39,16 +41,19 @@ import java.util.Collection; public class SignedPushPreReceiveHook implements PreReceiveHook { private final GitRepositoryManager repoManager; private final AllUsersName allUsers; - private final PublicKeyChecker keyChecker; + private final Provider user; + private final GerritPublicKeyChecker.Factory keyCheckerFactory; @Inject public SignedPushPreReceiveHook( GitRepositoryManager repoManager, AllUsersName allUsers, - PublicKeyChecker keyChecker) { + Provider user, + GerritPublicKeyChecker.Factory keyCheckerFactory) { this.repoManager = repoManager; this.allUsers = allUsers; - this.keyChecker = keyChecker; + this.user = user; + this.keyCheckerFactory = keyCheckerFactory; } @Override @@ -58,6 +63,7 @@ public class SignedPushPreReceiveHook implements PreReceiveHook { if (cert == null) { return; } + PublicKeyChecker keyChecker = keyCheckerFactory.create(user.get()); PushCertificateChecker checker = new PushCertificateChecker(keyChecker) { @Override protected Repository getRepository() throws IOException { diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java index 714fbd7f13..abe96ad4b0 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java @@ -35,7 +35,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.gpg.CheckResult; import com.google.gerrit.gpg.Fingerprint; -import com.google.gerrit.gpg.PublicKeyChecker; +import com.google.gerrit.gpg.GerritPublicKeyChecker; import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.gpg.server.PostGpgKeys.Input; import com.google.gerrit.reviewdb.client.AccountExternalId; @@ -79,19 +79,19 @@ public class PostGpgKeys implements RestModifyView { private final Provider serverIdent; private final Provider db; private final Provider storeProvider; - private final PublicKeyChecker checker; + private final GerritPublicKeyChecker.Factory checkerFactory; private final AddKeySender.Factory addKeyFactory; @Inject PostGpgKeys(@GerritPersonIdent Provider serverIdent, Provider db, Provider storeProvider, - PublicKeyChecker checker, + GerritPublicKeyChecker.Factory checkerFactory, AddKeySender.Factory addKeyFactory) { this.serverIdent = serverIdent; this.db = db; this.storeProvider = storeProvider; - this.checker = checker; + this.checkerFactory = checkerFactory; this.addKeyFactory = addKeyFactory; } @@ -192,7 +192,7 @@ public class PostGpgKeys implements RestModifyView { for (PGPPublicKeyRing keyRing : keyRings) { PGPPublicKey key = keyRing.getPublicKey(); // Don't check web of trust; admins can fill in certifications later. - CheckResult result = checker.check(key); + CheckResult result = checkerFactory.create(rsrc.getUser()).check(key); if (!result.isOk()) { throw new BadRequestException(String.format( "Problems with public key %s:\n%s", diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java index 51d19aaf5e..3d0c5c6741 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java @@ -16,8 +16,21 @@ package com.google.gerrit.gpg; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.gpg.GerritPublicKeyChecker.toExtIdKey; +import static com.google.gerrit.gpg.PublicKeyStore.keyToString; +import static com.google.gerrit.gpg.testutil.TestTrustKeys.keyA; +import static com.google.gerrit.gpg.testutil.TestTrustKeys.keyB; +import static com.google.gerrit.gpg.testutil.TestTrustKeys.keyC; +import static com.google.gerrit.gpg.testutil.TestTrustKeys.keyD; +import static com.google.gerrit.gpg.testutil.TestTrustKeys.keyE; +import static com.google.gerrit.reviewdb.client.AccountExternalId.SCHEME_MAILTO; +import static org.eclipse.jgit.lib.RefUpdate.Result.FAST_FORWARD; +import static org.eclipse.jgit.lib.RefUpdate.Result.FORCED; +import static org.eclipse.jgit.lib.RefUpdate.Result.NEW; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterators; +import com.google.gerrit.extensions.common.GpgKeyInfo.Status; import com.google.gerrit.gpg.testutil.TestKey; import com.google.gerrit.gpg.testutil.TestKeys; import com.google.gerrit.lifecycle.LifecycleManager; @@ -40,11 +53,22 @@ import com.google.inject.Injector; import com.google.inject.Provider; import com.google.inject.util.Providers; +import org.bouncycastle.openpgp.PGPPublicKey; +import org.bouncycastle.openpgp.PGPPublicKeyRing; +import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.PushCertificateIdent; import org.junit.After; import org.junit.Before; import org.junit.Test; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; /** Unit tests for {@link GerritPublicKeyChecker}. */ public class GerritPublicKeyCheckerTest { @@ -55,7 +79,7 @@ public class GerritPublicKeyCheckerTest { private AccountManager accountManager; @Inject - private GerritPublicKeyChecker checker; + private GerritPublicKeyChecker.Factory checkerFactory; @Inject private IdentifiedUser.GenericFactory userFactory; @@ -73,10 +97,18 @@ public class GerritPublicKeyCheckerTest { private ReviewDb db; private Account.Id userId; private IdentifiedUser user; + private Repository storeRepo; + private PublicKeyStore store; @Before public void setUpInjector() throws Exception { - Injector injector = Guice.createInjector(new InMemoryModule()); + Config cfg = InMemoryModule.newDefaultConfig(); + cfg.setInt("receive", null, "maxTrustDepth", 2); + cfg.setStringList("receive", null, "trustedKey", ImmutableList.of( + Fingerprint.toString(keyB().getPublicKey().getFingerprint()), + Fingerprint.toString(keyD().getPublicKey().getFingerprint()))); + Injector injector = Guice.createInjector(new InMemoryModule(cfg)); + lifecycle = new LifecycleManager(); lifecycle.add(injector); injector.injectMembers(this); @@ -103,6 +135,21 @@ public class GerritPublicKeyCheckerTest { return Providers.of(db); } }); + + storeRepo = new InMemoryRepository(new DfsRepositoryDescription("repo")); + store = new PublicKeyStore(storeRepo); + } + + @After + public void tearDown() throws Exception { + store.close(); + storeRepo.close(); + } + + private IdentifiedUser addUser(String name) throws Exception { + AuthRequest req = AuthRequest.forUser(name); + Account.Id id = accountManager.authenticate(req).getAccountId(); + return userFactory.create(Providers.of(db), id); } private IdentifiedUser reloadUser() { @@ -125,22 +172,25 @@ public class GerritPublicKeyCheckerTest { @Test public void defaultGpgCertificationMatchesEmail() throws Exception { TestKey key = TestKeys.key5(); + GerritPublicKeyChecker checker = checkerFactory.create(user); assertProblems( - TestKeys.key5(), + checker.check(key.getPublicKey()), Status.BAD, "Key must contain a valid certification for one of the following " + "identities:\n" + " gerrit:user\n" + " username:user"); addExternalId("test", "test", "test5@example.com"); - assertNoProblems(key); + checker = checkerFactory.create(user); + assertNoProblems(checker.check(key.getPublicKey())); } @Test public void defaultGpgCertificationDoesNotMatchEmail() throws Exception { addExternalId("test", "test", "nobody@example.com"); + GerritPublicKeyChecker checker = checkerFactory.create(user); assertProblems( - TestKeys.key5(), + checker.check(TestKeys.key5().getPublicKey()), Status.BAD, "Key must contain a valid certification for one of the following " + "identities:\n" + " gerrit:user\n" @@ -152,14 +202,16 @@ public class GerritPublicKeyCheckerTest { @Test public void manualCertificationMatchesExternalId() throws Exception { addExternalId("foo", "myId", null); - assertNoProblems(TestKeys.key5()); + GerritPublicKeyChecker checker = checkerFactory.create(user); + assertNoProblems(checker.check(TestKeys.key5().getPublicKey())); } @Test - public void manualCertificationDoesNotExternalId() throws Exception { + public void manualCertificationDoesNotMatchExternalId() throws Exception { addExternalId("foo", "otherId", null); + GerritPublicKeyChecker checker = checkerFactory.create(user); assertProblems( - TestKeys.key5(), + checker.check(TestKeys.key5().getPublicKey()), Status.BAD, "Key must contain a valid certification for one of the following " + "identities:\n" + " foo:otherId\n" @@ -172,24 +224,219 @@ public class GerritPublicKeyCheckerTest { db.accountExternalIds().delete( db.accountExternalIds().byAccount(user.getAccountId())); reloadUser(); + + TestKey key = TestKeys.key5(); + GerritPublicKeyChecker checker = checkerFactory.create(user); assertProblems( - TestKeys.key5(), + checker.check(key.getPublicKey()), Status.BAD, "No identities found for user; check" + " http://test/#/settings/web-identities"); + + checker = checkerFactory.create(); + assertProblems( + checker.check(key.getPublicKey()), Status.BAD, + "Key is not associated with any users"); + + db.accountExternalIds().insert(Collections.singleton( + new AccountExternalId( + user.getAccountId(), toExtIdKey(key.getPublicKey())))); + reloadUser(); + assertProblems( + checker.check(key.getPublicKey()), Status.BAD, + "No identities found for user"); } - private void assertNoProblems(TestKey key) throws Exception { - assertThat(checker.check(key.getPublicKey()).getProblems()).isEmpty(); + @Test + public void checkValidTrustChainAndCorrectExternalIds() throws Exception { + // A---Bx + // \ + // \---C---D + // \ + // \---Ex + // + // The server ultimately trusts B and D. + // D and E trust C to be a valid introducer of depth 2. + IdentifiedUser userB = addUser("userB"); + TestKey keyA = add(keyA(), user); + TestKey keyB = add(keyB(), userB); + add(keyC(), addUser("userC")); + add(keyD(), addUser("userD")); + add(keyE(), addUser("userE")); + + // Checker for A, checking A. + GerritPublicKeyChecker checkerA = checkerFactory.create(user); + assertNoProblems(checkerA.check(keyA.getPublicKey(), store)); + + // Checker for B, checking B. Trust chain and IDs are correct, so the only + // problem is with the key itself. + GerritPublicKeyChecker checkerB = checkerFactory.create(userB); + assertProblems( + checkerB.check(keyB.getPublicKey(), store), Status.BAD, + "Key is expired"); } - private void assertProblems(TestKey key, String... expected) + @Test + public void checkWithValidKeyButWrongExpectedUserInChecker() throws Exception { - checkArgument(expected.length > 0); - assertThat(checker.check(key.getPublicKey()).getProblems()) - .containsExactly((Object[]) expected) + // A---Bx + // \ + // \---C---D + // \ + // \---Ex + // + // The server ultimately trusts B and D. + // D and E trust C to be a valid introducer of depth 2. + IdentifiedUser userB = addUser("userB"); + TestKey keyA = add(keyA(), user); + TestKey keyB = add(keyB(), userB); + add(keyC(), addUser("userC")); + add(keyD(), addUser("userD")); + add(keyE(), addUser("userE")); + + // Checker for A, checking B. + GerritPublicKeyChecker checkerA = checkerFactory.create(user); + assertProblems( + checkerA.check(keyB.getPublicKey(), store), Status.BAD, + "Key is expired", + "Key must contain a valid certification for one of the following" + + " identities:\n" + + " gerrit:user\n" + + " mailto:testa@example.com\n" + + " testa@example.com\n" + + " username:user"); + + // Checker for B, checking A. + GerritPublicKeyChecker checkerB = checkerFactory.create(userB); + assertProblems( + checkerB.check(keyA.getPublicKey(), store), Status.BAD, + "Key must contain a valid certification for one of the following" + + " identities:\n" + + " gerrit:userB\n" + + " mailto:testb@example.com\n" + + " testb@example.com\n" + + " username:userB"); + } + + @Test + public void checkTrustChainWithExpiredKey() throws Exception { + // A---Bx + // + // The server ultimately trusts B. + TestKey keyA = add(keyA(), user); + TestKey keyB = add(keyB(), addUser("userB")); + + GerritPublicKeyChecker checker = checkerFactory.create(user); + assertProblems( + checker.check(keyA.getPublicKey(), store), Status.OK, + "No path to a trusted key", + "Certification by " + keyToString(keyB.getPublicKey()) + + " is valid, but key is not trusted", + "Key D24FE467 used for certification is not in store"); + } + + @Test + public void checkTrustChainUsingCheckerWithoutExpectedKey() throws Exception { + // A---Bx + // \ + // \---C---D + // \ + // \---Ex + // + // The server ultimately trusts B and D. + // D and E trust C to be a valid introducer of depth 2. + TestKey keyA = add(keyA(), user); + TestKey keyB = add(keyB(), addUser("userB")); + TestKey keyC = add(keyC(), addUser("userC")); + TestKey keyD = add(keyD(), addUser("userD")); + TestKey keyE = add(keyE(), addUser("userE")); + + // This checker can check any key, so the only problems come from issues + // with the keys themselves, not having invalid user IDs. + GerritPublicKeyChecker checker = checkerFactory.create(); + assertNoProblems(checker.check(keyA.getPublicKey(), store)); + assertProblems( + checker.check(keyB.getPublicKey(), store), Status.BAD, + "Key is expired"); + assertNoProblems(checker.check(keyC.getPublicKey(), store)); + assertNoProblems(checker.check(keyD.getPublicKey(), store)); + assertProblems( + checker.check(keyE.getPublicKey(), store), Status.BAD, + "Key is expired", + "No path to a trusted key"); + } + + @Test + public void keyLaterInTrustChainMissingUserId() throws Exception { + // A---Bx + // \ + // \---C + // + // The server ultimately trusts B. + // C signed A's key but is not in the store. + TestKey keyA = add(keyA(), user); + + PGPPublicKeyRing keyRingB = keyB().getPublicKeyRing(); + PGPPublicKey keyB = keyRingB.getPublicKey(); + keyB = PGPPublicKey.removeCertification( + keyB, (String) keyB.getUserIDs().next()); + keyRingB = PGPPublicKeyRing.insertPublicKey(keyRingB, keyB); + add(keyRingB, addUser("userB")); + + GerritPublicKeyChecker checkerA = checkerFactory.create(user); + assertProblems(checkerA.check(keyA.getPublicKey(), store), Status.OK, + "No path to a trusted key", + "Certification by " + keyToString(keyB) + + " is valid, but key is not trusted", + "Key D24FE467 used for certification is not in store"); + } + + private void add(PGPPublicKeyRing kr, IdentifiedUser user) throws Exception { + Account.Id id = user.getAccountId(); + List newExtIds = new ArrayList<>(2); + newExtIds.add(new AccountExternalId(id, toExtIdKey(kr.getPublicKey()))); + + @SuppressWarnings("unchecked") + String userId = (String) Iterators.getOnlyElement( + kr.getPublicKey().getUserIDs(), null); + if (userId != null) { + String email = PushCertificateIdent.parse(userId).getEmailAddress(); + assertThat(email).contains("@"); + AccountExternalId mailto = new AccountExternalId( + id, new AccountExternalId.Key(SCHEME_MAILTO, email)); + mailto.setEmailAddress(email); + newExtIds.add(mailto); + } + + store.add(kr); + PersonIdent ident = new PersonIdent("A U Thor", "author@example.com"); + CommitBuilder cb = new CommitBuilder(); + cb.setAuthor(ident); + cb.setCommitter(ident); + assertThat(store.save(cb)).isAnyOf(NEW, FAST_FORWARD, FORCED); + + db.accountExternalIds().insert(newExtIds); + accountCache.evict(user.getAccountId()); + } + + private TestKey add(TestKey k, IdentifiedUser user) throws Exception { + add(k.getPublicKeyRing(), user); + return k; + } + + private void assertProblems(CheckResult result, Status expectedStatus, + String... expectedProblems) throws Exception { + checkArgument(expectedProblems.length > 0); + assertThat(result.getStatus()).isEqualTo(expectedStatus); + assertThat(result.getProblems()) + .containsExactly((Object[]) expectedProblems) .inOrder(); } + private void assertNoProblems(CheckResult result) { + assertThat(result.getStatus()).isEqualTo(Status.TRUSTED); + assertThat(result.getProblems()).isEmpty(); + } + private void addExternalId(String scheme, String id, String email) throws Exception { AccountExternalId extId = new AccountExternalId(user.getAccountId(),