Repository: cxf Updated Branches: refs/heads/3.1.x-fixes 6eb9cd56b -> b86ac6d0d
[CXF-7070] Avoiding logging the well-known senstive header values by default, patch from Andy McCright applied with a minor update, This closes #178 Project: http://git-wip-us.apache.org/repos/asf/cxf/repo Commit: http://git-wip-us.apache.org/repos/asf/cxf/commit/b86ac6d0 Tree: http://git-wip-us.apache.org/repos/asf/cxf/tree/b86ac6d0 Diff: http://git-wip-us.apache.org/repos/asf/cxf/diff/b86ac6d0 Branch: refs/heads/3.1.x-fixes Commit: b86ac6d0d12c25ca8a50e8e4fcc494db258387f1 Parents: 6eb9cd5 Author: Sergey Beryozkin <sberyoz...@gmail.com> Authored: Thu Oct 13 12:50:34 2016 +0100 Committer: Sergey Beryozkin <sberyoz...@gmail.com> Committed: Thu Oct 13 12:52:06 2016 +0100 ---------------------------------------------------------------------- .../org/apache/cxf/transport/http/Headers.java | 98 ++++++++++------ .../apache/cxf/transport/http/HeadersTest.java | 111 ++++++++++++++++--- 2 files changed, 156 insertions(+), 53 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cxf/blob/b86ac6d0/rt/transports/http/src/main/java/org/apache/cxf/transport/http/Headers.java ---------------------------------------------------------------------- diff --git a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/Headers.java b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/Headers.java index 23b7975..7ec9f4a 100644 --- a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/Headers.java +++ b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/Headers.java @@ -61,8 +61,8 @@ public class Headers { * Each header value is added as a separate HTTP header, example, given A header with 'a' and 'b' * values, two A headers will be added as opposed to a single A header with the "a,b" value. */ - public static final String ADD_HEADERS_PROPERTY = "org.apache.cxf.http.add-headers"; - + public static final String ADD_HEADERS_PROPERTY = "org.apache.cxf.http.add-headers"; + public static final String PROTOCOL_HEADERS_CONTENT_TYPE = Message.CONTENT_TYPE.toLowerCase(); public static final String HTTP_HEADERS_SETCOOKIE = "Set-Cookie"; public static final String HTTP_HEADERS_LINK = "Link"; @@ -70,7 +70,10 @@ public class Headers { private static final String SET_EMPTY_REQUEST_CT_PROPERTY = "set.content.type.for.empty.request"; private static final TimeZone TIME_ZONE_GMT = TimeZone.getTimeZone("GMT"); private static final Logger LOG = LogUtils.getL7dLogger(Headers.class); - + + private static final List<String> SENSITIVE_HEADERS = Arrays.asList("Authorization", "Proxy-Authorization"); + private static final List<String> SENSITIVE_HEADER_MARKER = Arrays.asList("***"); + private static final String ALLOW_LOGGING_SENSITIVE_HEADERS = "allow.logging.sensitive.headers"; /** * Known HTTP headers whose values have to be represented as individual HTTP headers */ @@ -108,6 +111,25 @@ public class Headers { return name + "/" + version; } + /** + * Returns a traceable string representation of the passed-in headers map. + * The value for any keys in the map that are in the <code>SENSITIVE_HEADERS</code> + * array will be filtered out of the returned string. + * Note that this method is expensive as it will copy the map (except for the + * filtered keys), so it should be used sparingly - i.e. only when debug is + * enabled. + */ + static String toString(Map<String, List<String>> headers, boolean logSensitiveHeaders) { + Map<String, List<String>> filteredHeaders = new TreeMap<String, List<String>>(String.CASE_INSENSITIVE_ORDER); + filteredHeaders.putAll(headers); + if (!logSensitiveHeaders) { + for (String filteredKey : SENSITIVE_HEADERS) { + filteredHeaders.put(filteredKey, SENSITIVE_HEADER_MARKER); + } + } + return filteredHeaders.toString(); + } + public Map<String, List<String>> headerMap() { return headers; } @@ -189,7 +211,7 @@ public class Headers { createMutableList(policy.getReferer())); } } - + void setFromServerPolicy(HTTPServerPolicy policy) { if (policy.isSetCacheControl()) { headers.put("Cache-Control", @@ -217,9 +239,8 @@ public class Headers { } else if (policy.isSetKeepAliveParameters()) { headers.put("Keep-Alive", createMutableList(policy.getKeepAliveParameters())); } - - - + + /* * TODO - hook up these policies <xs:attribute name="SuppressClientSendErrors" type="xs:boolean" use="optional" default="false"> @@ -241,8 +262,8 @@ public class Headers { headers.put("Proxy-Authorization", createMutableList(authorization)); } - - + + /** * While extracting the Message.PROTOCOL_HEADERS property from the Message, * this call ensures that the Message.PROTOCOL_HEADERS property is @@ -254,7 +275,7 @@ public class Headers { */ public static Map<String, List<String>> getSetProtocolHeaders(final Message message) { Map<String, List<String>> headers = - CastUtils.cast((Map<?, ?>)message.get(Message.PROTOCOL_HEADERS)); + CastUtils.cast((Map<?, ?>)message.get(Message.PROTOCOL_HEADERS)); if (null == headers) { headers = new TreeMap<String, List<String>>(String.CASE_INSENSITIVE_ORDER); } else if (headers instanceof HashMap) { @@ -281,26 +302,31 @@ public class Headers { private static List<String> createMutableList(String val) { return new ArrayList<String>(Arrays.asList(new String[] {val})); } - + /** * This procedure logs the PROTOCOL_HEADERS from the * Message at the specified logging level. * + * @param logger The Logger to log to. * @param level The Logging Level. * @param headers The Message protocol headers. */ - void logProtocolHeaders(Level level) { - if (LOG.isLoggable(level)) { - for (Map.Entry<String, List<String>> entry : headers.entrySet()) { - List<String> headerList = entry.getValue(); + static void logProtocolHeaders(Logger logger, Level level, + Map<String, List<String>> headersMap, + boolean logSensitiveHeaders) { + if (logger.isLoggable(level)) { + for (Map.Entry<String, List<String>> entry : headersMap.entrySet()) { + String key = entry.getKey(); + boolean sensitive = !logSensitiveHeaders && SENSITIVE_HEADERS.contains(key); + List<String> headerList = sensitive ? SENSITIVE_HEADER_MARKER : entry.getValue(); for (String value : headerList) { - LOG.log(level, entry.getKey() + ": " + logger.log(level, key + ": " + (value == null ? "<null>" : value.toString())); } } } } - + /** * Set content type and protocol headers (Message.PROTOCOL_HEADERS) headers into the URL * connection. @@ -331,7 +357,6 @@ public class Headers { // otherwise if it is GET then just drop it dropContentType = true; } - } if (!dropContentType) { String ct = emptyRequest && !contentTypeSet ? "*/*" : determineContentType(); @@ -342,7 +367,7 @@ public class Headers { } transferProtocolHeadersToURLConnection(connection); - logProtocolHeaders(Level.FINE); + logProtocolHeaders(LOG, Level.FINE, headers, logSensitiveHeaders()); } public String determineContentType() { @@ -369,7 +394,7 @@ public class Headers { } return ct; } - + /** * This procedure sets the URLConnection request properties * from the PROTOCOL_HEADERS in the message. @@ -380,7 +405,7 @@ public class Headers { for (Map.Entry<String, List<String>> entry : headers.entrySet()) { String header = entry.getKey(); List<String> headerList = entry.getValue(); - + if (HttpHeaderHelper.CONTENT_TYPE.equalsIgnoreCase(header)) { continue; } @@ -404,7 +429,7 @@ public class Headers { connection.addRequestProperty("User-Agent", USER_AGENT); } } - + /** * Copy the request headers into the message. * @@ -431,14 +456,19 @@ public class Headers { headers.put(Message.CONTENT_TYPE, Collections.singletonList(req.getContentType())); } if (LOG.isLoggable(Level.FINE)) { - LOG.log(Level.FINE, "Request Headers: " + headers.toString()); + LOG.log(Level.FINE, "Request Headers: " + toString(headers, + logSensitiveHeaders())); } } + private boolean logSensitiveHeaders() { + // Not allowed by default + return PropertyUtils.isTrue(message.getContextualProperty(ALLOW_LOGGING_SENSITIVE_HEADERS)); + } private String getContentTypeFromMessage() { final String ct = (String)message.get(Message.CONTENT_TYPE); final String enc = (String)message.get(Message.ENCODING); - + if (null != ct && null != enc && ct.indexOf("charset=") == -1 @@ -448,7 +478,7 @@ public class Headers { return ct; } } - + // Assumes that response body is not available only // if Content-Length is available and set to 0 private boolean isResponseBodyAvailable() { @@ -465,7 +495,7 @@ public class Headers { } return true; } - + /** * Copy the response headers into the response. * @@ -485,7 +515,7 @@ public class Headers { for (Map.Entry<String, List<String>> entry : headers.entrySet()) { String header = entry.getKey(); List<?> headerList = entry.getValue(); - + if (addHeaders || HTTP_HEADERS_SINGLE_VALUE_ONLY.contains(header)) { for (int i = 0; i < headerList.size(); i++) { Object headerObject = headerList.get(i); @@ -500,18 +530,16 @@ public class Headers { if (headerObject != null) { sb.append(headerObjectToString(headerObject)); } - + if (i + 1 < headerList.size()) { sb.append(','); } } response.setHeader(header, sb.toString()); } - - } } - + private String headerObjectToString(Object headerObject) { if (headerObject.getClass() == String.class) { // Most likely @@ -521,7 +549,7 @@ public class Headers { // so that the below code may be pushed back to the JAX-RS // front-end where non String header objects are more likely // to be set. Though the below code may be generally useful - + String headerString; if (headerObject instanceof Date) { headerString = toHttpDate((Date)headerObject); @@ -533,7 +561,7 @@ public class Headers { return headerString; } } - + void removeContentType() { headers.remove(PROTOCOL_HEADERS_CONTENT_TYPE); } @@ -551,12 +579,12 @@ public class Headers { dateFormat.setTimeZone(TIME_ZONE_GMT); return dateFormat; } - + public static String toHttpDate(Date date) { SimpleDateFormat format = getHttpDateFormat(); return format.format(date); } - + public static String toHttpLanguage(Locale locale) { StringBuilder sb = new StringBuilder(); sb.append(locale.getLanguage()); http://git-wip-us.apache.org/repos/asf/cxf/blob/b86ac6d0/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HeadersTest.java ---------------------------------------------------------------------- diff --git a/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HeadersTest.java b/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HeadersTest.java index 25be560..bf5cf84 100755 --- a/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HeadersTest.java +++ b/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HeadersTest.java @@ -23,6 +23,10 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; import javax.servlet.http.HttpServletRequest; @@ -31,27 +35,28 @@ import org.apache.cxf.message.Message; import org.apache.cxf.message.MessageImpl; import org.easymock.EasyMock; import org.easymock.IMocksControl; -import org.junit.After; + +import org.junit.AfterClass; import org.junit.Assert; -import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; /** * */ public class HeadersTest extends Assert { - private IMocksControl control; - - @Before - public void setUp() { + private static IMocksControl control; + + @BeforeClass + public static void setUpClass() { control = EasyMock.createNiceControl(); } - - @After - public void tearDown() { + + @AfterClass + public static void tearDown() { control.verify(); } - + @Test public void setHeadersTest() throws Exception { String[] headerNames = {"Content-Type", "authorization", "soapAction"}; @@ -60,7 +65,7 @@ public class HeadersTest extends Assert { for (int i = 0; i < headerNames.length; i++) { inmap.put(headerNames[i], Arrays.asList(headerValues[i])); } - + HttpServletRequest req = control.createMock(HttpServletRequest.class); EasyMock.expect(req.getHeaderNames()).andReturn(Collections.enumeration(inmap.keySet())); for (int i = 0; i < headerNames.length; i++) { @@ -68,20 +73,20 @@ public class HeadersTest extends Assert { andReturn(Collections.enumeration(inmap.get(headerNames[i]))); } EasyMock.expect(req.getContentType()).andReturn(headerValues[0]).anyTimes(); - + control.replay(); Message message = new MessageImpl(); message.put(AbstractHTTPDestination.HTTP_REQUEST, req); - + Headers headers = new Headers(message); headers.copyFromRequest(req); - + Map<String, List<String>> protocolHeaders = CastUtils.cast((Map<?, ?>)message.get(Message.PROTOCOL_HEADERS)); - + assertTrue("unexpected size", protocolHeaders.size() == headerNames.length); - + assertEquals("unexpected header", protocolHeaders.get("Content-Type").get(0), headerValues[0]); assertEquals("unexpected header", protocolHeaders.get("content-type").get(0), headerValues[0]); assertEquals("unexpected header", protocolHeaders.get("CONTENT-TYPE").get(0), headerValues[0]); @@ -91,11 +96,81 @@ public class HeadersTest extends Assert { assertEquals("unexpected header", protocolHeaders.get("authorization").get(0), headerValues[1]); assertEquals("unexpected header", protocolHeaders.get("AUTHORIZATION").get(0), headerValues[1]); assertEquals("unexpected header", protocolHeaders.get("authoriZATION").get(0), headerValues[1]); - + assertEquals("unexpected header", protocolHeaders.get("SOAPAction").get(0), headerValues[2]); assertEquals("unexpected header", protocolHeaders.get("soapaction").get(0), headerValues[2]); assertEquals("unexpected header", protocolHeaders.get("SOAPACTION").get(0), headerValues[2]); assertEquals("unexpected header", protocolHeaders.get("soapAction").get(0), headerValues[2]); - + + } + + @Test + public void sensitiveHeadersTest() { + Map<String, List<String>> headerMap = new HashMap<String, List<String>>(); + headerMap.put("Authorization", Arrays.asList("FAIL")); + headerMap.put("Proxy-Authorization", Arrays.asList("FAIL")); + headerMap.put("Content-Type", Arrays.asList("application/xml")); + headerMap.put("Accept", Arrays.asList("text/plain")); + + String loggedString = Headers.toString(headerMap, false); + assertFalse("The value of a sensitive header could be logged: " + loggedString, loggedString.contains("FAIL")); + assertTrue("The value of a non-sensitive header would not be logged: " + loggedString, + loggedString.contains("application/xml") && loggedString.contains("text/plain")); + assertTrue("Expected header keys were not logged: " + loggedString, + loggedString.contains("Authorization") && loggedString.contains("Proxy-Authorization") + && loggedString.contains("Accept") && loggedString.contains("Content-Type")); + } + + @Test + public void logProtocolHeadersTest() { + Map<String, List<String>> headerMap = new HashMap<String, List<String>>(); + headerMap.put("Normal-Header", Arrays.asList("normal")); + headerMap.put("Multivalue-Header", Arrays.asList("first", "second")); + headerMap.put("Authorization", Arrays.asList("myPassword")); + headerMap.put("Null-Header", Arrays.asList((String)null)); + + //Set up test logger + Logger logger = Logger.getAnonymousLogger(); + // remove all "normal" handlers and just use our custom handler for testing + logger.setUseParentHandlers(false); + logger.setLevel(Level.INFO); + for (Handler h : logger.getHandlers()) { + logger.removeHandler(h); + } + logger.addHandler(new Handler() { + + @Override + public void publish(LogRecord record) { + String msg = record.getMessage(); + if (msg.startsWith("Normal-Header")) { + assertTrue("Unexpected output for normal header - expected Normal-Header: normal, received " + msg, + "Normal-Header: normal".equals(msg)); + } else if (msg.startsWith("Multivalue-Header")) { + assertTrue("Unexpected output for multi-value header - expected Multivalue-Header: first or " + + "Multivalue-Header: second, received: " + msg, + "Multivalue-Header: first".equals(msg) || "Multivalue-Header: second".equals(msg)); + } else if (msg.startsWith("Authorization")) { + assertTrue("Unexpected output for sensitive header - expected Authorization: ***, received " + msg, + "Authorization: ***".equals(msg)); + } else if (msg.startsWith("Null-Header")) { + assertTrue("Unexpected output for null header - expected Null-Header: <null>, received " + msg, + "Null-Header: <null>".equals(msg)); + } else { + fail("Unexpected header logged: " + msg); + } + + } + + @Override + public void flush() { + // no-op + } + + @Override + public void close() throws SecurityException { + // no-op + } }); + + Headers.logProtocolHeaders(logger, Level.INFO, headerMap, false); } }
