Change SymmetricEncryptionAlgorithmNegotiator to return the 'best' avail. alg

pgpainless-core/src/main/java/org/pgpainless/algorithm/negotiation/SymmetricKeyAlgorithmNegotiator.java

@@ -17,7 +17,7 @@ package org.pgpainless.algorithm.negotiation;
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Comparator;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -53,8 +53,8 @@ public interface SymmetricKeyAlgorithmNegotiator {
                     return override;
                 }
 
+                // Count score (occurrences) of each algorithm
                 Map<SymmetricKeyAlgorithm, Integer> supportWeight = new LinkedHashMap<>();
-
                 for (Set<SymmetricKeyAlgorithm> keyPreferences : preferences) {
                     for (SymmetricKeyAlgorithm preferred : keyPreferences) {
                         if (supportWeight.containsKey(preferred)) {
@@ -65,21 +65,33 @@ public interface SymmetricKeyAlgorithmNegotiator {
                     }
                 }
 
-                List<SymmetricKeyAlgorithm> scoreboard = new ArrayList<>(supportWeight.keySet());
+                // Pivot the score map
-                // Sort scoreboard by descending popularity
+                Map<Integer, List<SymmetricKeyAlgorithm>> byScore = new HashMap<>();
-                Collections.sort(scoreboard, new Comparator<SymmetricKeyAlgorithm>() {
+                for (SymmetricKeyAlgorithm algorithm : supportWeight.keySet()) {
-                    @Override
+                    int score = supportWeight.get(algorithm);
-                    public int compare(SymmetricKeyAlgorithm t0, SymmetricKeyAlgorithm t1) {
+                    List<SymmetricKeyAlgorithm> withSameScore = byScore.get(score);
-                        return -supportWeight.get(t0).compareTo(supportWeight.get(t1));
+                    if (withSameScore == null) {
+                        withSameScore = new ArrayList<>();
+                        byScore.put(score, withSameScore);
+                    }
+                    withSameScore.add(algorithm);
                 }
-                });
 
-                for (SymmetricKeyAlgorithm mostWanted : scoreboard) {
+                List<Integer> scores = new ArrayList<>(byScore.keySet());
-                    if (policy.isAcceptable(mostWanted)) {
+
-                        return mostWanted;
+                // Sort map and iterate from highest to lowest score
+                Collections.sort(scores);
+                for (int i = scores.size() - 1; i >= 0; i--) {
+                    int score = scores.get(i);
+                    List<SymmetricKeyAlgorithm> withSameScore = byScore.get(score);
+                    // Select best algorithm
+                    SymmetricKeyAlgorithm best = policy.selectBest(withSameScore);
+                    if (best != null) {
+                        return best;
                     }
                 }
 
+                // If no algorithm is acceptable, choose fallback
                 return policy.getDefaultSymmetricKeyAlgorithm();
             }
         };

pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java

@@ -231,14 +231,14 @@ public final class Policy {
         public static SymmetricKeyAlgorithmPolicy defaultSymmetricKeyEncryptionAlgorithmPolicy() {
             return new SymmetricKeyAlgorithmPolicy(SymmetricKeyAlgorithm.AES_256, Arrays.asList(
                     // Reject: Unencrypted, IDEA, TripleDES, CAST5
-                    SymmetricKeyAlgorithm.BLOWFISH,
-                    SymmetricKeyAlgorithm.AES_128,
-                    SymmetricKeyAlgorithm.AES_192,
                     SymmetricKeyAlgorithm.AES_256,
+                    SymmetricKeyAlgorithm.AES_192,
+                    SymmetricKeyAlgorithm.AES_128,
+                    SymmetricKeyAlgorithm.BLOWFISH,
                     SymmetricKeyAlgorithm.TWOFISH,
-                    SymmetricKeyAlgorithm.CAMELLIA_128,
+                    SymmetricKeyAlgorithm.CAMELLIA_256,
                     SymmetricKeyAlgorithm.CAMELLIA_192,
-                    SymmetricKeyAlgorithm.CAMELLIA_256
+                    SymmetricKeyAlgorithm.CAMELLIA_128
             ));
         }
 
@@ -251,16 +251,34 @@ public final class Policy {
             return new SymmetricKeyAlgorithmPolicy(SymmetricKeyAlgorithm.AES_256, Arrays.asList(
                     // Reject: Unencrypted, IDEA, TripleDES
                     SymmetricKeyAlgorithm.CAST5,
-                    SymmetricKeyAlgorithm.BLOWFISH,
-                    SymmetricKeyAlgorithm.AES_128,
-                    SymmetricKeyAlgorithm.AES_192,
                     SymmetricKeyAlgorithm.AES_256,
+                    SymmetricKeyAlgorithm.AES_192,
+                    SymmetricKeyAlgorithm.AES_128,
+                    SymmetricKeyAlgorithm.BLOWFISH,
                     SymmetricKeyAlgorithm.TWOFISH,
-                    SymmetricKeyAlgorithm.CAMELLIA_128,
+                    SymmetricKeyAlgorithm.CAMELLIA_256,
                     SymmetricKeyAlgorithm.CAMELLIA_192,
-                    SymmetricKeyAlgorithm.CAMELLIA_256
+                    SymmetricKeyAlgorithm.CAMELLIA_128
             ));
         }
+
+        /**
+         * Select the best acceptable algorithm from the options list.
+         * The best algorithm is the first algorithm we encounter in our list of acceptable algorithms that
+         * is also contained in the list of options.
+         *
+         *
+         * @param options list of algorithm options
+         * @return best
+         */
+        public SymmetricKeyAlgorithm selectBest(List<SymmetricKeyAlgorithm> options) {
+            for (SymmetricKeyAlgorithm acceptable : acceptableSymmetricKeyAlgorithms) {
+                if (options.contains(acceptable)) {
+                    return acceptable;
+                }
+            }
+            return null;
+        }
     }
 
     public static final class HashAlgorithmPolicy {

pgpainless-core/src/test/java/org/pgpainless/algorithm/SymmetricKeyAlgorithmNegotiatorTest.java

@@ -76,4 +76,66 @@ public class SymmetricKeyAlgorithmNegotiatorTest {
         // AES 192 is most popular
         assertEquals(SymmetricKeyAlgorithm.AES_192, byPopularity.negotiate(policy, null, preferences));
     }
+
+    @Test
+    public void byPopularityIgnoresRejectedAlgorithms() {
+        List<Set<SymmetricKeyAlgorithm>> preferences = new ArrayList<>();
+
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_128);
+            add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected
+            add(SymmetricKeyAlgorithm.AES_256); // <- accepted
+        }});
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_128);
+            add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected
+        }});
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected
+            add(SymmetricKeyAlgorithm.AES_256); // <- accepted
+        }});
+
+        // AES 192 is most popular
+        assertEquals(SymmetricKeyAlgorithm.AES_256, byPopularity.negotiate(policy, null, preferences));
+    }
+
+    @Test
+    public void byPopularityChoosesFallbackWhenNoAlgIsAcceptable() {
+        List<Set<SymmetricKeyAlgorithm>> preferences = new ArrayList<>();
+
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_128);
+            add(SymmetricKeyAlgorithm.CAMELLIA_192);
+        }});
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_128);
+            add(SymmetricKeyAlgorithm.CAMELLIA_192);
+        }});
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.CAMELLIA_192);
+            add(SymmetricKeyAlgorithm.BLOWFISH);
+        }});
+
+        // AES 192 is most popular
+        assertEquals(SymmetricKeyAlgorithm.CAMELLIA_256, byPopularity.negotiate(policy, null, preferences));
+    }
+
+    @Test
+    public void byPopularitySelectsBestOnDraw() {
+        List<Set<SymmetricKeyAlgorithm>> preferences = new ArrayList<>();
+
+        // Create draw between AES 128 and AES 256
+        // The recipients prefer AES 128 first, but we prioritize our policies order
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.AES_128);
+            add(SymmetricKeyAlgorithm.AES_192);
+            add(SymmetricKeyAlgorithm.AES_256);
+        }});
+        preferences.add(new LinkedHashSet<SymmetricKeyAlgorithm>(){{
+            add(SymmetricKeyAlgorithm.AES_128);
+            add(SymmetricKeyAlgorithm.AES_256);
+        }});
+
+        assertEquals(SymmetricKeyAlgorithm.AES_256, byPopularity.negotiate(policy, null, preferences));
+    }
 }

