Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http_proxy: print proxy errors in response body #502

Merged
merged 4 commits into from
Nov 9, 2023
Merged

Conversation

Choraden
Copy link
Contributor

@Choraden Choraden commented Nov 3, 2023

Responses will now contain proxy name and error:
First line is a generic message, second the error.

curl -x localhost:3128 not.exist   
forwarder failed to connect to remote host
dial tcp: lookup not.exist on 192.168.1.1:53: no such host

Header:

X-Forwarder-Error: forwarder dial tcp: lookup www on 10.255.3.12:53: no such host

http_proxy_errors.go Outdated Show resolved Hide resolved
@mmatczuk
Copy link
Contributor

mmatczuk commented Nov 7, 2023

Can it be like

forwarder failed to connect to remote host
dial tcp: lookup not.exist on 192.168.1.1:53: no such host

@mmatczuk
Copy link
Contributor

mmatczuk commented Nov 7, 2023

Also, check the response headers, the proxy name must be both in headers and body.

var body bytes.Buffer
body.WriteString(hp.config.Name)
body.WriteString(" ")
body.WriteString(strings.ToLower(msg))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we fixed the messages so that they do not start with upper case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -121,7 +130,7 @@ func handleStatusText(req *http.Request, err error) (code int, msg, label string
if req.URL.Scheme == "https" && err != nil {
for i := 400; i < 600; i++ {
if err.Error() == http.StatusText(i) {
return i, err.Error(), "https_status_text"
return i, strings.ToLower(err.Error()), "https_status_text"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add some text here, "encountered an unexpected internal error: "

@@ -121,7 +130,8 @@ func handleStatusText(req *http.Request, err error) (code int, msg, label string
if req.URL.Scheme == "https" && err != nil {
for i := 400; i < 600; i++ {
if err.Error() == http.StatusText(i) {
return i, err.Error(), "https_status_text"
msg = fmt.Sprintf("encountered an unexpected internal error: %s", strings.ToLower(err.Error()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do we need strings.ToLower here?

@@ -50,16 +51,24 @@ func (hp *HTTPProxy) errorResponse(req *http.Request, err error) *http.Response
}
if code == 0 {
code = http.StatusInternalServerError
msg = "An unexpected error occurred"
msg = "an unexpected error occurred"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not play well with config.Name

@Choraden Choraden force-pushed the hg/err_in_body branch 2 times, most recently from a8ec8e3 to 00acc8b Compare November 8, 2023 09:04

// Check if the error message is correctly propagated to the client.
// Especially when several proxies are chained.
if !strings.Contains(string(res.Body), expectedErrorMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, just skip it if MITM is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that our current httpexpect implementation also does the same trick, so all https requests made by the client do not return an original response in case of a connect error.

func (c *Client) do(req *http.Request) (*http.Response, error) {
	if c.trace != nil {
		req = req.WithContext(httptrace.WithClientTrace(req.Context(), c.trace))
	}
	resp, err := c.rt.RoundTrip(req)

	// There is a difference between sending HTTP and HTTPS requests.
	// For HTTPS client issues a CONNECT request to the proxy and then sends the original request.
	// In case the proxy responds with status code 4XX or 5XX to the CONNECT request, the client interprets it as URL error.
	//
	// This is to cover this case.
	if req.URL.Scheme == "https" && err != nil {
		for i := 400; i < 600; i++ {
			if err.Error() == http.StatusText(i) {
				return &http.Response{
					StatusCode: i,
					Status:     http.StatusText(i),
					ProtoMajor: 1,
					ProtoMinor: 1,
					Header:     http.Header{},
					Body:       http.NoBody,
					Request:    req,
				}, nil
			}
		}
	}

	return resp, err
}

Solution 1. is to keep this test and later address it in both proxy http client and our http expect client.

Solution 2. is to limit the scope and make it a dedicated MITM flag test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wanted to print a warning there, but I'm afraid, it won't be visible in a non-debug mode. So there we have this "half skip" - it always checks the status code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add text checking only for http requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@Choraden Choraden force-pushed the hg/err_in_body branch 2 times, most recently from 50f5d7e to 2df651f Compare November 8, 2023 14:40
@mmatczuk mmatczuk merged commit 4a91f5f into main Nov 9, 2023
4 checks passed
@mmatczuk mmatczuk deleted the hg/err_in_body branch November 9, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants