From 15046cdc32d0fe0eb14910892bc0742dc04f2954 Mon Sep 17 00:00:00 2001
From: Paul Schaub <vanitasvitae@fsfe.org>
Date: Wed, 31 Aug 2022 21:37:31 +0200
Subject: [PATCH] Switch default S2K for secret key protection over to use
 SHA256 and add documentation

---
 .../protection/KeyRingProtectionSettings.java | 61 +++++++++++++++++--
 .../InvalidProtectionSettingsTest.java        | 20 ++++++
 2 files changed, 77 insertions(+), 4 deletions(-)
 create mode 100644 pgpainless-core/src/test/java/org/pgpainless/key/protection/InvalidProtectionSettingsTest.java

diff --git a/pgpainless-core/src/main/java/org/pgpainless/key/protection/KeyRingProtectionSettings.java b/pgpainless-core/src/main/java/org/pgpainless/key/protection/KeyRingProtectionSettings.java
index ede286c6..a93534ab 100644
--- a/pgpainless-core/src/main/java/org/pgpainless/key/protection/KeyRingProtectionSettings.java
+++ b/pgpainless-core/src/main/java/org/pgpainless/key/protection/KeyRingProtectionSettings.java
@@ -9,18 +9,39 @@ import javax.annotation.Nonnull;
 import org.pgpainless.algorithm.HashAlgorithm;
 import org.pgpainless.algorithm.SymmetricKeyAlgorithm;
 
+/**
+ * Secret key protection settings for iterated and salted S2K.
+ */
 public class KeyRingProtectionSettings {
 
     private final SymmetricKeyAlgorithm encryptionAlgorithm;
     private final HashAlgorithm hashAlgorithm;
     private final int s2kCount;
 
+    /**
+     * Create a {@link KeyRingProtectionSettings} object using the given encryption algorithm, SHA1 and
+     * 65536 iterations.
+     *
+     * @param encryptionAlgorithm encryption algorithm
+     */
     public KeyRingProtectionSettings(@Nonnull SymmetricKeyAlgorithm encryptionAlgorithm) {
-        this(encryptionAlgorithm, HashAlgorithm.SHA1, 0x60); // Same s2kCount as used in BC.
+        this(encryptionAlgorithm, HashAlgorithm.SHA1, 0x60); // Same s2kCount (encoded) as used in BC.
     }
 
+    /**
+     * Constructor for custom salted and iterated S2K protection settings.
+     * The salt gets randomly chosen by the library each time.
+     *
+     * Note, that the s2kCount is the already encoded single-octet number.
+     *
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc4880#section-3.7.1.3">Encoding Formula</a>
+     *
+     * @param encryptionAlgorithm encryption algorithm
+     * @param hashAlgorithm hash algorithm
+     * @param s2kCount encoded s2k iteration count
+     */
     public KeyRingProtectionSettings(@Nonnull SymmetricKeyAlgorithm encryptionAlgorithm, @Nonnull HashAlgorithm hashAlgorithm, int s2kCount) {
-        this.encryptionAlgorithm = encryptionAlgorithm;
+        this.encryptionAlgorithm = validateEncryptionAlgorithm(encryptionAlgorithm);
         this.hashAlgorithm = hashAlgorithm;
         if (s2kCount < 1) {
             throw new IllegalArgumentException("s2kCount cannot be less than 1.");
@@ -28,18 +49,50 @@ public class KeyRingProtectionSettings {
         this.s2kCount = s2kCount;
     }
 
-    public static KeyRingProtectionSettings secureDefaultSettings() {
-        return new KeyRingProtectionSettings(SymmetricKeyAlgorithm.AES_256);
+    private static SymmetricKeyAlgorithm validateEncryptionAlgorithm(SymmetricKeyAlgorithm encryptionAlgorithm) {
+        switch (encryptionAlgorithm) {
+            case NULL:
+                throw new IllegalArgumentException("Unencrypted is not allowed here!");
+            default:
+                return encryptionAlgorithm;
+        }
     }
 
+    /**
+     * Secure default settings using {@link SymmetricKeyAlgorithm#AES_256}, {@link HashAlgorithm#SHA256}
+     * and an iteration count of 65536.
+     *
+     * @return secure protection settings
+     */
+    public static KeyRingProtectionSettings secureDefaultSettings() {
+        return new KeyRingProtectionSettings(SymmetricKeyAlgorithm.AES_256, HashAlgorithm.SHA256, 0x60);
+    }
+
+    /**
+     * Return the encryption algorithm.
+     *
+     * @return encryption algorithm
+     */
     public @Nonnull SymmetricKeyAlgorithm getEncryptionAlgorithm() {
         return encryptionAlgorithm;
     }
 
+    /**
+     * Return the hash algorithm.
+     *
+     * @return hash algorithm
+     */
     public @Nonnull HashAlgorithm getHashAlgorithm() {
         return hashAlgorithm;
     }
 
+    /**
+     * Return the (encoded!) s2k iteration count.
+     *
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc4880#section-3.7.1.3">Encoding Formula</a>
+     *
+     * @return encoded s2k count
+     */
     public int getS2kCount() {
         return s2kCount;
     }
diff --git a/pgpainless-core/src/test/java/org/pgpainless/key/protection/InvalidProtectionSettingsTest.java b/pgpainless-core/src/test/java/org/pgpainless/key/protection/InvalidProtectionSettingsTest.java
new file mode 100644
index 00000000..b0746398
--- /dev/null
+++ b/pgpainless-core/src/test/java/org/pgpainless/key/protection/InvalidProtectionSettingsTest.java
@@ -0,0 +1,20 @@
+// SPDX-FileCopyrightText: 2022 Paul Schaub <vanitasvitae@fsfe.org>
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package org.pgpainless.key.protection;
+
+import org.junit.jupiter.api.Test;
+import org.pgpainless.algorithm.HashAlgorithm;
+import org.pgpainless.algorithm.SymmetricKeyAlgorithm;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class InvalidProtectionSettingsTest {
+
+    @Test
+    public void unencryptedKeyRingProtectionSettingsThrows() {
+        assertThrows(IllegalArgumentException.class, () ->
+                new KeyRingProtectionSettings(SymmetricKeyAlgorithm.NULL, HashAlgorithm.SHA256, 0x60));
+    }
+}