You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/08/03 18:28:19 UTC
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3667: ARTEMIS-3367 Set verifyHost true by default
gemmellr commented on a change in pull request #3667:
URL: https://github.com/apache/activemq-artemis/pull/3667#discussion_r681925548
##########
File path: examples/features/standard/jmx-ssl/readme.md
##########
@@ -16,12 +16,54 @@ With these properties, ActiveMQ Artemis broker will be manageable remotely using
The various keystore files are generated using the following commands:
-* `keytool -genkey -keystore server-side-keystore.jks -storepass secureexample -keypass secureexample -dname "CN=ActiveMQ Artemis Server, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -keyalg RSA`
-* `keytool -export -keystore server-side-keystore.jks -file server-side-cert.cer -storepass secureexample`
-* `keytool -import -keystore client-side-truststore.jks -file server-side-cert.cer -storepass secureexample -keypass secureexample -noprompt`
-* `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
Review comment:
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).
##########
File path: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/ssl/CoreClientOverOneWaySSLTest.java
##########
@@ -82,60 +85,18 @@ public CoreClientOverOneWaySSLTest(String storeProvider, String storeType, boole
if (suffix.equalsIgnoreCase("PKCS12")) {
suffix = "p12";
}
- 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;
Review comment:
Related to earlier comment, you then get somewhat less obvious things like this seemingly (but not actually) contradictory constant definition.
##########
File path: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/remoting/impl/netty/NettyConnectorTest.java
##########
@@ -56,10 +59,10 @@ public void setUp() throws Exception {
Map<String, Object> params = new HashMap<>();
params.put(TransportConstants.SSL_ENABLED_PROP_NAME, true);
params.put(TransportConstants.SSL_PROVIDER, TransportConstants.OPENSSL_PROVIDER);
- params.put(TransportConstants.KEYSTORE_PATH_PROP_NAME, "openssl-server-side-keystore.jks");
- params.put(TransportConstants.KEYSTORE_PASSWORD_PROP_NAME, "secureexample");
- params.put(TransportConstants.TRUSTSTORE_PATH_PROP_NAME, "openssl-server-side-truststore.jks");
- params.put(TransportConstants.TRUSTSTORE_PASSWORD_PROP_NAME, "secureexample");
+ 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");
Review comment:
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.
##########
File path: examples/features/standard/jmx-ssl/readme.md
##########
@@ -16,12 +16,54 @@ With these properties, ActiveMQ Artemis broker will be manageable remotely using
The various keystore files are generated using the following commands:
-* `keytool -genkey -keystore server-side-keystore.jks -storepass secureexample -keypass secureexample -dname "CN=ActiveMQ Artemis Server, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -keyalg RSA`
-* `keytool -export -keystore server-side-keystore.jks -file server-side-cert.cer -storepass secureexample`
-* `keytool -import -keystore client-side-truststore.jks -file server-side-cert.cer -storepass secureexample -keypass secureexample -noprompt`
-* `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
+#!/bin/bash
+set -e
+
+KEY_PASS=securepass
+STORE_PASS=securepass
+CA_VALIDITY=365000
+VALIDITY=36500
Review comment:
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.
##########
File path: tests/security-resources/build.sh
##########
@@ -0,0 +1,156 @@
+#!/bin/bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# The various SSL stores and certificates were created with the following commands:
+# Requires use of JDK 8+ keytool command.
+set -e
+
+KEY_PASS=securepass
+STORE_PASS=securepass
+CA_VALIDITY=365000
+VALIDITY=36500
+
+# Clean up existing files
+# -----------------------
+rm -f *.crt *.csr openssl-* *.jceks *.jks *.p12 *.pem
+
+# Create a key and self-signed certificate for the CA, to sign server certificate requests and use for trust:
+# ----------------------------------------------------------------------------------------------------
+keytool -storetype pkcs12 -keystore server-ca-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias server-ca -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Server Certification Authority, OU=Artemis, O=ActiveMQ" -validity $CA_VALIDITY -ext bc:c=ca:true
+keytool -storetype pkcs12 -keystore server-ca-keystore.p12 -storepass $STORE_PASS -alias server-ca -exportcert -rfc > server-ca.crt
+openssl pkcs12 -in server-ca-keystore.p12 -nodes -nocerts -out server-ca.pem -password pass:$STORE_PASS
+
+# Create trust store with the server CA cert:
+# -------------------------------------------------------
+keytool -storetype pkcs12 -keystore server-ca-truststore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias server-ca -file server-ca.crt -noprompt
+keytool -importkeystore -srckeystore server-ca-truststore.p12 -destkeystore server-ca-truststore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore server-ca-truststore.p12 -destkeystore server-ca-truststore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create a key pair for the server, and sign it with the CA:
+# ----------------------------------------------------------
+keytool -storetype pkcs12 -keystore server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias server -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Server, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -validity $VALIDITY -ext bc=ca:false -ext eku=sA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore server-keystore.p12 -storepass $STORE_PASS -alias server -certreq -file server.csr
+keytool -storetype pkcs12 -keystore server-ca-keystore.p12 -storepass $STORE_PASS -alias server-ca -gencert -rfc -infile server.csr -outfile server.crt -validity $VALIDITY -ext bc=ca:false -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias server-ca -file server-ca.crt -noprompt
+keytool -storetype pkcs12 -keystore server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias server -file server.crt
+
+keytool -importkeystore -srckeystore server-keystore.p12 -destkeystore server-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore server-keystore.p12 -destkeystore server-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create a key pair for the other server, and sign it with the CA:
+# ----------------------------------------------------------
+keytool -storetype pkcs12 -keystore other-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias other-server -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Other Server, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -validity $VALIDITY -ext bc=ca:false -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore other-server-keystore.p12 -storepass $STORE_PASS -alias other-server -certreq -file other-server.csr
+keytool -storetype pkcs12 -keystore server-ca-keystore.p12 -storepass $STORE_PASS -alias server-ca -gencert -rfc -infile other-server.csr -outfile other-server.crt -validity $VALIDITY -ext bc=ca:false -ext eku=sA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore other-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias server-ca -file server-ca.crt -noprompt
+keytool -storetype pkcs12 -keystore other-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias other-server -file other-server.crt
+
+keytool -importkeystore -srckeystore other-server-keystore.p12 -destkeystore other-server-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore other-server-keystore.p12 -destkeystore other-server-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create trust store with the other server cert:
+# -------------------------------------------------------
+keytool -storetype pkcs12 -keystore other-server-truststore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias other-server -file other-server.crt -noprompt
+keytool -importkeystore -srckeystore other-server-truststore.p12 -destkeystore other-server-truststore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore other-server-truststore.p12 -destkeystore other-server-truststore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create crl with the other server cert:
+# -------------------------------------------------------
+> openssl-database
+echo 00 > openssl-crlnumber
+openssl ca -config openssl.conf -revoke other-server.crt -keyfile server-ca.pem -cert server-ca.crt
+openssl ca -config openssl.conf -gencrl -keyfile server-ca.pem -cert server-ca.crt -out other-server-crl.pem -crldays $VALIDITY
+
+# Create a key pair for the broker with an unexpected hostname, and sign it with the CA:
+# --------------------------------------------------------------------------------------
+keytool -storetype pkcs12 -keystore unknown-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias unknown-server -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Unknown Server, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -validity $VALIDITY -ext bc=ca:false -ext eku=sA
+
+keytool -storetype pkcs12 -keystore unknown-server-keystore.p12 -storepass $STORE_PASS -alias unknown-server -certreq -file unknown-server.csr
+keytool -storetype pkcs12 -keystore server-ca-keystore.p12 -storepass $STORE_PASS -alias server-ca -gencert -rfc -infile unknown-server.csr -outfile unknown-server.crt -validity $VALIDITY -ext bc=ca:false -ext eku=sA
+
+keytool -storetype pkcs12 -keystore unknown-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias server-ca -file server-ca.crt -noprompt
+keytool -storetype pkcs12 -keystore unknown-server-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias unknown-server -file unknown-server.crt
+
+keytool -importkeystore -srckeystore unknown-server-keystore.p12 -destkeystore unknown-server-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore unknown-server-keystore.p12 -destkeystore unknown-server-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create a key and self-signed certificate for the CA, to sign client certificate requests and use for trust:
+# ----------------------------------------------------------------------------------------------------
+keytool -storetype pkcs12 -keystore client-ca-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias client-ca -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Client Certification Authority, OU=Artemis, O=ActiveMQ" -validity $CA_VALIDITY -ext bc:c=ca:true
+keytool -storetype pkcs12 -keystore client-ca-keystore.p12 -storepass $STORE_PASS -alias client-ca -exportcert -rfc > client-ca.crt
+openssl pkcs12 -in client-ca-keystore.p12 -nodes -nocerts -out client-ca.pem -password pass:$STORE_PASS
+
+# Create trust store with the client CA cert:
+# -------------------------------------------------------
+keytool -storetype pkcs12 -keystore client-ca-truststore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias client-ca -file client-ca.crt -noprompt
+keytool -importkeystore -srckeystore client-ca-truststore.p12 -destkeystore client-ca-truststore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore client-ca-truststore.p12 -destkeystore client-ca-truststore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create a key pair for the client, and sign it with the CA:
+# ----------------------------------------------------------
+keytool -storetype pkcs12 -keystore client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias client -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Client, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -validity $VALIDITY -ext bc=ca:false -ext eku=cA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore client-keystore.p12 -storepass $STORE_PASS -alias client -certreq -file client.csr
+keytool -storetype pkcs12 -keystore client-ca-keystore.p12 -storepass $STORE_PASS -alias client-ca -gencert -rfc -infile client.csr -outfile client.crt -validity $VALIDITY -ext bc=ca:false -ext eku=cA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias client-ca -file client-ca.crt -noprompt
+keytool -storetype pkcs12 -keystore client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias client -file client.crt
+
+keytool -importkeystore -srckeystore client-keystore.p12 -destkeystore client-keystore.jceks -srcstoretype pkcs12 -deststoretype jceks -srcstorepass securepass -deststorepass securepass
+keytool -importkeystore -srckeystore client-keystore.p12 -destkeystore client-keystore.jks -srcstoretype pkcs12 -deststoretype jks -srcstorepass securepass -deststorepass securepass
+
+# Create a key pair for the other client, and sign it with the CA:
+# ----------------------------------------------------------
+keytool -storetype pkcs12 -keystore other-client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -alias other-client -genkey -keyalg "RSA" -keysize 2048 -dname "CN=ActiveMQ Artemis Other Client, OU=Artemis, O=ActiveMQ, L=AMQ, S=AMQ, C=AMQ" -validity $VALIDITY -ext bc=ca:false -ext eku=cA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore other-client-keystore.p12 -storepass $STORE_PASS -alias other-client -certreq -file other-client.csr
+keytool -storetype pkcs12 -keystore client-ca-keystore.p12 -storepass $STORE_PASS -alias client-ca -gencert -rfc -infile other-client.csr -outfile other-client.crt -validity $VALIDITY -ext bc=ca:false -ext eku=cA -ext san=dns:localhost,ip:127.0.0.1
+
+keytool -storetype pkcs12 -keystore other-client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias client-ca -file client-ca.crt -noprompt
+keytool -storetype pkcs12 -keystore other-client-keystore.p12 -storepass $STORE_PASS -keypass $KEY_PASS -importcert -alias other-client -file other-client.crt
+
+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
Review comment:
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.
--
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