api: Disable body limit middleware for admin endpoints.

[winder]

Summary

Disable body limit middleware for admin endpoints.

Test Plan

Unit test.

[jannotti]

I'm not sure exactly what the behavior of this middleware is, but I think I'd like to see some limit, even on admin access. Otherwise, I suspect algod can be crashed by a request that just never ends. Eventually the server exhausts memory by reading arbitrary data.

I guess you could say, "well, admins can crash the server" and I'm actually somewhat sympathetic to that if it's annoying to implement. But if it's just a different limit in the admin middleware, let's do it, with maybe 100mb?

[winder]

I'm not sure exactly what the behavior of this middleware is, but I think I'd like to see some limit, even on admin access. Otherwise, I suspect algod can be crashed by a request that just never ends. Eventually the server exhausts memory by reading arbitrary data.

I guess you could say, "well, admins can crash the server" and I'm actually somewhat sympathetic to that if it's annoying to implement. But if it's just a different limit in the admin middleware, let's do it, with maybe 100mb?

"admins can crash the server" was my thinking here. It would also be simple to add different middlewares for each set of endpoints, but is 100MB is the right number? Without some rational about why there should be a limit and what it should be, I'd rather leave it open.

[jannotti]

10mb had no rationale beyond "not large enough to exhaust memory, and should be enough for anything"

But we forgot part keys. So let's pick a value that still won't exhaust memory, but is big enough for the number of part keys we want to support.

By the way, what I'm really worried about is that leaving it open means that a request could be arbitrarily large, even before it is authenticated as admin.

[winder]

By the way, what I'm really worried about is that leaving it open means that a request could be arbitrarily large, even before it is authenticated as admin.

I don't know if this alleviates the concern, but the token check happens ahead of any reading. So you don't need to worry about this part.

Since it's rate limiting is implemented as a middleware, the enforcement needs to happen inline to the handler call chain. It does so using two mechanisms, each of these happen ahead of our handler code:

1. check the content size and return if it reports the size is too large (a malicious request would not set the content size correctly, so this doesn't help with your concern)
2. the response body reader is wrapped with a rate limited reader that returns an error once the configured number of bytes is reached.

[Eric-Warehime]

LGTM but needs rebase

indexer_test.go:88: 
        	Error Trace:	/opt/cibuild/project/node/indexer/indexer_test.go:88
        	            				/opt/cibuild/project/node/indexer/suite.go:132
        	            				/opt/cibuild/project/node/indexer/indexer_test.go:164

[codecov]

Codecov Report

Merging #5486 (63c1b1e) into master (b06de44) will increase coverage by 0.31%.
The diff coverage is 6.66%.

@@            Coverage Diff             @@
##           master    #5486      +/-   ##
==========================================
+ Coverage   55.41%   55.72%   +0.31%     
==========================================
  Files         446      446              
  Lines       63289    63291       +2     
==========================================
+ Hits        35069    35267     +198     
+ Misses      25826    25652     -174     
+ Partials     2394     2372      -22     

Impacted Files	Coverage Δ
daemon/algod/api/server/router.go	16.32% <0.00%> (-0.70%)	⬇️
daemon/algod/api/server/v2/test/helpers.go	76.56% <100.00%> (+1.04%)	⬆️

... and 35 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[algochoi]

Nice, LGTM

[algochoi]

👍

[onetechnical]

"admins can crash the server" was my thinking here. It would also be simple to add different middlewares for each set of endpoints, but is 100MB is the right number? Without some rational about why there should be a limit and what it should be, I'd rather leave it open.

10 million rounds (at the long end) is what we would recommend as maximum if you do NOT have a large amount of stake:

4.3s era 10m rounds = 497.7 days
3.7s era 10m rounds = 428.2 days
3.2s era 10m rounds = 370.4 days

I'm not inclined to recommend increasing this number, as longer than 1 year still seems too long.

That said, a 10m round participation key with dilution of 3162 is 192 MB, so I think an admin cap of, say, 250 MB might be sufficient. However, as this is limited to admin operations, I'm also inclined to leave it out, but your mileage may vary.