You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ko...@apache.org on 2013/11/12 08:17:39 UTC
git commit: updated refs/heads/master to bd67ccd
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 <la...@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 <la...@gmail.com>
Authored: Sun Nov 10 16:40:35 2013 +0100
Committer: Laszlo Hornyak <la...@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"));
}