Avoid duplicated verification if package is signed
Fix potential thread leak by using try-with-resource.
Change-Id: I20eda524cd028b9dfdfa83207a059f4e1dfeff5e
Signed-off-by: Vasyl Razinkov <vasyl.razinkov@est.tech>
Issue-ID: SDC-2580
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java b/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java
index bf029fb..97bc375 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/vendor-software-products-rest/vendor-software-products-rest-services/src/main/java/org/openecomp/sdcrests/vsp/rest/data/PackageArchive.java
@@ -19,6 +19,11 @@
*/
package org.openecomp.sdcrests.vsp.rest.data;
+import java.io.IOException;
+import java.security.cert.CertificateException;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.cxf.jaxrs.ext.multipart.Attachment;
@@ -29,17 +34,12 @@
import org.openecomp.sdc.vendorsoftwareproduct.security.SecurityManager;
import org.openecomp.sdc.vendorsoftwareproduct.security.SecurityManagerException;
-import java.io.IOException;
-import java.security.cert.CertificateException;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-
/**
- * Class responsible for processing zip archive and verify if this package corresponds
- * SOL004 option 2 signed package format, verifies the cms signature if package is signed
+ * Class responsible for processing zip archive and verify if this package corresponds SOL004 option 2 signed package
+ * format, verifies the cms signature if package is signed
*/
public class PackageArchive {
+
private static final Logger LOG = LoggerFactory.getLogger(PackageArchive.class);
private static final String[] ALLOWED_ARCHIVE_EXTENSIONS = {"csar", "zip"};
private static final String[] ALLOWED_SIGNATURE_EXTENSIONS = {"cms"};
@@ -49,6 +49,7 @@
private final SecurityManager securityManager;
private final byte[] outerPackageFileBytes;
private Pair<FileContentHandler, List<String>> handlerPair;
+ private Boolean signatureValid;
public PackageArchive(Attachment uploadedFile) {
this(uploadedFile.getObject(byte[].class));
@@ -59,7 +60,7 @@
this.securityManager = SecurityManager.getInstance();
try {
handlerPair = CommonUtil.getFileContentMapFromOrchestrationCandidateZip(
- outerPackageFileBytes);
+ outerPackageFileBytes);
} catch (IOException exception) {
LOG.error("Error reading files inside archive", exception);
}
@@ -88,6 +89,7 @@
/**
* Gets csar/zip package content from zip archive
+ *
* @return csar package content
* @throws SecurityManagerException
*/
@@ -97,37 +99,38 @@
return handlerPair.getKey().getFiles().get(getArchiveFileName().orElseThrow(CertificateException::new));
}
} catch (CertificateException exception) {
- LOG.info("Error verifying signature " + exception);
+ LOG.info("Error verifying signature ", exception);
}
return outerPackageFileBytes;
}
/**
* Validates package signature against trusted certificates
+ *
* @return true if signature verified
* @throws SecurityManagerException
*/
public boolean isSignatureValid() throws SecurityManagerException {
- Map<String, byte[]> files = handlerPair.getLeft().getFiles();
- Optional<String> signatureFileName = getSignatureFileName();
- Optional<String> archiveFileName = getArchiveFileName();
- if (files.isEmpty() || !signatureFileName.isPresent() || !archiveFileName.isPresent()) {
- return false;
+ if (signatureValid == null) {
+ final Map<String, byte[]> files = handlerPair.getLeft().getFiles();
+ final Optional<String> signatureFileName = getSignatureFileName();
+ final Optional<String> archiveFileName = getArchiveFileName();
+ if (files.isEmpty() || !signatureFileName.isPresent() || !archiveFileName.isPresent()) {
+ signatureValid = false;
+ } else {
+ final Optional<String> certificateFile = getCertificateFileName();
+ signatureValid = securityManager.verifySignedData(files.get(signatureFileName.get()),
+ certificateFile.map(files::get).orElse(null), files.get(archiveFileName.get()));
+ }
+
}
- Optional<String> certificateFile = getCertificateFileName();
- if(certificateFile.isPresent()){
- return securityManager.verifySignedData(files.get(signatureFileName.get()),
- files.get(certificateFile.get()), files.get(archiveFileName.get()));
- }else {
- return securityManager.verifySignedData(files.get(signatureFileName.get()),
- null, files.get(archiveFileName.get()));
- }
+ return signatureValid;
}
private boolean isPackageSizeMatches() {
return handlerPair.getRight().isEmpty()
- && (handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITH_CERT_INSIDE
- || handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITHOUT_CERT_INSIDE);
+ && (handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITH_CERT_INSIDE
+ || handlerPair.getLeft().getFiles().size() == NUMBER_OF_FILES_FOR_SIGNATURE_WITHOUT_CERT_INSIDE);
}
private Optional<String> getSignatureFileName() {
@@ -147,7 +150,7 @@
private Optional<String> getCertificateFileName() {
Optional<String> certFileName = getFileByExtension(ALLOWED_CERTIFICATE_EXTENSIONS);
- if(!certFileName.isPresent()){
+ if (!certFileName.isPresent()) {
return Optional.empty();
}
String certNameWithoutExtension = FilenameUtils.removeExtension(certFileName.get());
diff --git a/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java b/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java
index 7b1890d..90bfb67 100644
--- a/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java
+++ b/openecomp-be/backend/openecomp-sdc-vendor-software-product-manager/src/main/java/org/openecomp/sdc/vendorsoftwareproduct/security/SecurityManager.java
@@ -20,21 +20,6 @@
package org.openecomp.sdc.vendorsoftwareproduct.security;
import com.google.common.collect.ImmutableSet;
-import org.bouncycastle.asn1.cms.ContentInfo;
-import org.bouncycastle.cert.X509CertificateHolder;
-import org.bouncycastle.cms.CMSException;
-import org.bouncycastle.cms.CMSProcessableByteArray;
-import org.bouncycastle.cms.CMSSignedData;
-import org.bouncycastle.cms.CMSTypedData;
-import org.bouncycastle.cms.SignerInformation;
-import org.bouncycastle.cms.jcajce.JcaSimpleSignerInfoVerifierBuilder;
-import org.bouncycastle.jce.provider.BouncyCastleProvider;
-import org.bouncycastle.openssl.PEMParser;
-import org.bouncycastle.operator.OperatorCreationException;
-import org.bouncycastle.util.Store;
-import org.openecomp.sdc.logging.api.Logger;
-import org.openecomp.sdc.logging.api.LoggerFactory;
-
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileInputStream;
@@ -65,14 +50,30 @@
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
+import org.bouncycastle.asn1.cms.ContentInfo;
+import org.bouncycastle.cert.X509CertificateHolder;
+import org.bouncycastle.cms.CMSException;
+import org.bouncycastle.cms.CMSProcessableByteArray;
+import org.bouncycastle.cms.CMSSignedData;
+import org.bouncycastle.cms.CMSTypedData;
+import org.bouncycastle.cms.SignerInformation;
+import org.bouncycastle.cms.jcajce.JcaSimpleSignerInfoVerifierBuilder;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.openssl.PEMParser;
+import org.bouncycastle.operator.OperatorCreationException;
+import org.bouncycastle.util.Store;
+import org.openecomp.sdc.logging.api.Logger;
+import org.openecomp.sdc.logging.api.LoggerFactory;
/**
- * This is temporary solution. When AAF provides functionality for verifying trustedCertificates, this class should be reviewed
- * Class is responsible for providing root trustedCertificates from configured location in onboarding container.
+ * This is temporary solution. When AAF provides functionality for verifying trustedCertificates, this class should be
+ * reviewed Class is responsible for providing root trustedCertificates from configured location in onboarding
+ * container.
*/
public class SecurityManager {
+
private static final String CERTIFICATE_DEFAULT_LOCATION = "cert";
- private static final SecurityManager INSTANCE = new SecurityManager();
+ private static SecurityManager INSTANCE = null;
private Logger logger = LoggerFactory.getLogger(SecurityManager.class);
private Set<X509Certificate> trustedCertificates = new HashSet<>();
@@ -88,12 +89,14 @@
certificateDirectory = this.getcertDirectory();
}
- public static SecurityManager getInstance(){
+ public static SecurityManager getInstance() {
+ if (INSTANCE == null) {
+ INSTANCE = new SecurityManager();
+ }
return INSTANCE;
}
/**
- *
* Checks the configured location for available trustedCertificates
*
* @return set of trustedCertificates
@@ -116,12 +119,11 @@
/**
* Cleans certificate collection
*/
- public void cleanTrustedCertificates(){
+ public void cleanTrustedCertificates() {
trustedCertificates.clear();
}
/**
- *
* Verifies if packaged signed with trusted certificate
*
* @param messageSyntaxSignature - signature data in cms format
@@ -131,34 +133,33 @@
* @throws SecurityManagerException
*/
public boolean verifySignedData(final byte[] messageSyntaxSignature, final byte[] packageCert,
- final byte[] innerPackageFile) throws SecurityManagerException{
- try (ByteArrayInputStream signatureStream = new ByteArrayInputStream(messageSyntaxSignature)) {
- Object parsedObject = new PEMParser(new InputStreamReader(signatureStream)).readObject();
+ final byte[] innerPackageFile) throws SecurityManagerException {
+ try (ByteArrayInputStream signatureStream = new ByteArrayInputStream(messageSyntaxSignature);
+ final PEMParser pemParser = new PEMParser(new InputStreamReader(signatureStream))) {
+ final Object parsedObject = pemParser.readObject();
if (!(parsedObject instanceof ContentInfo)) {
throw new SecurityManagerException("Signature is not recognized");
}
- ContentInfo signature = ContentInfo.getInstance(parsedObject);
- CMSTypedData signedContent = new CMSProcessableByteArray(innerPackageFile);
- CMSSignedData signedData = new CMSSignedData(signedContent, signature);
+ final ContentInfo signature = ContentInfo.getInstance(parsedObject);
+ final CMSTypedData signedContent = new CMSProcessableByteArray(innerPackageFile);
+ final CMSSignedData signedData = new CMSSignedData(signedContent, signature);
- Collection<SignerInformation> signers = signedData.getSignerInfos().getSigners();
- SignerInformation firstSigner = signers.iterator().next();
- Store certificates = signedData.getCertificates();
- X509Certificate cert;
+ final Collection<SignerInformation> signers = signedData.getSignerInfos().getSigners();
+ final SignerInformation firstSigner = signers.iterator().next();
+ final X509Certificate cert;
if (packageCert == null) {
- Collection<X509CertificateHolder> firstSignerCertificates = certificates.getMatches(firstSigner.getSID());
- if(!firstSignerCertificates.iterator().hasNext()){
- throw new SecurityManagerException("No certificate found in cms signature that should contain one!");
+ final Collection<X509CertificateHolder> firstSignerCertificates = signedData.getCertificates()
+ .getMatches(firstSigner.getSID());
+ if (!firstSignerCertificates.iterator().hasNext()) {
+ throw new SecurityManagerException(
+ "No certificate found in cms signature that should contain one!");
}
- X509CertificateHolder firstSignerFirstCertificate = firstSignerCertificates.iterator().next();
- cert = loadCertificate(firstSignerFirstCertificate.getEncoded());
+ cert = loadCertificate(firstSignerCertificates.iterator().next().getEncoded());
} else {
cert = loadCertificate(packageCert);
}
- PKIXCertPathBuilderResult result = verifyCertificate(cert, getTrustedCertificates());
-
- if (result == null) {
+ if (verifyCertificate(cert, getTrustedCertificates()) == null) {
return false;
}
@@ -166,7 +167,7 @@
} catch (OperatorCreationException | IOException | CMSException e) {
logger.error(e.getMessage(), e);
throw new SecurityManagerException("Unexpected error occurred during signature validation!", e);
- } catch (GeneralSecurityException e){
+ } catch (GeneralSecurityException e) {
throw new SecurityManagerException("Could not verify signature!", e);
}
}
@@ -214,36 +215,37 @@
}
private PKIXCertPathBuilderResult verifyCertificate(X509Certificate cert,
- Set<X509Certificate> additionalCerts) throws GeneralSecurityException, SecurityManagerException {
- if (null == cert) {
- throw new SecurityManagerException("The certificate is empty!");
- }
+ Set<X509Certificate> additionalCerts)
+ throws GeneralSecurityException, SecurityManagerException {
+ if (null == cert) {
+ throw new SecurityManagerException("The certificate is empty!");
+ }
- if (isExpired(cert)) {
- throw new SecurityManagerException("The certificate expired on: " + cert.getNotAfter());
- }
+ if (isExpired(cert)) {
+ throw new SecurityManagerException("The certificate expired on: " + cert.getNotAfter());
+ }
- if (isSelfSigned(cert)) {
- throw new SecurityManagerException("The certificate is self-signed.");
- }
+ if (isSelfSigned(cert)) {
+ throw new SecurityManagerException("The certificate is self-signed.");
+ }
- Set<X509Certificate> trustedRootCerts = new HashSet<>();
- Set<X509Certificate> intermediateCerts = new HashSet<>();
- for (X509Certificate additionalCert : additionalCerts) {
- if (isSelfSigned(additionalCert)) {
- trustedRootCerts.add(additionalCert);
- } else {
- intermediateCerts.add(additionalCert);
- }
+ Set<X509Certificate> trustedRootCerts = new HashSet<>();
+ Set<X509Certificate> intermediateCerts = new HashSet<>();
+ for (X509Certificate additionalCert : additionalCerts) {
+ if (isSelfSigned(additionalCert)) {
+ trustedRootCerts.add(additionalCert);
+ } else {
+ intermediateCerts.add(additionalCert);
}
+ }
- return verifyCertificate(cert, trustedRootCerts, intermediateCerts);
+ return verifyCertificate(cert, trustedRootCerts, intermediateCerts);
}
private PKIXCertPathBuilderResult verifyCertificate(X509Certificate cert,
Set<X509Certificate> allTrustedRootCerts,
Set<X509Certificate> allIntermediateCerts)
- throws GeneralSecurityException {
+ throws GeneralSecurityException {
// Create the selector that specifies the starting certificate
X509CertSelector selector = new X509CertSelector();
@@ -272,13 +274,15 @@
pkixParams.addCertStore(createCertStore(allIntermediateCerts));
pkixParams.addCertStore(createCertStore(allTrustedRootCerts));
- CertPathBuilder builder = CertPathBuilder.getInstance(CertPathBuilder.getDefaultType(), BouncyCastleProvider.PROVIDER_NAME);
+ CertPathBuilder builder = CertPathBuilder
+ .getInstance(CertPathBuilder.getDefaultType(), BouncyCastleProvider.PROVIDER_NAME);
return (PKIXCertPathBuilderResult) builder.build(pkixParams);
}
private CertStore createCertStore(Set<X509Certificate> certificateSet) throws InvalidAlgorithmParameterException,
- NoSuchAlgorithmException, NoSuchProviderException {
- return CertStore.getInstance("Collection", new CollectionCertStoreParameters(certificateSet), BouncyCastleProvider.PROVIDER_NAME);
+ NoSuchAlgorithmException, NoSuchProviderException {
+ return CertStore.getInstance("Collection", new CollectionCertStoreParameters(certificateSet),
+ BouncyCastleProvider.PROVIDER_NAME);
}
private boolean isExpired(X509Certificate cert) {
@@ -295,8 +299,8 @@
}
private boolean isSelfSigned(Certificate cert)
- throws CertificateException, NoSuchAlgorithmException,
- NoSuchProviderException {
+ throws CertificateException, NoSuchAlgorithmException,
+ NoSuchProviderException {
try {
// Try to verify certificate signature with its own public key
PublicKey key = cert.getPublicKey();