Project

General

Profile

Actions

Bug #11537

closed

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

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
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 open1 closed)

Task #11573: Review 11537-keepproxy-loopResolvedRadhika Chippada04/20/2017Actions
Actions #1

Updated by Tom Clegg almost 7 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 #2

Updated by Tom Morris almost 7 years ago

  • Target version set to 2017-05-10 sprint
  • Story points set to 1.0
Actions #3

Updated by Tom Clegg almost 7 years ago

  • Assigned To set to Tom Clegg
Actions #4

Updated by Tom Clegg almost 7 years ago

  • Status changed from New to In Progress
Actions #5

Updated by Tom Clegg almost 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 #6

Updated by Radhika Chippada almost 7 years ago

11537-keepproxy-loop LGTM

Actions #7

Updated by Tom Clegg almost 7 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:e56ae6aad06c37d5512537047871d7363dd97620.

Actions

Also available in: Atom PDF