ARTEMIS-3367 Set verifyHost true by default by brusdev · Pull Request #3667 · apache/artemis

brusdev · 2021-07-22T10:32:22Z

No description provided.

.project

jbertram · 2021-08-03T17:53:39Z

...va/org/apache/activemq/artemis/tests/integration/amqp/AmqpFailoverEndpointDiscoveryTest.java

-         String truststore = this.getClass().getClassLoader().getResource("client-side-truststore.jks").getFile();
-         return new JmsConnectionFactory("failover:(amqps://localhost:61616?transport.keyStoreLocation=" + keystore + "&transport.keyStorePassword=secureexample&transport.trustStoreLocation=" + truststore + "&transport.trustStorePassword=secureexample&transport.verifyHost=false)");
+         String keystore = this.getClass().getClassLoader().getResource("examples/features/standard/jmx-ssl/src/main/resources/activemq/server0/client-keystore.jks").getFile();
+         String truststore = this.getClass().getClassLoader().getResource("examples/features/standard/jmx-ssl/src/main/resources/activemq/server0/server-ca-truststore.jks").getFile();


Why not reference the resources in tests/security-resources like the comment at the beginning of the class suggests? Are the example resources unique in some way?

@jbertram thanks, it was due a refactoring issue of my IDE. I have just pushed a commit to fix.

gemmellr · 2021-08-03T16:36:20Z

examples/features/standard/jmx-ssl/readme.md

