Bug #11537: [keepproxy] detect proxy loop (broken config) -- return an error if keepproxy receives a request from itself - Arvados

Actions

Copy link

Bug #11537

closed

[keepproxy] detect proxy loop (broken config) -- return an error if keepproxy receives a request from itself

Added by Tom Clegg almost 8 years ago. Updated over 7 years ago.

Status:

Resolved

Priority:

Normal

Assigned To:

Tom Clegg

Category:

Keep

Target version:

2017-05-10 sprint

Story points:

1.0

Description

The manual setup procedure makes it easy to misconfigure keepproxy and Nginx such that keepproxy fulfills requests by forwarding requests to itself. Even when setup/configuration is automated, bugs and races could cause this condition sometimes.

Keepproxy should detect this situation and return an error and log an appropriate message, to make it easier for a sysadmin to detect.

The HTTP "Via" header is good for this. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.45

Implementation:

in sdk/go/keepclient, add a field "Do func(*http.Request) (*http.Response, error)".
if kc.Do is not nil, keepclient should use that function instead of instead of kc.Client.Do().
keepproxy should set kc.Do to a func that adds a Via header (e.g., "Via: HTTP/1.1 keepproxy" where HTTP/1.1 is req.Proto) and then calls kc.Client.Do().
if an incoming request has a Via header containing the string " keepproxy", respond 500 and log an error message ("proxy loop detected: perhaps keepproxy is misidentified by gateway config as an external client, or its keep_services record does not have service_type=proxy?")

Subtasks 1 (0 open — 1 closed)

Actions

Copy link

Updated by Tom Clegg almost 8 years ago

Subject changed from [keepproxy] return an error if keepproxy receives a request from itself to [keepproxy] detect proxy loop (broken config) -- return an error if keepproxy receives a request from itself

Actions

Copy link

Updated by Tom Morris over 7 years ago

Target version set to 2017-05-10 sprint
Story points set to 1.0

Actions

Copy link

Updated by Tom Clegg over 7 years ago

Assigned To set to Tom Clegg

Actions

Copy link

Updated by Tom Clegg over 7 years ago

Status changed from New to In Progress

Actions

Copy link

Updated by Tom Clegg over 7 years ago

Implementation:

Instead of adding a Do field to keepclient.KeepClient as described, I changed the Client field's type from *http.Client to a HTTPClient interface. Otherwise, using the Do field would make the Client field meaningless, except that the "set appropriate timeout according to proxy/disk services" code would edit it uselessly.
In order to make the "loop detected" error visible to the outer keepproxy on writes, I had to adjust the keepclient error handling. Now, instead of just returning a generic "insufficient replicas" error object, it returns the errors returned by the backend servers, like we do for reads. I also updated the error identifiers to match Go conventions ("FooError" is a type, and "ErrFoo" is an object).

Functional:

Requests initiated by keepproxy have a "Via: HTTP/1.1 keepproxy" header where HTTP/1.1 is the protocol of the original request.
Responses returned by keepproxy have a "Via: HTTP/1.1 keepproxy" header. It's possible the actual upstream protocol (between keepproxy and keepstore) is HTTP/2 but this distinction isn't exposed by the keepclient interface, and seems unimportant anyway.

When a loop is detected, the client receives a 500 response and the server log says:

2017/04/27 15:24:49 proxy loop detected (request has Via: "HTTP/1.1 keepproxy"): perhaps keepproxy is misidentified by gateway config as an external client, or its keep_services record does not have service_type=proxy?

11537-keepproxy-loop @ baa1c3db1f635828dda3cebdc47d98b33bb6518d

Actions

Copy link