From 5571b76cdad7ebf852857850bbdc177a4d083f35 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Fri, 22 May 2015 10:11:15 +0100 Subject: [PATCH] CLOUDSTACK-8505: Don't allow non-POST requests for default login API We add a new contract to pass Http request to authentication plugin system. In the default login API, we disallow non-POST requests. (cherry picked from commit 9e9b231672e934292f9940d1363039a553fc7ad9) Signed-off-by: Rohit Yadav Conflicts: plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java server/src/com/cloud/api/ApiServlet.java (cherry picked from commit 8b9b4832f483797c8ab123bf27262634430efcb9) Signed-off-by: Rohit Yadav --- .../org/apache/cloudstack/api/auth/APIAuthenticator.java | 3 ++- .../api/command/GetServiceProviderMetaDataCmd.java | 3 ++- .../api/command/SAML2LoginAPIAuthenticatorCmd.java | 3 ++- .../api/command/SAML2LogoutAPIAuthenticatorCmd.java | 3 ++- .../cloudstack/GetServiceProviderMetaDataCmdTest.java | 6 +++++- .../api/command/SAML2LoginAPIAuthenticatorCmdTest.java | 8 ++++++-- .../api/command/SAML2LogoutAPIAuthenticatorCmdTest.java | 6 +++++- server/src/com/cloud/api/ApiServlet.java | 2 +- .../cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java | 8 ++++++-- .../cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java | 3 ++- server/test/com/cloud/api/ApiServletTest.java | 6 +++--- 11 files changed, 36 insertions(+), 15 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/auth/APIAuthenticator.java b/api/src/org/apache/cloudstack/api/auth/APIAuthenticator.java index 67fa1d8816e..4139740e809 100644 --- a/api/src/org/apache/cloudstack/api/auth/APIAuthenticator.java +++ b/api/src/org/apache/cloudstack/api/auth/APIAuthenticator.java @@ -18,6 +18,7 @@ package org.apache.cloudstack.api.auth; import org.apache.cloudstack.api.ServerApiException; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.util.List; @@ -36,7 +37,7 @@ import java.util.Map; public interface APIAuthenticator { public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, - StringBuilder auditTrailSb, final HttpServletResponse resp) throws ServerApiException; + StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException; public APIAuthenticationType getAPIType(); diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java index 1f69e599e90..ffaad7af754 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/GetServiceProviderMetaDataCmd.java @@ -72,6 +72,7 @@ import org.opensaml.xml.security.x509.X509KeyInfoGeneratorFactory; import org.w3c.dom.Document; import javax.inject.Inject; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import javax.xml.parsers.DocumentBuilder; @@ -119,7 +120,7 @@ public class GetServiceProviderMetaDataCmd extends BaseCmd implements APIAuthent } @Override - public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, HttpServletResponse resp) throws ServerApiException { + public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException { SAMLMetaDataResponse response = new SAMLMetaDataResponse(); response.setResponseName(getCommandName()); diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java index 8e824797005..d4940ecb83a 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java @@ -65,6 +65,7 @@ import org.xml.sax.SAXException; import javax.inject.Inject; import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import javax.xml.parsers.ParserConfigurationException; @@ -137,7 +138,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent } @Override - public String authenticate(final String command, final Map params, final HttpSession session, final String remoteAddress, final String responseType, final StringBuilder auditTrailSb, final HttpServletResponse resp) throws ServerApiException { + public String authenticate(final String command, final Map params, final HttpSession session, final String remoteAddress, final String responseType, final StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException { try { if (!params.containsKey(SAMLPluginConstants.SAML_RESPONSE) && !params.containsKey("SAMLart")) { String idpId = null; diff --git a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmd.java b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmd.java index b29214cd550..a012431b29b 100644 --- a/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmd.java +++ b/plugins/user-authenticators/saml2/src/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmd.java @@ -42,6 +42,7 @@ import org.opensaml.xml.io.UnmarshallingException; import org.xml.sax.SAXException; import javax.inject.Inject; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import javax.xml.parsers.ParserConfigurationException; @@ -81,7 +82,7 @@ public class SAML2LogoutAPIAuthenticatorCmd extends BaseCmd implements APIAuthen } @Override - public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletResponse resp) throws ServerApiException { + public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException { auditTrailSb.append("=== SAML SLO Logging out ==="); LogoutCmdResponse response = new LogoutCmdResponse(); response.setDescription("success"); diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java index c5d11c42e84..0b176712cd8 100644 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/GetServiceProviderMetaDataCmdTest.java @@ -33,6 +33,7 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.lang.reflect.Field; @@ -60,6 +61,9 @@ public class GetServiceProviderMetaDataCmdTest { @Mock HttpServletResponse resp; + @Mock + HttpServletRequest req; + @Test public void testAuthenticate() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException, CertificateParsingException, CertificateEncodingException, NoSuchAlgorithmException, InvalidKeyException, NoSuchProviderException, SignatureException { GetServiceProviderMetaDataCmd cmd = new GetServiceProviderMetaDataCmd(); @@ -87,7 +91,7 @@ public class GetServiceProviderMetaDataCmdTest { Mockito.when(samlAuthManager.getSPMetadata()).thenReturn(providerMetadata); - String result = cmd.authenticate("command", null, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), resp); + String result = cmd.authenticate("command", null, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); Assert.assertTrue(result.contains("md:EntityDescriptor")); } diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java index dd166282adb..c98c0b59e8e 100644 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java @@ -61,6 +61,7 @@ import org.opensaml.saml2.core.impl.StatusBuilder; import org.opensaml.saml2.core.impl.StatusCodeBuilder; import org.opensaml.saml2.core.impl.SubjectBuilder; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.lang.reflect.Field; @@ -96,6 +97,9 @@ public class SAML2LoginAPIAuthenticatorCmdTest { @Mock HttpServletResponse resp; + @Mock + HttpServletRequest req; + private Response buildMockResponse() throws Exception { Response samlMessage = new ResponseBuilder().buildObject(); samlMessage.setID("foo"); @@ -176,14 +180,14 @@ public class SAML2LoginAPIAuthenticatorCmdTest { Map params = new HashMap(); // SSO redirection test - cmd.authenticate("command", params, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), resp); + cmd.authenticate("command", params, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); Mockito.verify(resp, Mockito.times(1)).sendRedirect(Mockito.anyString()); // SSO SAMLResponse verification test, this should throw ServerApiException for auth failure params.put(SAMLPluginConstants.SAML_RESPONSE, new String[]{"Some String"}); Mockito.stub(cmd.processSAMLResponse(Mockito.anyString())).toReturn(buildMockResponse()); try { - cmd.authenticate("command", params, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), resp); + cmd.authenticate("command", params, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); } catch (ServerApiException ignored) { } Mockito.verify(userAccountDao, Mockito.times(0)).getUserAccount(Mockito.anyString(), Mockito.anyLong()); diff --git a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmdTest.java b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmdTest.java index 9e451c2ede0..eff4b296d65 100644 --- a/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmdTest.java +++ b/plugins/user-authenticators/saml2/test/org/apache/cloudstack/api/command/SAML2LogoutAPIAuthenticatorCmdTest.java @@ -31,6 +31,7 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.lang.reflect.Field; @@ -51,6 +52,9 @@ public class SAML2LogoutAPIAuthenticatorCmdTest { @Mock HttpServletResponse resp; + @Mock + HttpServletRequest req; + @Test public void testAuthenticate() throws Exception { SAML2LogoutAPIAuthenticatorCmd cmd = new SAML2LogoutAPIAuthenticatorCmd(); @@ -68,7 +72,7 @@ public class SAML2LogoutAPIAuthenticatorCmdTest { X509Certificate cert = SAMLUtils.generateRandomX509Certificate(SAMLUtils.generateRandomKeyPair()); Mockito.when(session.getAttribute(Mockito.anyString())).thenReturn(null); - cmd.authenticate("command", null, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), resp); + cmd.authenticate("command", null, session, "random", HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); Mockito.verify(resp, Mockito.times(1)).sendRedirect(Mockito.anyString()); Mockito.verify(session, Mockito.atLeastOnce()).getAttribute(Mockito.anyString()); } diff --git a/server/src/com/cloud/api/ApiServlet.java b/server/src/com/cloud/api/ApiServlet.java index d8ff3609354..d9e77bd3c63 100644 --- a/server/src/com/cloud/api/ApiServlet.java +++ b/server/src/com/cloud/api/ApiServlet.java @@ -189,7 +189,7 @@ public class ApiServlet extends HttpServlet { } try { - responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, 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))); } diff --git a/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java b/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java index fa23abd43e4..ae633a3ec18 100644 --- a/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java +++ b/server/src/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java @@ -33,6 +33,7 @@ import org.apache.cloudstack.api.response.LoginCmdResponse; import org.apache.log4j.Logger; import javax.inject.Inject; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.util.List; @@ -103,8 +104,11 @@ public class DefaultLoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthe } @Override - public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletResponse resp) throws ServerApiException { - + public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException { + // Disallow non POST requests + if (HTTPMethod.valueOf(req.getMethod()) != HTTPMethod.POST) { + throw new ServerApiException(ApiErrorCode.METHOD_NOT_ALLOWED, "Please use HTTP POST to authenticate using this API"); + } // FIXME: ported from ApiServlet, refactor and cleanup final String[] username = (String[])params.get(ApiConstants.USERNAME); final String[] password = (String[])params.get(ApiConstants.PASSWORD); diff --git a/server/src/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java b/server/src/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java index ee7936ad734..5d25ae88560 100644 --- a/server/src/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java +++ b/server/src/com/cloud/api/auth/DefaultLogoutAPIAuthenticatorCmd.java @@ -28,6 +28,7 @@ import org.apache.cloudstack.api.auth.PluggableAPIAuthenticator; import org.apache.cloudstack.api.response.LogoutCmdResponse; import org.apache.log4j.Logger; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import java.util.List; @@ -60,7 +61,7 @@ public class DefaultLogoutAPIAuthenticatorCmd extends BaseCmd implements APIAuth } @Override - public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletResponse resp) throws ServerApiException { + public String authenticate(String command, Map params, HttpSession session, String remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException { auditTrailSb.append("=== Logging out ==="); LogoutCmdResponse response = new LogoutCmdResponse(); response.setDescription("success"); diff --git a/server/test/com/cloud/api/ApiServletTest.java b/server/test/com/cloud/api/ApiServletTest.java index 1a9c13d3e74..492ab0e41d4 100644 --- a/server/test/com/cloud/api/ApiServletTest.java +++ b/server/test/com/cloud/api/ApiServletTest.java @@ -99,7 +99,7 @@ public class ApiServletTest { Mockito.when(authManager.getAPIAuthenticator(Mockito.anyString())).thenReturn(authenticator); Mockito.when(authenticator.authenticate(Mockito.anyString(), Mockito.anyMap(), Mockito.isA(HttpSession.class), - Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletResponse.class))).thenReturn("{\"loginresponse\":{}"); + Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class))).thenReturn("{\"loginresponse\":{}"); Field authManagerField = ApiServlet.class.getDeclaredField("_authManager"); authManagerField.setAccessible(true); @@ -210,7 +210,7 @@ public class ApiServletTest { Mockito.verify(authManager).getAPIAuthenticator("logout"); Mockito.verify(authenticator).authenticate(Mockito.anyString(), Mockito.anyMap(), Mockito.isA(HttpSession.class), - Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletResponse.class)); + Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class)); Mockito.verify(session).invalidate(); } @@ -232,6 +232,6 @@ public class ApiServletTest { Mockito.verify(authManager).getAPIAuthenticator("login"); Mockito.verify(authenticator).authenticate(Mockito.anyString(), Mockito.anyMap(), Mockito.isA(HttpSession.class), - Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletResponse.class)); + Mockito.anyString(), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class)); } }