opendev/gerrit
Code Issues Proposed changes

PublicKeyChecker: Drop expected user ID certification checks

Browse Source 
This was originally intended to match up the pusher identity from the
push cert to a certified user ID associated with the public key.
However, the pusher identity provided by the C git client is just
whatever the user used to identify their GPG key with the
user.signingkey config option. In effect, it only reflects the local
setup of the client's machine.

If we want to do some additional check that the OpenPGP User ID is
associated with a particular Gerrit user, that can be done, but would
be based on certifications signed by a trusted key stored entirely
within the public key on the server.

Change-Id: I90f60c1ab83703979218e2fac688e4b200c05895
This commit is contained in:
Dave Borowitz
2015-07-09 15:15:20 -07:00
parent 61003e99ce
commit 2896e2308a
3 changed files with 6 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PublicKeyChecker.java
View File

@@ -15,38 +15,21 @@
package com.google.gerrit.server.git.gpg;
import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyIdToString;
import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyToString;
import static org.bouncycastle.openpgp.PGPSignature.CERTIFICATION_REVOCATION;
import static org.bouncycastle.openpgp.PGPSignature.DEFAULT_CERTIFICATION;
import static org.bouncycastle.openpgp.PGPSignature.POSITIVE_CERTIFICATION;
import org.bouncycastle.openpgp.PGPException;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPSignature;
import org.bouncycastle.openpgp.operator.bc.BcPGPContentVerifierBuilderProvider;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
/** Checker for GPG public keys for use in a push certificate. */
public class PublicKeyChecker {
private static final Logger log =
LoggerFactory.getLogger(PublicKeyChecker.class);
/**
* Check a public key.
*
* @param key the public key.
* @param expectedKeyId the key ID that the caller expects.
* @param expectedUserId a user ID that the caller expects to be present and
* correct.
*/
public final CheckResult check(PGPPublicKey key, long expectedKeyId,
String expectedUserId) {
public final CheckResult check(PGPPublicKey key, long expectedKeyId) {
List<String> problems = new ArrayList<>();
if (key.getKeyID() != expectedKeyId) {
problems.add(
@@ -66,8 +49,7 @@ public class PublicKeyChecker {
problems.add("Key is expired");
}
}
checkCertifications(key, expectedUserId, problems);
checkCustom(key, expectedKeyId, expectedUserId, problems);
checkCustom(key, expectedKeyId, problems);
return new CheckResult(problems);
}
@@ -78,57 +60,10 @@ public class PublicKeyChecker {
*
* @param key the public key.
* @param expectedKeyId the key ID that the caller expects.
* @param expectedUserId a user ID that the caller expects to be present and
* correct.
* @param problems list to which any problems should be added.
*/
public void checkCustom(PGPPublicKey key, long expectedKeyId,
String expectedUserId, List<String> problems) {
List<String> problems) {
// Default implementation does nothing.
}
// TODO(dborowitz): Remove some/all of these checks.
private static void checkCertifications(PGPPublicKey key, String userId,
List<String> problems) {
@SuppressWarnings("unchecked")
Iterator<PGPSignature> sigs = key.getSignaturesForID(userId);
if (sigs == null) {
sigs = Collections.emptyIterator();
}
boolean ok = false;
boolean revoked = false;
try {
while (sigs.hasNext()) {
PGPSignature sig = sigs.next();
if (sig.getKeyID() != key.getKeyID()) {
// TODO(dborowitz): Support certifications by other trusted keys?
continue;
} else if (sig.getSignatureType() != DEFAULT_CERTIFICATION
&& sig.getSignatureType() != POSITIVE_CERTIFICATION
&& sig.getSignatureType() != CERTIFICATION_REVOCATION) {
continue;
}
sig.init(new BcPGPContentVerifierBuilderProvider(), key);
if (sig.verifyCertification(userId, key)) {
if (sig.getSignatureType() == CERTIFICATION_REVOCATION) {
revoked = true;
} else {
ok = true;
}
} else {
problems.add("Invalid signature for User ID " + userId);
}
}
} catch (PGPException e) {
problems.add("Error in certifications");
log.warn("Error in certification verification for public key: "
+ keyToString(key), e);
}
if (revoked) {
problems.add("User ID " + userId + " is revoked");
} else if (!ok) {
problems.add("No certification for User ID " + userId);
}
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PushCertificateChecker.java
View File

@@ -136,8 +136,7 @@ public abstract class PushCertificateChecker {
"Signature not valid with public key: " + keyToString(k));
continue;
}
CheckResult result = publicKeyChecker.check(
k, sig.getKeyID(), cert.getPusherIdent().getUserId());
CheckResult result = publicKeyChecker.check(k, sig.getKeyID());
if (result.isOk()) {
return;
}

16
gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/PublicKeyCheckerTest.java
View File

@@ -38,23 +38,12 @@ public class PublicKeyCheckerTest {
public void wrongKeyId() throws Exception {
TestKey k = TestKey.key1();
long badId = k.getKeyId() + 1;
CheckResult result = checker.check(
k.getPublicKey(), badId, k.getFirstUserId());
CheckResult result = checker.check(k.getPublicKey(), badId);
assertEquals(
Arrays.asList("Public key does not match ID 46328A8D"),
result.getProblems());
}
@Test
public void wrongUserId() throws Exception {
TestKey k = TestKey.key1();
CheckResult result = checker.check(
k.getPublicKey(), k.getKeyId(), "test2@example.com");
assertEquals(
Arrays.asList("No certification for User ID test2@example.com"),
result.getProblems());
}
@Test
public void keyExpiringInFuture() throws Exception {
assertProblems(TestKey.key2());
@@ -71,8 +60,7 @@ public class PublicKeyCheckerTest {
}
private void assertProblems(TestKey tk, String... expected) throws Exception {
CheckResult result = checker.check(
tk.getPublicKey(), tk.getKeyId(), tk.getFirstUserId());
CheckResult result = checker.check(tk.getPublicKey(), tk.getKeyId());
assertEquals(Arrays.asList(expected), result.getProblems());
}
}