Don't fail on invalid HTTP request targets

[twz123]

Problem:
When broken or malicious clients issue HTTP requests with invalid request targets (e.g. wrongly escaped query parameters) in the HTTP Request Line, logbook fails by throwing a IllegalArgumentException/URISyntaxException. This results in unlogged HTTP requests and most probably in 500 Internal Server Error responses from HTTP backends.

Proposed fix:
Don't use java.net.URI as return type of BaseHttpRequest::getRequestUri, but a plain String. Implementation was straight forward, with one exception: Query Parameter Obfuscation. If the request URI is malformed, query parameters cannot be extracted in the usual way in order to apply the Obfuscator to them. Currently, malformed URIs are then forwarded as-is. This may not be desirable. What do you think?

[whiskeysierra]

oO

[twz123]

Yeah, seen in the wild by one of our services, along with a dozen similar
URIs, all leading 500s b/c of an IAE in URI.create.
Am 22.02.2016 12:32 schrieb "Willi Schönborn" notifications@github.com:

In
logbook-core/src/test/java/org/zalando/logbook/ObfuscatedHttpRequestTest.java
#49 (comment):

@@ -45,6 +45,20 @@
(contentType, body) -> body.replace("s3cr3t", "f4k3"));

 @Test

• public void shouldNotFailOnInvalidUri() {

•    final String invalidUri = "/af.cgi?_browser_out=.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2F.|.%2Fetc%2Fpasswd";
  

oO

—
Reply to this email directly or view it on GitHub
https://github.com/zalando/logbook/pull/49/files#r53613418.

[whiskeysierra]

Thanks for the pull request!

Implementation was straight forward, with one exception: Query Parameter Obfuscation. If the request URI is malformed, query parameters cannot be extracted in the usual way in order to apply the Obfuscator to them. Currently, malformed URIs are then forwarded as-is. This may not be desirable. What do you think?

That's a tough decision actually. I think the current approach is the best compromise, let's see what the others are saying.

[lukasniemeier-zalando]

👍

What would be the alternative? If we cannot parse the URI obfuscate it completely?

[whiskeysierra]

Yes. I'd assume since we can't read the parameters from the URI we can't obfuscate it partially, which would be the default strategy. So we are left with:

1. No obfuscation
2. Complete obfuscation

[twz123]

A complete obfuscation would make it impossible to analyze a possible attack, or broken client on the server side.

Another approach could be to try to parse the URL manually, in a more lenient way? A quick internet seach brought up for example OkHttp’s New URL Class: https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/HttpUrl.java.

[twz123]

We could also go for a dumb approach like this: twz123/logbook@757a56a

[jhorstmann]

I would actually prefer to not touch or try to parse the incoming url at all, instead to log it exactly as it was on the wire. This would mean changing the obfuscator though, probably to be based on regular expressions.

[whiskeysierra]

Sounds good to me, but that implies that we need to change the obfuscation
API as well, doesn't it?
On 22 Feb 2016 16:32, "Jörn Horstmann" notifications@github.com wrote:

I would actually prefer to not touch or try to parse the incoming url at
all, instead to log it exactly as it was on the wire. This would mean
changing the obfuscator though, probably to be based on regular expressions.

—
Reply to this email directly or view it on GitHub
#49 (comment).

[twz123]

While a better solution is being discussed, do you think this intermediary solution is good enough to be merged, so that we can get a new release that doesn't fail on invalid URLs?

[whiskeysierra]

While a better solution is being discussed, do you think this intermediary solution is good enough to be merged, so that we can get a new release that doesn't fail on invalid URLs?

I agree. The proposed solution is a step in the right direction, I think. For any further thoughts/plans/investigations we should open another issue.

Can someone else please approve with a 👍 so we can get this through?

[lukasniemeier-zalando]

👍

[whiskeysierra]

@twz123 I deployed 0.13.0 to central.

[whiskeysierra]

I created a followup issue #50

[twz123]

@whiskeysierra Perfect, thanks!