Updated Branches: refs/heads/4.3 742bf3ccc -> 9f4c46126
few cleanups in CertServiceTest and CertService Tests: - all tests are @Test rather than having one test to call them, so they can be run one by one - tests that expect exception from a method fail if there is none - no longer extends TestCase so that the original method names could be kept as test Implementation: - include root cause in exceptions when possible - helps at troubleshuting - close readers Signed-off-by: Laszlo Hornyak <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/9a3d32d1 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/9a3d32d1 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/9a3d32d1 Branch: refs/heads/4.3 Commit: 9a3d32d12e3e1b310db84f96f66af44aaca69c5c Parents: 742bf3c Author: Laszlo Hornyak <[email protected]> Authored: Sun Nov 10 16:40:35 2013 +0100 Committer: Laszlo Hornyak <[email protected]> Committed: Wed Nov 13 22:28:50 2013 +0100 ---------------------------------------------------------------------- .../cloudstack/network/lb/CertServiceImpl.java | 43 +++--- .../cloudstack/network/lb/CertServiceTest.java | 135 ++++++++----------- 2 files changed, 80 insertions(+), 98 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/9a3d32d1/server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java ---------------------------------------------------------------------- diff --git a/server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java b/server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java index 53dae50..74adb37 100644 --- a/server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java +++ b/server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java @@ -24,16 +24,20 @@ import com.cloud.network.rules.LoadBalancer; import com.cloud.user.dao.AccountDao; import com.cloud.utils.db.EntityManager; import com.cloud.utils.exception.CloudRuntimeException; + import org.apache.cloudstack.api.command.user.loadbalancer.DeleteSslCertCmd; import org.apache.cloudstack.api.command.user.loadbalancer.ListSslCertsCmd; import org.apache.cloudstack.api.command.user.loadbalancer.UploadSslCertCmd; import org.apache.cloudstack.api.response.SslCertResponse; + import com.cloud.exception.InvalidParameterValueException; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.utils.db.DB; + import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.context.CallContext; +import org.apache.commons.io.IOUtils; import org.apache.log4j.Logger; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.bouncycastle.openssl.PEMReader; @@ -45,6 +49,7 @@ import javax.crypto.IllegalBlockSizeException; import javax.crypto.NoSuchPaddingException; import javax.ejb.Local; import javax.inject.Inject; + import java.io.IOException; import java.io.StringReader; import java.io.UnsupportedEncodingException; @@ -228,7 +233,7 @@ public class CertServiceImpl implements CertService { } } catch (IOException e) { - throw new IllegalArgumentException("Parsing certificate/key failed: " + e.getMessage()); + throw new IllegalArgumentException("Parsing certificate/key failed: " + e.getMessage(), e); } validateCert(cert, _chain != null? true: false); @@ -273,7 +278,7 @@ public class CertServiceImpl implements CertService { try { ((X509Certificate)cert).checkValidity(); } catch (Exception e) { - throw new IllegalArgumentException("Certificate expired or not valid"); + throw new IllegalArgumentException("Certificate expired or not valid", e); } if( !chain_present ) { @@ -281,7 +286,7 @@ public class CertServiceImpl implements CertService { try { cert.verify(pubKey); } catch (Exception e) { - throw new IllegalArgumentException("No chain given and certificate not self signed"); + throw new IllegalArgumentException("No chain given and certificate not self signed", e); } } } @@ -309,15 +314,15 @@ public class CertServiceImpl implements CertService { throw new IllegalArgumentException("Bad public-private key"); } catch (BadPaddingException e) { - throw new IllegalArgumentException("Bad public-private key"); + throw new IllegalArgumentException("Bad public-private key", e); } catch (IllegalBlockSizeException e) { - throw new IllegalArgumentException("Bad public-private key"); + throw new IllegalArgumentException("Bad public-private key", e); } catch (NoSuchPaddingException e) { - throw new IllegalArgumentException("Bad public-private key"); + throw new IllegalArgumentException("Bad public-private key", e); } catch (InvalidKeyException e) { - throw new IllegalArgumentException("Invalid public-private key"); + throw new IllegalArgumentException("Invalid public-private key", e); } catch (NoSuchAlgorithmException e) { - throw new IllegalArgumentException("Invalid algorithm for public-private key"); + throw new IllegalArgumentException("Invalid algorithm for public-private key", e); } } @@ -363,11 +368,11 @@ public class CertServiceImpl implements CertService { CertPathBuilder builder = CertPathBuilder.getInstance("PKIX"); builder.build(params); } catch (InvalidAlgorithmParameterException e) { - throw new IllegalArgumentException("Invalid certificate chain",null); + throw new IllegalArgumentException("Invalid certificate chain", e); } catch (CertPathBuilderException e) { - throw new IllegalArgumentException("Invalid certificate chain",null); + throw new IllegalArgumentException("Invalid certificate chain", e); } catch (NoSuchAlgorithmException e) { - throw new IllegalArgumentException("Invalid certificate chain",null); + throw new IllegalArgumentException("Invalid certificate chain", e); } } @@ -380,7 +385,12 @@ public class CertServiceImpl implements CertService { pGet = new KeyPassword(password.toCharArray()); PEMReader privateKey = new PEMReader(new StringReader(key), pGet); - Object obj = privateKey.readObject(); + Object obj = null; + try { + obj = privateKey.readObject(); + } finally { + IOUtils.closeQuietly(privateKey); + } try { @@ -389,9 +399,8 @@ public class CertServiceImpl implements CertService { return (PrivateKey) obj; - }catch (Exception e){ - e.printStackTrace(); - throw new IOException("Invalid Key format or invalid password."); + } catch (Exception e){ + throw new IOException("Invalid Key format or invalid password.", e); } } @@ -402,6 +411,8 @@ public class CertServiceImpl implements CertService { return (Certificate) certPem.readObject(); } catch (Exception e) { throw new InvalidParameterValueException("Invalid Certificate format. Expected X509 certificate"); + } finally { + IOUtils.closeQuietly(certPem); } } @@ -419,7 +430,7 @@ public class CertServiceImpl implements CertService { } } if ( certs.size() == 0 ) - throw new IllegalArgumentException("Unable to decode certificate chain",null); + throw new IllegalArgumentException("Unable to decode certificate chain"); return certs; } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/9a3d32d1/server/test/org/apache/cloudstack/network/lb/CertServiceTest.java ---------------------------------------------------------------------- diff --git a/server/test/org/apache/cloudstack/network/lb/CertServiceTest.java b/server/test/org/apache/cloudstack/network/lb/CertServiceTest.java index e47fc01..97c8b7b 100644 --- a/server/test/org/apache/cloudstack/network/lb/CertServiceTest.java +++ b/server/test/org/apache/cloudstack/network/lb/CertServiceTest.java @@ -23,13 +23,10 @@ import com.cloud.user.AccountVO; import com.cloud.user.UserVO; import com.cloud.user.dao.AccountDao; import com.cloud.utils.db.EntityManager; -import com.cloud.utils.db.Transaction; import com.cloud.utils.db.TransactionLegacy; -import junit.framework.TestCase; import org.apache.cloudstack.api.command.user.loadbalancer.DeleteSslCertCmd; import org.apache.cloudstack.api.command.user.loadbalancer.UploadSslCertCmd; import org.apache.cloudstack.context.CallContext; -import org.apache.log4j.Logger; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -40,19 +37,17 @@ import java.io.IOException; import java.lang.reflect.Field; import java.net.URLEncoder; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.UUID; import static org.apache.commons.io.FileUtils.readFileToString; import static org.mockito.Matchers.*; import static org.mockito.Mockito.when; +import static org.junit.Assert.fail; +import static org.junit.Assert.assertTrue; -public class CertServiceTest extends TestCase { +public class CertServiceTest { - private static final Logger s_logger = Logger.getLogger( CertServiceTest.class); - - @Override @Before public void setUp() { Account account = new AccountVO("testaccount", 1, "networkdomain", (short)0, UUID.randomUUID().toString()); @@ -60,58 +55,16 @@ public class CertServiceTest extends TestCase { CallContext.register(user, account); } - @Override @After public void tearDown() { CallContext.unregister(); } @Test - public void testUploadSslCert() throws Exception { - - /* Test1 : Given a Certificate in bad format (Not PEM), upload should fail */ - runUploadSslCertBadFormat(); - - /* Test2: Given a Certificate which is not X509, upload should fail */ - runUploadSslCertNotX509(); - - /* Test3: Given an expired certificate, upload should fail */ - runUploadSslCertExpiredCert(); - - /* Test4: Given a private key which has a different algorithm than the certificate, - upload should fail. - */ - runUploadSslCertBadkeyAlgo(); - - /* Test5: Given a private key which does not match the public key in the certificate, - upload should fail - */ - runUploadSslCertBadkeyPair(); - - /* Test6: Given an encrypted private key with a bad password. Upload should fail. */ - runUploadSslCertBadPassword(); - - /* Test7: If no chain is given, the certificate should be self signed. Else, uploadShould Fail */ - runUploadSslCertNoChain(); - - /* Test8: Chain is given but does not have root certificate */ - runUploadSslCertNoRootCert(); - - /* Test9: The chain given is not the correct chain for the certificate */ - runUploadSslCertBadChain(); - - /* Test10: Given a Self-signed Certificate with encrypted key, upload should succeed */ - runUploadSslCertSelfSignedNoPassword(); - - /* Test11: Given a Self-signed Certificate with non-encrypted key, upload should succeed */ - runUploadSslCertSelfSignedWithPassword(); - - /* Test12: Given a certificate signed by a CA and a valid CA chain, upload should succeed */ - runUploadSslCertWithCAChain(); - - } - - private void runUploadSslCertWithCAChain() throws Exception { + /** + * Given a certificate signed by a CA and a valid CA chain, upload should succeed + */ + public void runUploadSslCertWithCAChain() throws Exception { //To change body of created methods use File | Settings | File Templates. TransactionLegacy txn = TransactionLegacy.open("runUploadSslCertWithCAChain"); @@ -165,7 +118,11 @@ public class CertServiceTest extends TestCase { certService.uploadSslCert(uploadCmd); } - private void runUploadSslCertSelfSignedWithPassword() throws Exception { + @Test + /** + * Given a Self-signed Certificate with non-encrypted key, upload should succeed + */ + public void runUploadSslCertSelfSignedWithPassword() throws Exception { TransactionLegacy txn = TransactionLegacy.open("runUploadSslCertSelfSignedWithPassword"); @@ -212,7 +169,11 @@ public class CertServiceTest extends TestCase { certService.uploadSslCert(uploadCmd); } - private void runUploadSslCertSelfSignedNoPassword() throws Exception { + @Test + /** + * Given a Self-signed Certificate with encrypted key, upload should succeed + */ + public void runUploadSslCertSelfSignedNoPassword() throws Exception { TransactionLegacy txn = TransactionLegacy.open("runUploadSslCertSelfSignedNoPassword"); @@ -255,7 +216,8 @@ public class CertServiceTest extends TestCase { } - private void runUploadSslCertBadChain() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertBadChain() throws IOException, IllegalAccessException, NoSuchFieldException { //To change body of created methods use File | Settings | File Templates. String certFile = getClass().getResource("/certs/rsa_ca_signed.crt").getFile(); String keyFile = getClass().getResource("/certs/rsa_ca_signed.key").getFile(); @@ -300,12 +262,14 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("The chain given is not the correct chain for the certificate"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid certificate chain")); } } - private void runUploadSslCertNoRootCert() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertNoRootCert() throws IOException, IllegalAccessException, NoSuchFieldException { //To change body of created methods use File | Settings | File Templates. String certFile = getClass().getResource("/certs/rsa_ca_signed.crt").getFile(); @@ -351,13 +315,15 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Chain is given but does not have root certificate"); } catch (Exception e) { assertTrue(e.getMessage().contains("No root certificates found")); } } - private void runUploadSslCertNoChain() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertNoChain() throws IOException, IllegalAccessException, NoSuchFieldException { String certFile = getClass().getResource("/certs/rsa_ca_signed.crt").getFile(); String keyFile = getClass().getResource("/certs/rsa_ca_signed.key").getFile(); @@ -394,16 +360,17 @@ public class CertServiceTest extends TestCase { passField.setAccessible(true); passField.set(uploadCmd, password); - try { certService.uploadSslCert(uploadCmd); + fail("If no chain is given, the certificate should be self signed. Else, uploadShould Fail"); } catch (Exception e) { assertTrue(e.getMessage().contains("No chain given and certificate not self signed")); } } - private void runUploadSslCertBadPassword() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertBadPassword() throws IOException, IllegalAccessException, NoSuchFieldException { String certFile = getClass().getResource("/certs/rsa_self_signed_with_pwd.crt").getFile(); String keyFile = getClass().getResource("/certs/rsa_self_signed_with_pwd.key").getFile(); @@ -443,13 +410,15 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Given an encrypted private key with a bad password. Upload should fail."); } catch (Exception e) { assertTrue(e.getMessage().contains("please check password and data")); } } - private void runUploadSslCertBadkeyPair() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertBadkeyPair() throws IOException, IllegalAccessException, NoSuchFieldException { // Reading appropritate files String certFile = getClass().getResource("/certs/rsa_self_signed.crt").getFile(); String keyFile = getClass().getResource("/certs/rsa_random_pkey.key").getFile(); @@ -487,7 +456,8 @@ public class CertServiceTest extends TestCase { } } - private void runUploadSslCertBadkeyAlgo() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertBadkeyAlgo() throws IOException, IllegalAccessException, NoSuchFieldException { // Reading appropritate files String certFile = getClass().getResource("/certs/rsa_self_signed.crt").getFile(); @@ -521,12 +491,14 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Given a private key which has a different algorithm than the certificate, upload should fail"); } catch (Exception e) { assertTrue(e.getMessage().contains("Public and private key have different algorithms")); } } - private void runUploadSslCertExpiredCert() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertExpiredCert() throws IOException, IllegalAccessException, NoSuchFieldException { // Reading appropritate files String certFile = getClass().getResource("/certs/expired_cert.crt").getFile(); @@ -560,12 +532,14 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Given an expired certificate, upload should fail"); } catch (Exception e) { assertTrue(e.getMessage().contains("Certificate expired")); } } - private void runUploadSslCertNotX509() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertNotX509() throws IOException, IllegalAccessException, NoSuchFieldException { // Reading appropritate files String certFile = getClass().getResource("/certs/non_x509_pem.crt").getFile(); String keyFile = getClass().getResource("/certs/rsa_self_signed.key").getFile(); @@ -598,12 +572,14 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Given a Certificate which is not X509, upload should fail"); } catch (Exception e) { assertTrue(e.getMessage().contains("Expected X509 certificate")); } } - private void runUploadSslCertBadFormat() throws IOException, IllegalAccessException, NoSuchFieldException { + @Test + public void runUploadSslCertBadFormat() throws IOException, IllegalAccessException, NoSuchFieldException { // Reading appropritate files String certFile = getClass().getResource("/certs/bad_format_cert.crt").getFile(); @@ -637,6 +613,7 @@ public class CertServiceTest extends TestCase { try { certService.uploadSslCert(uploadCmd); + fail("Given a Certificate in bad format (Not PEM), upload should fail"); } catch (Exception e) { assertTrue(e.getMessage().contains("Invalid certificate format")); } @@ -644,20 +621,10 @@ public class CertServiceTest extends TestCase { @Test - public void testDeleteSslCert() throws Exception { - /* Test1: Delete with an invalid ID should fail */ - runDeleteSslCertInvalidId(); - - /* Test2: Delete with a cert id bound to a lb should fail */ - runDeleteSslCertBoundCert(); - - /* Test3: Delete with a valid Id should succeed */ - runDeleteSslCertValid(); - - - } - - private void runDeleteSslCertValid() throws Exception { + /** + * Delete with a valid Id should succeed + */ + public void runDeleteSslCertValid() throws Exception { TransactionLegacy txn = TransactionLegacy.open("runDeleteSslCertValid"); @@ -690,7 +657,8 @@ public class CertServiceTest extends TestCase { certService.deleteSslCert(deleteCmd); } - private void runDeleteSslCertBoundCert() throws NoSuchFieldException, IllegalAccessException { + @Test + public void runDeleteSslCertBoundCert() throws NoSuchFieldException, IllegalAccessException { TransactionLegacy txn = TransactionLegacy.open("runDeleteSslCertBoundCert"); @@ -731,6 +699,7 @@ public class CertServiceTest extends TestCase { try { certService.deleteSslCert(deleteCmd); + fail("Delete with a cert id bound to a lb should fail"); }catch (Exception e){ assertTrue(e.getMessage().contains("Certificate in use by a loadbalancer")); } @@ -738,7 +707,8 @@ public class CertServiceTest extends TestCase { } - private void runDeleteSslCertInvalidId() throws NoSuchFieldException, IllegalAccessException { + @Test + public void runDeleteSslCertInvalidId() throws NoSuchFieldException, IllegalAccessException { TransactionLegacy txn = TransactionLegacy.open("runDeleteSslCertInvalidId"); @@ -768,6 +738,7 @@ public class CertServiceTest extends TestCase { try { certService.deleteSslCert(deleteCmd); + fail("Delete with an invalid ID should fail"); }catch (Exception e){ assertTrue(e.getMessage().contains("Invalid certificate id")); }
