cloudstack-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ko...@apache.org
Subject git commit: updated refs/heads/master to bd67ccd
Date Tue, 12 Nov 2013 07:17:39 GMT
Updated Branches:
  refs/heads/master 5420cbe98 -> bd67ccdd6


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 <laszlo.hornyak@gmail.com>


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/bd67ccdd
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/bd67ccdd
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/bd67ccdd

Branch: refs/heads/master
Commit: bd67ccdd6dedf1f56a0fa45de17690ce7608fa5d
Parents: 5420cbe
Author: Laszlo Hornyak <laszlo.hornyak@gmail.com>
Authored: Sun Nov 10 16:40:35 2013 +0100
Committer: Laszlo Hornyak <laszlo.hornyak@gmail.com>
Committed: Tue Nov 12 08:13:59 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/bd67ccdd/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/bd67ccdd/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"));
         }


Mime
View raw message