CLOUDSTACK-7958: Add configuration for limit to CIDRs for Admin API c…

…alls (#2046)

* Cleanup and Improve NetUtils

This class had many unused methods, inconsistent names and redundant code.

This commit cleans up code, renames a few methods and constants.

The global/account setting 'api.allowed.source.cidr.list' is set
to 0.0.0.0/0,::/0 by default preserve the current behavior and thus
allow API calls for accounts from all IPv4 and IPv6 subnets.

Users can set it to a comma-separated list of IPv4/IPv6 subnets to
restrict API calls for Admin accounts to certain parts of their network(s).

This is to improve Security. Should an attacker steal the Access/Secret key
of an account he/she still needs to be in a subnet from where accounts are
allowed to perform API calls.

This is a good security measure for APIs which are connected to the public internet.

Signed-off-by: Wido den Hollander <wido@widodh.nl>

agent/src/com/cloud/agent/resource/consoleproxy/ConsoleProxyResource.java

@@ -276,12 +276,12 @@ private void addRouteToInternalIpOrCidr(String localgw, String eth1ip, String et
             s_logger.debug("addRouteToInternalIp: destIp is null");
             return;
         }
-        if (!NetUtils.isValidIp(destIpOrCidr) && !NetUtils.isValidCIDR(destIpOrCidr)) {
+        if (!NetUtils.isValidIp4(destIpOrCidr) && !NetUtils.isValidIp4Cidr(destIpOrCidr)) {
             s_logger.warn(" destIp is not a valid ip address or cidr destIp=" + destIpOrCidr);
             return;
         }
         boolean inSameSubnet = false;
-        if (NetUtils.isValidIp(destIpOrCidr)) {
+        if (NetUtils.isValidIp4(destIpOrCidr)) {
             if (eth1ip != null && eth1mask != null) {
                 inSameSubnet = NetUtils.sameSubnet(eth1ip, destIpOrCidr, eth1mask);
             } else {

api/src/org/apache/cloudstack/api/ApiServerService.java

@@ -24,7 +24,7 @@
 import com.cloud.exception.CloudAuthenticationException;
 
 public interface ApiServerService {
-    public boolean verifyRequest(Map<String, Object[]> requestParameters, Long userId) throws ServerApiException;
+    public boolean verifyRequest(Map<String, Object[]> requestParameters, Long userId, InetAddress remoteAddress) throws ServerApiException;
 
     public Long fetchDomainId(String domainUUID);
 

api/src/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmd.java

@@ -246,10 +246,10 @@ public void create() {
             String guestCidr = _networkService.getNetwork(getNetworkId()).getCidr();
 
             for (String cidr : getSourceCidrList()) {
-                if (!NetUtils.isValidCIDR(cidr)) {
+                if (!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) {
                     throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Source cidrs formatting error " + cidr);
                 }
-                if (cidr.equals(NetUtils.ALL_CIDRS)) {
+                if (cidr.equals(NetUtils.ALL_IP4_CIDRS)) {
                     continue;
                 }
                 if (!NetUtils.isNetworkAWithinNetworkB(cidr, guestCidr)) {
@@ -261,7 +261,7 @@ public void create() {
         //Destination CIDR formatting check. Since it's optional param, no need to set a default as in the case of source.
         if(destCidrList != null){
             for(String cidr : destCidrList){
-                if(!NetUtils.isValidCIDR(cidr)) {
+                if(!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) {
                     throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Destination cidrs formatting error" + cidr);
                 }
             }

api/src/org/apache/cloudstack/api/command/user/firewall/CreateFirewallRuleCmd.java

@@ -108,7 +108,7 @@ public List<String> getSourceCidrList() {
             return cidrlist;
         } else {
             List<String> oneCidrList = new ArrayList<String>();
-            oneCidrList.add(NetUtils.ALL_CIDRS);
+            oneCidrList.add(NetUtils.ALL_IP4_CIDRS);
             return oneCidrList;
         }
 
@@ -242,7 +242,7 @@ public long getDomainId() {
     public void create() {
         if (getSourceCidrList() != null) {
             for (String cidr : getSourceCidrList()) {
-                if (!NetUtils.isValidCIDR(cidr)) {
+                if (!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) {
                     throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Source CIDRs formatting error " + cidr);
                 }
             }

api/src/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java

@@ -348,7 +348,7 @@ public void create() {
 
         Ip privateIp = getVmSecondaryIp();
         if (privateIp != null) {
-            if (!NetUtils.isValidIp(privateIp.toString())) {
+            if (!NetUtils.isValidIp4(privateIp.toString())) {
                 throw new InvalidParameterValueException("Invalid vm ip address");
             }
         }

api/src/org/apache/cloudstack/api/command/user/loadbalancer/AssignToLoadBalancerRuleCmd.java

@@ -140,7 +140,7 @@ public Map<Long, List<String>> getVmIdIpListMap() {
                 }
 
                 //check wether the given ip is valid ip or not
-                if (vmIp == null || !NetUtils.isValidIp(vmIp)) {
+                if (vmIp == null || !NetUtils.isValidIp4(vmIp)) {
                     throw new InvalidParameterValueException("Invalid ip address "+ vmIp +" passed in vmidipmap for " +
                             "vmid " + vmId);
                 }

api/src/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java

@@ -134,7 +134,7 @@ public List<String> getSourceCidrList() {
             return cidrlist;
         } else {
             List<String> oneCidrList = new ArrayList<String>();
-            oneCidrList.add(NetUtils.ALL_CIDRS);
+            oneCidrList.add(NetUtils.ALL_IP4_CIDRS);
             return oneCidrList;
         }
     }

api/src/org/apache/cloudstack/api/command/user/vm/AddIpToVmNicCmd.java

@@ -173,7 +173,7 @@ public void create() throws ResourceAllocationException {
         NicSecondaryIp result;
         String secondaryIp = null;
         if ((ip = getIpaddress()) != null) {
-            if (!NetUtils.isValidIp(ip)) {
+            if (!NetUtils.isValidIp4(ip)) {
                 throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Invalid ip address " + ip);
             }
         }

api/src/org/apache/cloudstack/api/command/user/vm/UpdateVmNicIpCmd.java

@@ -150,7 +150,7 @@ public void execute() throws ResourceUnavailableException, ResourceAllocationExc
         CallContext.current().setEventDetails("Nic Id: " + getNicId() );
         String ip;
         if ((ip = getIpaddress()) != null) {
-            if (!NetUtils.isValidIp(ip)) {
+            if (!NetUtils.isValidIp4(ip)) {
                 throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Invalid ip address " + ip);
             }
         }

api/src/org/apache/cloudstack/config/ApiServiceConfiguration.java

@@ -25,15 +25,18 @@ public class ApiServiceConfiguration implements Configurable {
             "API end point. Can be used by CS components/services deployed remotely, for sending CS API requests", true);
     public static final ConfigKey<Long> DefaultUIPageSize = new ConfigKey<Long>("Advanced", Long.class, "default.ui.page.size", "20",
             "The default pagesize to be used by UI and other clients when making list* API calls", true, ConfigKey.Scope.Global);
-
+    public static final ConfigKey<Boolean> ApiSourceCidrChecksEnabled = new ConfigKey<>("Advanced", Boolean.class, "api.source.cidr.checks.enabled",
+            "true", "Are the source checks on API calls enabled (true) or not (false)? See api.allowed.source.cidr.list", true, ConfigKey.Scope.Global);
+    public static final ConfigKey<String> ApiAllowedSourceCidrList = new ConfigKey<String>("Advanced", String.class, "api.allowed.source.cidr.list",
+            "0.0.0.0/0,::/0", "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.", true, ConfigKey.Scope.Account);
     @Override
     public String getConfigComponentName() {
         return ApiServiceConfiguration.class.getSimpleName();
     }
 
     @Override
     public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey<?>[] {ManagementHostIPAdr, ApiServletPath, DefaultUIPageSize};
+        return new ConfigKey<?>[] {ManagementHostIPAdr, ApiServletPath, DefaultUIPageSize, ApiSourceCidrChecksEnabled, ApiAllowedSourceCidrList};
     }
 
 }

core/src/com/cloud/network/HAProxyConfigurator.java

@@ -94,7 +94,7 @@ public String[] generateConfiguration(final List<PortForwardingRuleTO> fwRules)
     private List<String> getRulesForPool(final String poolName, final List<PortForwardingRuleTO> fwRules) {
         final PortForwardingRuleTO firstRule = fwRules.get(0);
         final String publicIP = firstRule.getSrcIp();
-        final String publicPort = Integer.toString(firstRule.getSrcPortRange()[0]);
+        final int publicPort = firstRule.getSrcPortRange()[0];
         // FIXEME: String algorithm = firstRule.getAlgorithm();
 
         final List<String> result = new ArrayList<String>();
@@ -108,9 +108,7 @@ private List<String> getRulesForPool(final String poolName, final List<PortForwa
         sb = new StringBuilder();
         // FIXME sb.append("\t").append("balance ").append(algorithm);
         result.add(sb.toString());
-        if (publicPort.equals(NetUtils.HTTP_PORT)
-                // && global option httpclose set (or maybe not in this spot???)
-                ) {
+        if (publicPort == NetUtils.HTTP_PORT) {
             sb = new StringBuilder();
             sb.append("\t").append("mode http");
             result.add(sb.toString());
@@ -473,7 +471,7 @@ private List<String> getRulesForPool(final LoadBalancerTO lbTO, final boolean ke
         StringBuilder sb = new StringBuilder();
         final String poolName = sb.append(lbTO.getSrcIp().replace(".", "_")).append('-').append(lbTO.getSrcPort()).toString();
         final String publicIP = lbTO.getSrcIp();
-        final String publicPort = Integer.toString(lbTO.getSrcPort());
+        final int publicPort = lbTO.getSrcPort();
         final String algorithm = lbTO.getAlgorithm();
 
         final List<String> result = new ArrayList<String>();
@@ -544,7 +542,7 @@ private List<String> getRulesForPool(final LoadBalancerTO lbTO, final boolean ke
         if (stickinessSubRule != null && !destsAvailable) {
             s_logger.warn("Haproxy stickiness policy for lb rule: " + lbTO.getSrcIp() + ":" + lbTO.getSrcPort() + ": Not Applied, cause:  backends are unavailable");
         }
-        if (publicPort.equals(NetUtils.HTTP_PORT) && !keepAliveEnabled || httpbasedStickiness) {
+        if (publicPort == NetUtils.HTTP_PORT && !keepAliveEnabled || httpbasedStickiness) {
             sb = new StringBuilder();
             sb.append("\t").append("mode http");
             result.add(sb.toString());

plugins/alert-handlers/snmp-alerts/src/org/apache/cloudstack/alert/snmp/SnmpTrapAppender.java

@@ -161,7 +161,7 @@ private boolean validateIpAddresses() {
             if (ipAddress.trim().equalsIgnoreCase("localhost")) {
                 continue;
             }
-            if (!NetUtils.isValidIp(ipAddress)) {
+            if (!NetUtils.isValidIp4(ipAddress)) {
                 return false;
             }
         }

plugins/alert-handlers/syslog-alerts/src/org/apache/cloudstack/syslog/AlertsSyslogAppender.java

@@ -168,7 +168,7 @@ private boolean validateIpAddresses() {
                 if (ip.equalsIgnoreCase("localhost")) {
                     continue;
                 }
-                if (!NetUtils.isValidIp(ip)) {
+                if (!NetUtils.isValidIp4(ip)) {
                     return false;
                 }
             } else

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtGetVmIpAddressCommandWrapper.java

@@ -49,7 +49,7 @@ public Answer execute(final GetVmIpAddressCommand command, final LibvirtComputin
                     String ipAddr = Script.runSimpleBashScript(new StringBuilder().append("virt-cat ").append(command.getVmName())
                             .append(" /var/lib/dhclient/" + leaseFile + " | tail -16 | grep 'fixed-address' | awk '{print $2}' | sed -e 's/;//'").toString());
                     // Check if the IP belongs to the network
-                    if((ipAddr != null) && NetUtils.isIpWithtInCidrRange(ipAddr, networkCidr)){
+                    if((ipAddr != null) && NetUtils.isIpWithInCidrRange(ipAddr, networkCidr)){
                         ip = ipAddr;
                         break;
                     }
@@ -65,7 +65,7 @@ public Answer execute(final GetVmIpAddressCommand command, final LibvirtComputin
                 String[] ips = ipList.split("\n");
                 for (String ipAddr : ips){
                     // Check if the IP belongs to the network
-                    if((ipAddr != null) && NetUtils.isIpWithtInCidrRange(ipAddr, networkCidr)){
+                    if((ipAddr != null) && NetUtils.isIpWithInCidrRange(ipAddr, networkCidr)){
                         ip = ipAddr;
                         break;
                     }

plugins/hypervisors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/helpers/Ovm3Configuration.java

@@ -130,7 +130,7 @@ private void validatePoolAndCluster() {
             LOGGER.debug("Clustering requires a pool, setting pool to true");
             agentInOvm3Pool = true;
         }
-        if (!NetUtils.isValidIp(ovm3PoolVip)) {
+        if (!NetUtils.isValidIp4(ovm3PoolVip)) {
             LOGGER.debug("No VIP, Setting ovm3pool and ovm3cluster to false");
             agentInOvm3Pool = false;
             agentInOvm3Cluster = false;

plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixGetVmIpAddressCommandWrapper.java

@@ -56,7 +56,7 @@ public Answer execute(final GetVmIpAddressCommand command, final CitrixResourceB
             Map<String, String> vmIpsMap = rec.networks;
 
             for (String ipAddr: vmIpsMap.values()) {
-                if (NetUtils.isIpWithtInCidrRange(ipAddr, networkCidr)) {
+                if (NetUtils.isIpWithInCidrRange(ipAddr, networkCidr)) {
                     vmIp = ipAddr;
                     break;
                 }

plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java

@@ -2574,7 +2574,7 @@ private void deleteServersInGuestVlan(final long vlanTag, final String vlanSelfI
     }
 
     private String getNetScalerProtocol(final LoadBalancerTO loadBalancer) throws ExecutionException {
-        final String port = Integer.toString(loadBalancer.getSrcPort());
+        final int port = loadBalancer.getSrcPort();
         String lbProtocol = loadBalancer.getLbProtocol();
         final StickinessPolicyTO[] stickyPolicies = loadBalancer.getStickinessPolicies();
         String nsProtocol = "TCP";
@@ -2596,7 +2596,7 @@ private String getNetScalerProtocol(final LoadBalancerTO loadBalancer) throws Ex
             return lbProtocol.toUpperCase();
         }
 
-        if (port.equals(NetUtils.HTTP_PORT)) {
+        if (port == NetUtils.HTTP_PORT) {
             nsProtocol = "HTTP";
         } else if (NetUtils.TCP_PROTO.equalsIgnoreCase(lbProtocol)) {
             nsProtocol = "TCP";

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

@@ -46,6 +46,7 @@
 import com.cloud.utils.Pair;
 import com.cloud.utils.ReflectUtil;
 import com.cloud.utils.StringUtils;
+import com.cloud.utils.net.NetUtils;
 import com.cloud.utils.component.ComponentContext;
 import com.cloud.utils.component.ManagerBase;
 import com.cloud.utils.component.PluggableService;
@@ -97,6 +98,7 @@
 import org.apache.cloudstack.api.response.ExceptionResponse;
 import org.apache.cloudstack.api.response.ListResponse;
 import org.apache.cloudstack.api.response.LoginCmdResponse;
+import org.apache.cloudstack.config.ApiServiceConfiguration;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.framework.config.Configurable;
@@ -795,7 +797,7 @@ private void buildAuditTrail(final StringBuilder auditTrailSb, final String comm
     }
 
     @Override
-    public boolean verifyRequest(final Map<String, Object[]> requestParameters, final Long userId) throws ServerApiException {
+    public boolean verifyRequest(final Map<String, Object[]> requestParameters, final Long userId, InetAddress remoteAddress) throws ServerApiException {
         try {
             String apiKey = null;
             String secretKey = null;
@@ -814,21 +816,18 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
             if (userId != null) {
                 final User user = ApiDBUtils.findUserById(userId);
 
-                try {
-                    checkCommandAvailable(user, commandName);
-                } catch (final RequestLimitException ex) {
-                    s_logger.debug(ex.getMessage());
-                    throw new ServerApiException(ApiErrorCode.API_LIMIT_EXCEED, ex.getMessage());
-                } catch (final PermissionDeniedException ex) {
-                    s_logger.debug("The user with id:" + userId + " is not allowed to request the API command or the API command does not exist: " + commandName);
-                    throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, "The user is not allowed to request the API command or the API command does not exist");
+                if (!commandAvailable(remoteAddress, commandName, user)) {
+                    return false;
                 }
+
                 return true;
             } else {
                 // check against every available command to see if the command exists or not
                 if (!s_apiNameCmdClassMap.containsKey(commandName) && !commandName.equals("login") && !commandName.equals("logout")) {
-                    s_logger.debug("The user with id:" + userId + " is not allowed to request the API command or the API command does not exist: " + commandName);
-                    throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, "The user is not allowed to request the API command or the API command does not exist");
+                    final String errorMessage = "The given command " + commandName + " either does not exist, is not available" +
+                            " for user, or not available from ip address '" + remoteAddress.getHostAddress() + "'.";
+                    s_logger.debug(errorMessage);
+                    return false;
                 }
             }
 
@@ -916,15 +915,8 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
                 return false;
             }
 
-            try {
-                checkCommandAvailable(user, commandName);
-            } catch (final RequestLimitException ex) {
-                s_logger.debug(ex.getMessage());
-                throw new ServerApiException(ApiErrorCode.API_LIMIT_EXCEED, ex.getMessage());
-            } catch (final PermissionDeniedException ex) {
-                s_logger.debug("The given command:" + commandName + " does not exist or it is not available for user");
-                throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, "The given command:" + commandName + " does not exist or it is not available for user with id:"
-                        + userId);
+            if (!commandAvailable(remoteAddress, commandName, user)) {
+                return false;
             }
 
             // verify secret key exists
@@ -959,6 +951,21 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
         return false;
     }
 
+    private boolean commandAvailable(final InetAddress remoteAddress, final String commandName, final User user) {
+        try {
+            checkCommandAvailable(user, commandName, remoteAddress);
+        } catch (final RequestLimitException ex) {
+            s_logger.debug(ex.getMessage());
+            throw new ServerApiException(ApiErrorCode.API_LIMIT_EXCEED, ex.getMessage());
+        } catch (final PermissionDeniedException ex) {
+            final String errorMessage = "The given command '" + commandName + "' either does not exist, is not available" +
+                    " for user, or not available from ip address '" + remoteAddress + "'.";
+            s_logger.debug(errorMessage);
+            return false;
+        }
+        return true;
+    }
+
     @Override
     public Long fetchDomainId(final String domainUUID) {
         final Domain domain = domainMgr.getDomain(domainUUID);
@@ -1113,11 +1120,24 @@ public boolean verifyUser(final Long userId) {
         return true;
     }
 
-    private void checkCommandAvailable(final User user, final String commandName) throws PermissionDeniedException {
+    private void checkCommandAvailable(final User user, final String commandName, final InetAddress remoteAddress) throws PermissionDeniedException {
         if (user == null) {
             throw new PermissionDeniedException("User is null for role based API access check for command" + commandName);
         }
 
+        final Account account = accountMgr.getAccount(user.getAccountId());
+        final String accessAllowedCidrs = ApiServiceConfiguration.ApiAllowedSourceCidrList.valueIn(account.getId()).replaceAll("\\s","");
+        final Boolean apiSourceCidrChecksEnabled = ApiServiceConfiguration.ApiSourceCidrChecksEnabled.value();
+
+        if (apiSourceCidrChecksEnabled) {
+            s_logger.debug("CIDRs from which account '" + account.toString() + "' is allowed to perform API calls: " + accessAllowedCidrs);
+            if (!NetUtils.isIpInCidrList(remoteAddress, accessAllowedCidrs.split(","))) {
+                s_logger.warn("Request by account '" + account.toString() + "' was denied since " + remoteAddress + " does not match " + accessAllowedCidrs);
+                throw new PermissionDeniedException("Calls for domain '" + account.getAccountName() + "' are not allowed from ip address '" + remoteAddress.getHostAddress());
+                }
+        }
+
+
         for (final APIChecker apiChecker : apiAccessCheckers) {
             apiChecker.checkAccess(user, commandName);
         }