From 4f00efb6aa28704ae6c255903b66df7c38edd8c5 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 18 May 2022 08:59:08 +0200 Subject: [PATCH 1/6] login/-out constants --- .../apache/cloudstack/api/ApiConstants.java | 2 + .../auth/DefaultLoginAPIAuthenticatorCmd.java | 54 +++++++++++-------- .../DefaultLogoutAPIAuthenticatorCmd.java | 3 +- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index 49d035610a67..bc293837c566 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -905,6 +905,8 @@ public class ApiConstants { public static final String ADMINS_ONLY = "adminsonly"; public static final String ANNOTATION_FILTER = "annotationfilter"; + public static final String LOGIN = "login"; + public static final String LOGOUT = "logout"; public enum BootType { UEFI, BIOS; diff --git a/server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java b/server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java index bb488b0bd21f..7fb9d2ef17b9 100644 --- a/server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java +++ b/server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java @@ -16,6 +16,7 @@ // under the License. package com.cloud.api.auth; +import com.cloud.api.ApiServlet; import com.cloud.domain.Domain; import com.cloud.user.User; import com.cloud.user.UserAccount; @@ -34,6 +35,7 @@ import org.apache.cloudstack.api.auth.PluggableAPIAuthenticator; import org.apache.cloudstack.api.response.LoginCmdResponse; import org.apache.log4j.Logger; +import org.jetbrains.annotations.Nullable; import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; @@ -43,7 +45,7 @@ import java.util.Map; import java.net.InetAddress; -@APICommand(name = "login", description = "Logs a user into the CloudStack. A successful login attempt will generate a JSESSIONID cookie value that can be passed in subsequent Query command calls until the \"logout\" command has been issued or the session has expired.", requestHasSensitiveInfo = true, responseObject = LoginCmdResponse.class, entityType = {}) +@APICommand(name = ApiConstants.LOGIN, description = "Logs a user into the CloudStack. A successful login attempt will generate a JSESSIONID cookie value that can be passed in subsequent Query command calls until the \"logout\" command has been issued or the session has expired.", requestHasSensitiveInfo = true, responseObject = LoginCmdResponse.class, entityType = {}) public class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthenticator { public static final Logger s_logger = Logger.getLogger(DefaultLoginAPIAuthenticatorCmd.class.getName()); @@ -79,7 +81,7 @@ public String getPassword() { return password; } - public String getDomain() { + public String getDomainName() { return domain; } @@ -141,19 +143,7 @@ public String authenticate(String command, Map params, HttpSes } String domain = null; - if (domainName != null) { - domain = domainName[0]; - auditTrailSb.append(" domain=" + domain); - if (domain != null) { - // ensure domain starts with '/' and ends with '/' - if (!domain.endsWith("/")) { - domain += '/'; - } - if (!domain.startsWith("/")) { - domain = "/" + domain; - } - } - } + domain = getDomainName(auditTrailSb, domainName, domain); String serializedResponse = null; if (username != null) { @@ -172,22 +162,40 @@ public String authenticate(String command, Map params, HttpSes return ApiResponseSerializer.toSerializedString(_apiServer.loginUser(session, username[0], pwd, domainId, domain, remoteAddress, params), responseType); } catch (final CloudAuthenticationException ex) { + ApiServlet.invalidateHttpSession(session, "fall through to API key,"); // TODO: fall through to API key, or just fail here w/ auth error? (HTTP 401) - try { - session.invalidate(); - } catch (final IllegalStateException ise) { + String msg = String.format("%s", ex.getMessage() != null ? + ex.getMessage() : + "failed to authenticate user, check if username/password are correct"); + auditTrailSb.append(" " + ApiErrorCode.ACCOUNT_ERROR + " " + msg); + serializedResponse = _apiServer.getSerializedApiError(ApiErrorCode.ACCOUNT_ERROR.getHttpCode(), msg, params, responseType); + if (s_logger.isTraceEnabled()) { + s_logger.trace(msg); } - auditTrailSb.append(" " + ApiErrorCode.ACCOUNT_ERROR + " " + ex.getMessage() != null ? ex.getMessage() - : "failed to authenticate user, check if username/password are correct"); - serializedResponse = - _apiServer.getSerializedApiError(ApiErrorCode.ACCOUNT_ERROR.getHttpCode(), ex.getMessage() != null ? ex.getMessage() - : "failed to authenticate user, check if username/password are correct", params, responseType); } } // We should not reach here and if we do we throw an exception throw new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, serializedResponse); } + @Nullable + private String getDomainName(StringBuilder auditTrailSb, String[] domainName, String domain) { + if (domainName != null) { + domain = domainName[0]; + auditTrailSb.append(" domain=" + domain); + if (domain != null) { + // ensure domain starts with '/' and ends with '/' + if (!domain.endsWith("/")) { + domain += '/'; + } + if (!domain.startsWith("/")) { + domain = "/" + domain; + } + } + } + return domain; + } + @Override public APIAuthenticationType getAPIType() { return APIAuthenticationType.LOGIN_API; diff --git a/server/src/main/java/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java b/server/src/main/java/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java index b4b59ef00783..71fd955f3f33 100644 --- a/server/src/main/java/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java +++ b/server/src/main/java/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java @@ -19,6 +19,7 @@ import com.cloud.api.response.ApiResponseSerializer; import com.cloud.user.Account; import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.ServerApiException; @@ -35,7 +36,7 @@ import java.util.Map; import java.net.InetAddress; -@APICommand(name = "logout", description = "Logs out the user", responseObject = LogoutCmdResponse.class, entityType = {}) +@APICommand(name = ApiConstants.LOGOUT, description = "Logs out the user", responseObject = LogoutCmdResponse.class, entityType = {}) public class DefaultLogoutAPIAuthenticatorCmd extends BaseCmd implements APIAuthenticator { public static final Logger s_logger = Logger.getLogger(DefaultLogoutAPIAuthenticatorCmd.class.getName()); From 2379967520b54a70b90cdeab3655191456a290bd Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 16 May 2022 16:57:50 +0200 Subject: [PATCH 2/6] no request listener --- .../apache/cloudstack/api/ApiConstants.java | 1 + .../cloudstack/api/command/ListIdpsCmd.java | 7 +- .../main/java/com/cloud/api/ApiServlet.java | 157 +++++++++++------- .../com/cloud/api/ApiSessionListener.java | 40 ++--- 4 files changed, 117 insertions(+), 88 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java index bc293837c566..55002f70b1b2 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiConstants.java @@ -907,6 +907,7 @@ public class ApiConstants { public static final String ANNOTATION_FILTER = "annotationfilter"; public static final String LOGIN = "login"; public static final String LOGOUT = "logout"; + public static final String LIST_IDPS = "listIdps"; public enum BootType { UEFI, BIOS; diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListIdpsCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListIdpsCmd.java index 67a297000697..545b17eeb978 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListIdpsCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListIdpsCmd.java @@ -19,6 +19,7 @@ import com.cloud.api.response.ApiResponseSerializer; import com.cloud.user.Account; import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.ApiServerService; import org.apache.cloudstack.api.BaseCmd; @@ -41,10 +42,10 @@ import java.util.List; import java.util.Map; -@APICommand(name = "listIdps", description = "Returns list of discovered SAML Identity Providers", responseObject = IdpResponse.class, entityType = {}) +@APICommand(name = ApiConstants.LIST_IDPS, description = "Returns list of discovered SAML Identity Providers", responseObject = IdpResponse.class, entityType = {}) public class ListIdpsCmd extends BaseCmd implements APIAuthenticator { public static final Logger s_logger = Logger.getLogger(ListIdpsCmd.class.getName()); - private static final String s_name = "listidpsresponse"; + public static final String APINAME = ApiConstants.LIST_IDPS; @Inject ApiServerService _apiServer; @@ -57,7 +58,7 @@ public class ListIdpsCmd extends BaseCmd implements APIAuthenticator { @Override public String getCommandName() { - return s_name; + return APINAME.toLowerCase() + RESPONSE_SUFFIX; } @Override diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index 09195e0fe227..30f451b0c61b 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -98,6 +98,14 @@ protected void doPost(final HttpServletRequest req, final HttpServletResponse re processRequest(req, resp); } + /** + * For HTTP GET requests, it seems that HttpServletRequest.getParameterMap() actually tries + * to unwrap URL encoded content from ISO-9959-1. + * After failed in using setCharacterEncoding() to control it, end up with following hacking: + * for all GET requests, we will override it with our-own way of UTF-8 based URL decoding. + * @param req request containing parameters + * @param params output of "our" map of parameters/values + */ void utf8Fixup(final HttpServletRequest req, final Map params) { if (req.getQueryString() == null) { return; @@ -171,10 +179,6 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp checkSingleQueryParameterValue(reqParams); params.putAll(reqParams); - // For HTTP GET requests, it seems that HttpServletRequest.getParameterMap() actually tries - // to unwrap URL encoded content from ISO-9959-1. - // After failed in using setCharacterEncoding() to control it, end up with following hacking: - // for all GET requests, we will override it with our-own way of UTF-8 based URL decoding. utf8Fixup(req, params); // logging the request start and end in management log for easy debugging @@ -186,22 +190,28 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } try { - - if (HttpUtils.RESPONSE_TYPE_JSON.equalsIgnoreCase(responseType)) { - resp.setContentType(ApiServer.JSONcontentType.value()); - } else if (HttpUtils.RESPONSE_TYPE_XML.equalsIgnoreCase(responseType)){ - resp.setContentType(HttpUtils.XML_CONTENT_TYPE); - } + resp.setContentType(HttpUtils.XML_CONTENT_TYPE); HttpSession session = req.getSession(false); + if (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("session found: %s", session)); + } final Object[] responseTypeParam = params.get(ApiConstants.RESPONSE); if (responseTypeParam != null) { responseType = (String)responseTypeParam[0]; } final Object[] commandObj = params.get(ApiConstants.COMMAND); - if (commandObj != null) { - final String command = (String) commandObj[0]; + 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 (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("command %s processing for user \"%s\"", command, username)); + } + + List loglessAPIs = Arrays.asList(ApiConstants.LOGOUT, ApiConstants.LIST_IDPS, + "cloudianIsEnabled", "listLdapConfigurations", "listZones", "listApis", "listCapabilities" ); + if (command != null) { APIAuthenticator apiAuthenticator = authManager.getAPIAuthenticator(command); if (apiAuthenticator != null) { @@ -213,10 +223,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGIN_API) { if (session != null) { - try { - session.invalidate(); - } catch (final IllegalStateException ise) { - } + invalidateHttpSession(session, "invalidating session for login call"); } session = req.getSession(true); @@ -229,6 +236,10 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } try { + if (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("apiAuthenticator.authenticate(%s, %s, %s, %s, %s, %s, %s,%s)", + command, params, session, remoteAddress, responseType, auditTrailSb, req, resp)); + } responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, req, resp); if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) { resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY))); @@ -251,10 +262,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp if (userId != null) { apiServer.logoutUser(userId); } - try { - session.invalidate(); - } catch (final IllegalStateException ignored) { - } + invalidateHttpSession(session, "invalidating session after logout call"); } final Cookie[] cookies = req.getCookies(); if (cookies != null) { @@ -268,8 +276,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp HttpUtils.writeHttpResponse(resp, responseString, httpResponseCode, responseType, ApiServer.JSONcontentType.value()); return; } + } else { + if (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("no command available")); + } } - auditTrailSb.append(cleanQueryString); final boolean isNew = ((session == null) ? true : session.isNew()); @@ -278,52 +289,31 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp // if a API key exists Long userId = null; + if (isNew && s_logger.isTraceEnabled()) { + s_logger.trace(String.format("new session: %s", session)); + } if (!isNew) { userId = (Long)session.getAttribute("userid"); final String account = (String) session.getAttribute("account"); final Object accountObj = session.getAttribute("accountobj"); - if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY)) { - try { - session.invalidate(); - } catch (final IllegalStateException ise) { - } - auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials"); - final String serializedResponse = - apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "unable to verify user credentials", params, responseType); - HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, ApiServer.JSONcontentType.value()); - return; - } - - // Do a sanity check here to make sure the user hasn't already been deleted - if ((userId != null) && (account != null) && (accountObj != null) && apiServer.verifyUser(userId)) { - final String[] command = (String[])params.get(ApiConstants.COMMAND); - if (command == null) { - s_logger.info("missing command, ignoring request..."); - auditTrailSb.append(" " + HttpServletResponse.SC_BAD_REQUEST + " " + "no command specified"); - final String serializedResponse = apiServer.getSerializedApiError(HttpServletResponse.SC_BAD_REQUEST, "no command specified", params, responseType); - HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_BAD_REQUEST, responseType, ApiServer.JSONcontentType.value()); - return; - } - final User user = entityMgr.findById(User.class, userId); - CallContext.register(user, (Account)accountObj); + if (account != null) { + if (invalidateHttpSesseionIfNeeded(req, resp, auditTrailSb, responseType, params, session, account)) return; } else { - // Invalidate the session to ensure we won't allow a request across management server - // restarts if the userId was serialized to the stored session - try { - session.invalidate(); - } catch (final IllegalStateException ise) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("no account, this request will be validated through apikey(%s)/signature"); } + } - auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials"); - final String serializedResponse = - apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "unable to verify user credentials", params, responseType); - HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, ApiServer.JSONcontentType.value()); + if (! requestChecksoutAsSane(resp, auditTrailSb, responseType, params, session, command, userId, account, accountObj)) return; - } } else { CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount()); } setProjectContext(params); + if (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("verifying request for user %s from %s with params %s", + userId, remoteAddress.getHostAddress(), org.apache.commons.lang3.StringUtils.join(params))); + } if (apiServer.verifyRequest(params, userId, remoteAddress)) { auditTrailSb.insert(0, "(userId=" + CallContext.current().getCallingUserId() + " accountId=" + CallContext.current().getCallingAccount().getId() + " sessionId=" + (session != null ? session.getId() : null) + ")"); @@ -335,10 +325,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp HttpUtils.writeHttpResponse(resp, response != null ? response : "", HttpServletResponse.SC_OK, responseType, ApiServer.JSONcontentType.value()); } else { if (session != null) { - try { - session.invalidate(); - } catch (final IllegalStateException ise) { - } + invalidateHttpSession(session, String.format("request verification failed for %s from %s", userId, remoteAddress.getHostAddress())); } auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials and/or request signature"); @@ -366,6 +353,58 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } } + /** + * Do a sanity check here to make sure the user hasn't already been deleted + */ + private boolean requestChecksoutAsSane(HttpServletResponse resp, StringBuilder auditTrailSb, String responseType, Map params, HttpSession session, String command, Long userId, String account, Object accountObj) { + if ((userId != null) && (account != null) && (accountObj != null) && apiServer.verifyUser(userId)) { + if (command == null) { + s_logger.info("missing command, ignoring request..."); + auditTrailSb.append(" " + HttpServletResponse.SC_BAD_REQUEST + " " + "no command specified"); + final String serializedResponse = apiServer.getSerializedApiError(HttpServletResponse.SC_BAD_REQUEST, "no command specified", params, responseType); + HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_BAD_REQUEST, responseType, ApiServer.JSONcontentType.value()); + return true; + } + final User user = entityMgr.findById(User.class, userId); + CallContext.register(user, (Account) accountObj); + } else { + invalidateHttpSession(session, "Invalidate the session to ensure we won't allow a request across management server restarts if the userId was serialized to the stored session"); + + auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials"); + final String serializedResponse = + apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "unable to verify user credentials", params, responseType); + HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, ApiServer.JSONcontentType.value()); + return false; + } + return true; + } + + private boolean invalidateHttpSesseionIfNeeded(HttpServletRequest req, HttpServletResponse resp, StringBuilder auditTrailSb, String responseType, Map params, HttpSession session, String account) { + if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY)) { + String msg = String.format("invalidating session %s for account %s", session.getId(), account); + invalidateHttpSession(session, msg); + auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials"); + final String serializedResponse = + apiServer.getSerializedApiError(HttpServletResponse.SC_UNAUTHORIZED, "unable to verify user credentials", params, responseType); + HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_UNAUTHORIZED, responseType, ApiServer.JSONcontentType.value()); + return true; + } + return false; + } + + public static void invalidateHttpSession(HttpSession session, String msg) { + try { + if (s_logger.isTraceEnabled()) { + s_logger.trace(msg); + } + session.invalidate(); + } catch (final IllegalStateException ise) { + if (s_logger.isTraceEnabled()) { + s_logger.trace(String.format("failed to invalidate session %s", session.getId())); + } + } + } + private void setProjectContext(Map requestParameters) { final String[] command = (String[])requestParameters.get(ApiConstants.COMMAND); if (command == null) { diff --git a/server/src/main/java/com/cloud/api/ApiSessionListener.java b/server/src/main/java/com/cloud/api/ApiSessionListener.java index 7f99bf596d69..a36fcc6c0ec3 100644 --- a/server/src/main/java/com/cloud/api/ApiSessionListener.java +++ b/server/src/main/java/com/cloud/api/ApiSessionListener.java @@ -16,10 +16,7 @@ // under the License. package com.cloud.api; -import javax.servlet.ServletRequestEvent; -import javax.servlet.ServletRequestListener; import javax.servlet.annotation.WebListener; -import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; import javax.servlet.http.HttpSessionEvent; import javax.servlet.http.HttpSessionListener; @@ -29,9 +26,8 @@ import java.util.concurrent.ConcurrentHashMap; @WebListener -public class ApiSessionListener implements HttpSessionListener, ServletRequestListener { +public class ApiSessionListener implements HttpSessionListener { public static final Logger LOGGER = Logger.getLogger(ApiSessionListener.class.getName()); - private static final String ATTRIBUTE_NAME = "SessionCounter"; private static Map sessions = new ConcurrentHashMap<>(); /** @@ -47,37 +43,29 @@ public static long getSessionCount() { public static long getNumberOfSessions() { return sessions.size(); } + public void sessionCreated(HttpSessionEvent event) { - LOGGER.debug("Session created by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString()); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Session created by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString()); + } synchronized (this) { HttpSession session = event.getSession(); sessions.put(session, event.getSource()); } - LOGGER.debug("Sessions count: " + getSessionCount()); + if (LOGGER.isTraceEnabled()) { + LOGGER.trace("Sessions count: " + getSessionCount()); + } } + public void sessionDestroyed(HttpSessionEvent event) { - LOGGER.debug("Session destroyed by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString()); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Session destroyed by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString()); + } synchronized (this) { sessions.remove(event.getSession()); } - LOGGER.debug("Sessions count: " + getSessionCount()); - } - - @Override - public void requestDestroyed(ServletRequestEvent event) { - LOGGER.debug("request destroyed"); - } - - @Override - public void requestInitialized(ServletRequestEvent event) { - LOGGER.debug("request initialized"); - HttpServletRequest request = (HttpServletRequest) event.getServletRequest(); - HttpSession session = request.getSession(); - if (session.isNew()) { - synchronized (this) { - // replace the source object for the address - sessions.put(session, request.getRemoteAddr()); - } + if (LOGGER.isTraceEnabled()) { + LOGGER.trace("Sessions count: " + getSessionCount()); } } } From 97f5aae106fd7915b8ab4db32ac320e2082a807d Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 19 May 2022 11:00:24 +0200 Subject: [PATCH 3/6] store session as value, using id as key --- server/src/main/java/com/cloud/api/ApiSessionListener.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiSessionListener.java b/server/src/main/java/com/cloud/api/ApiSessionListener.java index a36fcc6c0ec3..56da456b8e22 100644 --- a/server/src/main/java/com/cloud/api/ApiSessionListener.java +++ b/server/src/main/java/com/cloud/api/ApiSessionListener.java @@ -28,7 +28,7 @@ @WebListener public class ApiSessionListener implements HttpSessionListener { public static final Logger LOGGER = Logger.getLogger(ApiSessionListener.class.getName()); - private static Map sessions = new ConcurrentHashMap<>(); + private static Map sessions = new ConcurrentHashMap<>(); /** * @return the internal adminstered session count @@ -50,7 +50,7 @@ public void sessionCreated(HttpSessionEvent event) { } synchronized (this) { HttpSession session = event.getSession(); - sessions.put(session, event.getSource()); + sessions.put(session.getId(), event.getSession()); } if (LOGGER.isTraceEnabled()) { LOGGER.trace("Sessions count: " + getSessionCount()); @@ -62,7 +62,7 @@ public void sessionDestroyed(HttpSessionEvent event) { LOGGER.debug("Session destroyed by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString()); } synchronized (this) { - sessions.remove(event.getSession()); + sessions.remove(event.getSession().getId()); } if (LOGGER.isTraceEnabled()) { LOGGER.trace("Sessions count: " + getSessionCount()); From 290b914353825b1a254b9123ba8ed6ad5076975e Mon Sep 17 00:00:00 2001 From: dahn Date: Thu, 19 May 2022 12:43:08 +0200 Subject: [PATCH 4/6] Apply suggestions from sonarcloud.io code review three instances of unsafe parameters to logging --- server/src/main/java/com/cloud/api/ApiServlet.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index 30f451b0c61b..99ddd4c45ce0 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -206,7 +206,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp final Object[] userObj = params.get(ApiConstants.USERNAME); String username = userObj == null ? null : (String)userObj[0]; if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("command %s processing for user \"%s\"", command, username)); + s_logger.trace(String.format("command %s processing for user \"%s\"", command, username.replaceAll("[\n\r\t]", "_"))); } List loglessAPIs = Arrays.asList(ApiConstants.LOGOUT, ApiConstants.LIST_IDPS, @@ -237,8 +237,8 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp try { if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("apiAuthenticator.authenticate(%s, %s, %s, %s, %s, %s, %s,%s)", - command, params, session, remoteAddress, responseType, auditTrailSb, req, resp)); + s_logger.trace(String.format("apiAuthenticator.authenticate(%s, params[%d], %s, %s, %s, %s, %s,%s)", + command, params.size(), session, remoteAddress, responseType, auditTrailSb, req, resp)); } responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, req, resp); if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) { @@ -311,8 +311,8 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } setProjectContext(params); if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("verifying request for user %s from %s with params %s", - userId, remoteAddress.getHostAddress(), org.apache.commons.lang3.StringUtils.join(params))); + s_logger.trace(String.format("verifying request for user %s from %s with %d parameters", + userId, remoteAddress.getHostAddress(), params.size())); } if (apiServer.verifyRequest(params, userId, remoteAddress)) { auditTrailSb.insert(0, "(userId=" + CallContext.current().getCallingUserId() + " accountId=" + CallContext.current().getCallingAccount().getId() + From f90ae8a0d23c3a71aab09981d6015fdb77fb9a2e Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 19 May 2022 14:57:15 +0200 Subject: [PATCH 5/6] new sonar issues --- .../main/java/com/cloud/api/ApiServlet.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index 99ddd4c45ce0..ab2fd6afe135 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -44,6 +44,7 @@ import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.managed.context.ManagedContext; import org.apache.log4j.Logger; +import org.jetbrains.annotations.Nullable; import org.springframework.stereotype.Component; import org.springframework.web.context.support.SpringBeanAutowiringSupport; @@ -66,6 +67,7 @@ public class ApiServlet extends HttpServlet { private final static List s_clientAddressHeaders = Collections .unmodifiableList(Arrays.asList("X-Forwarded-For", "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr")); + public static final String REPLACEMENT = "_"; @Inject ApiServerService apiServer; @@ -79,6 +81,7 @@ public class ApiServlet extends HttpServlet { APIAuthenticationManager authManager; @Inject private ProjectDao projectDao; + private final static String LOG_REPLACEMENTS = "[\n\r\t]"; public ApiServlet() { } @@ -206,11 +209,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp final Object[] userObj = params.get(ApiConstants.USERNAME); String username = userObj == null ? null : (String)userObj[0]; if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("command %s processing for user \"%s\"", command, username.replaceAll("[\n\r\t]", "_"))); + s_logger.trace(String.format("command %s processing for user \"%s\"", + saveLogString(command), + saveLogString(username))); } - List loglessAPIs = Arrays.asList(ApiConstants.LOGOUT, ApiConstants.LIST_IDPS, - "cloudianIsEnabled", "listLdapConfigurations", "listZones", "listApis", "listCapabilities" ); if (command != null) { APIAuthenticator apiAuthenticator = authManager.getAPIAuthenticator(command); @@ -238,7 +241,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp try { if (s_logger.isTraceEnabled()) { s_logger.trace(String.format("apiAuthenticator.authenticate(%s, params[%d], %s, %s, %s, %s, %s,%s)", - command, params.size(), session, remoteAddress, responseType, auditTrailSb, req, resp)); + saveLogString(command), params.size(), session.getId(), remoteAddress.getHostAddress(), saveLogString(responseType), "auditTrailSb", "req", "resp")); } responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, req, resp); if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) { @@ -277,9 +280,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp return; } } else { - if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("no command available")); - } + s_logger.trace("no command available"); } auditTrailSb.append(cleanQueryString); final boolean isNew = ((session == null) ? true : session.isNew()); @@ -353,6 +354,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } } + @Nullable + private Object saveLogString(String stringToLog) { + return stringToLog == null ? null : stringToLog.replace(LOG_REPLACEMENTS, REPLACEMENT); + } + /** * Do a sanity check here to make sure the user hasn't already been deleted */ From cdff5f9f6217b42cffcd9cb9a7a17c4d62b3b36d Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 19 May 2022 16:45:30 +0200 Subject: [PATCH 6/6] sonar issues --- server/src/main/java/com/cloud/api/ApiServlet.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index ab2fd6afe135..4bdf31defaf4 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -67,7 +67,8 @@ public class ApiServlet extends HttpServlet { private final static List s_clientAddressHeaders = Collections .unmodifiableList(Arrays.asList("X-Forwarded-For", "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr")); - public static final String REPLACEMENT = "_"; + private static final String REPLACEMENT = "_"; + private static final String LOG_REPLACEMENTS = "[\n\r\t]"; @Inject ApiServerService apiServer; @@ -81,7 +82,6 @@ public class ApiServlet extends HttpServlet { APIAuthenticationManager authManager; @Inject private ProjectDao projectDao; - private final static String LOG_REPLACEMENTS = "[\n\r\t]"; public ApiServlet() { } @@ -209,9 +209,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp final Object[] userObj = params.get(ApiConstants.USERNAME); String username = userObj == null ? null : (String)userObj[0]; if (s_logger.isTraceEnabled()) { + String logCommand = saveLogString(command); + String logName = saveLogString(username); s_logger.trace(String.format("command %s processing for user \"%s\"", - saveLogString(command), - saveLogString(username))); + logCommand, + logName)); } if (command != null) { @@ -355,7 +357,7 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp } @Nullable - private Object saveLogString(String stringToLog) { + private String saveLogString(String stringToLog) { return stringToLog == null ? null : stringToLog.replace(LOG_REPLACEMENTS, REPLACEMENT); }