You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "brusdev (via GitHub)" <gi...@apache.org> on 2023/04/19 17:10:10 UTC
[GitHub] [activemq-artemis] brusdev opened a new pull request, #4441: ARTEMIS-4245 Expose web SNI settings
brusdev opened a new pull request, #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441
This PR depends on #4440
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1178936369
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -283,6 +298,40 @@ public void simpleSecureServer() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckEnabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(true);
+ }
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckDisabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(false);
+ }
Review Comment:
Do we want to test the default behaviour is 'enabled' (i.e change the boolean to Boolean and pass null as well)?
Currently nothing looks to be checking the default, since these 2 tests only verify it succeeds/fails using the IP address when the setting was _explicitly_ disabled or enabled.
##########
artemis-dto/src/main/java/org/apache/activemq/artemis/dto/BindingDTO.java:
##########
@@ -181,6 +187,22 @@ public void addApp(AppDTO app) {
apps.add(app);
}
+ public Boolean getSniHostCheck() {
+ return sniHostCheck;
+ }
+
+ public void setSniHostCheck(Boolean sniHostCheck) {
+ this.sniHostCheck = sniHostCheck;
+ }
+
+ public Boolean getSniRequired() {
+ return sniRequired;
+ }
+
+ public void setSniRequired(Boolean sniRequired) {
+ this.sniRequired = sniRequired;
+ }
+
Review Comment:
org.apache.activemq.artemis.dto.test.BindingDTOTest.testDefault() should be updated to check these are null by default since the new default behaviour depends on it.
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -215,11 +221,11 @@ public void testComponentStopStartBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
- @Test
- public void simpleSecureServer() throws Exception {
+ private WebServerComponent startSimpleSecureServer(String keyStorePath, String keyStorePassword, Boolean sniHostCheck) throws Exception {
BindingDTO bindingDTO = new BindingDTO();
bindingDTO.uri = "https://localhost:0";
bindingDTO.keyStorePath = "./src/test/resources/server.keystore";
+ bindingDTO.setSniHostCheck(sniHostCheck);
Review Comment:
might be nice to only set it if non-null?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1182763373
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/web/WebServerDTOConfigTest.java:
##########
@@ -97,6 +97,8 @@ private void testSetWebBindingProperties(WebServerDTO webServer, String bindingN
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".excludedCipherSuites", "test-excludedCipherSuites,3");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".keyStorePassword", "test-keyStorePassword");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".trustStorePassword", "test-trustStorePassword");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniHostCheck", "true");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniRequired", "true");
Review Comment:
oops, for some reason I thought it was asserting the final value on WebServerComponent, not the DTO.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1179543812
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -283,6 +298,40 @@ public void simpleSecureServer() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckEnabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(true);
+ }
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckDisabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(false);
+ }
Review Comment:
We could do that instead yes.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1182438994
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -245,7 +245,10 @@ private ServerConnector createServerConnector(HttpConfiguration httpConfiguratio
SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ SecureRequestCustomizer secureRequestCustomizer = new SecureRequestCustomizer();
+ secureRequestCustomizer.setSniHostCheck(binding.getSniHostCheck() != null ? binding.getSniHostCheck() : true);
+ secureRequestCustomizer.setSniRequired(binding.getSniRequired() != null ? binding.getSniRequired() : false);
Review Comment:
Perhaps constants for the defaults?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/web/WebServerDTOConfigTest.java:
##########
@@ -97,6 +97,8 @@ private void testSetWebBindingProperties(WebServerDTO webServer, String bindingN
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".excludedCipherSuites", "test-excludedCipherSuites,3");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".keyStorePassword", "test-keyStorePassword");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".trustStorePassword", "test-trustStorePassword");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniHostCheck", "true");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniRequired", "true");
Review Comment:
These are both being set true...meaning one is being set to its already-expected default and the other isnt. Perhaps have both set the non-default value?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr merged pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr merged PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1179539330
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -215,11 +221,11 @@ public void testComponentStopStartBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
- @Test
- public void simpleSecureServer() throws Exception {
+ private WebServerComponent startSimpleSecureServer(String keyStorePath, String keyStorePassword, Boolean sniHostCheck) throws Exception {
BindingDTO bindingDTO = new BindingDTO();
bindingDTO.uri = "https://localhost:0";
bindingDTO.keyStorePath = "./src/test/resources/server.keystore";
+ bindingDTO.setSniHostCheck(sniHostCheck);
Review Comment:
Sure, but the comment was just about 'getting the default behaviour' rather than explicitly setting the null (whichone testdoes), and instead relying on the bonafiide default behaviour so it could be verified to match what we expect.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1179376675
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -215,11 +221,11 @@ public void testComponentStopStartBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
- @Test
- public void simpleSecureServer() throws Exception {
+ private WebServerComponent startSimpleSecureServer(String keyStorePath, String keyStorePassword, Boolean sniHostCheck) throws Exception {
BindingDTO bindingDTO = new BindingDTO();
bindingDTO.uri = "https://localhost:0";
bindingDTO.keyStorePath = "./src/test/resources/server.keystore";
+ bindingDTO.setSniHostCheck(sniHostCheck);
Review Comment:
If the test doesn't set bindingDTO.sniHostCheck the WebServerComponent behaviour depends on org.eclipse.jetty.server.SecureRequestCustomizer._sniHostCheck default value and it already changed from 9.x to 10.x
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -283,6 +298,40 @@ public void simpleSecureServer() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
}
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckEnabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(true);
+ }
+
+ @Test
+ public void testSimpleSecureServerWithSniHostCheckDisabled() throws Exception {
+ testSimpleSecureServerWithSniHostCheck(false);
+ }
Review Comment:
Maybe we should enable sniHostCheck by default because org.eclipse.jetty.server.SecureRequestCustomizer._sniHostCheck default value already changed from 9.x to 10.x
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on a diff in pull request #4441: ARTEMIS-4245 Expose web SNI settings
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on code in PR #4441:
URL: https://github.com/apache/activemq-artemis/pull/4441#discussion_r1182573038
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/web/WebServerDTOConfigTest.java:
##########
@@ -97,6 +97,8 @@ private void testSetWebBindingProperties(WebServerDTO webServer, String bindingN
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".excludedCipherSuites", "test-excludedCipherSuites,3");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".keyStorePassword", "test-keyStorePassword");
properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".trustStorePassword", "test-trustStorePassword");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniHostCheck", "true");
+ properties.put(ActiveMQDefaultConfiguration.getDefaultSystemWebPropertyPrefix() + "bindings." + bindingName + ".sniRequired", "true");
Review Comment:
BindingDTO default values for `sniHostCheck` and `sniRequired` are `null` but using WebServerComponent non-default value should to avoid confusion.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org