From f81254583534b43760242d0683ac2767f1b20194 Mon Sep 17 00:00:00 2001 From: Janelle Law Date: Tue, 22 Jun 2021 17:31:09 -0400 Subject: [PATCH] Remove empty ssl file upload (#518) * Delete empty file when handling exception * Add unit test, apply formatting * Handle case when file.delete() fails * Prevent empty file creation * Close outstream when finished writing * Refactor file write, verify file not created * Pass outputStreamFunction to constructor * Apply spotless formatting * Add integration test * Move empty cert to test resources * Rename test * Remove unneeded assertion --- .../http/api/v2/CertificatePostHandler.java | 22 +++-- .../api/v2/CertificatePostHandlerTest.java | 10 ++- src/test/java/itest/UploadCertificateIT.java | 88 +++++++++++++++++++ src/test/resources/empty.cer | 0 4 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 src/test/java/itest/UploadCertificateIT.java create mode 100644 src/test/resources/empty.cer diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/CertificatePostHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/CertificatePostHandler.java index 7db6db4eae..9e70dfefe5 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v2/CertificatePostHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v2/CertificatePostHandler.java @@ -45,7 +45,6 @@ import java.security.cert.Certificate; import java.security.cert.CertificateEncodingException; import java.util.Collection; -import java.util.Iterator; import java.util.function.Function; import javax.inject.Inject; @@ -151,16 +150,21 @@ public IntermediateResponse handle(RequestParameters params) throws ApiExc throw new ApiException(409, filePath.toString() + " Certificate already exists"); } - try (InputStream fis = fs.newInputStream(certPath); - FileOutputStream out = outputStreamFunction.apply(filePath.toFile())) { - + try (InputStream fis = fs.newInputStream(certPath)) { Collection certificates = certValidator.parseCertificates(fis); - Iterator it = certificates.iterator(); - while (it.hasNext()) { - Certificate certificate = (Certificate) it.next(); - byte[] buf = certificate.getEncoded(); - out.write(buf); + + if (certificates.isEmpty()) { + throw new ApiException(500, "No certificates found"); + } + + try (FileOutputStream out = outputStreamFunction.apply(filePath.toFile())) { + + for (Certificate certificate : certificates) { + byte[] buf = certificate.getEncoded(); + out.write(buf); + } } + } catch (IOException ioe) { throw new ApiException(500, ioe.getMessage(), ioe); } catch (CertificateEncodingException cee) { diff --git a/src/test/java/io/cryostat/net/web/http/api/v2/CertificatePostHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v2/CertificatePostHandlerTest.java index 1d98e96f74..e3a0188507 100644 --- a/src/test/java/io/cryostat/net/web/http/api/v2/CertificatePostHandlerTest.java +++ b/src/test/java/io/cryostat/net/web/http/api/v2/CertificatePostHandlerTest.java @@ -38,6 +38,7 @@ package io.cryostat.net.web.http.api.v2; import java.io.ByteArrayInputStream; +import java.io.File; import java.io.FileOutputStream; import java.io.InputStream; import java.nio.file.Path; @@ -48,6 +49,7 @@ import java.util.Iterator; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.function.Function; import io.cryostat.MainModule; import io.cryostat.core.log.Logger; @@ -92,6 +94,7 @@ class CertificatePostHandlerTest { @Mock RoutingContext ctx; @Mock FileOutputStream outStream; + @Mock Function outputStreamFunction; @Mock FileUpload fu; @Mock Path truststorePath; @Mock Path fileUploadPath; @@ -103,7 +106,8 @@ class CertificatePostHandlerTest { @BeforeEach void setup() { this.handler = - new CertificatePostHandler(auth, env, fs, gson, (file) -> outStream, certValidator); + new CertificatePostHandler( + auth, env, fs, gson, outputStreamFunction, certValidator); HttpServerRequest req = Mockito.mock(HttpServerRequest.class); MultiMap headers = MultiMap.caseInsensitiveMultiMap(); @@ -182,6 +186,8 @@ void shouldThrowExceptionIfCertIsMalformed() throws Exception { ApiException ex = Assertions.assertThrows(ApiException.class, () -> handler.handle(ctx)); MatcherAssert.assertThat(ex.getFailureReason(), Matchers.equalTo("parsing error")); + Mockito.verify(outputStreamFunction, Mockito.never()).apply(Mockito.any()); + Mockito.verify(outStream, Mockito.never()).write(Mockito.any()); } @Test @@ -200,6 +206,8 @@ void shouldAddCertToTruststore() throws Exception { InputStream instream = new ByteArrayInputStream("certificate".getBytes()); Mockito.when(fs.newInputStream(fileUploadPath)).thenReturn(instream); + + Mockito.when(outputStreamFunction.apply(Mockito.any())).thenReturn(outStream); Mockito.when(certValidator.parseCertificates(Mockito.any())).thenReturn(certificates); Mockito.when(certificates.iterator()).thenReturn(iterator); Mockito.when(iterator.hasNext()).thenReturn(true).thenReturn(false); diff --git a/src/test/java/itest/UploadCertificateIT.java b/src/test/java/itest/UploadCertificateIT.java new file mode 100644 index 0000000000..df3d09c8a0 --- /dev/null +++ b/src/test/java/itest/UploadCertificateIT.java @@ -0,0 +1,88 @@ +/* + * Copyright The Cryostat Authors + * + * The Universal Permissive License (UPL), Version 1.0 + * + * Subject to the condition set forth below, permission is hereby granted to any + * person obtaining a copy of this software, associated documentation and/or data + * (collectively the "Software"), free of charge and under any and all copyright + * rights in the Software, and any and all patent rights owned or freely + * licensable by each licensor hereunder covering either (i) the unmodified + * Software as contributed to or provided by such licensor, or (ii) the Larger + * Works (as defined below), to deal in both + * + * (a) the Software, and + * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if + * one is included with the Software (each a "Larger Work" to which the Software + * is contributed by such licensors), + * + * without restriction, including without limitation the rights to copy, create + * derivative works of, display, perform, and distribute the Software and make, + * use, sell, offer for sale, import, export, have made, and have sold the + * Software and the Larger Work(s), and to sublicense the foregoing rights on + * either these or other terms. + * + * This license is subject to the following condition: + * The above copyright notice and either this complete permission notice or at + * a minimum a reference to the UPL must be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package itest; + +import java.io.File; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; + +import io.vertx.core.buffer.Buffer; +import io.vertx.ext.web.client.HttpResponse; +import io.vertx.ext.web.multipart.MultipartForm; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +public class UploadCertificateIT extends TestBase { + + static final String CERT_NAME = "cert"; + static final String FILE_NAME = "empty.cer"; + static final String TRUSTSTORE_CERT = "truststore/" + FILE_NAME; + static final String MEDIA_TYPE = "application/pkix-cert"; + + @Test + public void shouldNotAddEmptyCertToTrustStore() throws Exception { + + CompletableFuture uploadRespFuture = new CompletableFuture<>(); + ClassLoader classLoader = getClass().getClassLoader(); + File emptyCert = new File(classLoader.getResource(FILE_NAME).getFile()); + String path = emptyCert.getAbsolutePath(); + + MultipartForm form = + MultipartForm.create() + .attribute("name", CERT_NAME) + .binaryFileUpload(CERT_NAME, FILE_NAME, path, MEDIA_TYPE); + + webClient + .post(String.format("/api/v2/certificates")) + .sendMultipartForm( + form, + ar -> { + if (ar.failed()) { + uploadRespFuture.completeExceptionally(ar.cause()); + return; + } + HttpResponse result = ar.result(); + uploadRespFuture.complete(result.statusCode()); + }); + + int statusCode = uploadRespFuture.get(REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS); + + MatcherAssert.assertThat(statusCode, Matchers.equalTo(500)); + } +} diff --git a/src/test/resources/empty.cer b/src/test/resources/empty.cer new file mode 100644 index 0000000000..e69de29bb2