GitHub#454 Sanitise HTTP message logs #457

OwenLindsell · 2019-09-06T10:19:22Z

Added two new configuration options to hide headers and cookies in request logging:

    request-logging:
      hideHeaders:
          - Content-Type
      hideCookies:
          - sessionID
          - samlToken

The hideHeaders and hideCookies options take a list of header or cookie names. Any header or cookie in these lists will be obfuscated in the logged message output e.g.

headers=[Content-Type:****, Cookie:sessionID=****;samlToken=****]

components/proxy/src/main/java/com/hotels/styx/StyxServer.java

components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java

...s/client/src/main/java/com/hotels/styx/client/netty/connectionpool/HttpRequestOperation.java

components/common/src/main/java/com/hotels/styx/common/format/HttpHeaderFormatter.java

components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java

components/api/src/main/java/com/hotels/styx/api/HttpRequest.java

components/api/src/main/java/com/hotels/styx/api/HttpHeader.java

components/api/src/main/java/com/hotels/styx/api/LiveHttpRequest.java

components/api/src/test/java/com/hotels/styx/api/HttpHeaderTest.java

components/proxy/src/main/java/com/hotels/styx/StyxServer.java

...s/server/src/test/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoderTest.java

- removed formatting from everywhere except HttpErrorStatusCauseLogger and HttpRequestMessageLogger - removed singleton - static import for emptyList

mikkokar · 2019-09-09T06:51:14Z

Fixes #454.

components/common/src/main/java/com/hotels/styx/common/format/HttpHeaderFormatter.java

components/api/src/main/java/com/hotels/styx/api/LiveHttpRequest.java

...s/client/src/main/java/com/hotels/styx/client/netty/connectionpool/HttpRequestOperation.java

components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java

components/common/src/main/java/com/hotels/styx/common/format/HttpHeaderFormatter.java

components/proxy/src/main/java/com/hotels/styx/Environment.java

components/proxy/src/main/java/com/hotels/styx/StyxServer.java

components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java

components/proxy/src/main/java/com/hotels/styx/proxy/HttpErrorStatusCauseLogger.java

components/proxy/src/main/java/com/hotels/styx/startup/StyxServerComponents.java

header toString reverted to use '=' instead of ':' request toString reverted to use 'uri' instead of 'url' added null checks for HttpMessageFormatter made HttpMessageFormatter an interface and added default implementation some other general tidying

- added null checks for HttpMessageFormatter - added default value for HttpMessageFormatter - removed redundant null checks

- removed redundant HttpMessageFormatter - general tidying

- removed formatting from everywhere except HttpErrorStatusCauseLogger and HttpRequestMessageLogger - removed singleton - static import for emptyList

header toString reverted to use '=' instead of ':' request toString reverted to use 'uri' instead of 'url' added null checks for HttpMessageFormatter made HttpMessageFormatter an interface and added default implementation some other general tidying

- added null checks for HttpMessageFormatter - added default value for HttpMessageFormatter - removed redundant null checks

- removed redundant HttpMessageFormatter - general tidying

…itise-http-messages # Conflicts: # components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java

mikkokar

Hi. It is pretty good. However I spotted an issue with access logging I feel we need to address before merging.

Also, please add a small Kotlin functional test to exercise this feature. Something as simple as configuring this feature to remove a header and a cookie, and then checking the access logs that they has been masked.

mikkokar · 2019-09-17T10:33:11Z

...em-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/HttpOutboundMessageLoggingSpec.scala

@@ -112,10 +112,10 @@ class HttpOutboundMessageLoggingSpec extends FunSpec
        assertThat(logger.log.size(), is(2))

        assertThat(logger.log(), hasItem(loggingEvent(INFO,
-          "requestId=[-a-z0-9]+, request=\\{method=GET, uri=http://localhost:[0-9]+/foobar, origin=\"localhost:[0-9]+\", headers=\\[.*\\]}")))
+          "requestId=[-a-z0-9]+, request=LiveHttpRequest\\{version=HTTP/1.1, method=GET, uri=http://localhost:[0-9]+/foobar, headers=\\[.*\\], id=[-a-z0-9]+}, origin=appOne:generic-app-01:localhost:[0-9]+")))


I see we are now exposing class names (implementation detail) in our access logs (a documented feature).

Let's remove them from the access logs. The class names are okay for error logs etc but not here. As a documented feature we need to consider the access logs like any other external interface with possible backwards compatibility implications.

Therefore please remove the class name from this message.

Also, I'd move the origin either in its original place, or in front of the request message. My reasoning is:

Trying to keep the message format as similar as possible

Headers field is often really large, and therefore should appear after all "fixed size" attributes. This is because it is difficult to find and see the fixed size attributes after a large body of header text.

The origin isn't part of the request, so I moved it out. Good point about long headers field. I'll move it to in front.

The origin isn't part of the request, so I moved it out.

Yeah, it was a right thing to do.

moved to front and removed class name

components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java

components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java

…order of logging items so request is at end - added headers to StaticResponseHandler to support kotlin test

mikkokar · 2019-09-27T09:32:48Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/StaticResponseHandler.java

