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

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<PublicKeyStore> publicKeyStoreProvider;
 
+  @Inject
+  private AllUsersName allUsers;
+
   private List<AccountExternalId> 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<AccountExternalId> getExternalIds(TestAccount account)
       throws Exception {
     return db.accountExternalIds().byAccount(account.getId()).toList();

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);
+  }
+}

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<PGPPublicKeyRing> pending;
+  private Map<Fingerprint, PGPPublicKeyRing> toAdd;
+  private Set<Fingerprint> 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.
+   * <p>
+   * 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<PGPPublicKeyRing> 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());

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 <test5@example.com>",
+        "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<PGPPublicKeyRing> keyRings = store.get(key.getKeyID()).iterator();
+    keyRing = keyRings.next();
+    assertFalse(keyRings.hasNext());
+    assertUserIds(keyRing, "Testuser Five <test5@example.com>");
+  }
+
+  @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<String> expectedStrings = new TreeSet<>();
@@ -174,4 +227,25 @@ public class PublicKeyStoreTest {
     }
     assertEquals(expectedStrings, actualStrings);
   }
+
+  private void assertUserIds(PGPPublicKeyRing keyRing, String... expected)
+      throws Exception {
+    List<String> actual = new ArrayList<>();
+    @SuppressWarnings("unchecked")
+    Iterator<String> 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;
+  }
 }