[GitHub] [james-project] chibenwa commented on a diff in pull request #1132: JAMES-3788 Implement SMTP proxy support for incoming connections usin…

[GitBox <gi...@apache.org>]

chibenwa commented on code in PR #1132:
URL: https://github.com/apache/james-project/pull/1132#discussion_r947527801


##########
protocols/smtp/src/test/java/org/apache/james/protocols/smtp/netty/NettyProxySMTPServerTest.java:
##########
@@ -0,0 +1,135 @@
+package org.apache.james.protocols.smtp.netty;

Review Comment:
   The ASF V2 license header is compulsory.



##########
protocols/netty/src/main/java/org/apache/james/protocols/netty/BasicChannelInboundHandler.java:
##########
@@ -158,6 +163,21 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
         try (Closeable closeable = mdc(ctx).build()) {
             ProtocolSession pSession = (ProtocolSession) ctx.channel().attr(SESSION_ATTRIBUTE_KEY).get();
 
+            if (msg instanceof HAProxyMessage) {
+                HAProxyMessage haproxyMsg = (HAProxyMessage) msg;
+
+                if (haproxyMsg.proxiedProtocol().equals(HAProxyProxiedProtocol.TCP4) || haproxyMsg.proxiedProtocol().equals(HAProxyProxiedProtocol.TCP6)) {
+                    pSession.setProxyDestinationAddress(new InetSocketAddress(haproxyMsg.destinationAddress(), haproxyMsg.destinationPort()));
+                    pSession.setProxySourceAddress(new InetSocketAddress(haproxyMsg.sourceAddress(), haproxyMsg.sourcePort()));
+                } else {
+                    // TODO how to handle?

Review Comment:
   Maybe just throw an IllegalArgumentException ?



##########
protocols/netty/pom.xml:
##########
@@ -50,6 +50,11 @@
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>
         </dependency>
+        <dependency>
+            <groupId>io.netty</groupId>
+            <artifactId>netty-codec-haproxy</artifactId>
+            <version>4.1.72.Final</version>

Review Comment:
   ```suggestion
               <version>${netty.version}</version>
   ```



##########
protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSession.java:
##########
@@ -148,10 +148,39 @@ public String toString() {
 
     
     /**
-     * Return the {@link InetSocketAddress} of the remote peer
+     * Return the {@link InetSocketAddress} of the remote peer. If proxy support
+     * is enabled, then it returns the remote peer given by the proxy.
      */
     InetSocketAddress getRemoteAddress();
 
+    /**
+     * Return true if PROXY is required by the configuration
+     *
+     * @return supported
+     */
+    boolean isProxyRequired();

Review Comment:
   Doing a search on `isProxyRequired()` in this PR I do not see where this method is called, except in tests. Can we get rid of it?



##########
server/apps/distributed-app/docs/modules/ROOT/pages/configure/smtp.adoc:
##########
@@ -54,6 +54,12 @@ this case, if no body is present, the value "localhost" will be used.
 | connectionLimitPerIP
 | Set the maximum simultaneous incoming connections per IP for this service.
 
+| proxyRequired
+| Enables proxy support for this service for incoming connections. HAProxy's protocol
+(https://www.haproxy.org/download/2.7/doc/proxy-protocol.txt) is used and might be compatible
+with other proxies (e.g. traefik). If enabled, it is *required* to initiate the connection
+using HAProxy's proxy protocol.
+

Review Comment:
   Can this be documented in imap.adoc too?



##########
protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSession.java:
##########
@@ -148,10 +148,39 @@ public String toString() {
 
     
     /**
-     * Return the {@link InetSocketAddress} of the remote peer
+     * Return the {@link InetSocketAddress} of the remote peer. If proxy support
+     * is enabled, then it returns the remote peer given by the proxy.
      */
     InetSocketAddress getRemoteAddress();
 
+    /**
+     * Return true if PROXY is required by the configuration
+     *
+     * @return supported
+     */
+    boolean isProxyRequired();
+
+    /**
+     * Return the {@link InetSocketAddress} of the proxy peer or null if proxy support is disabled.
+     */
+    InetSocketAddress getProxyDestinationAddress();
+
+    /**
+     * Sets the {@link InetSocketAddress} of the proxy peer.
+     */
+    void setProxyDestinationAddress(InetSocketAddress addr);
+
+    /**
+     * Return the {@link InetSocketAddress} of the remote peer if proxy is enabled or null if proxy
+     * support is disabled.
+     */
+    InetSocketAddress getProxySourceAddress();
+
+    /**
+     * Sets the {@link InetSocketAddress} remote peer provided by the proxy.
+     */
+    void setProxySourceAddress(InetSocketAddress addr);

Review Comment:
   I do not like this API change much as...
   
    - 1. We do not know that a null value might be returned. NPEs are waiting to happen
    - 2. Nothing forbids me to set the proxy source and not the destination, or vice versa.
    
    Let me suggest to...
    
     - Create a `ProxyInformation` POJO
     
   ```
   class ProxyInformation {
       private final InetSocketAddress source;
       private final InetSocketAddress destination;
       
       // constructor, equals, hash code, getters...
   ```
   
   Then in protocol session we can... 
   
   ```
       void setProxyInformation(ProxyInformation info);
       Optional<ProxyInformation> getProxyInformation();
   ```
   
   No more null, no more partially specified proxy info.
   
   WDYT ?



-- 
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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org