Inconsistency when signing data (#656)

- Fixes a NPE in case the Testcontainers module receives an immutable set of vault names - Fixes RSA signature and verification algorithms (allowing the client to do hashing) - Changes EC signature and verification logic to use BC provider - Simplifies signature and verification implementation by extracting common parts - Adds additional validation steps to verify digest length based on the accepted hashes when signature and verification are used - Adds new test cases to make sure correct algorithms are used by sign logic - Updates tests where necessary Resolves #651 {minor} Signed-off-by: Esta Nagy <[email protected]>
nagyesta · Aug 7, 2023 · 943b7de · 943b7de
1 parent a17d4f9
commit 943b7de
Show file tree

Hide file tree

Showing 14 changed files with 336 additions and 179 deletions.
diff --git a/...src/main/java/com/github/nagyesta/lowkeyvault/model/v7_2/key/constants/HashAlgorithm.java b/...src/main/java/com/github/nagyesta/lowkeyvault/model/v7_2/key/constants/HashAlgorithm.java
@@ -0,0 +1,57 @@
+package com.github.nagyesta.lowkeyvault.model.v7_2.key.constants;
+
+import org.bouncycastle.asn1.ASN1ObjectIdentifier;
+import org.bouncycastle.asn1.DERNull;
+import org.bouncycastle.asn1.nist.NISTObjectIdentifiers;
+import org.bouncycastle.asn1.x509.AlgorithmIdentifier;
+import org.bouncycastle.asn1.x509.DigestInfo;
+import org.springframework.util.Assert;
+
+import java.io.IOException;
+import java.lang.reflect.Array;
+import java.security.spec.MGF1ParameterSpec;
+import java.security.spec.PSSParameterSpec;
+import java.util.Optional;
+
+@SuppressWarnings("checkstyle:JavadocVariable")
+public enum HashAlgorithm {
+    SHA256("SHA-256", 32, NISTObjectIdentifiers.id_sha256),
+    SHA384("SHA-384", 48, NISTObjectIdentifiers.id_sha384),
+    SHA512("SHA-512", 64, NISTObjectIdentifiers.id_sha512);
+
+    private final String algorithmName;
+    private final ASN1ObjectIdentifier algorithmIdentifier;
+    private final int digestLength;
+
+    HashAlgorithm(final String algorithmName, final int digestLength, final ASN1ObjectIdentifier algorithmIdentifier) {
+        this.algorithmName = algorithmName;
+        this.algorithmIdentifier = algorithmIdentifier;
+        this.digestLength = digestLength;
+    }
+
+    public String getAlgorithmName() {
+        return algorithmName;
+    }
+
+    public byte[] encodeDigest(final byte[] digest) throws IOException {
+        return new DigestInfo(new AlgorithmIdentifier(algorithmIdentifier, DERNull.INSTANCE), digest).getEncoded();
+    }
+
+    public PSSParameterSpec getPssParameter() {
+        return new PSSParameterSpec(
+                algorithmName,
+                "MGF1",
+                new MGF1ParameterSpec(algorithmName),
+                digestLength,
+                PSSParameterSpec.TRAILER_FIELD_BC
+        );
+    }
+
+    public void verifyDigestLength(final byte[] digest) {
+        final int length = Optional.ofNullable(digest)
+                .map(Array::getLength)
+                .orElseThrow(() -> new IllegalArgumentException("Digest is null."));
+        Assert.isTrue(digestLength == length,
+                "This algorithm does not support digest length: " + length + ". Expected: " + digestLength);
+    }
+}
diff --git a/...ain/java/com/github/nagyesta/lowkeyvault/model/v7_2/key/constants/SignatureAlgorithm.java b/...ain/java/com/github/nagyesta/lowkeyvault/model/v7_2/key/constants/SignatureAlgorithm.java
@@ -3,76 +3,116 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonValue;
+import com.github.nagyesta.lowkeyvault.service.key.util.KeyGenUtil;
 
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+import java.security.Signature;
 import java.util.Arrays;
 
 @SuppressWarnings("checkstyle:JavadocVariable")
 public enum SignatureAlgorithm {
 
-    ES256("ES256", "NONEwithECDSAinP1363Format", KeyType.EC) {
+    ES256("ES256", "NONEwithECDSAinP1363Format", KeyType.EC, HashAlgorithm.SHA256) {
         @Override
         public boolean isCompatibleWithCurve(final KeyCurveName keyCurveName) {
             return KeyCurveName.P_256 == keyCurveName;
         }
 
-        @SuppressWarnings("checkstyle:MagicNumber")
         @Override
-        public boolean supportsDigestLength(final int digestLength) {
-            return digestLength == 32;
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            return Signature.getInstance(getAlg());
         }
     },
-    ES256K("ES256K", "NONEwithECDSAinP1363Format", KeyType.EC) {
+    ES256K("ES256K", "NONEwithECDSAinP1363Format", KeyType.EC, HashAlgorithm.SHA256) {
         @Override
         public boolean isCompatibleWithCurve(final KeyCurveName keyCurveName) {
             return KeyCurveName.P_256K == keyCurveName;
         }
 
-        @SuppressWarnings("checkstyle:MagicNumber")
         @Override
-        public boolean supportsDigestLength(final int digestLength) {
-            return digestLength == 32;
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            return Signature.getInstance(getAlg());
         }
     },
-    ES384("ES384", "NONEwithECDSAinP1363Format", KeyType.EC) {
+    ES384("ES384", "NONEwithECDSAinP1363Format", KeyType.EC, HashAlgorithm.SHA384) {
         @Override
         public boolean isCompatibleWithCurve(final KeyCurveName keyCurveName) {
             return KeyCurveName.P_384 == keyCurveName;
         }
 
-        @SuppressWarnings("checkstyle:MagicNumber")
         @Override
-        public boolean supportsDigestLength(final int digestLength) {
-            return digestLength == 48;
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            return Signature.getInstance(getAlg());
         }
     },
-    ES512("ES512", "NONEwithECDSAinP1363Format", KeyType.EC) {
+    ES512("ES512", "NONEwithECDSAinP1363Format", KeyType.EC, HashAlgorithm.SHA512) {
         @Override
         public boolean isCompatibleWithCurve(final KeyCurveName keyCurveName) {
             return KeyCurveName.P_521 == keyCurveName;
         }
 
-        @SuppressWarnings("checkstyle:MagicNumber")
         @Override
-        public boolean supportsDigestLength(final int digestLength) {
-            return digestLength == 64;
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            return Signature.getInstance(getAlg());
         }
     },
-    PS256("PS256", "SHA256withRSAandMGF1", KeyType.RSA),
-    PS384("PS384", "SHA384withRSAandMGF1", KeyType.RSA),
-    PS512("PS512", "SHA512withRSAandMGF1", KeyType.RSA),
-    RS256("RS256", "SHA256withRSA", KeyType.RSA),
-    RS384("RS384", "SHA384withRSA", KeyType.RSA),
-    RS512("RS512", "SHA512withRSA", KeyType.RSA);
+    PS256("PS256", "NONEwithRSAandMGF1", KeyType.RSA, HashAlgorithm.SHA256) {
+        @Override
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            final Signature signature = super.getSignatureInstance();
+            signature.setParameter(getHashAlgorithm().getPssParameter());
+            return signature;
+        }
+    },
+    PS384("PS384", "NONEwithRSAandMGF1", KeyType.RSA, HashAlgorithm.SHA384) {
+        @Override
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            final Signature signature = super.getSignatureInstance();
+            signature.setParameter(getHashAlgorithm().getPssParameter());
+            return signature;
+        }
+    },
+    PS512("PS512", "NONEwithRSAandMGF1", KeyType.RSA, HashAlgorithm.SHA512) {
+        @Override
+        public Signature getSignatureInstance() throws GeneralSecurityException {
+            final Signature signature = super.getSignatureInstance();
+            signature.setParameter(getHashAlgorithm().getPssParameter());
+            return signature;
+        }
+    },
+    RS256("RS256", "NoneWithRSA", KeyType.RSA, HashAlgorithm.SHA256) {
+        @Override
+        public byte[] transformDigest(final byte[] digest) throws IOException {
+            return getHashAlgorithm().encodeDigest(digest);
+        }
+    },
+    RS384("RS384", "NoneWithRSA", KeyType.RSA, HashAlgorithm.SHA384) {
+        @Override
+        public byte[] transformDigest(final byte[] digest) throws IOException {
+            return getHashAlgorithm().encodeDigest(digest);
+        }
+    },
+    RS512("RS512", "NoneWithRSA", KeyType.RSA, HashAlgorithm.SHA512) {
+        @Override
+        public byte[] transformDigest(final byte[] digest) throws IOException {
+            return getHashAlgorithm().encodeDigest(digest);
+        }
+    };
 
     private final String value;
     private final String alg;
     private final KeyType compatibleType;
+    private final HashAlgorithm hashAlgorithm;
 
     SignatureAlgorithm(final String value,
-                       final String alg, final KeyType compatibleType) {
+                       final String alg,
+                       final KeyType compatibleType,
+                       final HashAlgorithm hashAlgorithm) {
         this.value = value;
         this.alg = alg;
         this.compatibleType = compatibleType;
+        this.hashAlgorithm = hashAlgorithm;
     }
 
     @JsonCreator
@@ -101,8 +141,17 @@ public boolean isCompatibleWithCurve(final KeyCurveName keyCurveName) {
     }
 
     @JsonIgnore
-    public boolean supportsDigestLength(final int digestLength) {
-        return false;
+    public byte[] transformDigest(final byte[] digest) throws IOException {
+        return digest;
     }
 
+    @JsonIgnore
+    public HashAlgorithm getHashAlgorithm() {
+        return hashAlgorithm;
+    }
+
+    @JsonIgnore
+    public Signature getSignatureInstance() throws GeneralSecurityException {
+        return Signature.getInstance(getAlg(), KeyGenUtil.BOUNCY_CASTLE_PROVIDER);
+    }
 }
diff --git a/...p/src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/EcKeyVaultKeyEntity.java b/...p/src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/EcKeyVaultKeyEntity.java
@@ -8,13 +8,11 @@
 import org.springframework.lang.NonNull;
 import org.springframework.util.Assert;
 
-import java.lang.reflect.Array;
 import java.security.KeyPair;
-import java.security.Signature;
 import java.security.interfaces.ECPrivateKey;
 import java.security.interfaces.ECPublicKey;
 import java.util.List;
-import java.util.Optional;
+import java.util.concurrent.Callable;
 
 import static com.github.nagyesta.lowkeyvault.service.key.util.KeyGenUtil.generateEc;
 
@@ -87,37 +85,19 @@ public byte[] decryptToBytes(final byte[] encrypted, final EncryptionAlgorithm e
 
     @Override
     public byte[] signBytes(final byte[] digest, final SignatureAlgorithm signatureAlgorithm) {
-        Assert.state(getOperations().contains(KeyOperation.SIGN), getId() + " does not have SIGN operation assigned.");
-        Assert.state(isEnabled(), getId() + " is not enabled.");
+        validateGenericSignOrVerifyInputs(digest, signatureAlgorithm, KeyOperation.SIGN);
         Assert.state(signatureAlgorithm.isCompatibleWithCurve(getKeyCurveName()), getId() + " is not using the right key curve.");
-        final int length = Optional.ofNullable(digest)
-                .map(Array::getLength)
-                .orElseThrow(() -> new IllegalArgumentException("Digest is null."));
-        Assert.isTrue(signatureAlgorithm.supportsDigestLength(length), getId() + " does not support digest length: " + length + ".");
-        return doCrypto(() -> {
-            final Signature ecSign = Signature.getInstance(signatureAlgorithm.getAlg());
-            ecSign.initSign(getKey().getPrivate());
-            ecSign.update(digest);
-            return ecSign.sign();
-        }, "Cannot sign message.", log);
+        final Callable<byte[]> signCallable = signCallable(digest, signatureAlgorithm, getKey().getPrivate());
+        return doCrypto(signCallable, "Cannot sign message.", log);
     }
 
     @Override
     public boolean verifySignedBytes(final byte[] digest,
                                      final SignatureAlgorithm signatureAlgorithm,
                                      final byte[] signature) {
-        Assert.state(getOperations().contains(KeyOperation.VERIFY), getId() + " does not have VERIFY operation assigned.");
-        Assert.state(isEnabled(), getId() + " is not enabled.");
+        validateGenericSignOrVerifyInputs(digest, signatureAlgorithm, KeyOperation.VERIFY);
         Assert.state(signatureAlgorithm.isCompatibleWithCurve(getKeyCurveName()), getId() + " is not using the right key curve.");
-        final int length = Optional.ofNullable(digest)
-                .map(Array::getLength)
-                .orElseThrow(() -> new IllegalArgumentException("Digest is null."));
-        Assert.isTrue(signatureAlgorithm.supportsDigestLength(length), getId() + " does not support digest length: " + length + ".");
-        return doCrypto(() -> {
-            final Signature ecVerify = Signature.getInstance(signatureAlgorithm.getAlg());
-            ecVerify.initVerify(getKey().getPublic());
-            ecVerify.update(digest);
-            return ecVerify.verify(signature);
-        }, "Cannot verify digest message.", log);
+        final Callable<Boolean> verifyCallable = verifyCallable(digest, signatureAlgorithm, signature, getKey().getPublic());
+        return doCrypto(verifyCallable, "Cannot verify digest message.", log);
     }
 }
diff --git a/...app/src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/KeyVaultKeyEntity.java b/...app/src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/KeyVaultKeyEntity.java
@@ -1,6 +1,7 @@
 package com.github.nagyesta.lowkeyvault.service.key.impl;
 
 import com.github.nagyesta.lowkeyvault.model.v7_2.key.constants.KeyOperation;
+import com.github.nagyesta.lowkeyvault.model.v7_2.key.constants.SignatureAlgorithm;
 import com.github.nagyesta.lowkeyvault.service.common.impl.KeyVaultBaseEntity;
 import com.github.nagyesta.lowkeyvault.service.exception.CryptoException;
 import com.github.nagyesta.lowkeyvault.service.key.ReadOnlyKeyVaultKeyEntity;
@@ -10,6 +11,9 @@
 import org.slf4j.Logger;
 import org.springframework.util.Assert;
 
+import java.security.PrivateKey;
+import java.security.PublicKey;
+import java.security.Signature;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Callable;
@@ -84,4 +88,31 @@ protected <R> R doCrypto(final Callable<R> task, final String message, final Log
         }
     }
 
+    protected void validateGenericSignOrVerifyInputs(
+            final byte[] digest, final SignatureAlgorithm signatureAlgorithm, final KeyOperation keyOperation) {
+        Assert.state(getOperations().contains(keyOperation),
+                getId() + " does not have " + keyOperation.name() + " operation assigned.");
+        Assert.state(isEnabled(), getId() + " is not enabled.");
+        signatureAlgorithm.getHashAlgorithm().verifyDigestLength(digest);
+    }
+
+    protected Callable<byte[]> signCallable(
+            final byte[] digest, final SignatureAlgorithm signatureAlgorithm, final PrivateKey privateKey) {
+        return () -> {
+            final Signature sign = signatureAlgorithm.getSignatureInstance();
+            sign.initSign(privateKey);
+            sign.update(signatureAlgorithm.transformDigest(digest));
+            return sign.sign();
+        };
+    }
+
+    protected Callable<Boolean> verifyCallable(
+            final byte[] digest, final SignatureAlgorithm signatureAlgorithm, final byte[] signature, final PublicKey publicKey) {
+        return () -> {
+            final Signature verify = signatureAlgorithm.getSignatureInstance();
+            verify.initVerify(publicKey);
+            verify.update(signatureAlgorithm.transformDigest(digest));
+            return verify.verify(signature);
+        };
+    }
 }
diff --git a/.../src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/RsaKeyVaultKeyEntity.java b/.../src/main/java/com/github/nagyesta/lowkeyvault/service/key/impl/RsaKeyVaultKeyEntity.java
@@ -15,9 +15,9 @@
 import javax.crypto.Cipher;
 import java.math.BigInteger;
 import java.security.KeyPair;
-import java.security.Signature;
 import java.security.interfaces.RSAPrivateCrtKey;
 import java.security.interfaces.RSAPublicKey;
+import java.util.concurrent.Callable;
 
 import static com.github.nagyesta.lowkeyvault.service.key.util.KeyGenUtil.generateRsa;
 
@@ -125,27 +125,17 @@ public byte[] decryptToBytes(@NonNull final byte[] encrypted, @NonNull final Enc
 
     @Override
     public byte[] signBytes(final byte[] digest, final SignatureAlgorithm signatureAlgorithm) {
-        Assert.state(getOperations().contains(KeyOperation.SIGN), getId() + " does not have SIGN operation assigned.");
-        Assert.state(isEnabled(), getId() + " is not enabled.");
-        return doCrypto(() -> {
-            final Signature rsaSign = Signature.getInstance(signatureAlgorithm.getAlg(), KeyGenUtil.BOUNCY_CASTLE_PROVIDER);
-            rsaSign.initSign(getKey().getPrivate());
-            rsaSign.update(digest);
-            return rsaSign.sign();
-        }, "Cannot sign message.", log);
+        validateGenericSignOrVerifyInputs(digest, signatureAlgorithm, KeyOperation.SIGN);
+        final Callable<byte[]> signCallable = signCallable(digest, signatureAlgorithm, getKey().getPrivate());
+        return doCrypto(signCallable, "Cannot sign message.", log);
     }
 
     @Override
     public boolean verifySignedBytes(final byte[] digest,
                                      final SignatureAlgorithm signatureAlgorithm,
                                      final byte[] signature) {
-        Assert.state(getOperations().contains(KeyOperation.VERIFY), getId() + " does not have VERIFY operation assigned.");
-        Assert.state(isEnabled(), getId() + " is not enabled.");
-        return doCrypto(() -> {
-            final Signature rsaVerify = Signature.getInstance(signatureAlgorithm.getAlg(), KeyGenUtil.BOUNCY_CASTLE_PROVIDER);
-            rsaVerify.initVerify(getKey().getPublic());
-            rsaVerify.update(digest);
-            return rsaVerify.verify(signature);
-        }, "Cannot verify digest message.", log);
+        validateGenericSignOrVerifyInputs(digest, signatureAlgorithm, KeyOperation.VERIFY);
+        final Callable<Boolean> verifyCallable = verifyCallable(digest, signatureAlgorithm, signature, getKey().getPublic());
+        return doCrypto(verifyCallable, "Cannot verify digest message.", log);
     }
 }
diff --git a/lowkey-vault-app/src/test/java/com/github/nagyesta/lowkeyvault/HashUtil.java b/lowkey-vault-app/src/test/java/com/github/nagyesta/lowkeyvault/HashUtil.java
@@ -0,0 +1,26 @@
+package com.github.nagyesta.lowkeyvault;
+
+import com.github.nagyesta.lowkeyvault.model.v7_2.key.constants.HashAlgorithm;
+
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+public final class HashUtil {
+
+    private HashUtil() {
+    }
+
+    public static byte[] hash(final byte[] data, final HashAlgorithm algorithm) {
+        try {
+            return hash(data, algorithm.getAlgorithmName());
+        } catch (final NoSuchAlgorithmException e) {
+            throw new IllegalArgumentException(e);
+        }
+    }
+
+    private static byte[] hash(final byte[] data, final String algorithm) throws NoSuchAlgorithmException {
+        final MessageDigest md = MessageDigest.getInstance(algorithm);
+        md.update(data);
+        return md.digest();
+    }
+}