Skip to content

remove request listener to prevent untimely session invalidation #6393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 24, 2022
Merged

remove request listener to prevent untimely session invalidation #6393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f00efb
login/-out constants
DaanHoogland May 18, 2022
2379967
no request listener
DaanHoogland May 16, 2022
97f5aae
store session as value, using id as key
DaanHoogland May 19, 2022
290b914
Apply suggestions from sonarcloud.io code review
DaanHoogland May 19, 2022
f90ae8a
new sonar issues
DaanHoogland May 19, 2022
cdff5f9
sonar issues
DaanHoogland May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 4 additions & 3 deletions ...ser-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListIdpsCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -57,7 +58,7 @@ public class ListIdpsCmd extends BaseCmd implements APIAuthenticator {

@Override
public String getCommandName() {
return s_name;
return APINAME.toLowerCase() + RESPONSE_SUFFIX;
}

@Override
Expand Down
165 changes: 106 additions & 59 deletions server/src/main/java/com/cloud/api/ApiServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -98,6 +101,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<String, Object[]> params) {
if (req.getQueryString() == null) {
return;
Expand Down Expand Up @@ -171,10 +182,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
Expand All @@ -186,22 +193,30 @@ 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);
Copy link
Member

@rohityadavcloud rohityadavcloud May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland should we force xml?

Copy link
Contributor Author

@DaanHoogland DaanHoogland May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn´t matter in the existing code as line 179 (former version 168) https://github.com/apache/cloudstack/pull/6393/files#diff-cc92beff292fdad7835437d622159db6e1ae90d0ebac7fb4ae73c813faa8896eR179 hard-codes the reponseType to HttpUtils.RESPONSE_TYPE_XML. The second branch would always be executed.


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) {
Expand All @@ -213,10 +228,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);

Expand All @@ -229,6 +241,10 @@ 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)",
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)));
Expand All @@ -251,10 +267,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) {
Expand All @@ -268,8 +281,9 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
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());

Expand All @@ -278,52 +292,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 %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) + ")");
Expand All @@ -335,10 +328,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");
Expand Down Expand Up @@ -366,6 +356,63 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
}
}

@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) {
Expand Down
Loading