api: client verification in servlet

This introduces new global settings to handle how client address checks
are handled by the API layer:

proxy.header.verify: enables/disables checking of ipaddresses from a
                     proxy set header
proxy.header.names: a list of names to check for allowed ipaddresses
                    from a proxy set header.
proxy.cidr: a list of cidrs for which \"proxy.header.names\" are
            honoured if the \"Remote_Addr\" is in this list.

(cherry picked from commit b655466)
Signed-off-by: Rohit Yadav <[email protected]>

Author: DaanHoogland

client/src/main/java/org/apache/cloudstack/ServerDaemon.java

@@ -31,7 +31,6 @@
 import org.apache.commons.daemon.Daemon;
 import org.apache.commons.daemon.DaemonContext;
 import org.eclipse.jetty.jmx.MBeanContainer;
-import org.eclipse.jetty.server.ForwardedRequestCustomizer;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.NCSARequestLog;
@@ -167,7 +166,7 @@ public void start() throws Exception {
 
         // HTTP config
         final HttpConfiguration httpConfig = new HttpConfiguration();
-        httpConfig.addCustomizer( new ForwardedRequestCustomizer() );
+// it would be nice to make this dynamic but we take care of this ourselves for now: httpConfig.addCustomizer( new ForwardedRequestCustomizer() );
         httpConfig.setSecureScheme("https");
         httpConfig.setSecurePort(httpsPort);
         httpConfig.setOutputBufferSize(32768);

framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java

@@ -34,6 +34,7 @@ public class ConfigKey<T> {
 
     public static final String CATEGORY_ADVANCED = "Advanced";
     public static final String CATEGORY_ALERT = "Alert";
+    public static final String CATEGORY_NETWORK = "Network";
 
     public enum Scope {
         Global, Zone, Cluster, StoragePool, Account, ManagementServer, ImageStore, Domain

server/src/main/java/com/cloud/api/ApiServer.java

@@ -234,56 +234,77 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
     @Inject
     private MessageBus messageBus;
 
-    private static final ConfigKey<Integer> IntegrationAPIPort = new ConfigKey<Integer>("Advanced"
+    private static final ConfigKey<Integer> IntegrationAPIPort = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , Integer.class
             , "integration.api.port"
             , "0"
             , "Integration (unauthenticated) API port. To disable set it to 0 or negative."
             , false
             , ConfigKey.Scope.Global);
-    private static final ConfigKey<Long> ConcurrentSnapshotsThresholdPerHost = new ConfigKey<Long>("Advanced"
+    private static final ConfigKey<Long> ConcurrentSnapshotsThresholdPerHost = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , Long.class
             , "concurrent.snapshots.threshold.perhost"
             , null
             , "Limits number of snapshots that can be handled by the host concurrently; default is NULL - unlimited"
             , true // not sure if this is to be dynamic
             , ConfigKey.Scope.Global);
-    private static final ConfigKey<Boolean> EncodeApiResponse = new ConfigKey<Boolean>("Advanced"
+    private static final ConfigKey<Boolean> EncodeApiResponse = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , Boolean.class
             , "encode.api.response"
             , "false"
             , "Do URL encoding for the api response, false by default"
             , false
             , ConfigKey.Scope.Global);
-    static final ConfigKey<String> JSONcontentType = new ConfigKey<String>( "Advanced"
+    static final ConfigKey<String> JSONcontentType = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , String.class
             , "json.content.type"
             , "application/json; charset=UTF-8"
             , "Http response content type for .js files (default is text/javascript)"
             , false
             , ConfigKey.Scope.Global);
-    static final ConfigKey<Boolean> EnableSecureSessionCookie = new ConfigKey<Boolean>("Advanced"
+    static final ConfigKey<Boolean> EnableSecureSessionCookie = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , Boolean.class
             , "enable.secure.session.cookie"
             , "false"
             , "Session cookie is marked as secure if this is enabled. Secure cookies only work when HTTPS is used."
             , false
             , ConfigKey.Scope.Global);
-    private static final ConfigKey<String> JSONDefaultContentType = new ConfigKey<String> ("Advanced"
+    private static final ConfigKey<String> JSONDefaultContentType = new ConfigKey<> (ConfigKey.CATEGORY_ADVANCED
             , String.class
             , "json.content.type"
             , "application/json; charset=UTF-8"
             , "Http response content type for JSON"
             , false
             , ConfigKey.Scope.Global);
 
-    private static final ConfigKey<Boolean> UseEventAccountInfo = new ConfigKey<Boolean>( "advanced"
+    private static final ConfigKey<Boolean> UseEventAccountInfo = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
             , Boolean.class
             , "event.accountinfo"
             , "false"
             , "use account info in event logging"
             , true
             , ConfigKey.Scope.Global);
+    static final ConfigKey<Boolean> useForwardHeader = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+            , Boolean.class
+            , "proxy.header.verify"
+            , "false"
+            , "enables/disables checking of ipaddresses from a proxy set header. See \"proxy.header.names\" for the headers to allow."
+            , true
+            , ConfigKey.Scope.Global);
+    static final ConfigKey<String> listOfForwardHeaders = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+            , String.class
+            , "proxy.header.names"
+            , "X-Forwarded-For,HTTP_CLIENT_IP,HTTP_X_FORWARDED_FOR"
+            , "a list of names to check for allowed ipaddresses from a proxy set header. See \"proxy.cidr\" for the proxies allowed to set these headers."
+            , true
+            , ConfigKey.Scope.Global);
+    static final ConfigKey<String> proxyForwardList = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+            , String.class
+            , "proxy.cidr"
+            , ""
+            , "a list of cidrs for which \"proxy.header.names\" are honoured if the \"Remote_Addr\" is in this list."
+            , true
+            , ConfigKey.Scope.Global);
 
     @Override
     public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException {
@@ -1499,7 +1520,10 @@ public ConfigKey<?>[] getConfigKeys() {
                 ConcurrentSnapshotsThresholdPerHost,
                 EncodeApiResponse,
                 EnableSecureSessionCookie,
-                JSONDefaultContentType
+                JSONDefaultContentType,
+                proxyForwardList,
+                useForwardHeader,
+                listOfForwardHeaders
         };
     }
 }