pgpainless-core/src/test/java/org/pgpainless/encryption_signing/EncryptDecryptTest.java

@@ -35,6 +35,7 @@ import org.bouncycastle.openpgp.PGPPublicKeyRing;
 import org.bouncycastle.openpgp.PGPSecretKeyRing;
 import org.bouncycastle.openpgp.PGPSignature;
 import org.bouncycastle.util.io.Streams;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -56,6 +57,7 @@ import org.pgpainless.key.generation.type.rsa.RsaLength;
 import org.pgpainless.key.protection.SecretKeyRingProtector;
 import org.pgpainless.key.protection.UnprotectedKeysProtector;
 import org.pgpainless.key.util.KeyRingUtils;
+import org.pgpainless.policy.Policy;
 import org.pgpainless.util.ArmoredOutputStreamFactory;
 
 public class EncryptDecryptTest {
@@ -71,6 +73,14 @@ public class EncryptDecryptTest {
                     "Unfold the imagined happiness that both\n" +
                     "Receive in either by this dear encounter.";
 
+    @BeforeEach
+    public void setDefaultPolicy() {
+        PGPainless.getPolicy().setSymmetricKeyEncryptionAlgorithmPolicy(
+                Policy.SymmetricKeyAlgorithmPolicy.defaultSymmetricKeyEncryptionAlgorithmPolicy());
+        PGPainless.getPolicy().setSymmetricKeyDecryptionAlgorithmPolicy(
+                Policy.SymmetricKeyAlgorithmPolicy.defaultSymmetricKeyDecryptionAlgorithmPolicy());
+    }
+
     @ParameterizedTest
     @MethodSource("org.pgpainless.util.TestUtil#provideImplementationFactories")
     public void freshKeysRsaToElGamalTest(ImplementationFactory implementationFactory)