+            return new StaticResponseHandler(config.status, config.response, nettyHeaders);
+        }
+
+        private HttpHeaders buildNettyHeaders(StaticResponseConfig config) {


A small comment: "Netty headers".

…sed imports

mikkokar

Looks good 🥇

Please fix the static analysis failures, and also consider couple of small small cosmetic comments.

Approving now.

mikkokar · 2019-09-27T09:45:04Z

system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt

+        feature("Styx request/response logging") {
+
+            scenario("Logger should hide cookies and headers") {
+                val proxyHost = "${styxServer.proxyHttpAddress().hostName}:${styxServer.proxyHttpAddress().port}"


Use StyxServer.proxyHttpHostHeader extension method from support/styxServerProvider.kt to simplify proxyHost.

mikkokar · 2019-09-27T09:45:32Z

system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt

+                val proxyHost = "${styxServer.proxyHttpAddress().hostName}:${styxServer.proxyHttpAddress().port}"
+
+                client.send(HttpRequest.get("/a/path")
+                        .header(HttpHeaderNames.HOST, proxyHost)


Static imports x2 ^^^.

mikkokar · 2019-09-27T09:49:00Z

system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt

+
+    val styxServer = StyxServer(StyxServerComponents.Builder()
+            .styxConfig(StyxConfig.fromYaml(yamlText))
+            .build())


There is a class called StyxServerProvider in support/StyxServerProvider.kt. It provides a unique reference point to a Styx server while allowing it to be restarted, and thus cleared from any acquired state`, between the tests.

You don't really have to use it in this test, but could make this more consistent with others.

mikkokar · 2019-09-27T09:53:56Z

system-tests/ft-suite/src/test/kotlin/com/hotels/styx/logging/HttpMessageLoggingSpec.kt

+                        .header("cookie", "cookie1=c1;cookie2=c2")
+                        .build())
+                        .toMono()
+                        .block()!!


There is also a wait extension method that combines .toMono().block()!!.

Owen Lindsell added 2 commits September 5, 2019 17:04

GitHub#454 Sanitise HTTP message logs

08d48d0

GitHub#454 removed unused import

a913858

mikkokar suggested changes Sep 6, 2019

View reviewed changes

dvlato suggested changes Sep 6, 2019

View reviewed changes

Owen Lindsell added 2 commits September 6, 2019 16:47

GitHub#454 fixes for PR comments:

5d4e48c

- removed formatting from everywhere except HttpErrorStatusCauseLogger and HttpRequestMessageLogger - removed singleton - static import for emptyList

GitHub#454 added documentation for the hideHeaders/hideCookies feature

4a114c5

mikkokar suggested changes Sep 12, 2019

View reviewed changes

components/common/src/main/java/com/hotels/styx/common/format/HttpHeaderFormatter.java Outdated Show resolved Hide resolved

mikkokar suggested changes Sep 13, 2019

View reviewed changes

Owen Lindsell added 4 commits September 16, 2019 15:56

GitHub#454

fa658a8

- added null checks for HttpMessageFormatter - added default value for HttpMessageFormatter - removed redundant null checks

GitHub#454

295f249

- removed redundant HttpMessageFormatter - general tidying

GitHub#454 made formatter final

a12c7a8

OwenLindsell requested review from mikkokar and dvlato September 16, 2019 16:04

dvlato approved these changes Sep 16, 2019

View reviewed changes

Owen Lindsell added 13 commits September 17, 2019 09:04

GitHub#454 Sanitise HTTP message logs

1e3bfbe

GitHub#454 removed unused import

6c76166

GitHub#454 fixes for PR comments:

d47c9d9

- removed formatting from everywhere except HttpErrorStatusCauseLogger and HttpRequestMessageLogger - removed singleton - static import for emptyList

GitHub#454 added documentation for the hideHeaders/hideCookies feature

63d6b47

GitHub#454

d827a53

- added null checks for HttpMessageFormatter - added default value for HttpMessageFormatter - removed redundant null checks

GitHub#454

e0df682

- removed redundant HttpMessageFormatter - general tidying

GitHub#454 made formatter final

6f97a05

GitHub#454 fixed checkstyle errors

7315f50

Merge remote-tracking branch 'origin/sanitise-http-messages' into san…

4a842e5

…itise-http-messages # Conflicts: # components/common/src/main/java/com/hotels/styx/common/format/DefaultHttpMessageFormatter.java

GitHub#454 fixed checkstyle errors

58686b5

GitHub#454 fixed checkstyle errors

ae7d61a

GitHub#454 fixed checkstyle errors

ccc972a

mikkokar suggested changes Sep 17, 2019

View reviewed changes

Owen Lindsell and others added 2 commits September 18, 2019 09:24

GitHub#454 fixed formatting in response to PR comments

9ad9f97

GitHub#454 Made fixes for PR comments: - added kotlin test - changed …

e309d19

…order of logging items so request is at end - added headers to StaticResponseHandler to support kotlin test

mikkokar reviewed Sep 27, 2019

View reviewed changes

OwenLindsell added 2 commits September 27, 2019 10:34

GitHub#454 fixed naming - nettyHeaders to httpHeaders and removed unu…

3d11609

…sed imports

GitHub#454 fixed imports for checkstyle

6c20fd2

mikkokar approved these changes Sep 27, 2019

View reviewed changes

mikkokar reviewed Sep 27, 2019

View reviewed changes

GitHub#454 tidied test

34a5329

OwenLindsell merged commit ed1b6fd into ExpediaGroup:master Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub#454 Sanitise HTTP message logs #457

GitHub#454 Sanitise HTTP message logs #457

OwenLindsell commented Sep 6, 2019

mikkokar commented Sep 9, 2019

mikkokar left a comment

mikkokar Sep 17, 2019

OwenLindsell Sep 17, 2019

mikkokar Sep 17, 2019

OwenLindsell Sep 18, 2019 •

edited

Loading

mikkokar Sep 27, 2019

mikkokar left a comment

mikkokar Sep 27, 2019

mikkokar Sep 27, 2019

mikkokar Sep 27, 2019

mikkokar Sep 27, 2019

GitHub#454 Sanitise HTTP message logs #457

GitHub#454 Sanitise HTTP message logs #457

Conversation

OwenLindsell commented Sep 6, 2019

mikkokar commented Sep 9, 2019

mikkokar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenLindsell Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkokar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenLindsell Sep 18, 2019 •

edited

Loading