From b43fe318313a666acefb7bb98b46c85d18b4eef3 Mon Sep 17 00:00:00 2001 From: Peter Suna Date: Wed, 25 Oct 2023 13:52:15 +0200 Subject: [PATCH] Throw exception if decryption/encryption fails Previously, in the case of a failure during encryption or decryption in AAA, the system would only log an error and return insered string. This could lead user to believe that the operation was successful, resulting in them receiving unencrypted/undecrypted data. Also is possible that IllegalArgumentException is thrown which is also wrong. This patch simplifies things by throwing GeneralSecurityException, as that is quite a natural thing to do. In terms of IAEs -- this relates to String encoding and not encryption, so we solve this by simply not providing String-based services, forcing users to deal with translation themselves. Since we are in the area, also convert unit tests to JUnit5, as they are extremely simplistic. Also make the service null-hostile, as that it almost is -- encrypt path would throw IAE on null bytes, this turns it into a NPE and guards the decrypt path the same way. Finally we take care of the asymmetry in encrypt/descrypt when we do not have a key -- simply by refusing to start it we fail to initialize. JIRA: AAA-266 Change-Id: I4c9078e293fe5b98f0e6b69568ca10a75a4fbe07 Signed-off-by: Peter Suna Signed-off-by: Robert Varga Signed-off-by: Yaroslav Lastivka --- .../aaa/cert/impl/KeyStoresDataUtils.java | 146 ++++++++++-------- .../cert/impl/AaaCertMdsalProviderTest.java | 17 +- .../cert/impl/AaaCertRpcServiceImplTest.java | 8 +- .../aaa/cert/impl/KeyStoresDataUtilsTest.java | 25 +-- .../aaa/encrypt/AAAEncryptionService.java | 35 ++--- .../impl/AAAEncryptionServiceImpl.java | 115 +++----------- .../OSGiEncryptionServiceConfigurator.java | 11 +- .../impl/AAAEncryptServiceImplTest.java | 143 ++++++++--------- 8 files changed, 229 insertions(+), 271 deletions(-) diff --git a/aaa-cert/src/main/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtils.java b/aaa-cert/src/main/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtils.java index 58f04682d..a6d610b9b 100644 --- a/aaa-cert/src/main/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtils.java +++ b/aaa-cert/src/main/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtils.java @@ -7,6 +7,9 @@ */ package org.opendaylight.aaa.cert.impl; +import java.nio.charset.Charset; +import java.security.GeneralSecurityException; +import java.util.Base64; import java.util.List; import org.opendaylight.aaa.encrypt.AAAEncryptionService; import org.opendaylight.mdsal.binding.api.DataBroker; @@ -29,7 +32,6 @@ * KeyStoresDataUtils manage the SslData operations add, delete and update. * * @author mserngawy - * */ public class KeyStoresDataUtils { @@ -62,9 +64,15 @@ public static OdlKeystore updateOdlKeystore(final OdlKeystore baseOdlKeyStore, f public SslData addSslData(final DataBroker dataBroker, final String bundleName, final OdlKeystore odlKeystore, final TrustKeystore trustKeystore, final List cipherSuites, final String tlsProtocols) { final SslDataKey sslDataKey = new SslDataKey(bundleName); - final SslData sslData = new SslDataBuilder().withKey(sslDataKey).setOdlKeystore(encryptOdlKeyStore(odlKeystore)) - .setTrustKeystore(encryptTrustKeystore(trustKeystore)).setCipherSuites(cipherSuites) - .setTlsProtocols(tlsProtocols).build(); + final SslData sslData; + try { + sslData = new SslDataBuilder().withKey(sslDataKey).setOdlKeystore(encryptOdlKeyStore(odlKeystore)) + .setTrustKeystore(encryptTrustKeystore(trustKeystore)).setCipherSuites(cipherSuites) + .setTlsProtocols(tlsProtocols).build(); + } catch (GeneralSecurityException e) { + LOG.error("Encryption of TrustKeystore for SslData failed.", e); + return null; + } if (MdsalUtils.put(dataBroker, LogicalDatastoreType.CONFIGURATION, getSslDataIid(bundleName), sslData)) { return new SslDataBuilder().withKey(sslDataKey).setOdlKeystore(odlKeystore).setTrustKeystore(trustKeystore) @@ -99,13 +107,13 @@ public OdlKeystore createOdlKeystore(final String name, final String alias, fina LOG.debug("Odl keystore string {} ", keyStoreBytes); return new OdlKeystoreBuilder().setKeystoreFile(keyStoreBytes) - .setAlias(alias).setDname(dname).setKeyAlg(keyAlg) - .setKeysize(keySize) - .setName(name) - .setSignAlg(sigAlg) - .setStorePassword(password) - .setValidity(validity) - .build(); + .setAlias(alias).setDname(dname).setKeyAlg(keyAlg) + .setKeysize(keySize) + .setName(name) + .setSignAlg(sigAlg) + .setStorePassword(password) + .setValidity(validity) + .build(); } public TrustKeystore createTrustKeystore(final String name, final String password, final byte[] keyStoreBytes) { @@ -118,66 +126,76 @@ public TrustKeystore createTrustKeystore(final String name, final String passwor password); LOG.debug("trust keystore string {} ", keyStoreBytes); return new TrustKeystoreBuilder() - .setKeystoreFile(keyStoreBytes) - .setName(name) - .setStorePassword(password) - .build(); + .setKeystoreFile(keyStoreBytes) + .setName(name) + .setStorePassword(password) + .build(); } - private OdlKeystore decryptOdlKeyStore(final OdlKeystore odlKeystore) { - if (odlKeystore == null) { - return null; - } - final OdlKeystoreBuilder odlKeystoreBuilder = new OdlKeystoreBuilder(odlKeystore); - odlKeystoreBuilder.setKeystoreFile(encryService.decrypt(odlKeystore.getKeystoreFile())); - odlKeystoreBuilder.setStorePassword(encryService.decrypt(odlKeystore.getStorePassword())); - return odlKeystoreBuilder.build(); + private OdlKeystore decryptOdlKeyStore(final OdlKeystore odlKeystore) throws GeneralSecurityException { + return odlKeystore == null ? null : new OdlKeystoreBuilder(odlKeystore) + .setKeystoreFile(decryptNullable(odlKeystore.getKeystoreFile())) + .setStorePassword(decryptStringFromBase64(odlKeystore.getStorePassword())) + .build(); } - private SslData decryptSslData(final SslData sslData) { - if (sslData == null) { - return null; - } - final SslDataBuilder sslDataBuilder = new SslDataBuilder(sslData) - .setOdlKeystore(decryptOdlKeyStore(sslData.getOdlKeystore())) - .setTrustKeystore(decryptTrustKeystore(sslData.getTrustKeystore())); - return sslDataBuilder.build(); + private SslData decryptSslData(final SslData sslData) throws GeneralSecurityException { + return sslData == null ? null : new SslDataBuilder(sslData) + .setOdlKeystore(decryptOdlKeyStore(sslData.getOdlKeystore())) + .setTrustKeystore(decryptTrustKeystore(sslData.getTrustKeystore())) + .build(); } - private TrustKeystore decryptTrustKeystore(final TrustKeystore trustKeyStore) { - if (trustKeyStore == null) { - return null; - } - final TrustKeystoreBuilder trustKeyStoreBuilder = new TrustKeystoreBuilder(trustKeyStore); - trustKeyStoreBuilder.setKeystoreFile(encryService.decrypt(trustKeyStore.getKeystoreFile())); - trustKeyStoreBuilder.setStorePassword(encryService.decrypt(trustKeyStore.getStorePassword())); - return trustKeyStoreBuilder.build(); + private TrustKeystore decryptTrustKeystore(final TrustKeystore trustKeyStore) throws GeneralSecurityException { + return trustKeyStore == null ? null : new TrustKeystoreBuilder(trustKeyStore) + .setKeystoreFile(decryptNullable(trustKeyStore.getKeystoreFile())) + .setStorePassword(decryptStringFromBase64(trustKeyStore.getStorePassword())) + .build(); + } + + private byte[] decryptNullable(final byte[] bytes) throws GeneralSecurityException { + return bytes == null ? null : encryService.decrypt(bytes); + } + + private String decryptStringFromBase64(final String base64) throws GeneralSecurityException { + return base64 == null ? null + : new String(encryService.decrypt(Base64.getDecoder().decode(base64)), Charset.defaultCharset()); } - private OdlKeystore encryptOdlKeyStore(final OdlKeystore odlKeystore) { - final OdlKeystoreBuilder odlKeystoreBuilder = new OdlKeystoreBuilder(odlKeystore); - odlKeystoreBuilder.setKeystoreFile(encryService.encrypt(odlKeystore.getKeystoreFile())); - odlKeystoreBuilder.setStorePassword(encryService.encrypt(odlKeystore.getStorePassword())); - return odlKeystoreBuilder.build(); + private String encryptStringToBase64(final String str) throws GeneralSecurityException { + return str == null ? null + : Base64.getEncoder().encodeToString(encryService.encrypt(str.getBytes(Charset.defaultCharset()))); } - private SslData encryptSslData(final SslData sslData) { - final SslDataBuilder sslDataBuilder = new SslDataBuilder(sslData) - .setOdlKeystore(encryptOdlKeyStore(sslData.getOdlKeystore())) - .setTrustKeystore(encryptTrustKeystore(sslData.getTrustKeystore())); - return sslDataBuilder.build(); + private OdlKeystore encryptOdlKeyStore(final OdlKeystore odlKeystore) throws GeneralSecurityException { + return new OdlKeystoreBuilder(odlKeystore) + .setKeystoreFile(encryService.encrypt(odlKeystore.getKeystoreFile())) + .setStorePassword(encryptStringToBase64(odlKeystore.getStorePassword())) + .build(); } - private TrustKeystore encryptTrustKeystore(final TrustKeystore trustKeyStore) { - final TrustKeystoreBuilder trustKeyStoreBuilder = new TrustKeystoreBuilder(trustKeyStore); - trustKeyStoreBuilder.setKeystoreFile(encryService.encrypt(trustKeyStore.getKeystoreFile())); - trustKeyStoreBuilder.setStorePassword(encryService.encrypt(trustKeyStore.getStorePassword())); - return trustKeyStoreBuilder.build(); + private SslData encryptSslData(final SslData sslData) throws GeneralSecurityException { + return new SslDataBuilder(sslData) + .setOdlKeystore(encryptOdlKeyStore(sslData.getOdlKeystore())) + .setTrustKeystore(encryptTrustKeystore(sslData.getTrustKeystore())) + .build(); + } + + private TrustKeystore encryptTrustKeystore(final TrustKeystore trustKeyStore) throws GeneralSecurityException { + return new TrustKeystoreBuilder(trustKeyStore) + .setKeystoreFile(encryService.encrypt(trustKeyStore.getKeystoreFile())) + .setStorePassword(encryptStringToBase64(trustKeyStore.getStorePassword())) + .build(); } public SslData getSslData(final DataBroker dataBroker, final String bundleName) { final InstanceIdentifier sslDataIid = getSslDataIid(bundleName); - return decryptSslData(MdsalUtils.read(dataBroker, LogicalDatastoreType.CONFIGURATION, sslDataIid)); + try { + return decryptSslData(MdsalUtils.read(dataBroker, LogicalDatastoreType.CONFIGURATION, sslDataIid)); + } catch (GeneralSecurityException e) { + LOG.error("Decryption of KeyStore for SslData failed.", e); + return null; + } } public boolean removeSslData(final DataBroker dataBroker, final String bundleName) { @@ -187,25 +205,29 @@ public boolean removeSslData(final DataBroker dataBroker, final String bundleNam public boolean updateSslData(final DataBroker dataBroker, final SslData sslData) { final InstanceIdentifier sslDataIid = getSslDataIid(sslData.getBundleName()); - return MdsalUtils.merge(dataBroker, LogicalDatastoreType.CONFIGURATION, sslDataIid, encryptSslData(sslData)); + final SslData encryptedSslData; + try { + encryptedSslData = encryptSslData(sslData); + } catch (GeneralSecurityException e) { + LOG.error("Encryption of KeyStore for SslData failed.", e); + return false; + } + return MdsalUtils.merge(dataBroker, LogicalDatastoreType.CONFIGURATION, sslDataIid, encryptedSslData); } public boolean updateSslDataCipherSuites(final DataBroker dataBroker, final SslData baseSslData, final List cipherSuites) { - final SslDataBuilder sslDataBuilder = new SslDataBuilder(baseSslData).setCipherSuites(cipherSuites); - return updateSslData(dataBroker, sslDataBuilder.build()); + return updateSslData(dataBroker, new SslDataBuilder(baseSslData).setCipherSuites(cipherSuites).build()); } public boolean updateSslDataOdlKeystore(final DataBroker dataBroker, final SslData baseSslData, final OdlKeystore odlKeyStore) { - final SslDataBuilder sslDataBuilder = new SslDataBuilder(baseSslData).setOdlKeystore(odlKeyStore); - return updateSslData(dataBroker, sslDataBuilder.build()); + return updateSslData(dataBroker, new SslDataBuilder(baseSslData).setOdlKeystore(odlKeyStore).build()); } public boolean updateSslDataTrustKeystore(final DataBroker dataBroker, final SslData baseSslData, final TrustKeystore trustKeyStore) { - final SslDataBuilder sslDataBuilder = new SslDataBuilder(baseSslData).setTrustKeystore(trustKeyStore); - return updateSslData(dataBroker, sslDataBuilder.build()); + return updateSslData(dataBroker, new SslDataBuilder(baseSslData).setTrustKeystore(trustKeyStore).build()); } public TrustKeystore updateTrustKeystore(final TrustKeystore baseTrustKeyStore, final byte[] keyStoreBytes) { diff --git a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertMdsalProviderTest.java b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertMdsalProviderTest.java index d65b7044a..94469c69d 100644 --- a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertMdsalProviderTest.java +++ b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertMdsalProviderTest.java @@ -9,22 +9,20 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.opendaylight.aaa.cert.impl.TestUtils.mockDataBroker; import java.io.File; +import java.nio.charset.Charset; import java.security.KeyStore; import java.security.Security; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.junit.BeforeClass; import org.junit.Test; import org.opendaylight.aaa.encrypt.AAAEncryptionService; -import org.opendaylight.yang.gen.v1.urn.opendaylight.yang.aaa.cert.mdsal.rev160321.cipher.suite.CipherSuites; import org.opendaylight.yang.gen.v1.urn.opendaylight.yang.aaa.cert.mdsal.rev160321.cipher.suite.CipherSuitesBuilder; import org.opendaylight.yang.gen.v1.urn.opendaylight.yang.aaa.cert.mdsal.rev160321.key.stores.SslData; import org.opendaylight.yang.gen.v1.urn.opendaylight.yang.aaa.cert.mdsal.rev160321.key.stores.SslDataBuilder; @@ -68,11 +66,9 @@ public static void setUpBeforeClass() throws Exception { final TrustKeystore unsignedTrustKeyStore = keyStoresDataUtils.createTrustKeystore(TRUST_NAME, PASSWORD, odlKeyTool); - final CipherSuites cipherSuite = new CipherSuitesBuilder().setSuiteName(CIPHER_SUITE_NAME).build(); - - final List cipherSuites = new ArrayList<>(Arrays.asList(cipherSuite)); - - signedSslData = new SslDataBuilder().setCipherSuites(cipherSuites).setOdlKeystore(signedOdlKeystore) + signedSslData = new SslDataBuilder() + .setCipherSuites(List.of(new CipherSuitesBuilder().setSuiteName(CIPHER_SUITE_NAME).build())) + .setOdlKeystore(signedOdlKeystore) .setTrustKeystore(signedTrustKeyStore).setTlsProtocols(PROTOCOL).setBundleName(BUNDLE_NAME).build(); final OdlKeystore unsignedOdlKeystore = new OdlKeystoreBuilder().setAlias(ALIAS).setDname(D_NAME) @@ -84,11 +80,14 @@ public static void setUpBeforeClass() throws Exception { unsignedSslData = new SslDataBuilder().setOdlKeystore(unsignedOdlKeystore) .setTrustKeystore(unsignedTrustKeyStore).setBundleName(BUNDLE_NAME).build(); + when(aaaEncryptionServiceInit.encrypt(PASSWORD.getBytes())) + .thenReturn(PASSWORD.getBytes(Charset.defaultCharset())); when(aaaEncryptionServiceInit.decrypt(unsignedTrustKeyStore.getKeystoreFile())) .thenReturn(unsignedTrustKeyStore.getKeystoreFile()); when(aaaEncryptionServiceInit.decrypt(signedOdlKeystore.getKeystoreFile())) .thenReturn(signedOdlKeystore.getKeystoreFile()); - when(aaaEncryptionServiceInit.decrypt(isA(String.class))).thenReturn(PASSWORD); + when(aaaEncryptionServiceInit.decrypt(new byte[] { -91, -85, 44, 90, -118, -35 })) + .thenReturn(PASSWORD.getBytes(Charset.defaultCharset())); aaaEncryptionService = aaaEncryptionServiceInit; // Create class diff --git a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertRpcServiceImplTest.java b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertRpcServiceImplTest.java index 044b3d2c0..17754011e 100644 --- a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertRpcServiceImplTest.java +++ b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/AaaCertRpcServiceImplTest.java @@ -9,14 +9,15 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.opendaylight.aaa.cert.impl.TestUtils.mockDataBroker; import com.google.common.util.concurrent.Futures; import java.io.File; +import java.nio.charset.Charset; import java.security.Security; +import java.util.Base64; import java.util.List; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.junit.BeforeClass; @@ -99,7 +100,10 @@ public static void setUpBeforeClass() throws Exception { .thenReturn(unsignedTrustKeyStore.getKeystoreFile()); when(aaaEncryptionServiceInit.decrypt(signedOdlKeystore.getKeystoreFile())) .thenReturn(signedOdlKeystore.getKeystoreFile()); - when(aaaEncryptionServiceInit.decrypt(any(String.class))).thenReturn(PASSWORD); + when(aaaEncryptionServiceInit.decrypt(Base64.getDecoder().decode(PASSWORD))) + .thenReturn(PASSWORD.getBytes(Charset.defaultCharset())); + when(aaaEncryptionServiceInit.encrypt(PASSWORD.getBytes(Charset.defaultCharset()))) + .thenReturn(PASSWORD.getBytes(Charset.defaultCharset())); aaaEncryptionService = aaaEncryptionServiceInit; // Create class diff --git a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtilsTest.java b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtilsTest.java index aceabf958..99cc95c22 100644 --- a/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtilsTest.java +++ b/aaa-cert/src/test/java/org/opendaylight/aaa/cert/impl/KeyStoresDataUtilsTest.java @@ -10,11 +10,10 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; import java.io.File; +import java.nio.charset.Charset; import java.security.Security; import java.util.ArrayList; import java.util.Arrays; @@ -23,6 +22,7 @@ import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.opendaylight.aaa.encrypt.AAAEncryptionService; import org.opendaylight.mdsal.binding.api.DataBroker; @@ -48,7 +48,6 @@ public class KeyStoresDataUtilsTest { Security.addProvider(new BouncyCastleProvider()); } - private static final AAAEncryptionService AAA_ENCRYPTION_SERVICE = mock(AAAEncryptionService.class); private static final byte[] ENCRYPTED_BYTE = new byte[] { 1, 2, 3 }; private static final String ALIAS = "fooTest"; private static final String BUNDLE_NAME = "opendaylight"; @@ -61,10 +60,17 @@ public class KeyStoresDataUtilsTest { private static final String TRUST_NAME = "trustTest.jks"; private static final String TEST_PATH = "target" + File.separator + "test" + File.separator; - private final DataBroker dataBroker = mock(DataBroker.class); + @Mock + private AAAEncryptionService encryptionService; + @Mock + private DataBroker dataBroker; + @Mock + private WriteTransaction wtx; + @Mock + private ReadTransaction rtx; @Test - public void keyStoresDataUtilsTest() { + public void keyStoresDataUtilsTest() throws Exception { // Test vars setup final OdlKeystore odlKeystore = new OdlKeystoreBuilder().setAlias(ALIAS).setDname(D_NAME).setName(ODL_NAME) .setStorePassword(PASSWORD).setValidity(KeyStoreConstant.DEFAULT_VALIDITY) @@ -81,22 +87,21 @@ public void keyStoresDataUtilsTest() { .setBundleName(BUNDLE_NAME).build(); final ODLKeyTool odlKeyTool = new ODLKeyTool(TEST_PATH); - final KeyStoresDataUtils keyStoresDataUtils = new KeyStoresDataUtils(AAA_ENCRYPTION_SERVICE); + final KeyStoresDataUtils keyStoresDataUtils = new KeyStoresDataUtils(encryptionService); // Mock setup - final WriteTransaction wtx = mock(WriteTransaction.class); doReturn(CommitInfo.emptyFluentFuture()).when(wtx).commit(); doReturn(wtx).when(dataBroker).newWriteOnlyTransaction(); - final ReadTransaction rtx = mock(ReadTransaction.class); doReturn(FluentFutures.immediateFluentFuture(Optional.of(sslData))).when(rtx).read( any(LogicalDatastoreType.class), any(InstanceIdentifier.class)); doReturn(rtx).when(dataBroker).newReadOnlyTransaction(); - doReturn(ENCRYPTED_STRING).when(AAA_ENCRYPTION_SERVICE).encrypt(isA(String.class)); + doReturn(ENCRYPTED_STRING.getBytes(Charset.defaultCharset())).when(encryptionService).encrypt(any()); + doReturn(PASSWORD.getBytes(Charset.defaultCharset())).when(encryptionService).decrypt(any()); // getKeystoresIid - InstanceIdentifier instanceIdentifierResult = KeyStoresDataUtils.getKeystoresIid(); + InstanceIdentifier instanceIdentifierResult = KeyStoresDataUtils.getKeystoresIid(); assertNotNull(instanceIdentifierResult); // getSslIid() diff --git a/aaa-encrypt-service/api/src/main/java/org/opendaylight/aaa/encrypt/AAAEncryptionService.java b/aaa-encrypt-service/api/src/main/java/org/opendaylight/aaa/encrypt/AAAEncryptionService.java index 82903574d..55f0a55eb 100644 --- a/aaa-encrypt-service/api/src/main/java/org/opendaylight/aaa/encrypt/AAAEncryptionService.java +++ b/aaa-encrypt-service/api/src/main/java/org/opendaylight/aaa/encrypt/AAAEncryptionService.java @@ -7,42 +7,31 @@ */ package org.opendaylight.aaa.encrypt; +import java.security.GeneralSecurityException; + /** * A generic encryption/decryption service for encrypting various data in ODL. * * @author - Sharon Aicler (saichler@gmail.com) */ public interface AAAEncryptionService { - - /** - * Encrypt data using a 2-way encryption mechanism. - * - * @param data plaintext data - * @return an encrypted representation of data - */ - String encrypt(String data); - /** - * Encrypt data using a 2-way encryption mechanism. + * Encrypt {@code data} using a 2-way encryption mechanism. * * @param data plaintext data - * @return an encrypted representation of data - */ - byte[] encrypt(byte[] data); - - /** - * Decrypt data using a 2-way decryption mechanism. - * - * @param encryptedData encrypted data - * @return plaintext data + * @return an encrypted representation of {@code data} + * @throws NullPointerException when {@code data} is {@code null} + * @throws GeneralSecurityException when encryption fails */ - String decrypt(String encryptedData); + byte[] encrypt(byte[] data) throws GeneralSecurityException; /** - * Decrypt data using a 2-way decryption mechanism. + * Decrypt {@code encryptedData} using a 2-way decryption mechanism. * * @param encryptedData encrypted data - * @return plaintext data + * @return plaintext bytes + * @throws NullPointerException when {@code encryptedData} is {@code null} + * @throws GeneralSecurityException when decryption fails */ - byte[] decrypt(byte[] encryptedData); + byte[] decrypt(byte[] encryptedData) throws GeneralSecurityException; } diff --git a/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptionServiceImpl.java b/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptionServiceImpl.java index 695c204f4..34c681e16 100644 --- a/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptionServiceImpl.java +++ b/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptionServiceImpl.java @@ -10,18 +10,11 @@ import static com.google.common.base.Verify.verifyNotNull; import static java.util.Objects.requireNonNull; -import java.nio.charset.Charset; -import java.security.InvalidAlgorithmParameterException; -import java.security.InvalidKeyException; -import java.security.NoSuchAlgorithmException; -import java.security.spec.InvalidKeySpecException; -import java.security.spec.KeySpec; -import java.util.Base64; +import java.security.GeneralSecurityException; import java.util.Map; import javax.crypto.BadPaddingException; import javax.crypto.Cipher; import javax.crypto.IllegalBlockSizeException; -import javax.crypto.NoSuchPaddingException; import javax.crypto.SecretKey; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.IvParameterSpec; @@ -54,37 +47,30 @@ public final class AAAEncryptionServiceImpl implements AAAEncryptionService { public AAAEncryptionServiceImpl(final EncryptServiceConfig configuration) { final byte[] encryptionKeySalt = configuration.requireEncryptSalt(); - IvParameterSpec tempIvSpec = null; - SecretKey tempKey = null; + final IvParameterSpec ivSpec; try { - final SecretKeyFactory keyFactory = SecretKeyFactory.getInstance(configuration.getEncryptMethod()); - final KeySpec spec = new PBEKeySpec(configuration.requireEncryptKey().toCharArray(), encryptionKeySalt, + final var keyFactory = SecretKeyFactory.getInstance(configuration.getEncryptMethod()); + final var spec = new PBEKeySpec(configuration.requireEncryptKey().toCharArray(), encryptionKeySalt, configuration.getEncryptIterationCount(), configuration.getEncryptKeyLength()); - tempKey = new SecretKeySpec(keyFactory.generateSecret(spec).getEncoded(), configuration.getEncryptType()); - tempIvSpec = new IvParameterSpec(encryptionKeySalt); - } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - LOG.error("Failed to initialize secret key", e); + key = new SecretKeySpec(keyFactory.generateSecret(spec).getEncoded(), configuration.getEncryptType()); + ivSpec = new IvParameterSpec(encryptionKeySalt); + } catch (GeneralSecurityException e) { + throw new IllegalStateException("Failed to initialize secret key", e); } - key = tempKey; - final var ivSpec = tempIvSpec; - Cipher cipher = null; try { - cipher = Cipher.getInstance(configuration.getCipherTransforms()); + final var cipher = Cipher.getInstance(configuration.getCipherTransforms()); cipher.init(Cipher.ENCRYPT_MODE, key, ivSpec); - } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidAlgorithmParameterException - | InvalidKeyException e) { - LOG.error("Failed to create encrypt cipher.", e); + encryptCipher = cipher; + } catch (GeneralSecurityException e) { + throw new IllegalStateException("Failed to create encrypt cipher.", e); } - encryptCipher = cipher; - cipher = null; try { - cipher = Cipher.getInstance(configuration.getCipherTransforms()); + final var cipher = Cipher.getInstance(configuration.getCipherTransforms()); cipher.init(Cipher.DECRYPT_MODE, key, ivSpec); - } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidAlgorithmParameterException - | InvalidKeyException e) { - LOG.error("Failed to create decrypt cipher.", e); + decryptCipher = cipher; + } catch (GeneralSecurityException e) { + throw new IllegalStateException("Failed to create decrypt cipher.", e); } - decryptCipher = cipher; LOG.info("AAAEncryptionService activated"); } @@ -104,73 +90,14 @@ void deactivate() { } @Override - public String encrypt(final String data) { - // We could not instantiate the encryption key, hence no encryption or - // decryption will be done. - if (key == null) { - LOG.warn("Encryption Key is NULL, will not encrypt data."); - return data; - } - - final byte[] cryptobytes; - try { - synchronized (encryptCipher) { - cryptobytes = encryptCipher.doFinal(data.getBytes(Charset.defaultCharset())); - } - } catch (IllegalBlockSizeException | BadPaddingException e) { - LOG.error("Failed to encrypt data.", e); - return data; - } - return Base64.getEncoder().encodeToString(cryptobytes); - } - - @Override - public byte[] encrypt(final byte[] data) { - // We could not instantiate the encryption key, hence no encryption or - // decryption will be done. - if (key == null) { - LOG.warn("Encryption Key is NULL, will not encrypt data."); - return data; - } - try { - synchronized (encryptCipher) { - return encryptCipher.doFinal(data); - } - } catch (IllegalBlockSizeException | BadPaddingException e) { - LOG.error("Failed to encrypt data.", e); - return data; + public byte[] encrypt(final byte[] data) throws BadPaddingException, IllegalBlockSizeException { + synchronized (encryptCipher) { + return encryptCipher.doFinal(requireNonNull(data)); } } @Override - public String decrypt(final String encryptedData) { - if (key == null || encryptedData == null || encryptedData.length() == 0) { - LOG.warn("String {} was not decrypted.", encryptedData); - return encryptedData; - } - - final byte[] cryptobytes = Base64.getDecoder().decode(encryptedData); - final byte[] clearbytes; - try { - clearbytes = decryptCipher.doFinal(cryptobytes); - } catch (IllegalBlockSizeException | BadPaddingException e) { - LOG.error("Failed to decrypt encoded data", e); - return encryptedData; - } - return new String(clearbytes, Charset.defaultCharset()); - } - - @Override - public byte[] decrypt(final byte[] encryptedData) { - if (encryptedData == null) { - LOG.warn("encryptedData is null."); - return encryptedData; - } - try { - return decryptCipher.doFinal(encryptedData); - } catch (IllegalBlockSizeException | BadPaddingException e) { - LOG.error("Failed to decrypt encoded data", e); - } - return encryptedData; + public byte[] decrypt(final byte[] encryptedData) throws BadPaddingException, IllegalBlockSizeException { + return decryptCipher.doFinal(requireNonNull(encryptedData)); } } diff --git a/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfigurator.java b/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfigurator.java index be485d03c..eca84bc4d 100644 --- a/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfigurator.java +++ b/aaa-encrypt-service/impl/src/main/java/org/opendaylight/aaa/encrypt/impl/OSGiEncryptionServiceConfigurator.java @@ -33,6 +33,7 @@ import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.binding.InstanceIdentifier; import org.osgi.framework.FrameworkUtil; +import org.osgi.service.component.ComponentException; import org.osgi.service.component.ComponentFactory; import org.osgi.service.component.ComponentInstance; import org.osgi.service.component.annotations.Activate; @@ -196,8 +197,14 @@ private synchronized void updateInstance(final AaaEncryptServiceConfig newConfig } disableInstance(); - instance = factory.newInstance(FrameworkUtil.asDictionary( - AAAEncryptionServiceImpl.props(new EncryptServiceConfigImpl(newConfig)))); + try { + instance = factory.newInstance(FrameworkUtil.asDictionary( + AAAEncryptionServiceImpl.props(new EncryptServiceConfigImpl(newConfig)))); + } catch (ComponentException e) { + LOG.error("Failed to start Encryption Service", e); + return; + } + current = newConfig; LOG.info("Encryption Service enabled"); } diff --git a/aaa-encrypt-service/impl/src/test/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptServiceImplTest.java b/aaa-encrypt-service/impl/src/test/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptServiceImplTest.java index 5eac2f18e..d0bbdf41e 100644 --- a/aaa-encrypt-service/impl/src/test/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptServiceImplTest.java +++ b/aaa-encrypt-service/impl/src/test/java/org/opendaylight/aaa/encrypt/impl/AAAEncryptServiceImplTest.java @@ -7,24 +7,28 @@ */ package org.opendaylight.aaa.encrypt.impl; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; -import java.nio.charset.StandardCharsets; -import java.util.Base64; -import org.junit.Before; -import org.junit.Test; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Arrays; +import javax.crypto.BadPaddingException; +import javax.crypto.IllegalBlockSizeException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.opendaylight.yang.gen.v1.config.aaa.authn.encrypt.service.config.rev160915.AaaEncryptServiceConfigBuilder; /* * @author - Sharon Aicler (saichler@gmail.com) */ @Deprecated -public class AAAEncryptServiceImplTest { +class AAAEncryptServiceImplTest { private AAAEncryptionServiceImpl impl; - @Before - public void setup() { + @BeforeEach + void setup() { impl = new AAAEncryptionServiceImpl(new EncryptServiceConfigImpl( OSGiEncryptionServiceConfigurator.generateConfig(new AaaEncryptServiceConfigBuilder() .setCipherTransforms("AES/CBC/PKCS5Padding") @@ -38,99 +42,100 @@ public void setup() { .build()))); } + private void changePadding() { + impl = new AAAEncryptionServiceImpl(new EncryptServiceConfigImpl( + OSGiEncryptionServiceConfigurator.generateConfig(new AaaEncryptServiceConfigBuilder() + .setCipherTransforms("AES/CBC/NoPadding") + .setEncryptIterationCount(32768) + .setEncryptKey("") + .setEncryptKeyLength(128) + .setEncryptMethod("PBKDF2WithHmacSHA1") + .setEncryptSalt("") + .setEncryptType("AES") + .setPasswordLength(12) + .build()))); + } + @Test - public void testShortString() { - String before = "shortone"; - String encrypt = impl.encrypt(before); - assertNotEquals(before, encrypt); - String after = impl.decrypt(encrypt); - assertEquals(before, after); + void testShortString() throws Exception { + final var before = "shortone".getBytes(); + final var encrypt = impl.encrypt(before); + assertFalse(Arrays.equals(before, encrypt)); + assertArrayEquals(before, impl.decrypt(encrypt)); } @Test - public void testLongString() { - String before = "This is a very long string to encrypt for testing 1...2...3"; - String encrypt = impl.encrypt(before); - assertNotEquals(before, encrypt); - String after = impl.decrypt(encrypt); - assertEquals(before, after); + void testLongString() throws Exception { + final var before = "This is a very long string to encrypt for testing 1...2...3".getBytes(); + final var encrypt = impl.encrypt(before); + assertFalse(Arrays.equals(before, encrypt)); + assertArrayEquals(before, impl.decrypt(encrypt)); } @Test - public void testNetconfEncodedPasswordWithoutPadding() { + void testNetconfEncodedPasswordWithoutPadding() { changePadding(); - String password = "bmV0Y29uZgo="; - String unencrypted = impl.decrypt(password); - assertEquals(password, unencrypted); + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("netconf\n".getBytes())); + assertEquals("Input length not multiple of 16 bytes", ex.getMessage()); } @Test - public void testNetconfEncodedPasswordWithPadding() { - String password = "bmV0Y29uZgo="; - String unencrypted = impl.decrypt(password); - assertEquals(password, unencrypted); + void testNetconfEncodedPasswordWithPadding() { + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("netconf\n".getBytes())); + assertEquals("Input length must be multiple of 16 when decrypting with padded cipher", ex.getMessage()); } @Test - public void testNetconfPasswordWithoutPadding() { + void testNetconfPasswordWithoutPadding() { changePadding(); - String password = "netconf"; - String encodedPassword = Base64.getEncoder().encodeToString(password.getBytes(StandardCharsets.UTF_8)); - String unencrypted = impl.decrypt(encodedPassword); - assertEquals(encodedPassword, unencrypted); + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("netconf".getBytes())); + assertEquals("Input length not multiple of 16 bytes", ex.getMessage()); } @Test - public void testNetconfPasswordWithPadding() { - String password = "netconf"; - String encodedPassword = Base64.getEncoder().encodeToString(password.getBytes(StandardCharsets.UTF_8)); - String unencrypted = impl.decrypt(encodedPassword); - assertEquals(encodedPassword, unencrypted); + void testNetconfPasswordWithPadding() { + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("netconf".getBytes())); + assertEquals("Input length must be multiple of 16 when decrypting with padded cipher", ex.getMessage()); } @Test - public void testAdminEncodedPasswordWithoutPadding() { + void testAdminEncodedPasswordWithoutPadding() { changePadding(); - String password = "YWRtaW4K"; - String unencrypted = impl.decrypt(password); - assertEquals(password, unencrypted); + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("admin\n".getBytes())); + assertEquals("Input length not multiple of 16 bytes", ex.getMessage()); } @Test - public void testAdminEncodedPasswordWithPadding() { - String password = "YWRtaW4K"; - String unencrypted = impl.decrypt(password); - assertEquals(password, unencrypted); + void testAdminEncodedPasswordWithPadding() { + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("admin\n".getBytes())); + assertEquals("Input length must be multiple of 16 when decrypting with padded cipher", ex.getMessage()); } @Test - public void testAdminPasswordWithoutPadding() { + void testAdminPasswordWithoutPadding() { changePadding(); - String password = "admin"; - String encodedPassword = Base64.getEncoder().encodeToString(password.getBytes(StandardCharsets.UTF_8)); - String unencrypted = impl.decrypt(encodedPassword); - assertEquals(encodedPassword, unencrypted); + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("admin".getBytes())); + assertEquals("Input length not multiple of 16 bytes", ex.getMessage()); } @Test - public void testAdminPasswordWithPadding() { - String password = "admin"; - String encodedPassword = Base64.getEncoder().encodeToString(password.getBytes(StandardCharsets.UTF_8)); - String unencrypted = impl.decrypt(encodedPassword); - assertEquals(encodedPassword, unencrypted); + void testAdminPasswordWithPadding() { + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("admin".getBytes())); + assertEquals("Input length must be multiple of 16 when decrypting with padded cipher", ex.getMessage()); } - private void changePadding() { - impl = new AAAEncryptionServiceImpl(new EncryptServiceConfigImpl( - OSGiEncryptionServiceConfigurator.generateConfig(new AaaEncryptServiceConfigBuilder() - .setCipherTransforms("AES/CBC/NoPadding") - .setEncryptIterationCount(32768) - .setEncryptKey("") - .setEncryptKeyLength(128) - .setEncryptMethod("PBKDF2WithHmacSHA1") - .setEncryptSalt("") - .setEncryptType("AES") - .setPasswordLength(12) - .build()))); + @Test + void testDecryptWithIllegalBlockSizeException() { + final var ex = assertThrows(IllegalBlockSizeException.class, () -> impl.decrypt("adminadmin".getBytes())); + assertEquals("Input length must be multiple of 16 when decrypting with padded cipher", ex.getMessage()); + } + + @Test + void testDecryptWithBadPaddingException() { + final var bytes = new byte[] { 85, -87, 98, 116, -23, -84, 123, -82, 4, -99, -54, 29, 121, -48, -38, -75 }; + final var ex = assertThrows(BadPaddingException.class, () -> impl.decrypt(bytes)); + assertEquals( + "Given final block not properly padded. Such issues can arise if a bad key is used during decryption.", + ex.getMessage()); } }