Allow max request size to be user-specified #4824

jefferai · 2018-06-24T16:40:50Z

This turned out to be way more impactful than I'd expected because I
felt like the right granularity was per-listener, since an org may want
to treat external clients differently from internal clients. It's pretty
straightforward though.

This also introduces actually using request contexts for values, which
so far we have not done (using our own logical.Request struct instead),
but this allows non-logical methods to still get this benefit.

This turned out to be way more impactful than I'd expected because I felt like the right granularity was per-listener, since an org may want to treat external clients differently from internal clients. It's pretty straightforward though. This also introduces actually using request contexts for values, which so far we have not done (using our own logical.Request struct instead), but this allows non-logical methods to still get this benefit.

jefferai · 2018-06-24T16:42:58Z

Pretty trivially tested (the acutal limiting logic is the same, just the plumbing):

Limiting to smaller than default:

$ vault kv put -address=http://127.0.0.1:8200 secret/foo [email protected]
Key              Value
---              -----
created_time     2018-06-24T16:27:25.632026852Z
deletion_time    n/a
destroyed        false
version          1

$ vault kv put -address=http://127.0.0.1:8202 secret/foo [email protected]
Error writing data to secret/data/foo: Error making API request.

URL: PUT http://127.0.0.1:8202/v1/secret/data/foo
Code: 413. Errors:

* failed to parse JSON input: http: request body too large

$ truncate --size=21000 CHANGELOG.md 

$ vault kv put -address=http://127.0.0.1:8202 secret/foo [email protected]
Key              Value
---              -----
created_time     2018-06-24T16:28:44.109742609Z
deletion_time    n/a
destroyed        false
version          2

Unlimited (ignore that I accidentally used vault write, doesn't matter):

$ vault write -address=http://127.0.0.1:8200 secret/foo k=@bin/vault
Error writing data to secret/foo: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/secret/foo
Code: 413. Errors:

* failed to parse JSON input: http: request body too large

$ vault write -address=http://127.0.0.1:8202 secret/foo k=@bin/vault
Error writing data to secret/foo: Error making API request.

URL: PUT http://127.0.0.1:8202/v1/secret/foo
Code: 404. Errors:


WARNING! The following warnings were returned from Vault:

  * Invalid path for a versioned K/V secrets engine. See the API docs for the
  appropriate API endpoints to use. If using the Vault CLI, use 'vault kv put'
  for this operation.

kalafut · 2018-06-24T16:57:10Z

helper/forwarding/util.go

+		}
+	}
+
+	buf := bytes.NewBuffer(nil)


Could use ioutil.ReadAll() here since you want []byte later.

That's fair, I just kept that part of the logic that was there. Feel free to pull down the branch and change it yourself if you like, too!

jefferai · 2018-06-24T21:10:42Z

Thanks!

vishalnayak · 2018-06-25T14:06:55Z

command/server.go

@@ -792,7 +808,9 @@ CLUSTER_SYNTHESIS_COMPLETE:
 	// This needs to happen before we first unseal, so before we trigger dev
 	// mode if it's set
 	core.SetClusterListenerAddrs(clusterAddrs)
-	core.SetClusterHandler(vaulthttp.Handler(core))
+	core.SetClusterHandler(vaulthttp.Handler(&vault.HandlerProperties{
+		Core: core,


Won't we ever need a size limit on the cluster connection?

By the time we get to the HTTP handler we've already handshaked the cluster connection so we already know if the connection is authorized -- if so we shouldn't limit it.

vishalnayak · 2018-06-25T14:11:46Z

http/handler.go

+			return errors.New("could not parse max_request_size from request context")
+		}
+		if max > 0 {
+			reader = http.MaxBytesReader(w, r.Body, max)


Out of curiosity, why do we use io.LimitReader above and http.MaxBytesReader here?

Different function signatures, based on what's available.

vishalnayak · 2018-06-25T14:13:43Z

vault/request_handling.go

@@ -26,6 +26,13 @@ const (
 	replTimeout = 10 * time.Second
 )

+// HanlderProperties is used to seed configuration into a vaulthttp.Handler.
+// It's in this package to avoid a circular dependency
+type HandlerProperties struct {


[Nitpick] Can we call this HandlerOptions instead, unless there was a reason to call this HandlerProperties?

I called it Properties since it's not just user-set options (e.g. Core).

jefferai added this to the 0.10.4 milestone Jun 24, 2018

kalafut reviewed Jun 24, 2018

View reviewed changes

Switch to ioutil.ReadAll()

e073371

Fix tests

a7e43b0

vishalnayak reviewed Jun 25, 2018

View reviewed changes

Merge branch 'master' into limit-request-size-user

21070dd

chrishoffman approved these changes Jul 6, 2018

View reviewed changes

jefferai merged commit 5a2d80e into master Jul 6, 2018

jefferai deleted the limit-request-size-user branch July 6, 2018 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow max request size to be user-specified #4824

Allow max request size to be user-specified #4824

jefferai commented Jun 24, 2018

jefferai commented Jun 24, 2018

kalafut Jun 24, 2018

jefferai Jun 24, 2018

kalafut Jun 24, 2018

jefferai commented Jun 24, 2018

vishalnayak Jun 25, 2018

jefferai Jul 5, 2018

vishalnayak Jun 25, 2018

jefferai Jul 5, 2018

vishalnayak Jun 25, 2018

jefferai Jul 5, 2018

Allow max request size to be user-specified #4824

Allow max request size to be user-specified #4824

Conversation

jefferai commented Jun 24, 2018

jefferai commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment