[GitHub] [shiro] bmhm commented on a change in pull request #259: Update AbstractContainerIT to allow for HTTPS connections Using a pre-generated keystore

[GitBox <gi...@apache.org>]

bmhm commented on a change in pull request #259:
URL: https://github.com/apache/shiro/pull/259#discussion_r505666891



##########
File path: integration-tests/support/src/main/java/org/apache/shiro/testing/web/AbstractContainerIT.java
##########
@@ -150,4 +193,30 @@ public static void stopContainer() {
             jetty.stop();
         }
     }
+
+    private static int getFreePort() {
+        try (ServerSocket socket = new ServerSocket(0)) {
+            return socket.getLocalPort();
+        } catch (IOException e) {
+            throw new IllegalStateException("Failed to allocate free port", e);
+        }
+    }
+
+    // Dealing with a keystore is NOT fun, it's easier to script one with the keytool
+    // see src/main/resources/createKeyStore.sh for more info
+    private static File setupKeyStore() {
+        try {
+            Path outKeyStoreFile = File.createTempFile("test-keystore", "jks").toPath();
+            URL keyStoreResource = Thread.currentThread().getContextClassLoader().getResource("test-keystore.jks");
+            Files.copy(keyStoreResource.openStream(), outKeyStoreFile, StandardCopyOption.REPLACE_EXISTING);
+            File keyStoreFile = outKeyStoreFile.toFile();
+
+            // _most_ clients will pick up the ssl keystore this way, so just set SSL properties

Review comment:
       I think you can remove the “most". It doesn't add valuable information of you don't specify which don't.
   All JAX-RS implementations and all Apache httpClients will pick them up.

##########
File path: samples/web/src/test/java/org/apache/shiro/test/WebAppContainerIntegrationIT.java
##########
@@ -31,14 +31,14 @@
 import java.io.IOException;
 import java.net.MalformedURLException;
 
-public class ContainerIntegrationIT extends AbstractContainerIT {
+public class WebAppContainerIntegrationIT extends AbstractContainerIT {
 
     protected final WebClient webClient = new WebClient();

Review comment:
       You could disable certificate validation here.

##########
File path: integration-tests/support/src/main/resources/createKeyStore.sh
##########
@@ -0,0 +1,65 @@
+#!/usr/bin/env bash

Review comment:
       This makes it incompatible to test on Windows?
   
   As we are not replying on secure conventions (by transport means) here, you could just disable certificate validation in the client and not generate a certificate via script.

##########
File path: integration-tests/support/src/main/java/org/apache/shiro/testing/web/AbstractContainerIT.java
##########
@@ -109,6 +128,26 @@ protected WebAppContext createdWebAppContext() throws Exception {
             }
         };
 
+        Server server = jetty.getDelegate();
+
+        // TLS
+        tlsPort = getFreePort();
+
+        final SslContextFactory sslContextFactory = new SslContextFactory.Server();
+        sslContextFactory.setKeyStorePath(TEST_KEYSTORE_PATH.getAbsolutePath());
+        sslContextFactory.setKeyStorePassword(TEST_KEYSTORE_PASSWORD);
+        sslContextFactory.setKeyManagerPassword(TEST_KEYSTORE_PASSWORD);
+
+        HttpConfiguration https = new HttpConfiguration();
+        https.addCustomizer(new SecureRequestCustomizer());
+
+        final ServerConnector httpsConnector = new ServerConnector(
+                server,
+                new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()),

Review comment:
       Any reason why we would explicitly don't want http/2 (e.g. for java 9+ builds)? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org