server/src/main/java/com/cloud/api/ApiServlet.java

@@ -21,7 +21,6 @@
 import java.net.URLDecoder;
 import java.net.UnknownHostException;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -69,13 +68,9 @@
 import com.cloud.utils.net.NetUtils;
 
 @Component("apiServlet")
-@SuppressWarnings("serial")
 public class ApiServlet extends HttpServlet {
     public static final Logger s_logger = Logger.getLogger(ApiServlet.class.getName());
     private static final Logger s_accessLogger = Logger.getLogger("apiserver." + ApiServlet.class.getName());
-    private final static List<String> s_clientAddressHeaders = Collections
-            .unmodifiableList(Arrays.asList("X-Forwarded-For",
-                    "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr"));
     private static final String REPLACEMENT = "_";
     private static final String LOG_REPLACEMENTS = "[\n\r\t]";
 
@@ -570,17 +565,39 @@ private boolean isSpecificAPI(String commandName) {
         }
         return false;
     }
+    boolean doUseForwardHeaders() {
+        return Boolean.TRUE.equals(ApiServer.useForwardHeader.value());
+    }
 
+    String[] proxyNets() {
+        return ApiServer.proxyForwardList.value().split(",");
+    }
     //This method will try to get login IP of user even if servlet is behind reverseProxy or loadBalancer
-    public static InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
-        for(final String header : s_clientAddressHeaders) {
-            final String ip = getCorrectIPAddress(request.getHeader(header));
-            if (ip != null) {
-                return InetAddress.getByName(ip);
-            }
+    public InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
+        String ip = null;
+        InetAddress pretender = InetAddress.getByName(request.getRemoteAddr());
+        if(doUseForwardHeaders()) {
+            if (NetUtils.isIpInCidrList(pretender, proxyNets())) {
+                for (String header : getClientAddressHeaders()) {
+                    header = header.trim();
+                    ip = getCorrectIPAddress(request.getHeader(header));
+                    if (StringUtils.isNotBlank(ip)) {
+                        s_logger.debug(String.format("found ip %s in header %s ", ip, header));
+                        break;
+                    }
+                } // no address found in header so ip is blank and use remote addr
+            } // else not an allowed proxy address, ip is blank and use remote addr
         }
+        if (StringUtils.isBlank(ip)) {
+            s_logger.trace(String.format("no ip found in headers, returning remote address %s.", pretender.getHostAddress()));
+            return pretender;
+        }
+
+        return InetAddress.getByName(ip);
+    }
 
-        return InetAddress.getByName(request.getRemoteAddr());
+    private String[] getClientAddressHeaders() {
+        return ApiServer.listOfForwardHeaders.value().split(",");
     }
 
     private static String getCorrectIPAddress(String ip) {

server/src/test/java/com/cloud/api/ApiServletTest.java

@@ -99,15 +99,17 @@ public class ApiServletTest {
     @Mock
     AccountService accountMgr;
 
+    @Mock ConfigKey<Boolean> useForwardHeader;
     StringWriter responseWriter;
 
     ApiServlet servlet;
-
+    ApiServlet spyServlet;
     @SuppressWarnings("unchecked")
     @Before
     public void setup() throws SecurityException, NoSuchFieldException,
     IllegalArgumentException, IllegalAccessException, IOException, UnknownHostException {
         servlet = new ApiServlet();
+        spyServlet = Mockito.spy(servlet);
         responseWriter = new StringWriter();
         Mockito.when(response.getWriter()).thenReturn(
                 new PrintWriter(responseWriter));
@@ -261,32 +263,43 @@ public void processRequestInContextLogin() throws UnknownHostException {
 
     @Test
     public void getClientAddressWithXForwardedFor() throws UnknownHostException {
+        String[] proxynet = {"127.0.0.0/8"};
+        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
         Mockito.when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressWithHttpXForwardedFor() throws UnknownHostException {
+        String[] proxynet = {"127.0.0.0/8"};
+        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
         Mockito.when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
     }
 
     @Test
-    public void getClientAddressWithXRemoteAddr() throws UnknownHostException {
-        Mockito.when(request.getHeader(Mockito.eq("Remote_Addr"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+    public void getClientAddressWithRemoteAddr() throws UnknownHostException {
+        String[] proxynet = {"127.0.0.0/8"};
+        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
+        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressWithHttpClientIp() throws UnknownHostException {
+        String[] proxynet = {"127.0.0.0/8"};
+        Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+        Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
         Mockito.when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");
-        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
     }
 
     @Test
     public void getClientAddressDefault() throws UnknownHostException {
         Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
-        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
+        Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
     }
 
     @Test

utils/src/main/java/com/cloud/utils/StringUtils.java

@@ -27,7 +27,7 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
-public class StringUtils {
+public class StringUtils extends org.apache.commons.lang3.StringUtils {
     private static final char[] hexChar = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
 
     private static Charset preferredACSCharset;