diff --git a/components/common/pom.xml b/components/common/pom.xml index 5a7d975600..133b2face5 100644 --- a/components/common/pom.xml +++ b/components/common/pom.xml @@ -15,6 +15,7 @@ + com.hotels.styx styx-api @@ -71,6 +72,11 @@ guava + + net.bytebuddy + byte-buddy + + org.mockito mockito-core diff --git a/components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java b/components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java index 0ba282a535..fe555ac82b 100644 --- a/components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java +++ b/components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -19,26 +19,40 @@ import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.LiveHttpRequest; import com.hotels.styx.api.LiveHttpResponse; +import io.netty.handler.codec.http.HttpObject; /** * Provides default formatting for requests and responses. */ public class DefaultHttpMessageFormatter implements HttpMessageFormatter { + @Override public String formatRequest(HttpRequest request) { return request == null ? null : request.toString(); } + @Override public String formatRequest(LiveHttpRequest request) { return request == null ? null : request.toString(); } + @Override public String formatResponse(HttpResponse response) { return response == null ? null : response.toString(); } + @Override public String formatResponse(LiveHttpResponse response) { return response == null ? null : response.toString(); } + @Override + public String formatNettyMessage(HttpObject message) { + return message == null ? null : message.toString(); + } + + @Override + public Throwable wrap(Throwable t) { + return t; + } } diff --git a/components/common/src/main/java/com/hotels/styx/common/format/HttpMessageFormatter.java b/components/common/src/main/java/com/hotels/styx/common/format/HttpMessageFormatter.java index 3afeec6ad3..a62eadcd1e 100644 --- a/components/common/src/main/java/com/hotels/styx/common/format/HttpMessageFormatter.java +++ b/components/common/src/main/java/com/hotels/styx/common/format/HttpMessageFormatter.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.LiveHttpRequest; import com.hotels.styx.api.LiveHttpResponse; +import io.netty.handler.codec.http.HttpObject; /** * A common interface for formatting requests and responses. @@ -33,4 +34,8 @@ public interface HttpMessageFormatter { String formatResponse(LiveHttpResponse response); + String formatNettyMessage(HttpObject message); + + Throwable wrap(Throwable t); + } diff --git a/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpHeaderFormatter.java b/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpHeaderFormatter.java index d8b3799585..3c13d0f502 100644 --- a/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpHeaderFormatter.java +++ b/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpHeaderFormatter.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -19,11 +19,13 @@ import com.hotels.styx.api.HttpHeaders; import com.hotels.styx.api.RequestCookie; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import static java.util.Collections.unmodifiableList; import static java.util.Objects.requireNonNull; /** @@ -37,8 +39,12 @@ public class SanitisedHttpHeaderFormatter { private final List cookiesToHide; public SanitisedHttpHeaderFormatter(List headersToHide, List cookiesToHide) { - this.headersToHide = requireNonNull(headersToHide); - this.cookiesToHide = requireNonNull(cookiesToHide); + this.headersToHide = unmodifiableList(new ArrayList<>(requireNonNull(headersToHide))); + this.cookiesToHide = unmodifiableList(new ArrayList<>(requireNonNull(cookiesToHide))); + } + + public List cookiesToHide() { + return cookiesToHide; } public String format(HttpHeaders headers) { @@ -69,13 +75,16 @@ private boolean isHeaderACookie(HttpHeader header) { } private String formatCookieHeader(HttpHeader header) { - String cookies = RequestCookie.decode(header.value()).stream() + return header.name() + "=" + formatCookieHeaderValue(header.value()); + } + + String formatCookieHeaderValue(String value) { + return RequestCookie.decode(value).stream() .map(this::hideOrFormatCookie) .collect(Collectors.joining(";")); - - return header.name() + "=" + cookies; } + private String hideOrFormatCookie(RequestCookie cookie) { return shouldHideCookie(cookie) ? cookie.name() + "=****" diff --git a/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpMessageFormatter.java b/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpMessageFormatter.java index eb37e45fa4..f757d29dee 100644 --- a/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpMessageFormatter.java +++ b/components/common/src/main/java/com/hotels/styx/common/format/SanitisedHttpMessageFormatter.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -24,6 +24,8 @@ import com.hotels.styx.api.LiveHttpRequest; import com.hotels.styx.api.LiveHttpResponse; import com.hotels.styx.api.Url; +import io.netty.handler.codec.http.HttpObject; +import io.netty.handler.codec.http.LastHttpContent; import static java.util.Objects.requireNonNull; @@ -79,6 +81,36 @@ public String formatResponse(LiveHttpResponse response) { response.headers()); } + @Override + public String formatNettyMessage(HttpObject message) { + if (message == null) { + return NULL; + } else if (message instanceof io.netty.handler.codec.http.HttpRequest) { + return formatNettyRequest((io.netty.handler.codec.http.HttpRequest) message); + } else if (message instanceof LastHttpContent) { + return formatNettyContent((LastHttpContent) message); + } else { + return message.toString(); + } + } + + @Override + public Throwable wrap(Throwable t) { + return t == null ? null : ThrowableSanitiser.instance().sanitise(t, sanitisedHttpHeaderFormatter); + } + + private String formatNettyRequest(io.netty.handler.codec.http.HttpRequest request) { + return "{version=" + request.protocolVersion() + + ", method=" + request.method() + + ", uri=" + request.uri() + + ", headers=[" + sanitisedHttpHeaderFormatter.format(convertToStyxHeaders(request.headers())) + "]}"; + } + + private String formatNettyContent(LastHttpContent content) { + return "{data=" + content.content() + + ", trailingHeaders=[" + sanitisedHttpHeaderFormatter.format(convertToStyxHeaders(content.trailingHeaders())) + "]}"; + } + private String formatRequest(HttpVersion version, HttpMethod method, Url url, Object id, HttpHeaders headers) { return "{version=" + version + ", method=" + method @@ -93,4 +125,11 @@ private String formatResponse(HttpVersion version, HttpResponseStatus status, Ht + ", headers=[" + sanitisedHttpHeaderFormatter.format(headers) + "]}"; } + private HttpHeaders convertToStyxHeaders(io.netty.handler.codec.http.HttpHeaders nettyHeaders) { + HttpHeaders.Builder styxHeaders = new HttpHeaders.Builder(); + nettyHeaders.names().forEach(name -> { + styxHeaders.add(name, nettyHeaders.getAll(name)); + }); + return styxHeaders.build(); + } } diff --git a/components/common/src/main/java/com/hotels/styx/common/format/ThrowableSanitiser.java b/components/common/src/main/java/com/hotels/styx/common/format/ThrowableSanitiser.java new file mode 100644 index 0000000000..b81e8dc30b --- /dev/null +++ b/components/common/src/main/java/com/hotels/styx/common/format/ThrowableSanitiser.java @@ -0,0 +1,176 @@ +/* + Copyright (C) 2013-2020 Expedia Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ +package com.hotels.styx.common.format; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.TypeCache; +import net.bytebuddy.description.modifier.Visibility; +import net.bytebuddy.implementation.FieldAccessor; +import net.bytebuddy.implementation.MethodCall; +import net.bytebuddy.implementation.MethodDelegation; +import net.bytebuddy.implementation.bind.annotation.AllArguments; +import net.bytebuddy.implementation.bind.annotation.Origin; +import net.bytebuddy.implementation.bind.annotation.RuntimeType; +import net.bytebuddy.matcher.ElementMatchers; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.Constructor; +import java.lang.reflect.Method; +import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static net.bytebuddy.TypeCache.Sort.SOFT; + +/** + * Wraps a {@link Throwable} in a dynamic proxy that sanitizes its message to hide sensitive cookie + * information. The supplied Throwable must be non-final, and must have a no-args constructor. The proxy-intercepted + * methods are handled by an instance of {@link Interceptor} + */ +public class ThrowableSanitiser { + + private static final Logger LOG = LoggerFactory.getLogger(ThrowableSanitiser.class); + + private static final ThrowableSanitiser INSTANCE = new ThrowableSanitiser(); + + /** + * Return the singleton instance of this factory. + * @return the singleton instance. + */ + public static ThrowableSanitiser instance() { + return INSTANCE; + } + + private final TypeCache typeCache = new TypeCache<>(SOFT); + + private ThrowableSanitiser() { /* Just making this private */ } + + /** + * Wrap a {@link Throwable} in a dynamic proxy that sanitizes its message to hide sensitive cookie + * information. The supplied Throwable must be non-final, and must have a no-args constructor. If the proxy + * cannot be created for any reason (including that it is proxying a final class, or one without a no-args constructor), + * then a warning is logged and the unproxied Throwable is returned back to the caller. + * @param original the Throwable to be proxied + * @param formatter hides the sensitive cookies. + * @return the proxied Throwable, or the original throwable if it cannot be proxied. + */ + public Throwable sanitise(Throwable original, SanitisedHttpHeaderFormatter formatter) { + + Class clazz = original.getClass(); + try { + Constructor defaultConstructor = clazz.getConstructor(); + + Class proxyClass = typeCache.findOrInsert(getClass().getClassLoader(), clazz.getName(), () -> + new ByteBuddy() + .subclass(clazz) + .defineField("methodInterceptor", Interceptor.class, Visibility.PRIVATE) + .defineConstructor(Visibility.PUBLIC) + .withParameters(Interceptor.class) + .intercept(FieldAccessor.ofField("methodInterceptor").setsArgumentAt(0) + .andThen(MethodCall.invoke(defaultConstructor))) + .method(ElementMatchers.any()) + .intercept(MethodDelegation.toField("methodInterceptor")) + .make() + .load(getClass().getClassLoader()) + .getLoaded()); + + return (Throwable) proxyClass + .getConstructor(Interceptor.class) + .newInstance(new Interceptor(original, formatter)); + } catch (Exception e) { + LOG.warn("Unable to proxy throwable class {} - {}", clazz, e.toString()); // No need to log stack trace here + } + return original; + } + + /** + * Intercepts methods of a {@link Throwable} so that the message can be modified to sanitize the values of any recognised cookies. + * Any cause is similarly wrapped before being returned. + */ + public static class Interceptor { + + private static final ConcurrentHashMap COOKIE_NAME_PATTERN_CACHE = new ConcurrentHashMap<>(); + private static Pattern cookieNamePattern(String cookieName) { + return COOKIE_NAME_PATTERN_CACHE.computeIfAbsent(cookieName, name -> Pattern.compile(name + "\\s*=")); + } + + private final Throwable target; + private final SanitisedHttpHeaderFormatter formatter; + + /** + * Enhance a throwable, using the given {@link SanitisedHttpHeaderFormatter} to recognise and sanitise cookie values. + * @param target the target throwable to enhance + * @param formatter provides the sanitising logic + */ + public Interceptor(Throwable target, SanitisedHttpHeaderFormatter formatter) { + this.target = target; + this.formatter = formatter; + } + + /** + * Default method interceptor, to delegate all other method calls to the target. + * @param method the method being proxied + * @param args the arguments of the method being proxied + * @return the response from the target object + * @throws Exception if the target method throws an exception + */ + @RuntimeType + public Object intercept(@Origin Method method, @AllArguments Object[] args) throws Exception { + return method.invoke(target, args); + } + + private String sanitiseCookies(String message) { + if (message == null) { + return null; + } + return formatter.cookiesToHide().stream() + // Find earliest 'cookiename=' in message + .map(cookie -> cookieNamePattern(cookie).matcher(message)) + .filter(Matcher::find) + .map(Matcher::start) + .min(Integer::compareTo) + .map(cookiesStart -> { + // Assume the cookies run to the end of the message + String cookies = message.substring(cookiesStart); + String sanitizedCookies = formatter.formatCookieHeaderValue(cookies); + return message.substring(0, cookiesStart) + sanitizedCookies; + }) + .orElse(message); + } + + public String getMessage() { + return target.getClass().getName() + ": " + sanitiseCookies(target.getMessage()); + } + + public String getLocalizedMessage() { + return target.getClass().getName() + ": " + sanitiseCookies(target.getLocalizedMessage()); + } + + public Throwable getCause() { + Throwable cause = target.getCause(); + return cause == null ? null : instance().sanitise(cause, formatter); + } + + public Throwable fillInStackTrace() { + return target; + } + + public String toString() { + return "Sanitized: " + target.toString(); + } + } +} diff --git a/components/common/src/test/java/com/hotels/styx/common/format/ThrowableSanitiserTest.java b/components/common/src/test/java/com/hotels/styx/common/format/ThrowableSanitiserTest.java new file mode 100644 index 0000000000..ce6d41ba37 --- /dev/null +++ b/components/common/src/test/java/com/hotels/styx/common/format/ThrowableSanitiserTest.java @@ -0,0 +1,128 @@ +/* + Copyright (C) 2013-2020 Expedia Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ +package com.hotels.styx.common.format; + +import org.junit.jupiter.api.Test; + +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.core.StringEndsWith.endsWith; + +class ThrowableSanitiserTest { + + SanitisedHttpHeaderFormatter formatter = new SanitisedHttpHeaderFormatter( + emptyList(), asList("secret-cookie", "private-cookie") + ); + + static Exception throwAndReturn(Exception ex) { + try { + throw ex; + } catch (Exception e) { + return e; + } + } + + @Test + public void messagesWithNoCookiesAreNotChanged() { + Exception e = throwAndReturn(new Exception("This does not contain any cookies.")); + Throwable proxy = ThrowableSanitiser.instance().sanitise(e, formatter); + + String prefix = "java.lang.Exception: "; + assertThat(proxy.getMessage(), equalTo(prefix + e.getMessage())); + assertThat(proxy.getLocalizedMessage(), equalTo(prefix + e.getLocalizedMessage())); + assertThat(proxy.toString(), equalTo("Sanitized: " + e.toString())); + } + + @Test + public void messagesWithUnrecognizedCookiesAreNotChanged() { + Exception e = throwAndReturn(new Exception("Some cookies: cookie1=c1;cookie2=c2")); + Throwable proxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(proxy.getMessage(), endsWith(e.getMessage())); + assertThat(proxy.getLocalizedMessage(), endsWith(e.getLocalizedMessage())); + assertThat(proxy.toString(), endsWith(e.toString())); + } + + @Test + public void messagesWithRecognizedCookiesAreSanitized() { + Exception e = throwAndReturn(new Exception("Some cookies: cookie1=c1;secret-cookie=secret;cookie2=c2;private-cookie=private")); + Throwable proxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(proxy.getMessage(), containsString("cookie1=c1")); + assertThat(proxy.getMessage(), containsString("cookie2=c2")); + assertThat(proxy.getMessage(), containsString("secret-cookie=****")); + assertThat(proxy.getMessage(), containsString("private-cookie=****")); + } + + @Test + public void exceptionCausesAreSanitized() { + Exception inner = throwAndReturn(new Exception("Inner: cookie1=c1;secret-cookie=secret")); + Exception outer = throwAndReturn(new Exception("Outer: cookie2=c2;private-cookie=private", inner)); + Throwable outerProxy = ThrowableSanitiser.instance().sanitise(outer, formatter); + assertThat(outerProxy.getMessage(), containsString("cookie2=c2")); + assertThat(outerProxy.getMessage(), containsString("private-cookie=****")); + assertThat(outerProxy.getCause().getMessage(), containsString("cookie1=c1")); + assertThat(outerProxy.getCause().getMessage(), containsString("secret-cookie=****")); + } + + @Test + public void nullMessagesAreAllowed() { + Exception e = throwAndReturn(new Exception((String) null)); + Throwable proxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(proxy.getMessage(), equalTo("java.lang.Exception: null")); + } + + @Test + public void exceptionsWithNoDefaultConstructorAreNotProxied() { + Exception e = throwAndReturn(new NoDefaultConstructorException("Ooops, no default constructor")); + Throwable notProxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(notProxy.getMessage(), equalTo("Ooops, no default constructor")); + assertThat(notProxy.getClass().getSuperclass(), not(instanceOf(NoDefaultConstructorException.class))); // i.e. not proxied. + } + + @Test + public void exceptionsOfFinalClassAreNotProxied() { + Exception e = throwAndReturn(new FinalClassException()); + Throwable notProxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(notProxy.getMessage(), equalTo("This is a final class.")); + assertThat(notProxy.getClass().getSuperclass(), not(instanceOf(FinalClassException.class))); // i.e. not proxied. + } + + @Test + public void messagesWithInvalidCookiesAreSanitized() { + Exception e = throwAndReturn(new Exception("Some cookies: cookie1=c1;secret-cookie=secret;bad-cookie=bad\u0000bad;private-cookie=private")); + Throwable proxy = ThrowableSanitiser.instance().sanitise(e, formatter); + assertThat(proxy.getMessage(), containsString("cookie1=c1")); + assertThat(proxy.getMessage(), containsString("bad-cookie=bad\u0000bad")); + assertThat(proxy.getMessage(), containsString("secret-cookie=****")); + assertThat(proxy.getMessage(), containsString("private-cookie=****")); + } + + static class NoDefaultConstructorException extends Exception { + NoDefaultConstructorException(String msg) { + super(msg); + } + } + + static final class FinalClassException extends Exception { + FinalClassException() { + super("This is a final class."); + } + } +} \ No newline at end of file diff --git a/components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java b/components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java index ed345fc9ee..102bb0df6c 100644 --- a/components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java +++ b/components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java @@ -18,6 +18,7 @@ import com.codahale.metrics.Histogram; import com.hotels.styx.api.HttpHandler; import com.hotels.styx.api.MetricRegistry; +import com.hotels.styx.common.format.HttpMessageFormatter; import com.hotels.styx.proxy.HttpCompressor; import com.hotels.styx.proxy.ServerProtocolDistributionRecorder; import com.hotels.styx.proxy.encoders.ConfigurableUnwiseCharsEncoder; @@ -69,24 +70,27 @@ public class ProxyConnectorFactory implements ServerConnectorFactory { private final String unwiseCharacters; private final ResponseEnhancer responseEnhancer; private final boolean requestTracking; + private final HttpMessageFormatter httpMessageFormatter; public ProxyConnectorFactory(NettyServerConfig serverConfig, MetricRegistry metrics, HttpErrorStatusListener errorStatusListener, String unwiseCharacters, ResponseEnhancer responseEnhancer, - boolean requestTracking) { + boolean requestTracking, + HttpMessageFormatter httpMessageFormatter) { this.serverConfig = requireNonNull(serverConfig); this.metrics = requireNonNull(metrics); this.errorStatusListener = requireNonNull(errorStatusListener); this.unwiseCharacters = requireNonNull(unwiseCharacters); this.responseEnhancer = requireNonNull(responseEnhancer); this.requestTracking = requestTracking; + this.httpMessageFormatter = httpMessageFormatter; } @Override public ServerConnector create(ConnectorConfig config) { - return new ProxyConnector(config, serverConfig, metrics, errorStatusListener, unwiseCharacters, responseEnhancer, requestTracking); + return new ProxyConnector(config, this); } private static final class ProxyConnector implements ServerConnector { @@ -101,29 +105,25 @@ private static final class ProxyConnector implements ServerConnector { private final Optional sslContext; private final ResponseEnhancer responseEnhancer; private final RequestTracker requestTracker; + private final HttpMessageFormatter httpMessageFormatter; - private ProxyConnector(ConnectorConfig config, - NettyServerConfig serverConfig, - MetricRegistry metrics, - HttpErrorStatusListener errorStatusListener, - String unwiseCharacters, - ResponseEnhancer responseEnhancer, - boolean requestTracking) { - this.responseEnhancer = requireNonNull(responseEnhancer); + private ProxyConnector(ConnectorConfig config, ProxyConnectorFactory factory) { this.config = requireNonNull(config); - this.serverConfig = requireNonNull(serverConfig); - this.metrics = requireNonNull(metrics); - this.httpErrorStatusListener = requireNonNull(errorStatusListener); + this.responseEnhancer = requireNonNull(factory.responseEnhancer); + this.serverConfig = requireNonNull(factory.serverConfig); + this.metrics = requireNonNull(factory.metrics); + this.httpErrorStatusListener = requireNonNull(factory.errorStatusListener); this.channelStatsHandler = new ChannelStatisticsHandler(metrics); this.requestStatsCollector = new RequestStatsCollector(metrics.scope("requests")); this.excessConnectionRejector = new ExcessConnectionRejector(new DefaultChannelGroup(GlobalEventExecutor.INSTANCE), serverConfig.maxConnectionsCount()); - this.unwiseCharEncoder = new ConfigurableUnwiseCharsEncoder(unwiseCharacters); + this.unwiseCharEncoder = new ConfigurableUnwiseCharsEncoder(factory.unwiseCharacters); if (isHttps()) { this.sslContext = Optional.of(newSSLContext((HttpsConnectorConfig) config, metrics)); } else { this.sslContext = Optional.empty(); } - this.requestTracker = requestTracking ? CurrentRequestTracker.INSTANCE : RequestTracker.NO_OP; + this.requestTracker = factory.requestTracking ? CurrentRequestTracker.INSTANCE : RequestTracker.NO_OP; + this.httpMessageFormatter = factory.httpMessageFormatter; } @Override @@ -181,6 +181,7 @@ private NettyToStyxRequestDecoder requestTranslator() { return new NettyToStyxRequestDecoder.Builder() .flowControlEnabled(true) .unwiseCharEncoder(unwiseCharEncoder) + .httpMessageFormatter(httpMessageFormatter) .build(); } diff --git a/components/proxy/src/main/java/com/hotels/styx/StyxServer.java b/components/proxy/src/main/java/com/hotels/styx/StyxServer.java index 9cfd3eaa30..f7bc796c37 100644 --- a/components/proxy/src/main/java/com/hotels/styx/StyxServer.java +++ b/components/proxy/src/main/java/com/hotels/styx/StyxServer.java @@ -245,7 +245,8 @@ private static InetServer httpServer(Environment environment, ConnectorConfig co environment.errorListener(), environment.configuration().get(ENCODE_UNWISECHARS).orElse(""), (builder, request) -> builder.header(styxInfoHeaderName, responseInfoFormat.format(request)), - environment.configuration().get("requestTracking", Boolean.class).orElse(false)) + environment.configuration().get("requestTracking", Boolean.class).orElse(false), + environment.httpMessageFormatter()) .create(connectorConfig); return NettyServerBuilder.newBuilder() diff --git a/components/proxy/src/main/kotlin/com/hotels/styx/servers/StyxHttpServer.kt b/components/proxy/src/main/kotlin/com/hotels/styx/servers/StyxHttpServer.kt index dc6869adc8..122c1e0dc4 100644 --- a/components/proxy/src/main/kotlin/com/hotels/styx/servers/StyxHttpServer.kt +++ b/components/proxy/src/main/kotlin/com/hotels/styx/servers/StyxHttpServer.kt @@ -124,7 +124,8 @@ internal class StyxHttpServerFactory : StyxServerFactory { environment.configuration().styxHeaderConfig().styxInfoHeaderName(), ResponseInfoFormat(environment).format(request)) }, - false) + false, + environment.httpMessageFormatter()) .create( if (config.tlsSettings == null) { HttpConnectorConfig(config.port) diff --git a/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java b/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java index 1ab86b1f3c..bd9a4bc59e 100644 --- a/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java +++ b/components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -21,6 +21,8 @@ import com.hotels.styx.api.HttpVersion; import com.hotels.styx.api.LiveHttpRequest; import com.hotels.styx.api.Url; +import com.hotels.styx.common.format.DefaultHttpMessageFormatter; +import com.hotels.styx.common.format.HttpMessageFormatter; import com.hotels.styx.server.BadRequestException; import com.hotels.styx.server.UniqueIdSupplier; import io.netty.buffer.ByteBuf; @@ -64,18 +66,23 @@ public final class NettyToStyxRequestDecoder extends MessageToMessageDecoder out) throws Exception { if (httpObject.getDecoderResult().isFailure()) { - throw new BadRequestException("Error while decoding request: " + httpObject, httpObject.getDecoderResult().cause()); + String formattedHttpObject = httpMessageFormatter.formatNettyMessage(httpObject); + throw new BadRequestException("Error while decoding request: " + formattedHttpObject, + httpMessageFormatter.wrap(httpObject.getDecoderResult().cause())); } try { @@ -251,6 +258,7 @@ public static final class Builder { private boolean flowControlEnabled; private UniqueIdSupplier uniqueIdSupplier = UUID_VERSION_ONE_SUPPLIER; private UnwiseCharsEncoder unwiseCharEncoder = IGNORE; + private HttpMessageFormatter httpMessageFormatter = new DefaultHttpMessageFormatter(); public Builder uniqueIdSupplier(UniqueIdSupplier uniqueIdSupplier) { this.uniqueIdSupplier = requireNonNull(uniqueIdSupplier); @@ -267,6 +275,11 @@ public Builder unwiseCharEncoder(UnwiseCharsEncoder unwiseCharEncoder) { return this; } + public Builder httpMessageFormatter(HttpMessageFormatter httpMessageFormatter) { + this.httpMessageFormatter = httpMessageFormatter; + return this; + } + public NettyToStyxRequestDecoder build() { return new NettyToStyxRequestDecoder(this); } diff --git a/components/server/src/test/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoderTest.java b/components/server/src/test/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoderTest.java index ac030d28f9..da8b3e2eff 100644 --- a/components/server/src/test/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoderTest.java +++ b/components/server/src/test/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoderTest.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -21,6 +21,9 @@ import com.hotels.styx.api.HttpHeader; import com.hotels.styx.api.HttpMethod; import com.hotels.styx.api.LiveHttpRequest; +import com.hotels.styx.common.format.HttpMessageFormatter; +import com.hotels.styx.common.format.SanitisedHttpHeaderFormatter; +import com.hotels.styx.common.format.SanitisedHttpMessageFormatter; import com.hotels.styx.server.BadRequestException; import com.hotels.styx.server.UniqueIdSupplier; import io.netty.buffer.ByteBuf; @@ -28,6 +31,7 @@ import io.netty.channel.SimpleChannelInboundHandler; import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.DecoderException; +import io.netty.handler.codec.DecoderResult; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.DefaultHttpContent; import io.netty.handler.codec.http.DefaultHttpRequest; @@ -46,6 +50,7 @@ import rx.observers.TestSubscriber; import java.net.URI; +import java.util.Arrays; import java.util.concurrent.CountDownLatch; import static com.google.common.base.Charsets.US_ASCII; @@ -68,7 +73,9 @@ import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1; import static io.netty.handler.codec.http.LastHttpContent.EMPTY_LAST_CONTENT; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.matchesPattern; +import static org.hamcrest.Matchers.not; import static org.hamcrest.core.Is.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -110,6 +117,27 @@ public void rejectsRequestsWithBadURL() throws Throwable { assertEquals(BadRequestException.class, e.getCause().getClass()); } + @Test + public void sanitisesHeadersInBadRequestException() throws Throwable { + FullHttpRequest originalRequest = newHttpRequest("/uri"); + HttpHeaders originalRequestHeaders = originalRequest.headers(); + originalRequestHeaders.add("Foo", "foo"); + originalRequestHeaders.add("secret-header", "secret"); + originalRequestHeaders.add("Cookie", "Bar=bar;secret-cookie=secret"); + originalRequest.setDecoderResult(DecoderResult.failure(new Exception("It just didn't work"))); + + Exception e = assertThrows(DecoderException.class, () -> decode(originalRequest)); + + assertEquals(BadRequestException.class, e.getCause().getClass()); + String breMsg = e.getCause().getMessage(); + assertThat(breMsg, containsString("Foo=foo")); + assertThat(breMsg, containsString("secret-header=****")); + assertThat(breMsg, containsString("Bar=bar")); + assertThat(breMsg, containsString("secret-cookie=****")); + assertThat(breMsg, not(containsString("secret-header=secret"))); + assertThat(breMsg, not(containsString("secret-cookie=secret"))); + } + @Test public void decodesNettyInternalRequestToStyxRequest() throws Exception { FullHttpRequest originalRequest = newHttpRequest("/uri"); @@ -391,8 +419,12 @@ private static FullHttpRequest newHttpRequest(String uri) { private LiveHttpRequest decode(HttpRequest request) { HttpRequestRecorder requestRecorder = new HttpRequestRecorder(); + HttpMessageFormatter formatter = new SanitisedHttpMessageFormatter(new SanitisedHttpHeaderFormatter( + Arrays.asList("secret-header"), Arrays.asList("secret-cookie") + )); EmbeddedChannel channel = new EmbeddedChannel(new NettyToStyxRequestDecoder.Builder() .uniqueIdSupplier(uniqueIdSupplier) + .httpMessageFormatter(formatter) .build(), requestRecorder); channel.writeInbound(request); return requestRecorder.styxRequest; diff --git a/pom.xml b/pom.xml index 6a61573e34..15614456eb 100644 --- a/pom.xml +++ b/pom.xml @@ -118,6 +118,7 @@ 1.0.2 3.3.0.RELEASE 3.0.3 + 1.10.6 1.3.21 @@ -332,6 +333,12 @@ ${jackson-module-kotlin.version} + + net.bytebuddy + byte-buddy + ${bytebuddy.version} + + io.dropwizard.metrics diff --git a/support/testsupport/src/main/java/com/hotels/styx/support/matchers/LoggingTestSupport.java b/support/testsupport/src/main/java/com/hotels/styx/support/matchers/LoggingTestSupport.java index 2f76bd8403..081f29de93 100644 --- a/support/testsupport/src/main/java/com/hotels/styx/support/matchers/LoggingTestSupport.java +++ b/support/testsupport/src/main/java/com/hotels/styx/support/matchers/LoggingTestSupport.java @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2018 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -28,8 +28,8 @@ import static org.slf4j.LoggerFactory.getLogger; public class LoggingTestSupport { - private final Logger logger; - private final ListAppender appender; + public final Logger logger; + public final ListAppender appender; public LoggingTestSupport(Class classUnderTest) { this(logger(classUnderTest)); diff --git a/system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt b/system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt index 2c6128c49e..e06a70f283 100644 --- a/system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt +++ b/system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt @@ -1,5 +1,5 @@ /* - Copyright (C) 2013-2019 Expedia Inc. + Copyright (C) 2013-2020 Expedia Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -16,16 +16,22 @@ package com.hotels.styx.logging import ch.qos.logback.classic.Level.INFO +import ch.qos.logback.classic.Level.ERROR +import ch.qos.logback.classic.Level.DEBUG import com.hotels.styx.api.HttpHeaderNames.HOST import com.hotels.styx.api.HttpRequest +import com.hotels.styx.api.HttpResponseStatus.BAD_REQUEST +import com.hotels.styx.client.BadHttpResponseException import com.hotels.styx.client.StyxHttpClient -import com.hotels.styx.support.StyxServerProvider +import com.hotels.styx.proxy.HttpErrorStatusCauseLogger +import com.hotels.styx.support.* import com.hotels.styx.support.matchers.LoggingTestSupport -import com.hotels.styx.support.proxyHttpHostHeader -import com.hotels.styx.support.shouldContain -import com.hotels.styx.support.wait import io.kotlintest.Spec +import io.kotlintest.matchers.string.shouldContain +import io.kotlintest.matchers.string.shouldNotContain +import io.kotlintest.shouldBe import io.kotlintest.specs.FeatureSpec +import kotlin.test.assertFailsWith class HttpMessageLoggingSpec : FeatureSpec() { @@ -52,10 +58,105 @@ class HttpMessageLoggingSpec : FeatureSpec() { logger.log().shouldContain(INFO, expectedRequest) logger.log().shouldContain(INFO, expectedResponse) } + + scenario("Requests with badly-formed headers should hide sensitive cookies and headers when logged") { + + httpErrorLogger.logger.level = ERROR + httpErrorLogger.appender.list.clear() + + val response = client.send(HttpRequest.get("/a/path") + .header(HOST, styxServer().proxyHttpHostHeader()) + .header("header1", "h1") + .header("header2", "h2") + .header("cookie", "cookie1=c1;cookie2=c2") + .header("badheader", "with\u0000nullchar") + .build()) + .wait()!! + + response.status() shouldBe BAD_REQUEST + + val event = httpErrorLogger.log().first() + event.level shouldBe ERROR + + var t = event.throwableProxy + while (t != null) { + anySensitiveHeadersAreHidden(t.message) + t = t.cause + } + } + + scenario("Requests with badly-formed cookies should hide sensitive cookies and headers when logged") { + + httpErrorLogger.logger.level = ERROR + httpErrorLogger.appender.list.clear() + + val response = client.send(HttpRequest.get("/a/path") + .header(HOST, styxServer().proxyHttpHostHeader()) + .header("header1", "h1") + .header("header2", "h2") + .header("cookie", "cookie1=c1;cookie2=c2;badcookie=bad\u0000bad") + .build()) + .wait()!! + + response.status() shouldBe BAD_REQUEST + + val event = httpErrorLogger.log().first() + event.level shouldBe ERROR + + var t = event.throwableProxy + while (t != null) { + anySensitiveHeadersAreHidden(t.message) + t = t.cause + } + } + + scenario("Responses with badly-formed headers should hide sensitive cookies and headers when logged") { + + rootLogger.appender.list.clear() + rootLogger.logger.level = DEBUG + + var exception: Throwable? = assertFailsWith { + client.send(HttpRequest.get("/bad/path") + .header(HOST, styxServer().proxyHttpHostHeader()) + .header("header1", "h1") + .header("header2", "h2") + .header("cookie", "cookie1=c1;cookie2=c2") + .build()) + .wait() + } + + while (exception != null) { + anySensitiveHeadersAreHidden(exception.message ?: "") + exception = exception.cause + } + + rootLogger.log().forEach { + anySensitiveHeadersAreHidden(it.message) + var t = it.throwableProxy + while (t != null) { + anySensitiveHeadersAreHidden(t.message) + t = t.cause + } + } + } } } - val logger = LoggingTestSupport("com.hotels.styx.http-messages.inbound") + private fun anySensitiveHeadersAreHidden(msg: String) { + msg shouldNotContain "header1=h1" + msg shouldNotContain "cookie1=c1" + if (msg.contains("header2=h2")) { + msg shouldContain "header1=****" + } + if (msg.contains("cookie2=c2")) { + msg shouldContain "cookie1=****" + } + } + + private val logger = LoggingTestSupport("com.hotels.styx.http-messages.inbound") + private val httpErrorLogger = LoggingTestSupport(HttpErrorStatusCauseLogger::class.java) + private val rootLogger = LoggingTestSupport("ROOT") + val client: StyxHttpClient = StyxHttpClient.Builder().build() @@ -87,6 +188,28 @@ class HttpMessageLoggingSpec : FeatureSpec() { routingObjects: root: + type: PathPrefixRouter + config: + routes: + - prefix: / + destination: default + - prefix: /bad + destination: bad + + default: + type: StaticResponseHandler + config: + status: 200 + content: "" + headers: + - name: "header1" + value: "h1" + - name: "header2" + value: "h2" + - name: "cookie" + value: "cookie1=c1;cookie2=c2" + + bad: type: StaticResponseHandler config: status: 200 @@ -98,6 +221,8 @@ class HttpMessageLoggingSpec : FeatureSpec() { value: "h2" - name: "cookie" value: "cookie1=c1;cookie2=c2" + - name: "badheader" + value: "bad\u0000bad" httpPipeline: root """.trimIndent())