-
Notifications
You must be signed in to change notification settings - Fork 672
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
use unix socket for Envoy admin webpage #3934
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3934 +/- ##
==========================================
+ Coverage 77.54% 77.71% +0.16%
==========================================
Files 112 112
Lines 8879 8978 +99
==========================================
+ Hits 6885 6977 +92
- Misses 1844 1852 +8
+ Partials 150 149 -1
|
14f34dc
to
0466dd9
Compare
I think this is ready for a first look. The next bit would be to update all the docs which requires more drawing to fix up the diagram, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments, still gotta try it out locally but looks good generally
we'll probably need some documentation on how this is going to work and what new knobs/limitations we've added here
I think besides the removal of the admin endpoints that let you modify things from the listener at port 9001 everything is functionally pretty much the same, would be good to enumerate if there are any differences now for the release notes to check if there are any breaking changes or workflow changes we can further mitigate
nice addition to the e2e tests!
internal/envoy/bootstrap.go
Outdated
// Value of "localhost" is invalid. | ||
if address == "localhost" || | ||
net.ParseIP(address) != nil || | ||
!strings.Contains(address, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edge case, but someone could conceivably want to use a relative file path with just the file name here (e.g. just foo.sock
), which could be valid
maybe we should make the validation be an absolute file path to prevent this and make sure people are exact (e.g. check for /
at the start of the string)? or relax the validation a bit to not require a slash?
could also make it check for https://pkg.go.dev/os#PathSeparator so its not another step to cross-platform convert if we want to in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you'd have to map the path of that to where Envoy is running to make that work. Maybe this should change to just the path and not the name? Just trying to keep this super simple for folks.
I'll take it out and not require the /
.
internal/envoy/bootstrap.go
Outdated
if address == "localhost" || | ||
net.ParseIP(address) != nil || | ||
!strings.Contains(address, "/") { | ||
return fmt.Errorf("invalid value %q, must be a unix socket name", address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe worth separating the error out for the localhost and ip address cases to say explicitly they are not allowed
internal/envoy/v3/listener.go
Outdated
@@ -524,6 +524,17 @@ func SocketAddress(address string, port int) *envoy_core_v3.Address { | |||
}, | |||
} | |||
} | |||
// Check if the address is a socket. | |||
if strings.Contains(address, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use https://pkg.go.dev/os#PathSeparator here though this could get interesting if we had a socket address that the contour controller (not bootstrap) was programming somehow for a non-unix host
internal/xdscache/v3/listener.go
Outdated
stats := envoy_v3.StatsListener(address, port) | ||
return &ListenerCache{ | ||
admin := envoy_v3.AdminListener("127.0.0.1", 9001) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the admin port is configurable currently and I think that flag has kinda been dropped in a sense?
should we deprecate the flag? wire it through to make the listen port here for the read-only admin page configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could reuse the port flag, but maybe that's confusing?
I could reuse it for now, introduce a new flag and deprecate the old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, I'm mixing up the bits. This would be a new flag on Contour to specify. Yeah let's do that, introduce a new config file item I guess and deprecate the bootstrap port.
Regarding troubleshooting experience (ref discussion) I myself depend on port-forward + curl config_dump as well. Could the troubleshooting use case be covered by new sub-command on contour instead of having option to enable additional admin listener? Something like |
62526cb
to
93bd281
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but I think it's worth calling out that this change has implications for running Envoy on Windows. I think it's more important to have a secure Linux deployment for now, so it's fine, but something for @sunjayBhatiaand @xaleeks to have a chat about maybe?
windows does now support UDS but we would have to see if it works in this context specifically with envoy: https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ i can see permissions in an emptydir volume possibly being a hangup etc. and would need to test things out |
@tsaarni ended up adding a new listener which exposes the read-only endpoints from the admin webpage over 127.0.0.1:9001 so you can still port-forward and get to those stats, but not expose the dangerous bits. |
@stevesloka This is surely better but wouldn't it still leave Envoy vulnerable for information exposure when |
Yup it would indeed do that if you enabled external name services. You'd still need to find a domain that points to localhost, etc, etc. I'm also adding a config item to Contour to remove the readonly endpoint. So for folks who want to enable externalname, they can disable the entire admin interface. |
Maybe I can just have it so that if the port is set to |
Since the parameter is new in contour config, in theory not setting |
That's a good point @tsaarni about it defaulting to on. Maybe we should keep that for now and if you want to disable, you have to explicitly set that. We could change that behavior in the future if we'd like. I'm going to write up a docs PR next to explain all of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change makes sense, had a few comments inline. I guess we also need to confirm that we're comfortable making this change in a minor release, although it could be considered a breaking change, in the interest of security.
cmd/contour/bootstrap.go
Outdated
@@ -26,8 +26,8 @@ func registerBootstrap(app *kingpin.Application) (*kingpin.CmdClause, *envoy.Boo | |||
bootstrap := app.Command("bootstrap", "Generate bootstrap configuration.") | |||
bootstrap.Arg("path", "Configuration file ('-' for standard output).").Required().StringVar(&config.Path) | |||
bootstrap.Flag("resources-dir", "Directory where configuration files will be written to.").StringVar(&config.ResourcesDir) | |||
bootstrap.Flag("admin-address", "Envoy admin interface address.").StringVar(&config.AdminAddress) | |||
bootstrap.Flag("admin-port", "Envoy admin interface port.").IntVar(&config.AdminPort) | |||
bootstrap.Flag("admin-address", "Envoy admin interface address.").Default("/admin/admin.sock").StringVar(&config.AdminAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding to the help text to clarify this must be a path to a unix domain socket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cmd/contour/serve.go
Outdated
// The enableDebugListener boolean is defaulted to true which matches Contour's current behavior. | ||
// Future iterations can allow for this to be customized by the user if they desire to disable the | ||
// Envoy admin interface entirely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment stale? (Not seeing enableDebugListener
anywhere, I think that was replaced by looking at the value of the port).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup good catch. I initially had that flag in there but took it out to reduce the amount of configuration items.
@@ -80,3 +146,20 @@ func StatsListener(address string, port int) *envoy_listener_v3.Listener { | |||
SocketOptions: TCPKeepaliveSocketOptions(), | |||
} | |||
} | |||
|
|||
func serviceStatsRoute(prefix string) *envoy_route_v3.Route { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not too big a deal but service-stats
is a little confusing in this context since many of the admin routes are not stats-related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup I agree. That's what it got called initially. Not sure if we want to split off this admin stuff into a new one? But that seems like a waste. Also not sure if folks have the service-stats
in any dashboards, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to be fair we need to do a proper deprecation of this field. I really doubt this would be a breaking change for most, but not 100% sure. Ok to make a followup PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//cc @sunjayBhatia
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me, should err on the side of being careful 👍🏽
- Moves the Envoy admin interface to use a unix socket - Creates new listener from Contour to expose read only information from Envoy admin page over port 9001 - Updates shutdown-manager to use new unix socket to begin Envoy draining procedure Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
176e503
to
16aa991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some small questions left from me, this is looking really great.
if address == "localhost" || net.ParseIP(address) != nil { | ||
return fmt.Errorf("invalid value %q, cannot be `localhost` or an ip address", address) | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check that the unix socket exists, or at least that the path is a valid one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the timing of that type of check. This is the bootstrap code which executes in the initContainer. Then the pod would startup after that. Need to double check if the volumes are all ready in init phase or not, I don't remember off the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So volumes should be there, but Envoy creates the actual socket, so we can't check for this in the bootstrap since Envoy's not yet started.
return &ListenerCache{ | ||
func NewListenerCache(config ListenerConfig, statsAddress string, statsPort, adminPort int) *ListenerCache { | ||
stats := envoy_v3.StatsListener(statsAddress, statsPort) | ||
admin := envoy_v3.AdminListener("127.0.0.1", adminPort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set this to the socket name to make sure there's no confusion, or is this only used if we're exposing the admin back on a network interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to create a listener which exposes the read-only bits from the admin page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
largely looks good! interested in the comment(s) around "service-stats" name etc. and not sure as well if it is ok to change that etc.
Signed-off-by: Steve Sloka <slokas@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last nit then I think this LGTM
bah, didn't push, sorry. fixed up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no further comments, LGTM!
Move to Envoy Admin over unix socket to mitigate security issues with external name services. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Updates the redeploy envoy docs as well as the Contour configuration docs for the Envoy Admin interface running as a unix socket. Updates projectcontour#3934 Signed-off-by: Steve Sloka <slokas@vmware.com>
Updates the redeploy envoy docs as well as the Contour configuration docs for the Envoy Admin interface running as a unix socket. Updates projectcontour#3934 Signed-off-by: Steve Sloka <slokas@vmware.com>
Updates the redeploy envoy docs as well as the Contour configuration docs for the Envoy Admin interface running as a unix socket. Updates #3934 Signed-off-by: Steve Sloka <slokas@vmware.com>
…erver. Squashed commit of the following: commit da2f09a Author: Zhenhuan <zhenhli@microsoft.com> Date: Thu Nov 4 12:58:50 2021 +0800 update cert provider client commit 9c7085f Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Nov 3 16:44:44 2021 +0800 update commit 61aaea5 Author: Zhenhuan <zhenhli@microsoft.com> Date: Tue Nov 2 12:40:26 2021 +0800 Use mariner as base image commit 4db3590 Author: Zhenhuan <zhenhli@microsoft.com> Date: Tue Nov 2 12:01:24 2021 +0800 Skip vertify bootstrap options when loading cert from sds server commit 83aba4c Author: Zhenhuan <zhenhli@microsoft.com> Date: Tue Nov 2 11:32:54 2021 +0800 Rename flags commit eb81432 Author: Zhenhuan <zhenhli@microsoft.com> Date: Mon Nov 1 17:30:50 2021 +0800 Add cert options to cli to get certs from cert server commit c5cea20 Author: Zhenhuan <zhenhli@microsoft.com> Date: Fri Oct 29 18:21:45 2021 +0800 avoid calling certificate loader repeatly commit e95e61c Author: Zhenhuan <zhenhli@microsoft.com> Date: Fri Oct 29 16:52:09 2021 +0800 Add more logs commit db5263d Author: Zhenhuan <zhenhli@microsoft.com> Date: Fri Oct 29 15:37:52 2021 +0800 Fix tls connection error with envoy commit 18152d2 Author: Chloe Wang <qiwang@microsoft.com> Date: Fri Oct 29 10:11:52 2021 +0800 move log.printf into retry loop commit 3996b4d Author: Chloe Wang <qiwang@microsoft.com> Date: Thu Oct 28 22:12:26 2021 +0800 retry mechanism to connect contour certificate loader commit 3deac77 Author: Chloe Wang <qiwang@microsoft.com> Date: Mon Oct 25 11:55:57 2021 +0800 fix bug commit 7ad7be8 Author: Chloe Wang <qiwang@microsoft.com> Date: Mon Oct 25 11:21:39 2021 +0800 add retry mechanism to load cert from certificate loader commit 2caf5eb Author: Zhenhuan <zhenhli@microsoft.com> Date: Thu Oct 21 12:35:38 2021 +0800 Fix bugs commit 25b59c3 Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 19:06:38 2021 +0800 Fix crash commit 1703f8f Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 18:52:11 2021 +0800 update commit 1444e94 Merge: 73b7dbf 7168c61 Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 18:26:29 2021 +0800 Merge branch 'users/chloewang/loadcertfromserver' into users/zhenhli/certificate-issuer commit 7168c61 Merge: 4441c5d cabf4bc Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 18:25:15 2021 +0800 Merge branch 'users/chloewang/loadcertfromserver' of ssh.dev.azure.com:v3/skype/ES/dev_azure_contour into users/chloewang/loadcertfromserver commit 4441c5d Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 18:24:10 2021 +0800 Load envoy cert from sds server commit 73b7dbf Author: Zhenhuan <zhenhli@microsoft.com> Date: Wed Oct 20 14:35:30 2021 +0800 Fix typo commit cabf4bc Author: Chloe Wang <qiwang@microsoft.com> Date: Wed Oct 20 14:30:40 2021 +0800 fix flag name commit 20f6bdd Author: Chloe Wang <qiwang@microsoft.com> Date: Tue Oct 19 15:44:03 2021 +0800 add command load-cert-from-file to allow to load cerlocally or remotely commit ee37eea Author: Zhenhuan <zhenhli@microsoft.com> Date: Mon Oct 18 13:22:15 2021 +0800 Update document commit 638a47a Author: Zhenhuan <zhenhli@microsoft.com> Date: Thu Oct 14 11:00:37 2021 +0800 Update commit 54eb3f3 Author: Zhenhuan <zhenhli@microsoft.com> Date: Tue Oct 12 17:18:49 2021 +0800 Update commit 99756f9 Author: Zhenhuan <zhenhli@microsoft.com> Date: Tue Oct 12 17:06:38 2021 +0800 Fix crashes commit 00f5c65 Author: Zhenhuan <zhenhli@microsoft.com> Date: Mon Oct 11 17:13:49 2021 +0800 Fix crash commit 5ee68ad Author: Zhenhuan <zhenhli@microsoft.com> Date: Mon Oct 11 16:54:30 2021 +0800 update commit caf0115 Author: Zhenhuan <zhenhli@microsoft.com> Date: Mon Oct 11 16:37:37 2021 +0800 Support external sds server commit 6a583fc Author: zhengyangdu@microsoft.com <zhengyangdu@microsoft.com> Date: Mon Oct 11 11:06:43 2021 +0800 support SDS server commit f362f4d Author: Sunjay Bhatia <sunjayb@vmware.com> Date: Thu Aug 26 14:44:01 2021 +0000 Update Contour Docker image to v1.18.1. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> commit 8a36305 Author: Sunjay Bhatia <5337253+sunjayBhatia@users.noreply.github.com> Date: Wed Aug 25 15:16:23 2021 -0400 Cherrypick projectcontour#3934 (projectcontour#3982) Move to Envoy Admin over unix socket to mitigate security issues with external name services. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Steve Sloka <slokas@vmware.com> commit 755c317 Author: Steve Kriss <krisss@vmware.com> Date: Wed Aug 25 07:03:05 2021 -0600 update Envoy to v1.19.1 (projectcontour#3966) Signed-off-by: Steve Kriss <krisss@vmware.com> commit 889ec61 Author: Sunjay Bhatia <sunjayb@vmware.com> Date: Wed Jul 28 11:51:29 2021 -0400 Update Contour Docker image to v1.18.0. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
from Envoy admin page over port 9001
draining procedure
Signed-off-by: Steve Sloka slokas@vmware.com