From b3ec3333cef2d2460cce5c49a0d3234176f91179 Mon Sep 17 00:00:00 2001
From: Paul Schaub <vanitasvitae@fsfe.org>
Date: Tue, 7 Dec 2021 14:42:03 +0100
Subject: [PATCH] CachingSecretKeyRingProtector: Prevent accidental passphrase
 override via addPassphrase()

---
 .../CachingSecretKeyRingProtector.java        | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/pgpainless-core/src/main/java/org/pgpainless/key/protection/CachingSecretKeyRingProtector.java b/pgpainless-core/src/main/java/org/pgpainless/key/protection/CachingSecretKeyRingProtector.java
index 4a58b5f9..a939c8af 100644
--- a/pgpainless-core/src/main/java/org/pgpainless/key/protection/CachingSecretKeyRingProtector.java
+++ b/pgpainless-core/src/main/java/org/pgpainless/key/protection/CachingSecretKeyRingProtector.java
@@ -23,6 +23,9 @@ import org.pgpainless.util.Passphrase;
  * Implementation of the {@link SecretKeyRingProtector} which holds a map of key ids and their passwords.
  * In case the needed passphrase is not contained in the map, the {@code missingPassphraseCallback} will be consulted,
  * and the passphrase is added to the map.
+ *
+ * If you need to unlock multiple {@link PGPKeyRing PGPKeyRings}, it is advised to use a separate
+ * {@link CachingSecretKeyRingProtector} instance for each ring.
  */
 public class CachingSecretKeyRingProtector implements SecretKeyRingProtector, SecretKeyPassphraseProvider {
 
@@ -52,21 +55,76 @@ public class CachingSecretKeyRingProtector implements SecretKeyRingProtector, Se
 
     /**
      * Add a passphrase to the cache.
+     * If the cache already contains a passphrase for the given key-id, a {@link IllegalArgumentException} is thrown.
+     * The reason for this is to prevent accidental override of passphrases when dealing with multiple key rings
+     * containing a key with the same key-id but different passphrases.
+     *
+     * If you can ensure that there will be no key-id clash, and you want to replace the passphrase, you can use
+     * {@link #replacePassphrase(Long, Passphrase)} to replace the passphrase.
      *
      * @param keyId id of the key
      * @param passphrase passphrase
      */
     public void addPassphrase(@Nonnull Long keyId, @Nonnull Passphrase passphrase) {
+        if (this.cache.containsKey(keyId)) {
+            throw new IllegalArgumentException("The cache already holds a passphrase for ID " + Long.toHexString(keyId) + ".\n" +
+                    "If you want to replace the passphrase, use replacePassphrase(Long, Passphrase) instead.");
+        }
+        this.cache.put(keyId, passphrase);
+    }
+
+    /**
+     * Replace the passphrase for the given key-id in the cache.
+     *
+     * @param keyId keyId
+     * @param passphrase passphrase
+     */
+    public void replacePassphrase(@Nonnull Long keyId, @Nonnull Passphrase passphrase) {
         this.cache.put(keyId, passphrase);
     }
 
     /**
      * Remember the given passphrase for all keys in the given key ring.
+     * If for the key-id of any key on the key ring the cache already contains a passphrase, a
+     * {@link IllegalArgumentException} is thrown before any changes are committed to the cache.
+     * This is to prevent accidental passphrase override when dealing with multiple key rings containing
+     * keys with conflicting key-ids.
+     *
+     * If you can ensure that there will be no key-id clashes and you want to replace the passphrases for the key ring,
+     * use {@link #replacePassphrase(PGPKeyRing, Passphrase)} instead.
+     *
+     * If you need to unlock multiple {@link PGPKeyRing PGPKeyRings}, it is advised to use a separate
+     * {@link CachingSecretKeyRingProtector} instance for each ring.
      *
      * @param keyRing key ring
      * @param passphrase passphrase
      */
     public void addPassphrase(@Nonnull PGPKeyRing keyRing, @Nonnull Passphrase passphrase) {
+        Iterator<PGPPublicKey> keys = keyRing.getPublicKeys();
+        // check for existing passphrases before doing anything
+        while (keys.hasNext()) {
+            long keyId = keys.next().getKeyID();
+            if (cache.containsKey(keyId)) {
+                throw new IllegalArgumentException("The cache already holds a passphrase for ID " + Long.toHexString(keyId) + ".\n" +
+                        "If you want to replace the passphrase, use replacePassphrase(PGPKeyRing, Passphrase) instead.");
+            }
+        }
+
+        // only then insert
+        keys = keyRing.getPublicKeys();
+        while (keys.hasNext()) {
+            PGPPublicKey publicKey = keys.next();
+            addPassphrase(publicKey, passphrase);
+        }
+    }
+
+    /**
+     * Replace the cached passphrases for all keys in the key ring with the provided passphrase.
+     *
+     * @param keyRing key ring
+     * @param passphrase passphrase
+     */
+    public void replacePassphrase(@Nonnull PGPKeyRing keyRing, @Nonnull Passphrase passphrase) {
         Iterator<PGPPublicKey> keys = keyRing.getPublicKeys();
         while (keys.hasNext()) {
             PGPPublicKey publicKey = keys.next();