gemmellr commented on code in PR #5908:
URL: https://github.com/apache/activemq-artemis/pull/5908#discussion_r2340978783
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ConnectionView.java:
##########
@@ -65,7 +66,9 @@ public JsonObjectBuilder toJson(RemotingConnection
connection) {
.add(ConnectionField.PROTOCOL.getName(),
toString(connection.getProtocolName()))
.add(ConnectionField.CLIENT_ID.getName(),
toString(connection.getClientID()))
.add(ConnectionField.LOCAL_ADDRESS.getName(),
toString(connection.getTransportLocalAddress()))
- .add(ConnectionField.SESSION_COUNT.getName(), sessions.size());
+ .add(ConnectionField.SESSION_COUNT.getName(), sessions.size())
+ .add(ConnectionField.PROXY_ADDRESS.getName(),
toString(NettyServerConnection.getProxyAddress(connection.getTransportConnection())))
+ .add(ConnectionField.PROXY_VERSION.getName(),
toString(NettyServerConnection.getProxyVersion(connection.getTransportConnection())));
Review Comment:
Related to earlier comment, maybe PROXY_PROTOCOL[_VERSION] ?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/view/ConnectionField.java:
##########
@@ -29,7 +29,9 @@ public enum ConnectionField {
LOCAL_ADDRESS("localAddress"),
SESSION_ID("sessionID"),
CREATION_TIME("creationTime"),
- IMPLEMENTATION("implementation");
+ IMPLEMENTATION("implementation"),
+ PROXY_ADDRESS("proxyAddress"),
+ PROXY_VERSION("proxyVersion");
Review Comment:
Maybe _proxyProtocolVersion_ would be clearer? Or even just _proxyProtocol_
to minimise overhead (though if really caring about that, I guess we would also
consider omitting it entirely)
##########
docs/user-manual/proxy-protocol.adoc:
##########
@@ -0,0 +1,59 @@
+= PROXY Protocol
+:idprefix:
+:idseparator: -
+:docinfo: shared
+
+As noted in the official
https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt[PROXY
Protocol documentation]:
Review Comment:
I'd probably go with a specific version link so it doesnt go stale, and e.g
exact links to the quoted text could be used and highlight the referenced bits?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/HAProxyMessageHandler.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.
+ */
+package org.apache.activemq.artemis.core.remoting.impl.netty;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.haproxy.HAProxyMessage;
+
+import static
org.apache.activemq.artemis.utils.ProxyProtocolUtil.PROXY_PROTOCOL_DESTINATION_ADDRESS;
+import static
org.apache.activemq.artemis.utils.ProxyProtocolUtil.PROXY_PROTOCOL_SOURCE_ADDRESS;
+import static
org.apache.activemq.artemis.utils.ProxyProtocolUtil.PROXY_PROTOCOL_VERSION;
+
+public class HAProxyMessageHandler extends ChannelInboundHandlerAdapter {
+
+ @Override
+ public void channelRead(ChannelHandlerContext ctx, Object msg) throws
Exception {
+ if (msg instanceof HAProxyMessage haProxyMessage) {
+
ctx.channel().attr(PROXY_PROTOCOL_SOURCE_ADDRESS).set(haProxyMessage.sourceAddress()
+ ":" + Integer.toString(haProxyMessage.sourcePort()));
+
ctx.channel().attr(PROXY_PROTOCOL_DESTINATION_ADDRESS).set(haProxyMessage.destinationAddress()
+ ":" + Integer.toString(haProxyMessage.destinationPort()));
+
ctx.channel().attr(PROXY_PROTOCOL_VERSION).set(haProxyMessage.protocolVersion().toString());
+ } else {
+ // once we get the HAProxyMessage and set the proper attributes our
job is done
+ ctx.pipeline().remove(this);
Review Comment:
I share Tim's view that this would ideally do a better job of validating the
state expectations itself, rather than assuming what happened elsewhere. I'd
have kept the prior record keeping over whether the HAProxyMessage object has
been seen. If it has to allow two executions, with the message, and then the
buffer, so be it..but that seems like more reason for it to ensure it has seen
the message and throw when unexpected things happen.
##########
tests/e2e-tests/src/test/java/org/apache/activemq/artemis/tests/e2e/proxy_protocol/HAProxyTest.java:
##########
@@ -0,0 +1,334 @@
+/*
+ * 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.
+ */
+package org.apache.activemq.artemis.tests.e2e.proxy_protocol;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.DeliveryMode;
+import javax.jms.JMSException;
+import javax.jms.Message;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Queue;
+import javax.jms.QueueConnection;
+import javax.jms.QueueConnectionFactory;
+import javax.jms.QueueRequestor;
+import javax.jms.QueueSession;
+import javax.jms.Session;
+import java.lang.invoke.MethodHandles;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.activemq.ActiveMQSslConnectionFactory;
+import
org.apache.activemq.artemis.api.core.ActiveMQConnectionTimedOutException;
+import org.apache.activemq.artemis.api.core.JsonUtil;
+import org.apache.activemq.artemis.api.core.management.ResourceNames;
+import org.apache.activemq.artemis.api.jms.ActiveMQJMSClient;
+import org.apache.activemq.artemis.api.jms.management.JMSManagementHelper;
+import
org.apache.activemq.artemis.core.management.impl.view.predicate.ActiveMQFilterPredicate;
+import org.apache.activemq.artemis.jms.client.ActiveMQQueueConnectionFactory;
+import org.apache.activemq.artemis.json.JsonArray;
+import org.apache.activemq.artemis.json.JsonObject;
+import org.apache.activemq.artemis.tests.e2e.common.ContainerService;
+import org.apache.activemq.artemis.tests.e2e.common.E2ETestBase;
+import org.apache.activemq.artemis.tests.e2e.common.ValidateContainer;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.apache.activemq.artemis.utils.Wait;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+import org.eclipse.paho.mqttv5.client.IMqttToken;
+import org.eclipse.paho.mqttv5.client.MqttCallback;
+import org.eclipse.paho.mqttv5.client.MqttClient;
+import org.eclipse.paho.mqttv5.client.MqttConnectionOptions;
+import org.eclipse.paho.mqttv5.client.MqttDisconnectResponse;
+import org.eclipse.paho.mqttv5.client.persist.MemoryPersistence;
+import org.eclipse.paho.mqttv5.common.MqttException;
+import org.eclipse.paho.mqttv5.common.MqttMessage;
+import org.eclipse.paho.mqttv5.common.packet.MqttProperties;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static io.netty.handler.codec.mqtt.MqttQoS.AT_LEAST_ONCE;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * You need to build the Artemis Docker image with 'mvn install
-De2e-tests.skipImageBuild=false' before this test is
+ * executed.
+ */
+public class HAProxyTest extends E2ETestBase {
+
+ private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ // see haproxy.cfg
+ private static final int PROXY_PORT_V1 = 51617;
+ private static final int PROXY_PORT_V2 = 51627;
+ private static final int PROXY_PORT_SSL = 51618;
+ private static final int PROXY_PORT_INVALID = 51616;
+
+ private static final int BROKER_PROXY_PORT = 61617;
+ private static final int BROKER_PROXY_SSL_PORT = 61618;
+ private static final int BROKER_STANDARD_PORT = 61616;
+
+ public static final String USER_PASS = "artemis";
+ public static final String PROXY_PROTOCOL_VERSION_1 = "V1";
+ public static final String PROXY_PROTOCOL_VERSION_2 = "V2";
+
+ static Object network;
+ static Object haProxy;
+ static Object artemisServer;
+
+ static ContainerService service = ContainerService.getService();
+
+ @BeforeEach
+ public void disableThreadcheck() {
+ disableCheckThread();
+ }
+
+ private static final String HAPROXY_HOME = basedir +
"/target/proxy-protocol";
+
+ @BeforeAll
+ public static void startServers() throws Exception {
+ ValidateContainer.assumeArtemisContainer();
+
+ assertNotNull(basedir);
+
+ network = service.newNetwork();
+
+ artemisServer = service.newBrokerImage();
+ service.setNetwork(artemisServer, network);
+ service.exposePorts(artemisServer, BROKER_PROXY_PORT,
BROKER_PROXY_SSL_PORT, BROKER_STANDARD_PORT);
+ service.exposeHosts(artemisServer, "broker");
+ service.prepareInstance(HAPROXY_HOME);
+ service.exposeBrokerHome(artemisServer, HAPROXY_HOME);
+ service.startLogging(artemisServer, "ArtemisServer:");
+
+ recreateBrokerDirectory(HAPROXY_HOME);
+
+ service.start(artemisServer);
+
+ haProxy = service.newHaProxyImage();
+ service.setNetwork(haProxy, network);
+ service.exposePorts(haProxy, PROXY_PORT_V1, PROXY_PORT_V2,
PROXY_PORT_SSL, PROXY_PORT_INVALID);
+ service.exposeHosts(haProxy, "haproxy");
+ service.exposeFile(haProxy, basedir +
"/src/main/resources/servers/proxy-protocol/haproxy.cfg",
"/usr/local/etc/haproxy/haproxy.cfg");
+ service.startLogging(haProxy, "haproxy:");
+ service.start(haProxy);
+ }
+
+ @AfterAll
+ public static void stopServer() {
+ service.stop(artemisServer);
+ service.stop(haProxy);
+ }
+
+ /*
+ * a non-proxied connection shouldn't be able to connect to an acceptor
using proxyEnabled=true
+ */
+ @Test
+ public void testNonProxiedConnectionToProxyAcceptor() {
+ testFailure(artemisServer, BROKER_PROXY_PORT);
+ }
+
+ /*
+ * a proxied connection shouldn't be able to connect to an acceptor using
proxyEnabled=false
+ */
+ @Test
+ public void testProxiedConnectionToNonProxyAcceptor() {
+ testFailure(haProxy, PROXY_PORT_INVALID);
+ }
+
+ private void testFailure(Object target, int port) {
+ Exception e = assertThrows(JMSException.class, () -> {
+ testSendReceive(service.createCF(target, "CORE", port,
"?callTimeout=3000"), PROXY_PROTOCOL_VERSION_1);
+ });
+ Throwable rootCause = ExceptionUtils.getRootCause(e);
+ assertNotNull(rootCause);
+ assertTrue(rootCause instanceof ActiveMQConnectionTimedOutException);
Review Comment:
Is there possibility some of these takes 3 seconds to fail? Doesnt seem like
it should, with it known as soon as a handful of bytes are parsed that the
connection should be destroyed at the broker side.
Is some of that due to using the actual proxy to do the test? E.g in the
'connect without proxy-protocol to port with it enabled' case I'd think we
should just be able to connect directly to the port, and have it fail
immediately, no proxy needed. In the other case 'connect with proxy-protocol
header to port without it enabled', should we even need a client to test that,
just connecting a socket to the proxy would seem like it should blow up.
Alternatively, the proxy could again be removed and test it directly by sending
the header bytes.
The 'without proxy' tests might also be good as they can be run without the
containers (I'm realising many of the test envs dont run these tests..).
##########
docs/user-manual/proxy-protocol.adoc:
##########
@@ -0,0 +1,59 @@
+= PROXY Protocol
+:idprefix:
+:idseparator: -
+:docinfo: shared
+
+As noted in the official
https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt[PROXY
Protocol documentation]:
+
+[quote,]
+____
+The PROXY protocol provides a convenient way to safely transport connection
information such as a client's address across multiple layers of NAT or TCP
proxies.
+____
+
+This essentially allows the broker to know a client's IP address even when the
connection is established through reverse proxy that supports the PROXY
protocol (e.g. HAProxy, nginx, etc.).
+Without PROXY protocol support the broker would see such client connections as
coming from the proxy itself which can be misleading for administrators and
complicate trouble-shooting.
+
+Both versions 1 & 2 of the PROXY Protocol are supported.
+
+Any of our supported messaging protocols can be used in combination with the
PROXY protocol with or without TLS.
+
+== Configuration
+
+Support for the PROXY Protocol is configured on a per-acceptor basis using the
`proxyProtocolEnabled` parameter, e.g.:
+
+[,xml]
+----
+<acceptor
name="proxy-artemis">tcp://0.0.0.0:61616?proxyProtocolEnabled=true</acceptor>
+----
+
+[NOTE]
+.Why can't PROXY Protocol detection be automatic?
+====
+Support for the PROXY Protocol must be explicitly configured due to security
reasons.
+As noted in the official
https://github.com/haproxy/haproxy/blob/master/doc/proxy-protocol.txt[PROXY
Protocol documentation]:
+
+[quote,]
+____
+The receiver MUST be configured to only receive the protocol described in this
specification and MUST not try to guess whether the protocol header is present
or not.
+This means that the protocol explicitly prevents port sharing between public
and private access.
+Otherwise it would open a major security breach by allowing untrusted parties
to spoof their connection addresses.
+The receiver SHOULD ensure proper access filtering so that only trusted
proxies are allowed to use this protocol.
Review Comment:
Possibly worth adding something to the sentence below stressing this element
too when enabling it?
##########
tests/artemis-test-support/src/main/java/org/apache/activemq/artemis/tests/util/CFUtil.java:
##########
@@ -26,13 +26,24 @@ public class CFUtil {
public static ConnectionFactory createConnectionFactory(String protocol,
String uri) {
if (protocol.toUpperCase().equals("OPENWIRE")) {
return new org.apache.activemq.ActiveMQConnectionFactory(uri);
+ } else if (protocol.toUpperCase().equals("OPENWIRE_SSL")) {
+ if (uri.startsWith("tcp://")) {
+ // replacing tcp:// by amqp://
+ uri = "ssl" + uri.substring(3);
+ }
+ return new org.apache.activemq.ActiveMQSslConnectionFactory(uri);
} else if (protocol.toUpperCase().equals("AMQP")) {
-
if (uri.startsWith("tcp://")) {
// replacing tcp:// by amqp://
uri = "amqp" + uri.substring(3);
}
return new JmsConnectionFactory(uri);
+ } else if (protocol.toUpperCase().equals("AMQPS")) {
+ if (uri.startsWith("tcp://")) {
+ // replacing tcp:// by amqp://
Review Comment:
Comment needs updated
##########
tests/artemis-test-support/src/main/java/org/apache/activemq/artemis/tests/util/CFUtil.java:
##########
@@ -26,13 +26,24 @@ public class CFUtil {
public static ConnectionFactory createConnectionFactory(String protocol,
String uri) {
if (protocol.toUpperCase().equals("OPENWIRE")) {
return new org.apache.activemq.ActiveMQConnectionFactory(uri);
+ } else if (protocol.toUpperCase().equals("OPENWIRE_SSL")) {
+ if (uri.startsWith("tcp://")) {
+ // replacing tcp:// by amqp://
Review Comment:
Comment needs updated
##########
tests/e2e-tests/pom.xml:
##########
@@ -331,6 +362,62 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <artifactId>maven-resources-plugin</artifactId>
+ <configuration>
+ <nonFilteredFileExtensions>
+ <nonFilteredFileExtension>jks</nonFilteredFileExtension>
+ </nonFilteredFileExtensions>
+ </configuration>
+ <executions>
+ <execution>
+ <id>copy-proxy-protocol-broker-security-resources</id>
+ <phase>validate</phase>
+ <goals>
+ <goal>copy-resources</goal>
+ </goals>
+ <configuration>
+
<outputDirectory>${basedir}/target/proxy-protocol/etc</outputDirectory>
+ <resources>
+ <resource>
+ <directory>../security-resources</directory>
+ <includes>
+ <include>server-keystore.jks</include>
+ </includes>
+ </resource>
+ </resources>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ <plugin>
+ <artifactId>maven-resources-plugin</artifactId>
Review Comment:
The duplicate plugin definition is causing build warnings, there should just
be one definition with multiple executions.
```
Warning: Some problems were encountered while building the effective model
for org.apache.activemq.tests:e2e-tests:jar:2.43.0-SNAPSHOT
Warning: 'build.plugins.plugin.(groupId:artifactId)' must be unique but
found duplicate declaration of plugin
org.apache.maven.plugins:maven-resources-plugin @ line 393, column 18
```
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java:
##########
@@ -215,7 +215,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf
in, List<Object> out) t
if (!protocolSet.isEmpty()) {
// Use getBytes(...) as this works with direct and heap buffers.
byte[] bytes = new byte[8];
- in.getBytes(0, bytes);
+ in.getBytes(in.readerIndex(), bytes);
Review Comment:
Is this needed in addition to the buffer slicing done elsewhere? Or was it
an alternative?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyServerConnection.java:
##########
@@ -51,4 +58,55 @@ public void setSNIHostname(String sniHostname) {
public String getRouter() {
return router;
}
+
+ /**
+ * {@return a string representation of the remote address of this
connection; if this connection was made via the
+ * proxy protocol then this will be the original address, not the proxy
address}
+ */
+ @Override
+ public String getRemoteAddress() {
+ String proxyProtocolSourceAddress =
channel.attr(PROXY_PROTOCOL_SOURCE_ADDRESS).get();
+ String proxyProtocolSourcePort =
channel.attr(PROXY_PROTOCOL_SOURCE_PORT).get();
+ if (proxyProtocolSourceAddress != null &&
!proxyProtocolSourceAddress.isEmpty() && proxyProtocolSourcePort != null &&
!proxyProtocolSourcePort.isEmpty()) {
+ return proxyProtocolSourceAddress + ":" + proxyProtocolSourcePort;
+ } else {
+ return super.getRemoteAddress();
+ }
+ }
+
+ /**
+ * {@return if this connection is made via the proxy protocol then this
will be the address of the proxy}
+ */
+ public String getProxyAddress() {
+ String proxyProtocolDestinationAddress =
channel.attr(PROXY_PROTOCOL_DESTINATION_ADDRESS).get();
+ String proxyProtocolDestinationPort =
channel.attr(PROXY_PROTOCOL_DESTINATION_PORT).get();
+ if (proxyProtocolDestinationAddress != null &&
!proxyProtocolDestinationAddress.isEmpty() && proxyProtocolDestinationPort !=
null && !proxyProtocolDestinationPort.isEmpty()) {
+ return proxyProtocolDestinationAddress + ":" +
proxyProtocolDestinationPort;
+ } else {
+ return null;
+ }
+ }
+
+ /**
+ * {@return the version of the proxy protocol used to make the connection
or null if not applicable}
+ */
+ public String getProxyVersion() {
+ return channel.attr(PROXY_PROTOCOL_VERSION).get() == null ? null :
channel.attr(PROXY_PROTOCOL_VERSION).get().toString();
+ }
+
+ public static String getProxyAddress(Connection connection) {
+ if (connection instanceof NettyServerConnection nettyServerConnection) {
Review Comment:
Not sure what I was looking at when skimming yesterday that made me think
this instanceof was gone, possibly the stuff added over in ProxyProtocolUtil,
but the instanceof certainly still seems firmly in place here upon looking
closer today hehe. Ugly an inefficient, but no a hill I'm looking to die on
today, and its still better than the original way updating it on all the client
methods.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -525,13 +530,13 @@ private static int parseDefaultVariable(String
variableName, int defaultValue) {
allowableConnectorKeys.add(TransportConstants.NIO_REMOTING_THREADS_PROPNAME);
allowableConnectorKeys.add(TransportConstants.REMOTING_THREADS_PROPNAME);
allowableConnectorKeys.add(TransportConstants.BATCH_DELAY);
- allowableConnectorKeys.add(TransportConstants.PROXY_ENABLED_PROP_NAME);
- allowableConnectorKeys.add(TransportConstants.PROXY_HOST_PROP_NAME);
- allowableConnectorKeys.add(TransportConstants.PROXY_PORT_PROP_NAME);
+ allowableConnectorKeys.add(TransportConstants.SOCKS_ENABLED_PROP_NAME);
+ allowableConnectorKeys.add(TransportConstants.SOCKS_HOST_PROP_NAME);
+ allowableConnectorKeys.add(TransportConstants.SOCKS_PORT_PROP_NAME);
allowableConnectorKeys.add(TransportConstants.PROXY_VERSION_PROP_NAME);
- allowableConnectorKeys.add(TransportConstants.PROXY_USERNAME_PROP_NAME);
- allowableConnectorKeys.add(TransportConstants.PROXY_PASSWORD_PROP_NAME);
-
allowableConnectorKeys.add(TransportConstants.PROXY_REMOTE_DNS_PROP_NAME);
+ allowableConnectorKeys.add(TransportConstants.SOCKS_USERNAME_PROP_NAME);
Review Comment:
(commenting about the line above this one)
With the several other constant name changes from PROXY_ -> SOCKS_, is there
a reason not to change PROXY_VERSION_PROP_NAME to SOCKS_VERSION_PROP_NAME too,
given its actual value is "socksVersion" ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
