From d40d70a15f686a859a57b128a8cd625d3f69f130 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 3 Aug 2015 13:33:59 -0700 Subject: [PATCH] PublicKeyStore: Support removing keys Since we support multiple keys per ID, keys are removed by fingerprint. Add a Fingerprint class for this purpose that is an immutable, hashable view of a fingerprint. Change-Id: Ifb90ef2b6bfab1ce63a9fe7b8796505980572d04 --- .../acceptance/api/accounts/AccountIT.java | 20 ++++ .../gerrit/server/git/gpg/Fingerprint.java | 67 +++++++++++++ .../gerrit/server/git/gpg/PublicKeyStore.java | 73 ++++++++++++-- .../server/git/gpg/PublicKeyStoreTest.java | 96 ++++++++++++++++--- 4 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 6d11bb22b9..7bc02dd235 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -31,6 +31,8 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.reviewdb.client.AccountExternalId; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.gpg.PublicKeyStore; import com.google.gerrit.server.git.gpg.TestKey; import com.google.inject.Inject; @@ -39,6 +41,9 @@ import com.google.inject.Provider; import org.bouncycastle.bcpg.ArmoredOutputStream; import org.bouncycastle.openpgp.PGPPublicKey; import org.bouncycastle.openpgp.PGPPublicKeyRing; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.PushCertificateIdent; import org.junit.After; import org.junit.Before; @@ -55,6 +60,9 @@ public class AccountIT extends AbstractDaemonTest { @Inject private Provider publicKeyStoreProvider; + @Inject + private AllUsersName allUsers; + private List savedExternalIds; @Before @@ -71,6 +79,18 @@ public class AccountIT extends AbstractDaemonTest { db.accountExternalIds().insert(savedExternalIds); } + @After + public void clearPublicKeyStore() throws Exception { + try (Repository repo = repoManager.openRepository(allUsers)) { + Ref ref = repo.getRef(RefNames.REFS_GPG_KEYS); + if (ref != null) { + RefUpdate ru = repo.updateRef(RefNames.REFS_GPG_KEYS); + ru.setForceUpdate(true); + assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); + } + } + } + private List getExternalIds(TestAccount account) throws Exception { return db.accountExternalIds().byAccount(account.getId()).toList(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java new file mode 100644 index 0000000000..6209a1477f --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/Fingerprint.java @@ -0,0 +1,67 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.git.gpg; + +import static com.google.common.base.Preconditions.checkArgument; + +import org.eclipse.jgit.util.NB; + +import java.nio.ByteBuffer; +import java.util.Arrays; + +class Fingerprint { + private final byte[] fp; + + Fingerprint(byte[] fp) { + // Don't bother with defensive copies; PGPPublicKey#getFingerprint() already + // does so. + checkArgument(fp.length == 20, + "fingerprint must be 20 bytes, got %s", fp.length); + this.fp = fp; + } + + byte[] get() { + return fp; + } + + boolean equalsBytes(byte[] bytes) { + return Arrays.equals(fp, bytes); + } + + @Override + public int hashCode() { + // Same hash code as ObjectId: second int word. + return NB.decodeInt32(fp, 4); + } + + @Override + public boolean equals(Object o) { + return (o instanceof Fingerprint) && equalsBytes(((Fingerprint) o).fp); + } + + @Override + public String toString() { + ByteBuffer buf = ByteBuffer.wrap(fp); + return String.format( + "(%04X %04X %04X %04X %04X %04X %04X %04X %04X %04X)", + buf.getShort(), buf.getShort(), buf.getShort(), buf.getShort(), + buf.getShort(), buf.getShort(), buf.getShort(), buf.getShort(), + buf.getShort(), buf.getShort()); + } + + long getId() { + return ByteBuffer.wrap(fp).getLong(12); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PublicKeyStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PublicKeyStore.java index 00436f193f..b21628151d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PublicKeyStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/gpg/PublicKeyStore.java @@ -46,8 +46,12 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; +import java.util.Set; /** * Store of GPG public keys in git notes. @@ -66,16 +70,21 @@ import java.util.List; * only trust keys after checking with a {@link PublicKeyChecker}. */ public class PublicKeyStore implements AutoCloseable { + private static final ObjectId EMPTY_TREE = + ObjectId.fromString("4b825dc642cb6eb9a060e54bf8d69288fbee4904"); + private final Repository repo; private ObjectReader reader; private RevCommit tip; private NoteMap notes; - private List pending; + private Map toAdd; + private Set toRemove; /** @param repo repository to read keys from. */ public PublicKeyStore(Repository repo) { this.repo = repo; - pending = new ArrayList<>(); + toAdd = new HashMap<>(); + toRemove = new HashSet<>(); } @Override @@ -169,7 +178,23 @@ public class PublicKeyStore implements AutoCloseable { throw new IllegalArgumentException( "Exactly 1 master key is required, found " + numMaster); } - pending.add(keyRing); + Fingerprint fp = new Fingerprint(keyRing.getPublicKey().getFingerprint()); + toAdd.put(fp, keyRing); + toRemove.remove(fp); + } + + /** + * Remove a public key from the store. + *

+ * Multiple calls may be made to buffer deletes in memory, and they are not + * saved until {@link #save(CommitBuilder)} is called. + * + * @param fingerprint the fingerprint of the key to remove. + */ + public void remove(byte[] fingerprint) { + Fingerprint fp = new Fingerprint(fingerprint); + toAdd.remove(fp); + toRemove.add(fp); } /** @@ -185,7 +210,7 @@ public class PublicKeyStore implements AutoCloseable { */ public RefUpdate.Result save(CommitBuilder cb) throws PGPException, IOException { - if (pending.isEmpty()) { + if (toAdd.isEmpty() && toRemove.isEmpty()) { return RefUpdate.Result.NO_CHANGE; } if (reader == null) { @@ -196,16 +221,25 @@ public class PublicKeyStore implements AutoCloseable { } ObjectId newTip; try (ObjectInserter ins = repo.newObjectInserter()) { - for (PGPPublicKeyRing keyRing : pending) { + for (PGPPublicKeyRing keyRing : toAdd.values()) { saveToNotes(ins, keyRing); } + for (Fingerprint fp : toRemove) { + deleteFromNotes(ins, fp); + } + cb.setTreeId(notes.writeTree(ins)); + if (cb.getTreeId().equals( + tip != null ? tip.getTree() : EMPTY_TREE)) { + return RefUpdate.Result.NO_CHANGE; + } + if (tip != null) { cb.setParentId(tip); } - cb.setTreeId(notes.writeTree(ins)); if (cb.getMessage() == null) { - cb.setMessage(String.format("Update %d public key%s", - pending.size(), pending.size() != 1 ? "s" : "")); + int n = toAdd.size() + toRemove.size(); + cb.setMessage( + String.format("Update %d public key%s", n, n != 1 ? "s" : "")); } newTip = ins.insert(cb); ins.flush(); @@ -222,7 +256,8 @@ public class PublicKeyStore implements AutoCloseable { case FAST_FORWARD: case NEW: case NO_CHANGE: - pending.clear(); + toAdd.clear(); + toRemove.clear(); break; default: break; @@ -251,6 +286,26 @@ public class PublicKeyStore implements AutoCloseable { ins.insert(OBJ_BLOB, keysToArmored(toWrite))); } + private void deleteFromNotes(ObjectInserter ins, Fingerprint fp) + throws PGPException, IOException { + long keyId = fp.getId(); + PGPPublicKeyRingCollection existing = get(keyId); + List toWrite = new ArrayList<>(existing.size()); + for (PGPPublicKeyRing kr : existing) { + if (!fp.equalsBytes(kr.getPublicKey().getFingerprint())) { + toWrite.add(kr); + } + } + if (toWrite.size() == existing.size()) { + return; + } else if (toWrite.size() > 0) { + notes.set(keyObjectId(keyId), + ins.insert(OBJ_BLOB, keysToArmored(toWrite))); + } else { + notes.remove(keyObjectId(keyId)); + } + } + private static boolean sameKey(PGPPublicKeyRing kr1, PGPPublicKeyRing kr2) { return Arrays.equals(kr1.getPublicKey().getFingerprint(), kr2.getPublicKey().getFingerprint()); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/PublicKeyStoreTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/PublicKeyStoreTest.java index 907ad156e8..c84757eb77 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/PublicKeyStoreTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/gpg/PublicKeyStoreTest.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyObjectId; import static com.google.gerrit.server.git.gpg.PublicKeyStore.keyToString; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import com.google.gerrit.reviewdb.client.RefNames; @@ -38,6 +39,10 @@ import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Before; import org.junit.Test; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Iterator; +import java.util.List; import java.util.Set; import java.util.TreeSet; @@ -115,11 +120,7 @@ public class PublicKeyStoreTest { store.add(key1.getPublicKeyRing()); store.add(key2.getPublicKeyRing()); - CommitBuilder cb = new CommitBuilder(); - PersonIdent ident = new PersonIdent("A U Thor", "author@example.com"); - cb.setAuthor(ident); - cb.setCommitter(ident); - assertEquals(RefUpdate.Result.NEW, store.save(cb)); + assertEquals(RefUpdate.Result.NEW, store.save(newCommitBuilder())); assertKeys(key1.getKeyId(), key1); assertKeys(key2.getKeyId(), key2); @@ -136,12 +137,7 @@ public class PublicKeyStoreTest { .create(); store.add(key1.getPublicKeyRing()); - - CommitBuilder cb = new CommitBuilder(); - PersonIdent ident = new PersonIdent("A U Thor", "author@example.com"); - cb.setAuthor(ident); - cb.setCommitter(ident); - assertEquals(RefUpdate.Result.FAST_FORWARD, store.save(cb)); + assertEquals(RefUpdate.Result.FAST_FORWARD, store.save(newCommitBuilder())); assertKeys(key1.getKeyId(), key1, key2); @@ -161,6 +157,63 @@ public class PublicKeyStoreTest { } } + @Test + public void updateExisting() throws Exception { + TestKey key5 = TestKey.key5(); + PGPPublicKeyRing keyRing = key5.getPublicKeyRing(); + PGPPublicKey key = keyRing.getPublicKey(); + store.add(keyRing); + assertEquals(RefUpdate.Result.NEW, store.save(newCommitBuilder())); + + assertUserIds(store.get(key5.getKeyId()).iterator().next(), + "Testuser Five ", + "foo:myId"); + + keyRing = PGPPublicKeyRing.removePublicKey(keyRing, key); + key = PGPPublicKey.removeCertification(key, "foo:myId"); + keyRing = PGPPublicKeyRing.insertPublicKey(keyRing, key); + store.add(keyRing); + assertEquals(RefUpdate.Result.FAST_FORWARD, store.save(newCommitBuilder())); + + Iterator keyRings = store.get(key.getKeyID()).iterator(); + keyRing = keyRings.next(); + assertFalse(keyRings.hasNext()); + assertUserIds(keyRing, "Testuser Five "); + } + + @Test + public void remove() throws Exception { + TestKey key1 = TestKey.key1(); + store.add(key1.getPublicKeyRing()); + assertEquals(RefUpdate.Result.NEW, store.save(newCommitBuilder())); + assertKeys(key1.getKeyId(), key1); + + store.remove(key1.getPublicKey().getFingerprint()); + assertEquals(RefUpdate.Result.FAST_FORWARD, store.save(newCommitBuilder())); + assertKeys(key1.getKeyId()); + } + + @Test + public void removeNonexisting() throws Exception { + TestKey key1 = TestKey.key1(); + store.add(key1.getPublicKeyRing()); + assertEquals(RefUpdate.Result.NEW, store.save(newCommitBuilder())); + + TestKey key2 = TestKey.key2(); + store.remove(key2.getPublicKey().getFingerprint()); + assertEquals(RefUpdate.Result.NO_CHANGE, store.save(newCommitBuilder())); + assertKeys(key1.getKeyId(), key1); + } + + @Test + public void addThenRemove() throws Exception { + TestKey key1 = TestKey.key1(); + store.add(key1.getPublicKeyRing()); + store.remove(key1.getPublicKey().getFingerprint()); + assertEquals(RefUpdate.Result.NO_CHANGE, store.save(newCommitBuilder())); + assertKeys(key1.getKeyId()); + } + private void assertKeys(long keyId, TestKey... expected) throws Exception { Set expectedStrings = new TreeSet<>(); @@ -174,4 +227,25 @@ public class PublicKeyStoreTest { } assertEquals(expectedStrings, actualStrings); } + + private void assertUserIds(PGPPublicKeyRing keyRing, String... expected) + throws Exception { + List actual = new ArrayList<>(); + @SuppressWarnings("unchecked") + Iterator userIds = store.get(keyRing.getPublicKey().getKeyID()) + .iterator().next().getPublicKey().getUserIDs(); + while (userIds.hasNext()) { + actual.add(userIds.next()); + } + + assertEquals(actual, Arrays.asList(expected)); + } + + private CommitBuilder newCommitBuilder() { + CommitBuilder cb = new CommitBuilder(); + PersonIdent ident = new PersonIdent("A U Thor", "author@example.com"); + cb.setAuthor(ident); + cb.setCommitter(ident); + return cb; + } }