Support ApiServer to enforce POST requests for state changing APIs and requests with timestamps (apache#10899)

Co-authored-by: Kevin Li <[email protected]>

Author: 2 people

api/src/main/java/org/apache/cloudstack/api/ApiErrorCode.java

@@ -22,6 +22,7 @@
  */
 public enum ApiErrorCode {
 
+    BAD_REQUEST(400),
     UNAUTHORIZED(401),
     UNAUTHORIZED2FA(511),
     METHOD_NOT_ALLOWED(405),

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

@@ -48,4 +48,6 @@ public ResponseObject loginUser(HttpSession session, String username, String pas
     boolean forgotPassword(UserAccount userAccount, Domain domain);
 
     boolean resetPassword(UserAccount userAccount, String token, String password);
+
+    boolean isPostRequestsAndTimestampsEnforced();
 }

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

@@ -209,6 +209,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
     private static final String SANITIZATION_REGEX = "[\n\r]";
 
     private static boolean encodeApiResponse = false;
+    private boolean isPostRequestsAndTimestampsEnforced = false;
 
     /**
      * Non-printable ASCII characters - numbers 0 to 31 and 127 decimal
@@ -298,6 +299,13 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
             , "Session cookie is marked as secure if this is enabled. Secure cookies only work when HTTPS is used."
             , false
             , ConfigKey.Scope.Global);
+    static final ConfigKey<Boolean> EnforcePostRequestsAndTimestamps = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
+            , Boolean.class
+            , "enforce.post.requests.and.timestamps"
+            , "false"
+            , "Enable/Disable whether the ApiServer should only accept POST requests for state-changing APIs and requests with timestamps."
+            , false
+            , ConfigKey.Scope.Global);
     private static final ConfigKey<String> JSONDefaultContentType = new ConfigKey<> (ConfigKey.CATEGORY_ADVANCED
             , String.class
             , "json.content.type"
@@ -476,6 +484,7 @@ protected void setupIntegrationPortListener(Integer apiPort) {
     public boolean start() {
         Security.addProvider(new BouncyCastleProvider());
         Integer apiPort = IntegrationAPIPort.value(); // api port, null by default
+        isPostRequestsAndTimestampsEnforced = EnforcePostRequestsAndTimestamps.value();
 
         final Long snapshotLimit = ConcurrentSnapshotsThresholdPerHost.value();
         if (snapshotLimit == null || snapshotLimit <= 0) {
@@ -755,6 +764,11 @@ public String handleRequest(final Map params, final String responseType, final S
         return response;
     }
 
+    @Override
+    public boolean isPostRequestsAndTimestampsEnforced() {
+        return isPostRequestsAndTimestampsEnforced;
+    }
+
     private String getBaseAsyncResponse(final long jobId, final BaseAsyncCmd cmd) {
         final AsyncJobResponse response = new AsyncJobResponse();
 
@@ -1002,7 +1016,6 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
 
             // put the name in a list that we'll sort later
             final List<String> parameterNames = new ArrayList<>(requestParameters.keySet());
-
             Collections.sort(parameterNames);
 
             String signatureVersion = null;
@@ -1054,12 +1067,22 @@ public boolean verifyRequest(final Map<String, Object[]> requestParameters, fina
                 }
 
                 final Date now = new Date(System.currentTimeMillis());
+                final Date thresholdTime = new Date(now.getTime() + 15 * 60 * 1000);
                 if (expiresTS.before(now)) {
                     signature = signature.replaceAll(SANITIZATION_REGEX, "_");
                     apiKey = apiKey.replaceAll(SANITIZATION_REGEX, "_");
                     logger.debug("Request expired -- ignoring ...sig [{}], apiKey [{}].", signature, apiKey);
                     return false;
+                } else if (isPostRequestsAndTimestampsEnforced && expiresTS.after(thresholdTime)) {
+                    signature = signature.replaceAll(SANITIZATION_REGEX, "_");
+                    apiKey = apiKey.replaceAll(SANITIZATION_REGEX, "_");
+                    logger.debug(String.format("Expiration parameter is set for too long -- ignoring ...sig [%s], apiKey [%s].", signature, apiKey));
+                    return false;
                 }
+            } else if (isPostRequestsAndTimestampsEnforced) {
+                // Force expiration parameter
+                logger.debug("Signature Version must be 3, and should be along with the Expires parameter -- ignoring request.");
+                return false;
             }
 
             final TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.CLOUD_DB);
@@ -1751,6 +1774,7 @@ public String getConfigComponentName() {
     @Override
     public ConfigKey<?>[] getConfigKeys() {
         return new ConfigKey<?>[] {
+                EnforcePostRequestsAndTimestamps,
                 IntegrationAPIPort,
                 ConcurrentSnapshotsThresholdPerHost,
                 EncodeApiResponse,

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

@@ -22,9 +22,13 @@
 import java.net.UnknownHostException;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Collections;
+import java.util.regex.Pattern;
+import java.util.Set;
+
 import javax.inject.Inject;
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
@@ -46,6 +50,7 @@
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.managed.context.ManagedContext;
 import org.apache.cloudstack.utils.consoleproxy.ConsoleAccessUtils;
+import org.apache.commons.collections.MapUtils;
 
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
@@ -78,6 +83,39 @@ public class ApiServlet extends HttpServlet {
     private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServlet.class.getName());
     private static final String REPLACEMENT = "_";
     private static final String LOGGER_REPLACEMENTS = "[\n\r\t]";
+    private static final Pattern GET_REQUEST_COMMANDS = Pattern.compile("^(get|list|query|find)(\\w+)+$");
+    private static final HashSet<String> GET_REQUEST_COMMANDS_LIST = new HashSet<>(Set.of("isaccountallowedtocreateofferingswithtags",
+            "readyforshutdown", "cloudianisenabled", "quotabalance", "quotasummary", "quotatarifflist", "quotaisenabled", "quotastatement", "verifyoauthcodeandgetuser"));
+    private static final HashSet<String> POST_REQUESTS_TO_DISABLE_LOGGING = new HashSet<>(Set.of(
+            "login",
+            "oauthlogin",
+            "createaccount",
+            "createuser",
+            "updateuser",
+            "forgotpassword",
+            "resetpassword",
+            "importrole",
+            "updaterolepermission",
+            "updateprojectrolepermission",
+            "createstoragepool",
+            "addhost",
+            "updatehostpassword",
+            "addcluster",
+            "addvmwaredc",
+            "configureoutofbandmanagement",
+            "uploadcustomcertificate",
+            "addciscovnmcresource",
+            "addnetscalerloadbalancer",
+            "createtungstenfabricprovider",
+            "addnsxcontroller",
+            "configtungstenfabricservice",
+            "createnetworkacl",
+            "updatenetworkaclitem",
+            "quotavalidateactivationrule",
+            "quotatariffupdate",
+            "listandswitchsamlaccount",
+            "uploadresourceicon"
+    ));
 
     @Inject
     ApiServerService apiServer;
@@ -193,11 +231,24 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
 
         utf8Fixup(req, params);
 
+        final Object[] commandObj = params.get(ApiConstants.COMMAND);
+        final String command = commandObj == null ? null : (String) commandObj[0];
+
         // logging the request start and end in management log for easy debugging
         String reqStr = "";
         String cleanQueryString = StringUtils.cleanString(req.getQueryString());
         if (LOGGER.isDebugEnabled()) {
             reqStr = auditTrailSb.toString() + " " + cleanQueryString;
+            if (req.getMethod().equalsIgnoreCase("POST") && org.apache.commons.lang3.StringUtils.isNotBlank(command)) {
+                if (!POST_REQUESTS_TO_DISABLE_LOGGING.contains(command.toLowerCase()) && !reqParams.containsKey(ApiConstants.USER_DATA)) {
+                    String cleanParamsString = getCleanParamsString(reqParams);
+                    if (org.apache.commons.lang3.StringUtils.isNotBlank(cleanParamsString)) {
+                        reqStr += "\n" + cleanParamsString;
+                    }
+                } else {
+                    reqStr += " " + command;
+                }
+            }
             LOGGER.debug("===START=== " + reqStr);
         }
 
@@ -213,8 +264,6 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
                 responseType = (String)responseTypeParam[0];
             }
 
-            final Object[] commandObj = params.get(ApiConstants.COMMAND);
-            final String command = commandObj == null ? null : (String) commandObj[0];
             final Object[] userObj = params.get(ApiConstants.USERNAME);
             String username = userObj == null ? null : (String)userObj[0];
             if (LOGGER.isTraceEnabled()) {
@@ -317,6 +366,19 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
                 }
             }
 
+            if (apiServer.isPostRequestsAndTimestampsEnforced() && !isStateChangingCommandUsingPOST(command, req.getMethod(), params)) {
+                String errorText = String.format("State changing command %s needs to be sent using POST request", command);
+                if (command.equalsIgnoreCase("updateConfiguration") && params.containsKey("name")) {
+                    errorText = String.format("Changes for configuration %s needs to be sent using POST request", params.get("name")[0]);
+                }
+                auditTrailSb.append(" " + HttpServletResponse.SC_BAD_REQUEST + " " + errorText);
+                final String serializedResponse =
+                        apiServer.getSerializedApiError(new ServerApiException(ApiErrorCode.BAD_REQUEST, errorText), params,
+                                responseType);
+                HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_BAD_REQUEST, responseType, ApiServer.JSONcontentType.value());
+                return;
+            }
+
             Long userId = null;
             if (!isNew) {
                 userId = (Long)session.getAttribute("userid");
@@ -407,6 +469,15 @@ private boolean checkIfAuthenticatorIsOf2FA(String command) {
         return verify2FA;
     }
 
+    private boolean isStateChangingCommandUsingPOST(String command, String method, Map<String, Object[]> params) {
+        if (command == null || (!GET_REQUEST_COMMANDS.matcher(command.toLowerCase()).matches() && !GET_REQUEST_COMMANDS_LIST.contains(command.toLowerCase())
+                && !command.equalsIgnoreCase("updateConfiguration") && !method.equals("POST"))) {
+            return false;
+        }
+        return !command.equalsIgnoreCase("updateConfiguration") || method.equals("POST") || (params.containsKey("name")
+                && params.get("name")[0].toString().equalsIgnoreCase(ApiServer.EnforcePostRequestsAndTimestamps.key()));
+    }
+
     protected boolean skip2FAcheckForAPIs(String command) {
         boolean skip2FAcheck = false;
 
@@ -644,4 +715,45 @@ private static String getCorrectIPAddress(String ip) {
         }
         return null;
     }
+
+    private String getCleanParamsString(Map<String, String[]> reqParams) {
+        if (MapUtils.isEmpty(reqParams)) {
+            return "";
+        }
+
+        StringBuilder cleanParamsString = new StringBuilder();
+        for (Map.Entry<String, String[]> reqParam : reqParams.entrySet()) {
+            if (org.apache.commons.lang3.StringUtils.isBlank(reqParam.getKey())) {
+                continue;
+            }
+
+            cleanParamsString.append(reqParam.getKey());
+            cleanParamsString.append("=");
+
+            if (reqParam.getKey().toLowerCase().contains("password")
+                    || reqParam.getKey().toLowerCase().contains("privatekey")
+                    || reqParam.getKey().toLowerCase().contains("accesskey")
+                    || reqParam.getKey().toLowerCase().contains("secretkey")) {
+                cleanParamsString.append("\n");
+                continue;
+            }
+
+            if (reqParam.getValue() == null || reqParam.getValue().length == 0) {
+                cleanParamsString.append("\n");
+                continue;
+            }
+
+            for (String param : reqParam.getValue()) {
+                if (org.apache.commons.lang3.StringUtils.isBlank(param)) {
+                    continue;
+                }
+                String cleanParamString = StringUtils.cleanString(param.trim());
+                cleanParamsString.append(cleanParamString);
+                cleanParamsString.append(" ");
+            }
+            cleanParamsString.append("\n");
+        }
+
+        return cleanParamsString.toString();
+    }
 }

ui/src/api/index.js

@@ -23,18 +23,10 @@ import {
   ACCESS_TOKEN
 } from '@/store/mutation-types'
 
-export function api (command, args = {}, method = 'GET', data = {}) {
-  let params = {}
+export function getAPI (command, args = {}) {
   args.command = command
   args.response = 'json'
 
-  if (data) {
-    params = new URLSearchParams()
-    Object.entries(data).forEach(([key, value]) => {
-      params.append(key, value)
-    })
-  }
-
   const sessionkey = vueProps.$localStorage.get(ACCESS_TOKEN) || Cookies.get('sessionkey')
   if (sessionkey) {
     args.sessionkey = sessionkey
@@ -45,8 +37,30 @@ export function api (command, args = {}, method = 'GET', data = {}) {
       ...args
     },
     url: '/',
-    method,
-    data: params || {}
+    method: 'GET'
+  })
+}
+
+export function postAPI (command, data = {}) {
+  const params = new URLSearchParams()
+  params.append('command', command)
+  params.append('response', 'json')
+  if (data) {
+    Object.entries(data).forEach(([key, value]) => {
+      if (value !== undefined && value !== null && value !== '') {
+        params.append(key, value)
+      }
+    })
+  }
+
+  const sessionkey = vueProps.$localStorage.get(ACCESS_TOKEN) || Cookies.get('sessionkey')
+  if (sessionkey) {
+    params.append('sessionkey', sessionkey)
+  }
+  return axios({
+    url: '/',
+    method: 'POST',
+    data: params
   })
 }
 
@@ -56,7 +70,7 @@ export function login (arg) {
   }
 
   // Logout before login is called to purge any duplicate sessionkey cookies
-  api('logout')
+  postAPI('logout')
 
   const params = new URLSearchParams()
   params.append('command', 'login')
@@ -66,7 +80,7 @@ export function login (arg) {
   params.append('response', 'json')
   return axios({
     url: '/',
-    method: 'post',
+    method: 'POST',
     data: params,
     headers: {
       'content-type': 'application/x-www-form-urlencoded'
@@ -77,7 +91,7 @@ export function login (arg) {
 export function logout () {
   message.destroy()
   notification.destroy()
-  return api('logout')
+  return postAPI('logout')
 }
 
 export function oauthlogin (arg) {
@@ -86,7 +100,7 @@ export function oauthlogin (arg) {
   }
 
   // Logout before login is called to purge any duplicate sessionkey cookies
-  api('logout')
+  postAPI('logout')
 
   const params = new URLSearchParams()
   params.append('command', 'oauthlogin')

ui/src/components/header/SamlDomainSwitcher.vue

@@ -51,7 +51,7 @@
 
 <script>
 import store from '@/store'
-import { api } from '@/api'
+import { postAPI } from '@/api'
 import _ from 'lodash'
 
 export default {
@@ -73,7 +73,7 @@ export default {
       const samlAccounts = []
       const getNextPage = () => {
         this.loading = true
-        api('listAndSwitchSamlAccount', { details: 'min', page: page, pageSize: 500 }).then(json => {
+        postAPI('listAndSwitchSamlAccount', { details: 'min', page: page, pageSize: 500 }).then(json => {
           if (json && json.listandswitchsamlaccountresponse && json.listandswitchsamlaccountresponse.samluseraccount) {
             samlAccounts.push(...json.listandswitchsamlaccountresponse.samluseraccount)
           }
@@ -102,7 +102,7 @@ export default {
     },
     changeAccount (index) {
       const account = this.samlAccounts[index]
-      api('listAndSwitchSamlAccount', {}, 'POST', {
+      postAPI('listAndSwitchSamlAccount', {
         userid: account.userId,
         domainid: account.domainId
       }).then(response => {