Customize handling of http 502 by viacheslav-fomin-main · Pull Request #12 · verygoodsecurity/LittleProxy

viacheslav-fomin-main · 2017-05-30T14:19:48Z

Prerequisite to https://github.com/verygoodsecurity/vault/pull/1380.

Adds ability to set up a custom http response on upstream server connection failure.

…ificate

viacheslav-fomin-main · 2017-05-30T19:38:57Z

-        FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body);
+        HttpResponseStatus status = HttpResponseStatus.BAD_GATEWAY;
+
+        if (cause instanceof SSLHandshakeException) {


the check may be improved

instead of doing this can we implement a method which can be overridden? this way implementors would be able to customize the message based on their own logic. if the writeBadGateway was protected for instance we could override it.

@mjallday hm, great idea!

viacheslav-fomin-main · 2017-05-31T11:33:01Z

@mjallday we can not just simply override writeBadRequest method as we do not inherit it. But your idea is implemented, the code is more complex though.

viacheslav-fomin-main · 2017-05-31T11:36:37Z

@@ -0,0 +1,37 @@
+package org.littleshoot.proxy;


The code in this class is copied from writeBadRequest method of ClientToProxyConnection class

pgyschuk

lgtm

viacheslav-fomin-main · 2017-05-31T15:58:31Z

@mjallday we need to come up with versioning system here..

mjallday · 2017-05-31T17:00:43Z

-     * @param httpRequest the HttpRequest that is resulting in the Bad Gateway response
-     * @return true if the connection will be kept open, or false if it will be disconnected
-     */
-    private boolean writeBadGateway(HttpRequest httpRequest) {


@viacheslav-fomin-main i see that the respondWithShortCircuitResponse method calls currentFilters.proxyToClientResponse.

i'm wondering if it's possible for us to implement similar functionality in that method without making this change.

e.g.

@Override public HttpObject handleProxyToClientResponse(HttpObject httpObject) { if (httpObect.statusCode() == HttpResponseStatus.BAD_GATEWAY) { // logic here } }

mjallday · 2017-05-31T17:22:50Z

+import io.netty.handler.codec.http.FullHttpResponse;
+import io.netty.handler.codec.http.HttpRequest;
+
+public interface ServerConnectionFailureHttpResponseComposer {


unsure about the naming on this. it seems wrong to limit it to just server connection failures. how about just HttpResponseComposer?

well, it should contain failure in it as it expects throwable cause.. it can not be used for constructing OK response.. so FailureHttpResponseComposer? @mjallday

javadoc is missing

mjallday · 2017-05-31T20:47:56Z

     **************************************************************************/

-    /**
-     * Tells the client that something went wrong trying to proxy its request. If the Bad Gateway is a response to


we should keep this comment i believe.

osklyarenko · 2017-06-01T08:25:43Z

+     *
+     * @param unrecoverableFailureHttpResponseComposer
+     * @return
+     */


Pls provide a proper javadoc

osklyarenko · 2017-06-01T08:27:16Z

+   * @return true if the connection will be kept open, or false if it will be disconnected
+   */
+  @Override
+  public FullHttpResponse compose(HttpRequest httpRequest, Throwable cause) {


we should unit test for this component.

osklyarenko · 2017-06-01T08:30:08Z

+  }
+
+  public FullHttpResponse compose(HttpRequest httpRequest) {
+    return this.compose(httpRequest, null);


seems like a leaky abstraction to me. let's just stick to compose(httpRequest, null); and not expose this method

osklyarenko · 2017-06-01T08:30:44Z

-        }
-
-        return respondWithShortCircuitResponse(response);
+    private boolean writeBadGateway (HttpRequest httpRequest) {


osklyarenko · 2017-06-01T09:23:24Z

+      // don't allow any body content in response to a HEAD request
+      response.content().clear();
+    }
+    return response;


the following part will be common between various implementations

FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body); if (ProxyUtils.isHEAD(httpRequest)) { // don't allow any body content in response to a HEAD request response.content().clear(); } return response;

so we should abstract it away

diff --git a/src/main/java/org/littleshoot/proxy/BadGatewayFailureHttpResponseComposer.java b/src/main/java/org/littleshoot/proxy/BadGatewayFailureHttpResponseComposer.java index 26bcd78..b7e8abd 100644 --- a/src/main/java/org/littleshoot/proxy/BadGatewayFailureHttpResponseComposer.java +++ b/src/main/java/org/littleshoot/proxy/BadGatewayFailureHttpResponseComposer.java @@ -20,7 +20,7 @@ public final class BadGatewayFailureHttpResponseComposer implements ServerConnec */ @Override public FullHttpResponse compose(HttpRequest httpRequest, Throwable cause) { - String body = "Bad Gateway: " + httpRequest.getUri(); + String body = provideCustomMessage(httpRequest, cause); FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body); @@ -31,6 +31,10 @@ public final class BadGatewayFailureHttpResponseComposer implements ServerConnec return response; } + protected String provideCustomMessage(HttpRequest httpRequest, Throwable cause) { + return "Bad Gateway: " + httpRequest.getUri(); + } + public FullHttpResponse compose(HttpRequest httpRequest) { return this.compose(httpRequest, null); }

this way you custom implementation would be a specialization of the default impl provided by BadGatewayFailureHttpResponseComposer.

osklyarenko

pls provide a better abstraction and add missing tests.

osklyarenko · 2017-06-01T12:49:06Z

+
+    FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body);
+
+    if (ProxyUtils.isHEAD(httpRequest)) {


not tested.

well, it is tested by the original littleproxy tests.. but okay, lets add additional test
@osklyarenko

Adds custom message when there is a problem with upstream server cert…

c7b71d4

…ificate

viacheslav-fomin-main commented May 30, 2017

View reviewed changes

viacheslav-fomin-main added 3 commits May 31, 2017 14:20

Makes the implementation generic.

0895f96

Renamed interface

f793f47

Removes unnecesssary if statement

08295fc

viacheslav-fomin-main commented May 31, 2017

View reviewed changes

viacheslav-fomin-main changed the title ~~Adds custom message when there is a problem with upstream server cert~~ Adds ability to set up a custom http response on upstream server connection failure May 31, 2017

Adds tests

bef40d7

pgyschuk approved these changes May 31, 2017

View reviewed changes

mjallday reviewed May 31, 2017

View reviewed changes

osklyarenko reviewed Jun 1, 2017

View reviewed changes

osklyarenko changed the title ~~Adds ability to set up a custom http response on upstream server connection failure~~ Customize handling of http 501 Jun 1, 2017

osklyarenko changed the title ~~Customize handling of http 501~~ Customize handling of http 502 Jun 1, 2017

osklyarenko self-requested a review June 1, 2017 10:27

osklyarenko suggested changes Jun 1, 2017

View reviewed changes

viacheslav-fomin-main added 3 commits June 1, 2017 14:03

Addresses code review comments

05fcfca

trigger build

48c70b8

trigger build 2

5cb5124

osklyarenko reviewed Jun 1, 2017

View reviewed changes

viacheslav-fomin-main added 2 commits June 1, 2017 16:14

Adds clear content on head request test

0790a6f

Added minor version

a3978a6

osklyarenko approved these changes Jun 1, 2017

View reviewed changes

osklyarenko merged commit 24f8ac9 into verygoodsecurity:vgs-edition Jun 1, 2017

osklyarenko added this to the Enhancements to HTTPS proxy milestone Jun 13, 2017

osklyarenko added enhancement ⚡️HIGH⚡️ customer request labels Jun 13, 2017

viacheslav-fomin-main mentioned this pull request Jun 13, 2017

Mitm manager factory #15

Merged


		FullHttpResponse response = ProxyUtils.createFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.BAD_GATEWAY, body);

		if (ProxyUtils.isHEAD(httpRequest)) {

Conversation

viacheslav-fomin-main commented May 30, 2017 • edited by osklyarenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viacheslav-fomin-main commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgyschuk left a comment

Choose a reason for hiding this comment

Uh oh!

viacheslav-fomin-main commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osklyarenko Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osklyarenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viacheslav-fomin-main commented May 30, 2017 •

edited by osklyarenko

Loading

osklyarenko Jun 1, 2017 •

edited

Loading