Apply suggestions from code review

logstash-plugins · Nov 28, 2023 · 4820a5a · 4820a5a
1 parent 967734c
commit 4820a5a
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 6 deletions.
diff --git a/spec/inputs/http_spec.rb b/spec/inputs/http_spec.rb
@@ -798,12 +798,20 @@ def setup_server_client(url = self.url)
           end
 
           context "with ssl_truststore_path set" do
-            let(:config) { super().merge("ssl_truststore_path" => [certificate_path( 'server_from_root.p12')], "ssl_truststore_password" => "12345678") }
+            let(:config) { super().merge("ssl_truststore_path" => certificate_path('truststore.jks'), "ssl_truststore_password" => "12345678") }
 
             it "raise a configuration error" do
               expect {subject.register}.to raise_error(LogStash::ConfigurationError, "The configuration of `ssl_truststore_path` requires setting `ssl_client_authentication` to `optional` or 'required'")
             end
           end
+
+          context "with ssl_truststore_path set with no trusted certificate" do
+            let(:config) { super().merge("ssl_truststore_path" => certificate_path('server_from_root.p12'), "ssl_truststore_password" => "12345678") }
+
+            it "doesn't raise a configuration error" do
+              expect {subject.register}.not_to raise_error
+            end
+          end
         end
 
         context "configured to 'required'" do
@@ -821,13 +829,22 @@ def setup_server_client(url = self.url)
             end
           end
 
-          context "with ssl_truststore_path set" do
-            let(:config) { super().merge("ssl_truststore_path" => [certificate_path( 'server_from_root.p12')], "ssl_truststore_password" => "12345678") }
+          context "with ssl_truststore_path set to a valid truststore" do
+            let(:config) { super().merge("ssl_truststore_path" => certificate_path('truststore.jks'), "ssl_truststore_password" => "12345678") }
 
             it "doesn't raise a configuration error" do
               expect {subject.register}.not_to raise_error
             end
           end
+
+          context "with ssl_truststore_path set with no trusted certificate" do
+            let(:truststore_path) { certificate_path('server_from_root.p12') }
+            let(:config) { super().merge("ssl_truststore_path" => truststore_path, "ssl_truststore_password" => "12345678") }
+
+            it "raise a configuration error" do
+              expect {subject.register}.to raise_error(LogStash::ConfigurationError, "The provided Trust Store file does not contains any trusted certificate entry: #{truststore_path}")
+            end
+          end
         end
 
         context "configured to 'optional'" do
@@ -846,7 +863,15 @@ def setup_server_client(url = self.url)
           end
 
           context "with ssl_truststore_path set" do
-            let(:config) { super().merge("ssl_truststore_path" => [certificate_path( 'server_from_root.p12')], "ssl_truststore_password" => "12345678") }
+            let(:config) { super().merge("ssl_truststore_path" => certificate_path('truststore.jks'), "ssl_truststore_password" => "12345678") }
+
+            it "doesn't raise a configuration error" do
+              expect {subject.register}.not_to raise_error
+            end
+          end
+
+          context "with ssl_truststore_path set with no trusted certificate" do
+            let(:config) { super().merge("ssl_truststore_path" => certificate_path('server_from_root.p12'), "ssl_truststore_password" => "12345678") }
 
             it "doesn't raise a configuration error" do
               expect {subject.register}.not_to raise_error

diff --git a/src/main/java/org/logstash/plugins/inputs/http/util/SslSimpleBuilder.java b/src/main/java/org/logstash/plugins/inputs/http/util/SslSimpleBuilder.java
@@ -188,8 +188,8 @@ public SslSimpleBuilder setTrustStore(String trustStoreType, String trustStoreFi
                 formatJksPassword(trustStorePassword)
         );
 
-        if (!hasTrustStoreEntry(this.trustStore)) {
-            logger.warn("The provided Trust Store file does not contains any trusted certificate entry: {}. Please confirm this is the correct certificate and the password is correct", trustStoreFile);
+        if (!hasTrustStoreEntry(this.trustStore) && isClientAuthenticationRequired()) {
+            throw new IllegalArgumentException(String.format("The provided Trust Store file does not contains any trusted certificate entry: %s", trustStoreFile));
         }
 
         return this;

diff --git a/src/test/java/org/logstash/plugins/inputs/http/util/SslSimpleBuilderTest.java b/src/test/java/org/logstash/plugins/inputs/http/util/SslSimpleBuilderTest.java
@@ -211,6 +211,17 @@ void testSetTrustStoreWithNullTrustStoreType() throws Exception {
         assertEquals(TRUSTSTORE_TYPE, sslSimpleBuilder.getTrustStore().getType());
     }
 
+    @Test
+    void testSetTrustStoreWithNoTrustedCertificate() {
+        assertThrows(
+                IllegalArgumentException.class,
+                () -> createPemSslSimpleBuilder()
+                        .setClientAuthentication(SslClientVerifyMode.REQUIRED)
+                        .setTrustStore(KEYSTORE_TYPE, KEYSTORE, KEYSTORE_PASSWORD),
+                String.format("The provided Trust Store file does not contains any trusted certificate entry: %s", KEYSTORE)
+        );
+    }
+
     @Test
     void testDefaultVerifyModeIsNone() {
         final SslSimpleBuilder sslSimpleBuilder = createPemSslSimpleBuilder();