Enforce deny-by-default approach on the admin listener by matching on exact paths and on GET requests

[davinci26]

We want to block all requests to the admin listener endpoint that are not GET. In particular someone know can do /runtime_modify?key1=value1&key2=value2&keyN=valueN to change runtime variables such as the max regexp program size

This is mostly for security reasons to prevent any potential attacks that could happen by an attacker modifying the runtime configuration of Envoy (or any other configuration).

Note that since shutdownmanager.go uses the admin socket /admin/admin.sock to send a POST request it should be unaffected by this change.

Some manual verifications:

curl http://localhost:9001/ready
LIVE

curl -vv --request POST http://localhost:9001/runtime
*   Trying [::1]:9001...
* Connected to localhost (::1) port 9001
> POST /runtime HTTP/1.1
> Host: localhost:9001
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< date: Wed, 15 May 2024 22:15:56 GMT
< server: envoy
< content-length: 0
<
* Connection #0 to host localhost left intact

[davinci26]

(will check CI a bit later)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.61%. Comparing base (7530c06) to head (0817f87).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6447      +/-   ##
==========================================
+ Coverage   81.60%   81.61%   +0.01%     
==========================================
  Files         133      133              
  Lines       15842    15854      +12     
==========================================
+ Hits        12928    12940      +12     
  Misses       2620     2620              
  Partials      294      294              

Files	Coverage Δ
internal/envoy/v3/stats.go	100.00% <100.00%> (ø)

[tsaarni]

Looks good to me, but a question: is there any reason not to change to path (exact) match instead of prefix?

[sunjayBhatia]

Looks good to me, but a question: is there any reason not to change to path (exact) match instead of prefix?

yeah thats a good shout, esp. since we explicitly list out all the /stats/* paths we want to expose, an exact match should also work and lock things down even more:

contour/internal/envoy/v3/stats.go

Lines 105 to 107 in c739e44

	"/stats",
	"/stats/prometheus",
	"/stats/recentlookups",

to be pedantic might just need to check that query params work fine with an exact match but I would expect that does just fine

[tsaarni]

to be pedantic might just need to check that query params work fine with an exact match but I would expect that does just fine

Yes, it seems it should since doc says

If specified, the route is an exact path rule meaning that the path must exactly match the :path header once the query string is removed

but good to double check by testing it as well 😅

[davinci26]

I thought it would be more future proof that way, in case there is an endpoint that supports both GET/POST so next time someone adds one we dont have to double check if it supports both GET and POST

[davinci26]

@sunjayBhatia @tsaarni I gave my reasoning but I will leave the final call to you. Just let me know what you prefer

[tsaarni]

I thought it would be more future proof that way, in case there is an endpoint that supports both GET/POST so next time someone adds one we dont have to double check if it supports both GET and POST

I'm not sure if I understood, but just to make sure there is no confusion: I also think that permitting only GET is good improvement. But it could be even safer to also change path matching from prefix to exact match to make sure we follow "deny-by-default" approach.

[davinci26]

But it could be even safer to also change path matching from prefix to exact match to make sure we follow "deny-by-default" approach.

Now I got you :D I thought it was mutually exclusive, will update

[davinci26]

🫡 updated

                {
                 "match": {
                  "path": "/stats/prometheus",
                  "headers": [
                   {
                    "name": ":method",
                    "string_match": {
                     "exact": "GET",
                     "ignore_case": true
                    }
                   }
                  ]
                 },

and then:

curl -vv  'http://localhost:9001/stats?usedonly'
*   Trying [::1]:9001...
* Connected to localhost (::1) port 9001
> GET /stats?usedonly HTTP/1.1
> Host: localhost:9001
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK

[tsaarni]

LGTM! Thank you!

[skriss]

One small comment, otherwise LGTM as well, thanks @davinci26!

[sunjayBhatia]

LGTM also tested it out manually and looks good