PublicKeyChecker: Convert to a builder pattern

Instead of overloading the constructor and check methods with optional
arguments, expose optional arguments via setters. We are about to add
another one, and this was getting unwieldy.

We still use a Factory to create GerritPublicKeyChecker, which parses
the config once in a @Singleton and subsequently configures all
checkers with that config. Since the checker is initially created when
a store might not be available (because the repo hasn't been opened),
we need to separate the trusted key config setter from the store
setter, and check state at use time.

Change-Id: Ide6bc76a8180f91caa6f03f1a0eb25ec6d20ba45
This commit is contained in:
Dave Borowitz
2015-10-20 13:09:07 -04:00
parent 9db0d8906d
commit 2737821ae3
9 changed files with 168 additions and 128 deletions

View File

@@ -19,6 +19,9 @@ import static com.google.common.base.Preconditions.checkArgument;
import org.eclipse.jgit.util.NB;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
public class Fingerprint {
private final byte[] fp;
@@ -33,6 +36,14 @@ public class Fingerprint {
NB.decodeUInt16(fp, 16), NB.decodeUInt16(fp, 18));
}
public static Map<Long, Fingerprint> byId(Iterable<Fingerprint> fps) {
Map<Long, Fingerprint> result = new HashMap<>();
for (Fingerprint fp : fps) {
result.put(fp.getId(), fp);
}
return Collections.unmodifiableMap(result);
}
private static byte[] checkLength(byte[] fp) {
checkArgument(fp.length == 20,
"fingerprint must be 20 bytes, got %s", fp.length);

View File

@@ -14,14 +14,14 @@
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.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
import com.google.common.io.BaseEncoding;
import com.google.gerrit.common.PageLinks;
@@ -44,11 +44,10 @@ import org.eclipse.jgit.transport.PushCertificateIdent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
/**
@@ -61,18 +60,13 @@ public class GerritPublicKeyChecker extends PublicKeyChecker {
private static final Logger log =
LoggerFactory.getLogger(GerritPublicKeyChecker.class);
private final Provider<ReviewDb> db;
private final String webUrl;
private final IdentifiedUser.GenericFactory userFactory;
private final IdentifiedUser expectedUser;
@Singleton
public static class Factory {
private final Provider<ReviewDb> db;
private final String webUrl;
private final IdentifiedUser.GenericFactory userFactory;
private final int maxTrustDepth;
private final ImmutableList<Fingerprint> trusted;
private final ImmutableMap<Long, Fingerprint> trusted;
@Inject
Factory(@GerritServerConfig Config cfg,
@@ -86,52 +80,50 @@ public class GerritPublicKeyChecker extends PublicKeyChecker {
String[] strs = cfg.getStringList("receive", null, "trustedKey");
if (strs.length != 0) {
List<Fingerprint> fps = new ArrayList<>(strs.length);
Map<Long, Fingerprint> fps =
Maps.newHashMapWithExpectedSize(strs.length);
for (String str : strs) {
str = CharMatcher.WHITESPACE.removeFrom(str).toUpperCase();
fps.add(new Fingerprint(BaseEncoding.base16().decode(str)));
Fingerprint fp = new Fingerprint(BaseEncoding.base16().decode(str));
fps.put(fp.getId(), fp);
}
trusted = ImmutableList.copyOf(fps);
trusted = ImmutableMap.copyOf(fps);
} else {
trusted = null;
}
}
/**
* Create a checker that can check arbitrary public keys.
* <p>
* 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.
* <p>
* 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 new GerritPublicKeyChecker(this);
}
}
private GerritPublicKeyChecker(Factory factory, IdentifiedUser expectedUser) {
super(factory.maxTrustDepth, factory.trusted);
private final Provider<ReviewDb> db;
private final String webUrl;
private final IdentifiedUser.GenericFactory userFactory;
private IdentifiedUser expectedUser;
private GerritPublicKeyChecker(Factory factory) {
this.db = factory.db;
this.webUrl = factory.webUrl;
this.userFactory = factory.userFactory;
if (factory.trusted != null) {
enableTrust(factory.maxTrustDepth, factory.trusted);
}
}
/**
* Set the expected user for this checker.
* <p>
* If set, the top-level key passed to {@link #check(PGPPublicKey)} 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.)
*/
public GerritPublicKeyChecker setExpectedUser(IdentifiedUser expectedUser) {
this.expectedUser = expectedUser;
return this;
}
@Override

View File

@@ -40,7 +40,7 @@ public class GerritPushCertificateChecker extends PushCertificateChecker {
AllUsersName allUsers,
@Assisted IdentifiedUser expectedUser,
@Assisted boolean checkNonce) {
super(keyCheckerFactory.create(expectedUser), checkNonce);
super(keyCheckerFactory.create().setExpectedUser(expectedUser), checkNonce);
this.repoManager = repoManager;
this.allUsers = allUsers;
}

View File

@@ -32,8 +32,6 @@ import org.bouncycastle.openpgp.PGPSignature;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
@@ -45,66 +43,71 @@ public class PublicKeyChecker {
// https://tools.ietf.org/html/rfc4880#section-5.2.3.13
private static final int COMPLETE_TRUST = 120;
private final Map<Long, Fingerprint> trusted;
private final int maxTrustDepth;
/** Create a new checker that does not check the web of trust. */
public PublicKeyChecker() {
this(0, null);
}
private PublicKeyStore store;
private Map<Long, Fingerprint> trusted;
private int maxTrustDepth;
/**
* Enable web-of-trust checks.
* <p>
* If enabled, a store must be set with {@link #setStore(PublicKeyStore)}.
* (These methods are separate since the store is a closeable resource that
* may not be available when reading trusted keys from a config.)
*
* @param maxTrustDepth maximum depth to search while looking for a trusted
* key.
* @param trusted ultimately trusted key fingerprints; may not be empty. If
* null, disable web-of-trust checks.
* @param trusted ultimately trusted key fingerprints, keyed by fingerprint;
* may not be empty. To construct a map, see {@link
* Fingerprint#byId(Iterable)}.
* @return a reference to this object.
*/
public PublicKeyChecker(int maxTrustDepth, Collection<Fingerprint> trusted) {
if (trusted != null) {
if (maxTrustDepth <= 0) {
public PublicKeyChecker enableTrust(int maxTrustDepth,
Map<Long, Fingerprint> trusted) {
if (maxTrustDepth <= 0) {
throw new IllegalArgumentException(
"maxTrustDepth must be positive, got: " + maxTrustDepth);
}
if (trusted == null || trusted.isEmpty()) {
throw new IllegalArgumentException(
"maxTrustDepth must be positive, got: " + maxTrustDepth);
}
if (trusted.isEmpty()) {
throw new IllegalArgumentException("at least one trusted key required");
}
this.trusted = new HashMap<>();
for (Fingerprint fp : trusted) {
this.trusted.put(fp.getId(), fp);
}
} else {
this.trusted = null;
"at least one trusted key is required");
}
this.maxTrustDepth = maxTrustDepth;
this.trusted = trusted;
return this;
}
/** Disable web-of-trust checks. */
public PublicKeyChecker disableTrust() {
store = null;
trusted = null;
return this;
}
/**
* Check a public key, including its web of trust.
*
* @param key the public key.
* @param store a store to read public keys from for trust checks. If this
* store is not configured for web-of-trust checks, this argument is
* ignored.
* @return the result of the check.
* Set the public key store for web-of-trust checks.
* <p>
* If set, {@link #enableTrust(int, Map)} must also be called.
*/
public final CheckResult check(PGPPublicKey key, PublicKeyStore store) {
if (trusted == null) {
return check(key);
} else if (store == null) {
throw new IllegalArgumentException(
"PublicKeyStore required for web of trust checks");
public PublicKeyChecker setStore(PublicKeyStore store) {
if (store == null) {
throw new IllegalArgumentException("PublicKeyStore is required");
}
return check(key, store, 0, true, new HashSet<Fingerprint>());
this.store = store;
return this;
}
/**
* Check only a public key, not including its web of trust.
* Check a public key.
*
* @param key the public key.
* @return the result of the check.
*/
public final CheckResult check(PGPPublicKey key) {
return check(key, null, 0, false, null);
if (store == null && trusted != null) {
throw new IllegalStateException("PublicKeyStore is required");
}
return check(key, 0, true,
trusted != null ? new HashSet<Fingerprint>() : null);
}
/**
@@ -115,16 +118,16 @@ public class PublicKeyChecker {
*
* @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}.
* PGPPublicKey)}: 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, int depth) {
return CheckResult.ok();
}
private CheckResult check(PGPPublicKey key, PublicKeyStore store, int depth,
boolean expand, Set<Fingerprint> seen) {
private CheckResult check(PGPPublicKey key, int depth, boolean expand,
Set<Fingerprint> seen) {
CheckResult basicResult = checkBasic(key);
CheckResult customResult = checkCustom(key, depth);
CheckResult trustResult = checkWebOfTrust(key, store, depth, seen);
@@ -181,7 +184,7 @@ public class PublicKeyChecker {
private CheckResult checkWebOfTrust(PGPPublicKey key, PublicKeyStore store,
int depth, Set<Fingerprint> seen) {
if (trusted == null || store == null) {
if (trusted == null) {
// Trust checking not configured, server trusts all OK keys.
return CheckResult.trusted();
}
@@ -222,8 +225,7 @@ public class PublicKeyChecker {
}
String subpacketProblem = checkTrustSubpacket(sig, depth);
if (subpacketProblem == null) {
CheckResult signerResult =
check(signer, store, depth + 1, false, seen);
CheckResult signerResult = check(signer, depth + 1, false, seen);
if (signerResult.isTrusted()) {
return CheckResult.trusted();
}

View File

@@ -202,7 +202,7 @@ public abstract class PushCertificateChecker {
CheckResult.bad("Signature by " + keyIdToString(sig.getKeyID())
+ " is not valid"));
}
CheckResult result = publicKeyChecker.check(signer, store);
CheckResult result = publicKeyChecker.check(signer);
if (!result.getProblems().isEmpty()) {
StringBuilder err = new StringBuilder("Invalid public key ")
.append(keyToString(signer))

View File

@@ -163,7 +163,7 @@ public class GpgKeys implements
found = true;
GpgKeyInfo info = toJson(
keyRing.getPublicKey(),
checkerFactory.create(rsrc.getUser()),
checkerFactory.create().setExpectedUser(rsrc.getUser()),
store);
keys.put(info.id, info);
info.id = null;
@@ -197,7 +197,7 @@ public class GpgKeys implements
try (PublicKeyStore store = storeProvider.get()) {
return toJson(
rsrc.getKeyRing().getPublicKey(),
checkerFactory.create(rsrc.getUser()),
checkerFactory.create().setExpectedUser(rsrc.getUser()),
store);
}
}
@@ -261,7 +261,7 @@ public class GpgKeys implements
static GpgKeyInfo toJson(PGPPublicKey key, PublicKeyChecker checker,
PublicKeyStore store) throws IOException {
return toJson(key, checker.check(key, store));
return toJson(key, checker.setStore(store).check(key));
}
public static void toJson(GpgKeyInfo info, CheckResult checkResult) {

View File

@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.gpg.CheckResult;
import com.google.gerrit.gpg.Fingerprint;
import com.google.gerrit.gpg.GerritPublicKeyChecker;
import com.google.gerrit.gpg.PublicKeyChecker;
import com.google.gerrit.gpg.PublicKeyStore;
import com.google.gerrit.gpg.server.PostGpgKeys.Input;
import com.google.gerrit.reviewdb.client.AccountExternalId;
@@ -193,7 +194,10 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
for (PGPPublicKeyRing keyRing : keyRings) {
PGPPublicKey key = keyRing.getPublicKey();
// Don't check web of trust; admins can fill in certifications later.
CheckResult result = checkerFactory.create(rsrc.getUser()).check(key);
CheckResult result = checkerFactory.create()
.setExpectedUser(rsrc.getUser())
.disableTrust()
.check(key);
if (!result.isOk()) {
throw new BadRequestException(String.format(
"Problems with public key %s:\n%s",
@@ -245,12 +249,14 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
throws IOException {
// Unlike when storing keys, include web-of-trust checks when producing
// result JSON, so the user at least knows of any issues.
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.setStore(store);
Map<String, GpgKeyInfo> infos =
Maps.newHashMapWithExpectedSize(keys.size() + deleted.size());
for (PGPPublicKeyRing keyRing : keys) {
PGPPublicKey key = keyRing.getPublicKey();
CheckResult result = checker.check(key, store);
CheckResult result = checker.check(key);
GpgKeyInfo info = GpgKeys.toJson(key, result);
infos.put(info.id, info);
info.id = null;

View File

@@ -172,7 +172,9 @@ public class GerritPublicKeyCheckerTest {
@Test
public void defaultGpgCertificationMatchesEmail() throws Exception {
TestKey key = validKeyWithSecondUserId();
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertProblems(
checker.check(key.getPublicKey()), Status.BAD,
"Key must contain a valid certification for one of the following "
@@ -181,14 +183,18 @@ public class GerritPublicKeyCheckerTest {
+ " username:user");
addExternalId("test", "test", "test5@example.com");
checker = checkerFactory.create(user);
checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertNoProblems(checker.check(key.getPublicKey()));
}
@Test
public void defaultGpgCertificationDoesNotMatchEmail() throws Exception {
addExternalId("test", "test", "nobody@example.com");
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertProblems(
checker.check(validKeyWithSecondUserId().getPublicKey()), Status.BAD,
"Key must contain a valid certification for one of the following "
@@ -202,14 +208,18 @@ public class GerritPublicKeyCheckerTest {
@Test
public void manualCertificationMatchesExternalId() throws Exception {
addExternalId("foo", "myId", null);
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertNoProblems(checker.check(validKeyWithSecondUserId().getPublicKey()));
}
@Test
public void manualCertificationDoesNotMatchExternalId() throws Exception {
addExternalId("foo", "otherId", null);
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertProblems(
checker.check(validKeyWithSecondUserId().getPublicKey()), Status.BAD,
"Key must contain a valid certification for one of the following "
@@ -226,13 +236,16 @@ public class GerritPublicKeyCheckerTest {
reloadUser();
TestKey key = validKeyWithSecondUserId();
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.disableTrust();
assertProblems(
checker.check(key.getPublicKey()), Status.BAD,
"No identities found for user; check"
+ " http://test/#/settings/web-identities");
checker = checkerFactory.create();
checker = checkerFactory.create()
.disableTrust();
assertProblems(
checker.check(key.getPublicKey()), Status.BAD,
"Key is not associated with any users");
@@ -264,14 +277,18 @@ public class GerritPublicKeyCheckerTest {
add(keyE(), addUser("userE"));
// Checker for A, checking A.
GerritPublicKeyChecker checkerA = checkerFactory.create(user);
assertNoProblems(checkerA.check(keyA.getPublicKey(), store));
PublicKeyChecker checkerA = checkerFactory.create()
.setExpectedUser(user)
.setStore(store);
assertNoProblems(checkerA.check(keyA.getPublicKey()));
// 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);
PublicKeyChecker checkerB = checkerFactory.create()
.setExpectedUser(userB)
.setStore(store);
assertProblems(
checkerB.check(keyB.getPublicKey(), store), Status.BAD,
checkerB.check(keyB.getPublicKey()), Status.BAD,
"Key is expired");
}
@@ -294,9 +311,11 @@ public class GerritPublicKeyCheckerTest {
add(keyE(), addUser("userE"));
// Checker for A, checking B.
GerritPublicKeyChecker checkerA = checkerFactory.create(user);
PublicKeyChecker checkerA = checkerFactory.create()
.setExpectedUser(user)
.setStore(store);
assertProblems(
checkerA.check(keyB.getPublicKey(), store), Status.BAD,
checkerA.check(keyB.getPublicKey()), Status.BAD,
"Key is expired",
"Key must contain a valid certification for one of the following"
+ " identities:\n"
@@ -306,9 +325,11 @@ public class GerritPublicKeyCheckerTest {
+ " username:user");
// Checker for B, checking A.
GerritPublicKeyChecker checkerB = checkerFactory.create(userB);
PublicKeyChecker checkerB = checkerFactory.create()
.setExpectedUser(userB)
.setStore(store);
assertProblems(
checkerB.check(keyA.getPublicKey(), store), Status.BAD,
checkerB.check(keyA.getPublicKey()), Status.BAD,
"Key must contain a valid certification for one of the following"
+ " identities:\n"
+ " gerrit:userB\n"
@@ -325,9 +346,11 @@ public class GerritPublicKeyCheckerTest {
TestKey keyA = add(keyA(), user);
TestKey keyB = add(keyB(), addUser("userB"));
GerritPublicKeyChecker checker = checkerFactory.create(user);
PublicKeyChecker checker = checkerFactory.create()
.setExpectedUser(user)
.setStore(store);
assertProblems(
checker.check(keyA.getPublicKey(), store), Status.OK,
checker.check(keyA.getPublicKey()), Status.OK,
"No path to a trusted key",
"Certification by " + keyToString(keyB.getPublicKey())
+ " is valid, but key is not trusted",
@@ -352,15 +375,16 @@ public class GerritPublicKeyCheckerTest {
// 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));
PublicKeyChecker checker = checkerFactory.create()
.setStore(store);
assertNoProblems(checker.check(keyA.getPublicKey()));
assertProblems(
checker.check(keyB.getPublicKey(), store), Status.BAD,
checker.check(keyB.getPublicKey()), Status.BAD,
"Key is expired");
assertNoProblems(checker.check(keyC.getPublicKey(), store));
assertNoProblems(checker.check(keyD.getPublicKey(), store));
assertNoProblems(checker.check(keyC.getPublicKey()));
assertNoProblems(checker.check(keyD.getPublicKey()));
assertProblems(
checker.check(keyE.getPublicKey(), store), Status.BAD,
checker.check(keyE.getPublicKey()), Status.BAD,
"Key is expired",
"No path to a trusted key");
}
@@ -382,8 +406,10 @@ public class GerritPublicKeyCheckerTest {
keyRingB = PGPPublicKeyRing.insertPublicKey(keyRingB, keyB);
add(keyRingB, addUser("userB"));
GerritPublicKeyChecker checkerA = checkerFactory.create(user);
assertProblems(checkerA.check(keyA.getPublicKey(), store), Status.OK,
PublicKeyChecker checkerA = checkerFactory.create()
.setExpectedUser(user)
.setStore(store);
assertProblems(checkerA.check(keyA.getPublicKey()), Status.OK,
"No path to a trusted key",
"Certification by " + keyToString(keyB)
+ " is valid, but key is not trusted",

View File

@@ -44,9 +44,9 @@ import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.HashMap;
import java.util.Map;
public class PublicKeyCheckerTest {
@Rule
@@ -175,11 +175,14 @@ public class PublicKeyCheckerTest {
}
private PublicKeyChecker newChecker(int maxTrustDepth, TestKey... trusted) {
List<Fingerprint> fps = new ArrayList<>(trusted.length);
Map<Long, Fingerprint> fps = new HashMap<>();
for (TestKey k : trusted) {
fps.add(new Fingerprint(k.getPublicKey().getFingerprint()));
Fingerprint fp = new Fingerprint(k.getPublicKey().getFingerprint());
fps.put(fp.getId(), fp);
}
return new PublicKeyChecker(maxTrustDepth, fps);
return new PublicKeyChecker()
.enableTrust(maxTrustDepth, fps)
.setStore(store);
}
private TestKey add(TestKey k) {
@@ -205,12 +208,12 @@ public class PublicKeyCheckerTest {
private void assertProblems(PublicKeyChecker checker, TestKey k,
String... expected) {
CheckResult result = checker.check(k.getPublicKey(), store);
CheckResult result = checker.check(k.getPublicKey());
assertEquals(Arrays.asList(expected), result.getProblems());
}
private void assertProblems(TestKey tk, String... expected) throws Exception {
CheckResult result = new PublicKeyChecker().check(tk.getPublicKey(), store);
CheckResult result = new PublicKeyChecker().check(tk.getPublicKey());
assertEquals(Arrays.asList(expected), result.getProblems());
}