Skip to content
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

Disable remote proxy patching except AWS Lambda #17415

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changelog/17415.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```release-note:security
extensions: Disable remote downstream proxy patching by Envoy Extensions other than AWS Lambda. Previously, an operator with service:write ACL permissions for an upstream service could modify Envoy proxy config for downstream services without equivalent permissions for those services. This issue only impacts the Lua extension. [[CVE-2023-2816](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-2816)]
```

```release-note:breaking-change
extensions: The Lua extension now targets local proxy listeners for the configured service's upstreams, rather than remote downstream listeners for the configured service, when ListenerType is set to outbound in extension configuration. See [CVE-2023-2816](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-2816) changelog entry for more details.
```
Comment on lines +1 to +7
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also seen xds: used often, but extensions: felt ideal if we're generally using that one.

Copy link
Member Author

@zalimeni zalimeni May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @picatz and @hashicorp/consul-docs in case you have input on changelog (I'm happy to address after merge as well)

21 changes: 18 additions & 3 deletions agent/envoyextensions/builtin/aws-lambda/aws_lambda.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func Constructor(ext api.EnvoyExtension) (extensioncommon.EnvoyExtender, error)
if err := a.fromArguments(ext.Arguments); err != nil {
return nil, err
}
return &extensioncommon.BasicEnvoyExtender{
return &extensioncommon.UpstreamEnvoyExtender{
Extension: &a,
}, nil
}
Expand All @@ -65,7 +65,7 @@ func (a *awsLambda) validate() error {
// CanApply returns true if the kind of the provided ExtensionConfiguration matches
// the kind of the lambda configuration
func (a *awsLambda) CanApply(config *extensioncommon.RuntimeConfig) bool {
return config.Kind == config.OutgoingProxyKind()
return config.Kind == config.UpstreamOutgoingProxyKind()
}

// PatchRoute modifies the routing configuration for a service of kind TerminatingGateway. If the kind is
Expand All @@ -75,6 +75,11 @@ func (a *awsLambda) PatchRoute(r *extensioncommon.RuntimeConfig, route *envoy_ro
return route, false, nil
}

// Only patch outbound routes.
if extensioncommon.IsRouteToLocalAppCluster(route) {
return route, false, nil
}

for _, virtualHost := range route.VirtualHosts {
for _, route := range virtualHost.Routes {
action, ok := route.Action.(*envoy_route_v3.Route_Route)
Expand All @@ -95,6 +100,11 @@ func (a *awsLambda) PatchRoute(r *extensioncommon.RuntimeConfig, route *envoy_ro

// PatchCluster patches the provided envoy cluster with data required to support an AWS lambda function
func (a *awsLambda) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3.Cluster) (*envoy_cluster_v3.Cluster, bool, error) {
// Only patch outbound clusters.
if extensioncommon.IsLocalAppCluster(c) {
return c, false, nil
}

transportSocket, err := extensioncommon.MakeUpstreamTLSTransportSocket(&envoy_tls_v3.UpstreamTlsContext{
Sni: "*.amazonaws.com",
})
Expand Down Expand Up @@ -156,7 +166,12 @@ func (a *awsLambda) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_clus

// PatchFilter patches the provided envoy filter with an inserted lambda filter being careful not to
// overwrite the http filters.
func (a *awsLambda) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
func (a *awsLambda) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
// Only patch outbound filters.
if isInboundListener {
return filter, false, nil
}

if filter.Name != "envoy.filters.network.http_connection_manager" {
return filter, false, nil
}
Expand Down
43 changes: 37 additions & 6 deletions agent/envoyextensions/builtin/aws-lambda/aws_lambda_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TestConstructor(t *testing.T) {

if tc.ok {
require.NoError(t, err)
require.Equal(t, &extensioncommon.BasicEnvoyExtender{Extension: &tc.expected}, e)
require.Equal(t, &extensioncommon.UpstreamEnvoyExtender{Extension: &tc.expected}, e)
} else {
require.Error(t, err)
}
Expand Down Expand Up @@ -323,10 +323,11 @@ func TestPatchFilter(t *testing.T) {
return v
}
tests := map[string]struct {
filter *envoy_listener_v3.Filter
expectFilter *envoy_listener_v3.Filter
expectBool bool
expectErr string
filter *envoy_listener_v3.Filter
isInboundFilter bool
expectFilter *envoy_listener_v3.Filter
expectBool bool
expectErr string
}{
"invalid filter name is ignored": {
filter: &envoy_listener_v3.Filter{Name: "something"},
Expand Down Expand Up @@ -416,6 +417,36 @@ func TestPatchFilter(t *testing.T) {
},
expectBool: true,
},
"inbound filter ignored": {
filter: &envoy_listener_v3.Filter{
Name: "envoy.filters.network.http_connection_manager",
ConfigType: &envoy_listener_v3.Filter_TypedConfig{
TypedConfig: makeAny(&envoy_http_v3.HttpConnectionManager{
HttpFilters: []*envoy_http_v3.HttpFilter{
{Name: "one"},
{Name: "two"},
{Name: "envoy.filters.http.router"},
{Name: "three"},
},
}),
},
},
expectFilter: &envoy_listener_v3.Filter{
Name: "envoy.filters.network.http_connection_manager",
ConfigType: &envoy_listener_v3.Filter_TypedConfig{
TypedConfig: makeAny(&envoy_http_v3.HttpConnectionManager{
HttpFilters: []*envoy_http_v3.HttpFilter{
{Name: "one"},
{Name: "two"},
{Name: "envoy.filters.http.router"},
{Name: "three"},
},
}),
},
},
isInboundFilter: true,
expectBool: false,
},
}

for name, tc := range tests {
Expand All @@ -425,7 +456,7 @@ func TestPatchFilter(t *testing.T) {
PayloadPassthrough: true,
InvocationMode: "asynchronous",
}
f, ok, err := l.PatchFilter(nil, tc.filter)
f, ok, err := l.PatchFilter(nil, tc.filter, tc.isInboundFilter)
require.Equal(t, tc.expectBool, ok)
if tc.expectErr == "" {
require.NoError(t, err)
Expand Down
12 changes: 8 additions & 4 deletions agent/envoyextensions/builtin/http/localratelimit/ratelimit.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ func (r *ratelimit) validate() error {

// CanApply determines if the extension can apply to the given extension configuration.
func (p *ratelimit) CanApply(config *extensioncommon.RuntimeConfig) bool {
// rate limit is only applied to the service itself since the limit is
// aggregated from all downstream connections.
return string(config.Kind) == p.ProxyType && !config.IsUpstream()
return string(config.Kind) == p.ProxyType
}

// PatchRoute does nothing.
Expand All @@ -114,7 +112,13 @@ func (p ratelimit) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_clust

// PatchFilter inserts a http local rate_limit filter at the head of
// envoy.filters.network.http_connection_manager filters
func (p ratelimit) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
func (p ratelimit) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
// rate limit is only applied to the inbound listener of the service itself
// since the limit is aggregated from all downstream connections.
if !isInboundListener {
return filter, false, nil
}

if filter.Name != "envoy.filters.network.http_connection_manager" {
return filter, false, nil
}
Expand Down
13 changes: 9 additions & 4 deletions agent/envoyextensions/builtin/lua/lua.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ func (l *lua) validate() error {

// CanApply determines if the extension can apply to the given extension configuration.
func (l *lua) CanApply(config *extensioncommon.RuntimeConfig) bool {
return string(config.Kind) == l.ProxyType && l.matchesListenerDirection(config)
return string(config.Kind) == l.ProxyType
}

func (l *lua) matchesListenerDirection(config *extensioncommon.RuntimeConfig) bool {
return (config.IsUpstream() && l.Listener == "outbound") || (!config.IsUpstream() && l.Listener == "inbound")
func (l *lua) matchesListenerDirection(isInboundListener bool) bool {
return (!isInboundListener && l.Listener == "outbound") || (isInboundListener && l.Listener == "inbound")
}

// PatchRoute does nothing.
Expand All @@ -82,7 +82,12 @@ func (l *lua) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3
}

// PatchFilter inserts a lua filter directly prior to envoy.filters.http.router.
func (l *lua) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
func (l *lua) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
// Make sure filter matches extension config.
if !l.matchesListenerDirection(isInboundListener) {
return filter, false, nil
}

if filter.Name != "envoy.filters.network.http_connection_manager" {
return filter, false, nil
}
Expand Down
2 changes: 1 addition & 1 deletion agent/envoyextensions/builtin/wasm/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (p *pluginConfig) asyncDataSource(rtCfg *extensioncommon.RuntimeConfig) (*e
// fetch the data from the upstream source.
remote := &p.VmConfig.Code.Remote
clusterSNI := ""
for service, upstream := range rtCfg.LocalUpstreams {
for service, upstream := range rtCfg.Upstreams {
if service == remote.HttpURI.Service {
for sni := range upstream.SNI {
clusterSNI = sni
Expand Down
15 changes: 10 additions & 5 deletions agent/envoyextensions/builtin/wasm/wasm.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,13 @@ func (w *wasm) fromArguments(args map[string]any) error {

// CanApply indicates if the WASM extension can be applied to the given extension configuration.
// Currently the Wasm extension can be applied if the extension configuration is for an inbound
// listener on the a local connect-proxy.
// It does not patch extensions for service upstreams.
// listener (checked below) on a local connect-proxy.
func (w wasm) CanApply(config *extensioncommon.RuntimeConfig) bool {
return config.IsLocal() && w.wasmConfig.ListenerType == "inbound" &&
config.Kind == w.wasmConfig.ProxyType
return config.Kind == w.wasmConfig.ProxyType
}

func (w wasm) matchesConfigDirection(isInboundListener bool) bool {
return isInboundListener && w.wasmConfig.ListenerType == "inbound"
}

// PatchRoute does nothing for the WASM extension.
Expand All @@ -88,7 +89,11 @@ func (w wasm) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3

// PatchFilter adds a Wasm filter to the HTTP filter chain.
// TODO (wasm/tcp): Add support for TCP filters.
func (w wasm) PatchFilter(cfg *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
func (w wasm) PatchFilter(cfg *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
if !w.matchesConfigDirection(isInboundListener) {
return filter, false, nil
}

if filter.Name != "envoy.filters.network.http_connection_manager" {
return filter, false, nil
}
Expand Down
68 changes: 47 additions & 21 deletions agent/envoyextensions/builtin/wasm/wasm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,24 @@ import (
func TestHttpWasmExtension(t *testing.T) {
t.Parallel()
cases := map[string]struct {
extName string
canApply bool
args func(bool) map[string]any
rtCfg func(bool) *extensioncommon.RuntimeConfig
inputFilters func() []*envoy_http_v3.HttpFilter
expFilters func(tc testWasmConfig) []*envoy_http_v3.HttpFilter
errStr string
debug bool
extName string
canApply bool
args func(bool) map[string]any
rtCfg func(bool) *extensioncommon.RuntimeConfig
isInboundFilter bool
inputFilters func() []*envoy_http_v3.HttpFilter
expFilters func(tc testWasmConfig) []*envoy_http_v3.HttpFilter
expPatched bool
errStr string
debug bool
}{
"http remote file": {
extName: api.BuiltinWasmExtension,
canApply: true,
args: func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
inputFilters: makeTestHttpFilters,
extName: api.BuiltinWasmExtension,
canApply: true,
args: func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
isInboundFilter: true,
inputFilters: makeTestHttpFilters,
expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
return []*envoy_http_v3.HttpFilter{
{Name: "one"},
Expand All @@ -65,6 +68,7 @@ func TestHttpWasmExtension(t *testing.T) {
{Name: "three"},
}
},
expPatched: true,
},
"local file": {
extName: api.BuiltinWasmExtension,
Expand All @@ -76,8 +80,9 @@ func TestHttpWasmExtension(t *testing.T) {
cfg.PluginConfig.VmConfig.Code.Local.Filename = "plugin.wasm"
return cfg.toMap(t)
},
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
inputFilters: makeTestHttpFilters,
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
isInboundFilter: true,
inputFilters: makeTestHttpFilters,
expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
return []*envoy_http_v3.HttpFilter{
{Name: "one"},
Expand All @@ -95,18 +100,38 @@ func TestHttpWasmExtension(t *testing.T) {
{Name: "three"},
}
},
expPatched: true,
},
"inbound filters ignored": {
extName: api.BuiltinWasmExtension,
canApply: true,
args: func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
isInboundFilter: false,
inputFilters: makeTestHttpFilters,
expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
return []*envoy_http_v3.HttpFilter{
{Name: "one"},
{Name: "two"},
{Name: "envoy.filters.http.router"},
{Name: "three"},
}
},
expPatched: false,
},
"no cluster for remote file": {
extName: api.BuiltinWasmExtension,
canApply: true,
args: func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
rtCfg: func(ent bool) *extensioncommon.RuntimeConfig {
rt := makeTestRuntimeConfig(ent)
rt.LocalUpstreams = nil
rt.Upstreams = nil
return rt
},
inputFilters: makeTestHttpFilters,
errStr: "no upstream found for remote service",
isInboundFilter: true,
inputFilters: makeTestHttpFilters,
errStr: "no upstream found for remote service",
expPatched: false,
},
}

Expand Down Expand Up @@ -140,10 +165,10 @@ func TestHttpWasmExtension(t *testing.T) {
require.NoError(t, err)

inputHttpConMgr := makeHttpConMgr(t, c.inputFilters())
obsHttpConMgr, patched, err := w.PatchFilter(c.rtCfg(enterprise), inputHttpConMgr)
obsHttpConMgr, patched, err := w.PatchFilter(c.rtCfg(enterprise), inputHttpConMgr, c.isInboundFilter)
if c.errStr == "" {
require.NoError(t, err)
require.True(t, patched)
require.Equal(t, c.expPatched, patched)

cfg := testWasmConfigFromMap(t, c.args(enterprise))
expHttpConMgr := makeHttpConMgr(t, c.expFilters(cfg))
Expand All @@ -156,6 +181,7 @@ func TestHttpWasmExtension(t *testing.T) {

prototest.AssertDeepEqual(t, expHttpConMgr, obsHttpConMgr)
} else {
require.Error(t, err)
require.Contains(t, err.Error(), c.errStr)
}

Expand Down Expand Up @@ -554,7 +580,7 @@ func makeTestRuntimeConfig(enterprise bool) *extensioncommon.RuntimeConfig {
return &extensioncommon.RuntimeConfig{
Kind: api.ServiceKindConnectProxy,
ServiceName: api.CompoundServiceName{Name: "test-service"},
LocalUpstreams: map[api.CompoundServiceName]*extensioncommon.UpstreamData{
Upstreams: map[api.CompoundServiceName]*extensioncommon.UpstreamData{
{
Name: "test-file-server",
Namespace: acl.NamespaceOrDefault(ns),
Expand Down
Loading