Return-Path: X-Original-To: apmail-cloudstack-commits-archive@www.apache.org Delivered-To: apmail-cloudstack-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 033F7C1B6 for ; Sun, 21 Jul 2013 01:05:39 +0000 (UTC) Received: (qmail 85165 invoked by uid 500); 21 Jul 2013 01:05:38 -0000 Delivered-To: apmail-cloudstack-commits-archive@cloudstack.apache.org Received: (qmail 85147 invoked by uid 500); 21 Jul 2013 01:05:38 -0000 Mailing-List: contact commits-help@cloudstack.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cloudstack.apache.org Delivered-To: mailing list commits@cloudstack.apache.org Received: (qmail 85140 invoked by uid 99); 21 Jul 2013 01:05:38 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 21 Jul 2013 01:05:38 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 354438AFE06; Sun, 21 Jul 2013 01:05:38 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: mchen@apache.org To: commits@cloudstack.apache.org Message-Id: <785c0aaf657e43e3871f923825e773e7@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: git commit: updated refs/heads/master to d423a75 Date: Sun, 21 Jul 2013 01:05:38 +0000 (UTC) Updated Branches: refs/heads/master 98dd501a8 -> d423a755f CLOUDSTACK-3274: API Refactoring: secretkey and accesskey of the backing store is found in plaintext in the logs. Conflicts: server/src/com/cloud/api/ApiServer.java server/src/com/cloud/api/ApiServlet.java Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/d423a755 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/d423a755 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/d423a755 Branch: refs/heads/master Commit: d423a755f51814dcd5b09cb987704a1ef174b7f4 Parents: 98dd501 Author: Min Chen Authored: Sat Jul 20 18:01:49 2013 -0700 Committer: Min Chen Committed: Sat Jul 20 18:01:49 2013 -0700 ---------------------------------------------------------------------- api/src/com/cloud/agent/api/to/S3TO.java | 10 +- server/src/com/cloud/api/ApiServer.java | 25 ++--- server/src/com/cloud/api/ApiServlet.java | 17 +-- utils/src/com/cloud/utils/StringUtils.java | 16 +-- utils/test/com/cloud/utils/StringUtilsTest.java | 104 ++++++++++++++++++- 5 files changed, 141 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/d423a755/api/src/com/cloud/agent/api/to/S3TO.java ---------------------------------------------------------------------- diff --git a/api/src/com/cloud/agent/api/to/S3TO.java b/api/src/com/cloud/agent/api/to/S3TO.java index 8a2f09d..b1b692a 100644 --- a/api/src/com/cloud/agent/api/to/S3TO.java +++ b/api/src/com/cloud/agent/api/to/S3TO.java @@ -18,6 +18,8 @@ package com.cloud.agent.api.to; import java.util.Date; +import com.cloud.agent.api.LogLevel; +import com.cloud.agent.api.LogLevel.Log4jLevel; import com.cloud.storage.DataStoreRole; import com.cloud.utils.S3Utils; @@ -25,7 +27,9 @@ public final class S3TO implements S3Utils.ClientOptions, DataStoreTO { private Long id; private String uuid; + @LogLevel(Log4jLevel.Off) private String accessKey; + @LogLevel(Log4jLevel.Off) private String secretKey; private String endPoint; private String bucketName; @@ -68,10 +72,12 @@ public final class S3TO implements S3Utils.ClientOptions, DataStoreTO { @Override public boolean equals(final Object thatObject) { - if (this == thatObject) + if (this == thatObject) { return true; - if (thatObject == null || getClass() != thatObject.getClass()) + } + if (thatObject == null || getClass() != thatObject.getClass()) { return false; + } final S3TO thatS3TO = (S3TO) thatObject; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/d423a755/server/src/com/cloud/api/ApiServer.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index 95f17af..57a4eaf 100755 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -189,14 +189,14 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer public static ApiServer getInstance() { return s_instance; } - - @Override - public boolean configure(String name, Map params) - throws ConfigurationException { - init(); - return true; - } - + + @Override + public boolean configure(String name, Map params) + throws ConfigurationException { + init(); + return true; + } + public void init() { Integer apiPort = null; // api port, null by default SearchCriteria sc = _configDao.createSearchCriteria(); @@ -293,12 +293,13 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer parameterMap.put(param.getName(), new String[] { param.getValue() }); } - // Get the type of http method being used. + // Get the type of http method being used. parameterMap.put("httpmethod", new String[] { request.getRequestLine().getMethod() }); // Check responseType, if not among valid types, fallback to JSON - if (!(responseType.equals(BaseCmd.RESPONSE_TYPE_JSON) || responseType.equals(BaseCmd.RESPONSE_TYPE_XML))) + if (!(responseType.equals(BaseCmd.RESPONSE_TYPE_JSON) || responseType.equals(BaseCmd.RESPONSE_TYPE_XML))) { responseType = BaseCmd.RESPONSE_TYPE_XML; + } try { // always trust commands from API port, user context will always be UID_SYSTEM/ACCOUNT_ID_SYSTEM @@ -318,7 +319,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw e; } } finally { - s_accessLogger.info(sb.toString()); + s_accessLogger.info(StringUtils.cleanString(sb.toString())); CallContext.unregister(); } } @@ -370,7 +371,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer cmdObj.configure(); cmdObj.setFullUrlParams(paramMap); cmdObj.setResponseType(responseType); - cmdObj.setHttpMethod(paramMap.get("httpmethod").toString()); + cmdObj.setHttpMethod(paramMap.get("httpmethod").toString()); // This is where the command is either serialized, or directly dispatched response = queueCommand(cmdObj, paramMap); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/d423a755/server/src/com/cloud/api/ApiServlet.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/api/ApiServlet.java b/server/src/com/cloud/api/ApiServlet.java index 4b49ee4..22047ff 100755 --- a/server/src/com/cloud/api/ApiServlet.java +++ b/server/src/com/cloud/api/ApiServlet.java @@ -63,9 +63,9 @@ public class ApiServlet extends HttpServlet { @Override public void init(ServletConfig config) throws ServletException { - SpringBeanAutowiringSupport.processInjectionBasedOnServletContext(this, config.getServletContext()); + SpringBeanAutowiringSupport.processInjectionBasedOnServletContext(this, config.getServletContext()); } - + @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) { processRequest(req, resp); @@ -77,8 +77,9 @@ public class ApiServlet extends HttpServlet { } private void utf8Fixup(HttpServletRequest req, Map params) { - if (req.getQueryString() == null) + if (req.getQueryString() == null) { return; + } String[] paramsInQueryString = req.getQueryString().split("&"); if (paramsInQueryString != null) { @@ -325,14 +326,14 @@ public class ApiServlet extends HttpServlet { } } catch (ServerApiException se) { String serializedResponseText = _apiServer.getSerializedApiError(se, params, responseType); - resp.setHeader("X-Description", se.getDescription()); + resp.setHeader("X-Description", se.getDescription()); writeResponse(resp, serializedResponseText, se.getErrorCode().getHttpCode(), responseType); - auditTrailSb.append(" " + se.getErrorCode() + " " + se.getDescription()); + auditTrailSb.append(" " + se.getErrorCode() + " " + se.getDescription()); } catch (Exception ex) { - s_logger.error("unknown exception writing api response", ex); - auditTrailSb.append(" unknown exception writing api response"); + s_logger.error("unknown exception writing api response", ex); + auditTrailSb.append(" unknown exception writing api response"); } finally { - s_accessLogger.info(auditTrailSb.toString()); + s_accessLogger.info(StringUtils.cleanString(auditTrailSb.toString())); if (s_logger.isDebugEnabled()) { s_logger.debug("===END=== " + StringUtils.cleanString(reqStr)); } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/d423a755/utils/src/com/cloud/utils/StringUtils.java ---------------------------------------------------------------------- diff --git a/utils/src/com/cloud/utils/StringUtils.java b/utils/src/com/cloud/utils/StringUtils.java index db32dd4..21c7220 100644 --- a/utils/src/com/cloud/utils/StringUtils.java +++ b/utils/src/com/cloud/utils/StringUtils.java @@ -91,10 +91,10 @@ public class StringUtils { tags += ","; } } - } + } return tags; - } + } public static String getExceptionStackInfo(Throwable e) { StringBuffer sb = new StringBuffer(); @@ -131,22 +131,24 @@ public class StringUtils { } public static String getMaskedPasswordForDisplay(String password) { - if(password == null || password.isEmpty()) + if(password == null || password.isEmpty()) { return "*"; + } StringBuffer sb = new StringBuffer(); sb.append(password.charAt(0)); - for(int i = 1; i < password.length(); i++) + for(int i = 1; i < password.length(); i++) { sb.append("*"); + } return sb.toString(); } // removes a password request param and it's value - private static final Pattern REGEX_PASSWORD_QUERYSTRING = Pattern.compile("&?password=.*?(?=[&'\"])"); + private static final Pattern REGEX_PASSWORD_QUERYSTRING = Pattern.compile("&?(password|accesskey|secretkey)=.*?(?=[&'\"])"); - // removes a password property from a response json object - private static final Pattern REGEX_PASSWORD_JSON = Pattern.compile("\"password\":\".*?\",?"); + // removes a password/accesskey/ property from a response json object + private static final Pattern REGEX_PASSWORD_JSON = Pattern.compile("\"(password|accesskey|secretkey)\":\".*?\",?"); // Responsible for stripping sensitive content from request and response strings public static String cleanString(String stringToClean){ http://git-wip-us.apache.org/repos/asf/cloudstack/blob/d423a755/utils/test/com/cloud/utils/StringUtilsTest.java ---------------------------------------------------------------------- diff --git a/utils/test/com/cloud/utils/StringUtilsTest.java b/utils/test/com/cloud/utils/StringUtilsTest.java index 796efba..ae37c24 100644 --- a/utils/test/com/cloud/utils/StringUtilsTest.java +++ b/utils/test/com/cloud/utils/StringUtilsTest.java @@ -24,7 +24,7 @@ public class StringUtilsTest { @Test public void testCleanPasswordFromJsonObjectAtEnd() { String input = "{\"foo\":\"bar\",\"password\":\"test\"}"; - //TODO: It would be nice to clean up the regex in question to not + //TODO: It would be nice to clean up the regex in question to not //have to return the trailing comma in the expected string below String expected = "{\"foo\":\"bar\",}"; String result = StringUtils.cleanString(input); @@ -78,7 +78,7 @@ public class StringUtilsTest { String result = StringUtils.cleanString(input); assertEquals(result, expected); } - + @Test public void testCleanPasswordFromRequestStringMatchedAtEndSingleQuote() { String input = "'username=foo&password=bar'"; @@ -108,4 +108,104 @@ public class StringUtilsTest { assertEquals("a-b-c", StringUtils.join("-", "a", "b", "c")); assertEquals("", StringUtils.join("-")); } + + @Test + public void testCleanSecretkeyFromJsonObjectAtEnd() { + String input = "{\"foo\":\"bar\",\"secretkey\":\"test\"}"; + // TODO: It would be nice to clean up the regex in question to not + // have to return the trailing comma in the expected string below + String expected = "{\"foo\":\"bar\",}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanSecretkeyFromJsonObjectInMiddle() { + String input = "{\"foo\":\"bar\",\"secretkey\":\"test\",\"test\":\"blah\"}"; + String expected = "{\"foo\":\"bar\",\"test\":\"blah\"}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanSecretkeyFromJsonObjectAlone() { + String input = "{\"secretkey\":\"test\"}"; + String expected = "{}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanSecretkeyFromJsonObjectAtStart() { + String input = "{\"secretkey\":\"test\",\"test\":\"blah\"}"; + String expected = "{\"test\":\"blah\"}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanSecretkeyFromJsonObjectWithMultiplePasswords() { + String input = "{\"description\":\"foo\"}],\"secretkey\":\"bar\",\"nic\":[{\"secretkey\":\"bar2\",\"id\":\"1\"}]}"; + String expected = "{\"description\":\"foo\"}],\"nic\":[{\"id\":\"1\"}]}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromJsonObjectAtEnd() { + String input = "{\"foo\":\"bar\",\"accesskey\":\"test\"}"; + // TODO: It would be nice to clean up the regex in question to not + // have to return the trailing comma in the expected string below + String expected = "{\"foo\":\"bar\",}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromJsonObjectInMiddle() { + String input = "{\"foo\":\"bar\",\"accesskey\":\"test\",\"test\":\"blah\"}"; + String expected = "{\"foo\":\"bar\",\"test\":\"blah\"}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromJsonObjectAlone() { + String input = "{\"accesskey\":\"test\"}"; + String expected = "{}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromJsonObjectAtStart() { + String input = "{\"accesskey\":\"test\",\"test\":\"blah\"}"; + String expected = "{\"test\":\"blah\"}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromJsonObjectWithMultiplePasswords() { + String input = "{\"description\":\"foo\"}],\"accesskey\":\"bar\",\"nic\":[{\"accesskey\":\"bar2\",\"id\":\"1\"}]}"; + String expected = "{\"description\":\"foo\"}],\"nic\":[{\"id\":\"1\"}]}"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanAccesskeyFromRequestString() { + String input = "username=foo&accesskey=bar&url=foobar"; + String expected = "username=foo&url=foobar"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } + + @Test + public void testCleanSecretkeyFromRequestString() { + String input = "username=foo&secretkey=bar&url=foobar"; + String expected = "username=foo&url=foobar"; + String result = StringUtils.cleanString(input); + assertEquals(result, expected); + } }