You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "pa-deasy (via GitHub)" <gi...@apache.org> on 2024/12/05 20:16:35 UTC
[PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
pa-deasy opened a new pull request, #1360:
URL: https://github.com/apache/activemq/pull/1360
**What problems does this PR solve?**
Add test coverage to ensure socket parameters can be set for tcp and ssl connections.
**Why is it beneficial to merge into ActiveMQ?**
The tests provide confidence and prevent future issues like [AMQ-9473](https://issues.apache.org/jira/browse/AMQ-9473).
**How do you make sure this PR is well tested?**
_Test A_
Ran the two new units tests and verified success.
_Test B_
Edited the `IntrospectionSupport` class to comment out the following:
```
// if (target instanceof SSLServerSocket) {
// // overcome illegal access issues with internal implementation class
// clazz = SSLServerSocket.class;
// } else if (target instanceof javax.net.ssl.SSLSocket) {
// // overcome illegal access issues with internal implementation class
// clazz = javax.net.ssl.SSLSocket.class;
// }
```
Ran the two new units tests and verified failure due to `java.lang.IllegalArgumentException: Invalid socket parameters`.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "pa-deasy (via GitHub)" <gi...@apache.org>.
pa-deasy commented on PR #1360:
URL: https://github.com/apache/activemq/pull/1360#issuecomment-2523848783
> This is a good start but I think the tests could be improved. The tests show there is no more exception on connection creation which is good, but it would be nice to try setting more than one parameter and also if it's possible to access it would be good to verify the settings worked. For example, the one test sets the the ssl enabled protocols to TLSv1.3 so it would be best to assert that it did in fact apply that setting.
That's a good suggestion, I've added more parameters to the unit tests. I tried verifying the parameters are in effect but it's a little difficult without exposing the `protected` `Socket` on the `TcpTransport`. I tried researching a bunch of libraries that might check the TLS version on a TCP capture but it didn't really work and would also have introduced an extra dependency. Do you have any suggestions on a better approach for verifying the socket changes? A lot of the logic resides in the JVM so it's difficult to verify.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "kenliao94 (via GitHub)" <gi...@apache.org>.
kenliao94 commented on code in PR #1360:
URL: https://github.com/apache/activemq/pull/1360#discussion_r1872048367
##########
activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQSslConnectionFactoryTest.java:
##########
@@ -103,6 +117,24 @@ public void testCreateSslConnection() throws Exception {
brokerStop();
}
+ public void testCreateSslConnectionWithSocketParameters() throws Exception {
+ // Create SSL/TLS connection with trusted cert from truststore.
+ String sslUri = "ssl://localhost:61611?socket.enabledProtocols=TLSv1.3";
+ broker = createSslBroker(sslUri);
+ assertNotNull(broker);
+
+ // This should create the connection.
+ ActiveMQSslConnectionFactory cf = getFactory(sslUri);
+ cf.setTrustStore("server.keystore");
+ cf.setTrustStorePassword("password");
+ connection = (ActiveMQConnection)cf.createConnection();
+ LOG.info("Created client connection");
Review Comment:
Do we need this LOG.info?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "pa-deasy (via GitHub)" <gi...@apache.org>.
pa-deasy commented on PR #1360:
URL: https://github.com/apache/activemq/pull/1360#issuecomment-2524125127
> Yeah, I wasn't sure if it would be possible to easily verify for that reason. You could try and use reflection to get access to the private/protected socket and make it accessible. I can also take a look next week as well...if it's not really possible that's ok but it would certainly be nice if we could do it.
I managed to gain access to the `Socket` by iterating through the transports and a little reflection magic as you suggested.
It's a little verbose but it provides the ability to verify changes to the `Socket`.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #1360:
URL: https://github.com/apache/activemq/pull/1360#discussion_r1875923956
##########
activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQSslConnectionFactoryTest.java:
##########
@@ -72,6 +81,34 @@ public void testCreateTcpConnectionUsingKnownPort() throws Exception {
brokerStop();
}
+ public void testCreateTcpConnectionWithSocketParameters() throws Exception {
+ // Control case: check that the factory can create an ordinary (non-ssl) connection.
+ String tcpUri = "tcp://localhost:61610?socket.OOBInline=true&socket.keepAlive=true&tcpNoDelay=true";
+ broker = createBroker(tcpUri);
+
+ // This should create the connection.
+ ActiveMQSslConnectionFactory cf = getFactory(tcpUri);
+ connection = (ActiveMQConnection)cf.createConnection();
+ assertNotNull(connection);
+
+ Transport transport = connection.getTransport();
Review Comment:
All of this code to get the socket is copied and pasted twice in both tests so you should move it into a helper method so it can be re-used. It might even come in handy as a utility for use in other tests as well but for now just making it a helper method for this test is ok just so it's not duplicated.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1360:
URL: https://github.com/apache/activemq/pull/1360#issuecomment-2521592941
This is a good start but I think the tests could be improved. The tests show there is no more exception on connection creation which is good, but it would be nice to try setting more than one parameter and also if it's possible to access it would be good to verify the settings worked. For example, the one test sets the the ssl enabled protocols to TLSv1.3 so it would be best to assert that it did in fact apply that setting.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #1360:
URL: https://github.com/apache/activemq/pull/1360
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "pa-deasy (via GitHub)" <gi...@apache.org>.
pa-deasy commented on code in PR #1360:
URL: https://github.com/apache/activemq/pull/1360#discussion_r1872191124
##########
activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQSslConnectionFactoryTest.java:
##########
@@ -103,6 +117,24 @@ public void testCreateSslConnection() throws Exception {
brokerStop();
}
+ public void testCreateSslConnectionWithSocketParameters() throws Exception {
+ // Create SSL/TLS connection with trusted cert from truststore.
+ String sslUri = "ssl://localhost:61611?socket.enabledProtocols=TLSv1.3";
+ broker = createSslBroker(sslUri);
+ assertNotNull(broker);
+
+ // This should create the connection.
+ ActiveMQSslConnectionFactory cf = getFactory(sslUri);
+ cf.setTrustStore("server.keystore");
+ cf.setTrustStorePassword("password");
+ connection = (ActiveMQConnection)cf.createConnection();
+ LOG.info("Created client connection");
Review Comment:
We don't really. Let me remove that.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1360:
URL: https://github.com/apache/activemq/pull/1360#issuecomment-2523855943
> That's a good suggestion, I've added more parameters to the unit tests. I tried verifying the parameters are in effect but it's a little difficult without exposing the `protected` `Socket` on the `TcpTransport`. I tried researching a bunch of libraries that might check the TLS version on a TCP capture but it didn't really work and would also have introduced an extra dependency. Do you have any suggestions on a better approach for verifying the socket changes? A lot of the logic resides in the JVM so it's difficult to verify.
Yeah, I wasn't sure if it would be possible to easily verify for that reason. You could try and use reflection to get access to the private/protected socket and make it accessible. I can also take a look next week as well...if it's not really possible that's ok but it would certainly be nice if we could do it.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] AMQ-9478 Add unit tests for tcp & ssl socket properties [activemq]
Posted by "pa-deasy (via GitHub)" <gi...@apache.org>.
pa-deasy commented on code in PR #1360:
URL: https://github.com/apache/activemq/pull/1360#discussion_r1876462030
##########
activemq-unit-tests/src/test/java/org/apache/activemq/ActiveMQSslConnectionFactoryTest.java:
##########
@@ -72,6 +81,34 @@ public void testCreateTcpConnectionUsingKnownPort() throws Exception {
brokerStop();
}
+ public void testCreateTcpConnectionWithSocketParameters() throws Exception {
+ // Control case: check that the factory can create an ordinary (non-ssl) connection.
+ String tcpUri = "tcp://localhost:61610?socket.OOBInline=true&socket.keepAlive=true&tcpNoDelay=true";
+ broker = createBroker(tcpUri);
+
+ // This should create the connection.
+ ActiveMQSslConnectionFactory cf = getFactory(tcpUri);
+ connection = (ActiveMQConnection)cf.createConnection();
+ assertNotNull(connection);
+
+ Transport transport = connection.getTransport();
Review Comment:
Good suggestion. I initially thought about keeping the logic separate as one test gets a `Socket` and the other gets an `SSLSocket`. A simple check to determine whether or not to get the superclass makes the helper method fit both tests though.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact