-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow /healthcheck/fail from not admin webpage #17508
Comments
What do you suggest? I could see different somewhat hacky ways of doing this such as custom signals, watching some file, listening on a unix domain socket, etc., but it seems like the system using Envoy should be able to protect from such external attacks? |
Could be something in the runtime config? Might not be the right place since its not only configurable using the local filesystem, but using a file watch was what i thought of as well |
I think changing the Admin server to listen on a socket file shared between containers would work for us, havent tried it yet with the admin server listener I see discussion in #2763 about making the admin server use a fully fledged listener, which would also be quite nice to give us the flexibility to fix it |
You should be able to change the admin config to use a unix domain socket today. |
I get a malformed IP address exception when trying to use a Pipe address type envoy/source/server/admin/admin.cc Lines 120 to 144 in 9bd86b8
if youre amenable to it, we can probably make the change to support it, seems simple as checking the requested address type and creating the correct listen socket type? |
I'm pretty positive people run admin over UDS, so not sure exactly what problem you are facing. I would debug it and if there is an actual bug, sure please fix it. |
yep youre right, had a typo somewhere else! yeah that should hopefully work for us, if we can also have some documentation how to still get to the admin page cc @stevesloka |
@sunjayBhatia you could suggest ie. admin:
address:
pipe: {"path":"/envoyproxy.sock"} curl --unix-socket /envoyproxy.sock http://localhost: Abstract sockets work too admin:
address:
pipe: {"path":"@/envoyproxy/admin"} package main
import (
"net/http"
"net"
"fmt"
"context"
"io/ioutil"
)
func main() {
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.DialContext = func(_ context.Context, _, _ string) (net.Conn, error) {
return net.Dial("unix", "@/envoyproxy/admin")
}
client := &http.Client{Transport: transport}
resp, err := client.Get("http://localhost:")
if err != nil {
fmt.Println(err)
return
}
defer resp.Body.Close()
fmt.Println(resp.StatusCode)
bytes, err := ioutil.ReadAll(resp.Body)
fmt.Println(string(bytes), err)
} |
Only worried if we use abstract sockets if we ever enable envoy running on windows we will have to make another change but yeah we should def do this |
I've got this working on a PR for Contour. I'll see if I can update docs in Envoy and also blog about it at least to explain the process if it helps anyone else out. =) Thanks everyone for the help and input! |
Title: Allow the result of calling
/healthcheck/fail
but not from the Admin WebpageDescription:
I'd like a way to tell Envoy to begin draining connections and start shutting down, but not require the admin page to do this. Currently we have a related CVE in Contour (GHSA-5ph6-qq5x-7jwc) due to this since you can create a Kubernetes Service type=ExternalName pointing to
localhost
which allow the admin webpage to be exposed.Contour has a way to enable a clean shutdown sequence (https://projectcontour.io/docs/v1.17.1/redeploy-envoy/) by first calling the
/healthcheck/fail
endpoint in Envoy and then polling for open connections.We also expose metrics on a new listener, so I can hide the rest of the admin functionality via that new listener. We're struggling with how to approach this. Our current solution is to disable externalName service but users want to use them for various different reasons.
The ultimate goal would be if we could drop the admin webpage from running, then we'd be able to better manage this issue.
The text was updated successfully, but these errors were encountered: