activemq-gitbox mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3667: ARTEMIS-3367 Set verifyHost true by default
Date Tue, 03 Aug 2021 18:28:19 GMT

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



Mime
View raw message