opendev/gerrit
Code Issues Proposed changes

PublicKeyChecker: Remove expected key ID checks

Browse Source 
The only place where we passed in a key ID that was not directly
extracted from the public key in question was in
PushCertificateChecker, where we used the key ID from the signature.
However, the only class of problems this would catch is where a key
in the PublicKeyStore returned by get(id) has an ID other than "id".
At this point we're confident enough in the implementation of
PublicKeyStore that we don't need to worry about this.

Change-Id: I7220b987c38f939d7564a2be4684c08c3013a14c
This commit is contained in:
Dave Borowitz
2015-08-31 13:46:59 -04:00
parent cbb09f8375
commit 83125bab35
4 changed files with 5 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-gpg/src/main/java/com/google/gerrit/gpg/GerritPublicKeyChecker.java
View File

@@ -65,8 +65,7 @@ public class GerritPublicKeyChecker extends PublicKeyChecker {
}
@Override
public void checkCustom(PGPPublicKey key, long expectedKeyId,
List<String> problems) {
public void checkCustom(PGPPublicKey key, List<String> problems) {
try {
Set<String> allowedUserIds = getAllowedUserIds();
if (allowedUserIds.isEmpty()) {

22
gerrit-gpg/src/main/java/com/google/gerrit/gpg/PublicKeyChecker.java
View File

@@ -14,8 +14,6 @@
package com.google.gerrit.gpg;
import static com.google.gerrit.gpg.PublicKeyStore.keyIdToString;
import org.bouncycastle.openpgp.PGPPublicKey;
import java.util.ArrayList;
@@ -29,21 +27,7 @@ public class PublicKeyChecker {
* @param key the public key.
*/
public final CheckResult check(PGPPublicKey key) {
return check(key, key.getKeyID());
}
/**
* Check a public key.
*
* @param key the public key.
* @param expectedKeyId the key ID that the caller expects.
*/
public final CheckResult check(PGPPublicKey key, long expectedKeyId) {
List<String> problems = new ArrayList<>();
if (key.getKeyID() != expectedKeyId) {
problems.add(
"Public key does not match ID " + keyIdToString(expectedKeyId));
}
if (key.isRevoked()) {
// TODO(dborowitz): isRevoked is overeager:
// http://www.bouncycastle.org/jira/browse/BJB-45
@@ -58,7 +42,7 @@ public class PublicKeyChecker {
problems.add("Key is expired");
}
}
checkCustom(key, expectedKeyId, problems);
checkCustom(key, problems);
return new CheckResult(problems);
}
@@ -68,11 +52,9 @@ public class PublicKeyChecker {
* Default implementation does nothing, but may be overridden by subclasses.
*
* @param key the public key.
* @param expectedKeyId the key ID that the caller expects.
* @param problems list to which any problems should be added.
*/
public void checkCustom(PGPPublicKey key, long expectedKeyId,
List<String> problems) {
public void checkCustom(PGPPublicKey key, List<String> problems) {
// Default implementation does nothing.
}
}

2
gerrit-gpg/src/main/java/com/google/gerrit/gpg/PushCertificateChecker.java
View File

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

12
gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java
View File

@@ -36,16 +36,6 @@ public class PublicKeyCheckerTest {
assertProblems(TestKey.key1());
}
@Test
public void wrongKeyId() throws Exception {
TestKey k = TestKey.key1();
long badId = k.getKeyId() + 1;
CheckResult result = checker.check(k.getPublicKey(), badId);
assertEquals(
Arrays.asList("Public key does not match ID 46328A8D"),
result.getProblems());
}
@Test
public void keyExpiringInFuture() throws Exception {
assertProblems(TestKey.key2());
@@ -62,7 +52,7 @@ public class PublicKeyCheckerTest {
}
private void assertProblems(TestKey tk, String... expected) throws Exception {
CheckResult result = checker.check(tk.getPublicKey(), tk.getKeyId());
CheckResult result = checker.check(tk.getPublicKey());
assertEquals(Arrays.asList(expected), result.getProblems());
}
}