remove request listener to prevent untimely session invalidation (#6393)

* login/-out constants

* no request listener

* store session as value, using id as key

* Apply suggestions from sonarcloud.io code review

three instances of unsafe parameters to logging

* new sonar issues

* sonar issues
This commit is contained in:
dahn 2022-05-24 15:00:06 +02:00 committed by GitHub
parent 96594aec28
commit c123c3fd2f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 165 additions and 117 deletions

View File

@ -905,6 +905,9 @@ 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 static final String LIST_IDPS = "listIdps";
public enum BootType {
UEFI, BIOS;

View File

@ -19,6 +19,7 @@ package org.apache.cloudstack.api.command;
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.ArrayList;
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

View File

@ -44,6 +44,7 @@ import org.apache.cloudstack.api.auth.APIAuthenticator;
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,8 @@ public class ApiServlet extends HttpServlet {
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]";
@Inject
ApiServerService apiServer;
@ -98,6 +101,14 @@ public class ApiServlet extends HttpServlet {
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<String, Object[]> params) {
if (req.getQueryString() == null) {
return;
@ -171,10 +182,6 @@ public class ApiServlet extends HttpServlet {
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 +193,30 @@ public class ApiServlet extends HttpServlet {
}
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()) {
String logCommand = saveLogString(command);
String logName = saveLogString(username);
s_logger.trace(String.format("command %s processing for user \"%s\"",
logCommand,
logName));
}
if (command != null) {
APIAuthenticator apiAuthenticator = authManager.getAPIAuthenticator(command);
if (apiAuthenticator != null) {
@ -213,10 +228,7 @@ public class ApiServlet extends HttpServlet {
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 +241,10 @@ public class ApiServlet extends HttpServlet {
}
try {
if (s_logger.isTraceEnabled()) {
s_logger.trace(String.format("apiAuthenticator.authenticate(%s, params[%d], %s, %s, %s, %s, %s,%s)",
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) {
resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY)));
@ -251,10 +267,7 @@ public class ApiServlet extends HttpServlet {
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 +281,9 @@ public class ApiServlet extends HttpServlet {
HttpUtils.writeHttpResponse(resp, responseString, httpResponseCode, responseType, ApiServer.JSONcontentType.value());
return;
}
} else {
s_logger.trace("no command available");
}
auditTrailSb.append(cleanQueryString);
final boolean isNew = ((session == null) ? true : session.isNew());
@ -278,52 +292,31 @@ public class ApiServlet extends HttpServlet {
// 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());
return;
}
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 %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() +
" sessionId=" + (session != null ? session.getId() : null) + ")");
@ -335,10 +328,7 @@ public class ApiServlet extends HttpServlet {
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 +356,63 @@ public class ApiServlet extends HttpServlet {
}
}
@Nullable
private String 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
*/
private boolean requestChecksoutAsSane(HttpServletResponse resp, StringBuilder auditTrailSb, String responseType, Map<String, Object[]> 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<String, Object[]> 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<String, Object[]> requestParameters) {
final String[] command = (String[])requestParameters.get(ApiConstants.COMMAND);
if (command == null) {

View File

@ -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,10 +26,9 @@ import java.util.Map;
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<HttpSession, Object> sessions = new ConcurrentHashMap<>();
private static Map<String, HttpSession> sessions = new ConcurrentHashMap<>();
/**
* @return the internal adminstered session count
@ -47,37 +43,29 @@ public class ApiSessionListener implements HttpSessionListener, ServletRequestLi
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());
sessions.put(session.getId(), event.getSession());
}
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Sessions count: " + getSessionCount());
}
LOGGER.debug("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());
synchronized (this) {
sessions.remove(event.getSession());
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Session destroyed by Id : " + event.getSession().getId() + " , session: " + event.getSession().toString() + " , source: " + event.getSource().toString() + " , event: " + event.toString());
}
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());
}
synchronized (this) {
sessions.remove(event.getSession().getId());
}
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Sessions count: " + getSessionCount());
}
}
}

View File

@ -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.APIAuthenticator;
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.List;
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 class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthe
return password;
}
public String getDomain() {
public String getDomainName() {
return domain;
}
@ -141,19 +143,7 @@ public class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthe
}
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 class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthe
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;

View File

@ -19,6 +19,7 @@ package com.cloud.api.auth;
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.List;
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());