-* `keytool -genkey -keystore client-side-keystore.jks -storepass secureexample -keypass secureexample -dname "CN=ActiveMQ Artemis Client, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -keyalg RSA`
-* `keytool -export -keystore client-side-keystore.jks -file client-side-cert.cer -storepass secureexample`
-* `keytool -import -keystore server-side-truststore.jks -file client-side-cert.cer -storepass secureexample -keypass secureexample -noprompt`
+```shell


This is a bit verbose for an example readme, where creating these files either isnt the interesting bit and can be 'hidden away', or is the interesting bit and then having it on its own would be good to isolate it out.

I would create another file or script and reference it, indicate how to run the bits. Can be easier to run later if needing to regenerate things. E.g see https://github.com/apache/activemq-artemis/blob/2.17.0/examples/features/broker-connection/amqp-sending-overssl/readme.md

Same would apply to other examples that follow (even if they had a smaller number of such commands in the readme already, but now have more).

gemmellr · 2021-08-03T16:43:46Z

examples/features/standard/jmx-ssl/readme.md

+CA_VALIDITY=365000
+VALIDITY=36500


Not sure there need to be 900 years difference between these two validities hehe. Validity is in days, so smaller values would seem doable and closer to 'representative use'. I use 9999 when I want to mean 'so long these wont expire before they are definitely not going to be used anymore'. For any such long value the algorithms will still be disabled and make us replace them before they expire.

Using such a massive value, based around multiple of 365, kind of suggests it isnt in days but something else.

I used a multiple of 365 because it is the number of days in a year, this was supposed to make it easier to evaluate how many years is the validity of the certificates.

I did get that, but I think the value is just unnecessarily large. So much so that people who dont know (and dont spot there is a 0 difference at the end, which I didnt initially) are likely to believe its something else.

gemmellr · 2021-08-03T17:39:03Z

...java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyConnectorTest.java

+      params.put(TransportConstants.KEYSTORE_PATH_PROP_NAME, "server-keystore.jks");
+      params.put(TransportConstants.KEYSTORE_PASSWORD_PROP_NAME, "securepass");
+      params.put(TransportConstants.TRUSTSTORE_PATH_PROP_NAME,  "client-ca-truststore.jks");
+      params.put(TransportConstants.TRUSTSTORE_PASSWORD_PROP_NAME, "securepass");


I'm not the biggest fan of the naming reversal that happened here.

For the keystores, the 'client' and 'server' bits of the name still somewhat denotes where it is being used (as the old 'server side' desginator did before), and additionally what it contains. For the trust stores however, the 'client CA' and 'server CA' bits now denote only what it contains rather than where it is used (which the old 'client-side' designator did), with it then used in the 'opposite' place. This is a bit awkward, you get a bit of cognitive dissonance where it seems wrong to use the 'server keystore' and the 'client...truststore' files together on the server, and the 'client keystore' and 'server...truststore' files together on the client.

I used names for security resources that denote their content and not the purpose to avoid confusion if they were to be used in different contexts.

The problem is one of them still reads like it mainly conveys the purpose rather than the content, which makes the other one seem like it does too, leading to the combination being confusing, more so when historically the name was historically where it was being used.

I didnt particularly see need for the '-side' expansion in the old names, but together the old names were far clearer to me than the replacements. I fully expect people not copying an existing test to mess up using these, I know I likely will.

gemmellr · 2021-08-03T18:16:23Z

...test/java/org/apache/activemq/artemis/tests/integration/ssl/CoreClientOverOneWaySSLTest.java

-      SERVER_SIDE_KEYSTORE = "server-side-keystore." + suffix;
-      CLIENT_SIDE_TRUSTSTORE = "client-side-truststore." + suffix;
+      SERVER_SIDE_KEYSTORE = "server-keystore." + suffix;
+      CLIENT_SIDE_TRUSTSTORE = "server-ca-truststore." + suffix;


Related to earlier comment, you then get somewhat less obvious things like this seemingly (but not actually) contradictory constant definition.

I see, this could not be obvious but it should clarify what the test is doing:
The SERVER_SIDE_KEYSTORE points to a keystore with the server key.
The CLIENT_SIDE_TRUSTSTORE points to a truststore with the server ca certificate.

I would hope the comments showed that I actually do understand what it is doing. I'm the one who argued that the tests should use CAs. I just think the different naming chosen adds scope for confusion, and is likely to trip people up who are used to the previous (and I would say more typical) naming format where the file name pointed to where it is used.

gemmellr · 2021-08-03T18:24:59Z

tests/security-resources/build.sh

+keytool -importkeystore -srckeystore other-client-keystore.p12 -destkeystore other-client-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore other-client-keystore.p12 -destkeystore other-client-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass


Not really convinced 'other client' (EDIT and now that I look, 'other server', 'unknown-client' and 'unknown-server') really needs to have a keystore and truststore of every type. Makes sense that the main set have the various different types to verify they all work (even though that has very little to do with the broker code in the end) but having them all for the various others seems like needless complexity.

The CoreClientOverOneWaySSLTest and CoreClientOverTwoWaySSLTest tests use most of them because they are parameterized tests. ATM only 2 truststores for the other-client and 2 truststores for the unknown-client are not used but we might need them in the future if they are used in a parametric test.

I would actually change the tests then, theres basically no need to run all those combinations. If the full suite wasted a little less time where easily possible (like here), it might not take so ridiculously long to run that important changes are made without running them (as clearly just happened on another PR).

Files can be added later when they are actually needed, no need to keep around unused files. Though again, I see no reason the fully-unused ones would ever actually be needed, using them implies time wasting. However, being able to easily regenerate things when needed is part of the argument to use a script to create them (Though this case is so trivial you wouldnt even need to regenerate the rest, you could import the same keys to the new store with the command being added to the script)

brusdev added the DO-NOT-MERGE-YET label Jul 22, 2021

This comment has been minimized.

Sign in to view

brusdev marked this pull request as draft July 22, 2021 17:03

brusdev removed the DO-NOT-MERGE-YET label Jul 22, 2021

brusdev force-pushed the enable_verify_host branch from ecc343f to d77dda3 Compare July 23, 2021 08:26

brusdev marked this pull request as ready for review July 23, 2021 08:27

clebertsuconic reviewed Jul 23, 2021

View reviewed changes

.project Outdated Show resolved Hide resolved

brusdev force-pushed the enable_verify_host branch from d77dda3 to 0f9754b Compare July 23, 2021 12:53

clebertsuconic marked this pull request as draft July 23, 2021 14:30

brusdev force-pushed the enable_verify_host branch from 0f9754b to a27fe68 Compare August 3, 2021 12:47

brusdev marked this pull request as ready for review August 3, 2021 12:48

brusdev force-pushed the enable_verify_host branch 2 times, most recently from 2569d5c to 549bb8b Compare August 3, 2021 15:37

jbertram reviewed Aug 3, 2021

View reviewed changes

ARTEMIS-3367 Set verifyHost true for connectors by default

4cdec14

brusdev force-pushed the enable_verify_host branch from 549bb8b to 4cdec14 Compare August 3, 2021 18:12

gemmellr reviewed Aug 3, 2021

View reviewed changes

asfgit closed this in 2715980 Aug 3, 2021

		keytool -importkeystore -srckeystore other-client-keystore.p12 -destkeystore other-client-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
		keytool -importkeystore -srckeystore other-client-keystore.p12 -destkeystore other-client-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass

		CA_VALIDITY=365000
		VALIDITY=36500

Conversation

brusdev commented Jul 22, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants