From 9047c0e373f97f46037256908ee86e21a25c7241 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Wed, 24 Jan 2024 13:33:50 +0100 Subject: [PATCH] [receiver/otlp, internal, featuregate] Add featuregate to switch to localhost defaults for server-like components (#8622) **Description:** - Define `component.UseLocalHostAsDefaultHost` in the `internal/localhostgate` package. - Define `featuregate.ErrIsAlreadyRegistered` error, returned by `Register` when a gate is already registered. - Adds support for the localhost gate on the OTLP receiver. This PR does not remove the current warning in any way, we can remove this separately. **Link to tracking Issue:** Updates #8510 **Testing:** Adds unit tests **Documentation:** Document on OTLP receiver template and add related logging. --- .chloggen/mx-psi_featuregate-localhost-2.yaml | 26 +++++++ .chloggen/mx-psi_featuregate-localhost.yaml | 26 +++++++ config/internal/go.mod | 20 +++++- config/internal/go.sum | 12 +--- config/internal/warning.go | 5 +- docs/security-best-practices.md | 2 + featuregate/registry.go | 8 ++- featuregate/registry_test.go | 2 +- internal/localhostgate/featuregate.go | 68 +++++++++++++++++++ internal/localhostgate/featuregate_test.go | 57 ++++++++++++++++ receiver/otlpreceiver/README.md | 4 +- receiver/otlpreceiver/factory.go | 7 +- service/service.go | 2 + 13 files changed, 220 insertions(+), 19 deletions(-) create mode 100755 .chloggen/mx-psi_featuregate-localhost-2.yaml create mode 100755 .chloggen/mx-psi_featuregate-localhost.yaml create mode 100644 internal/localhostgate/featuregate.go create mode 100644 internal/localhostgate/featuregate_test.go diff --git a/.chloggen/mx-psi_featuregate-localhost-2.yaml b/.chloggen/mx-psi_featuregate-localhost-2.yaml new file mode 100755 index 00000000000..6070b6316e5 --- /dev/null +++ b/.chloggen/mx-psi_featuregate-localhost-2.yaml @@ -0,0 +1,26 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: component + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Add `component.UseLocalHostAsDefaultHost` feature gate that changes default endpoints from 0.0.0.0 to localhost" + +# One or more tracking issues or pull requests related to the change +issues: [8510] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + The only component in this repository affected by this is the OTLP receiver. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] + diff --git a/.chloggen/mx-psi_featuregate-localhost.yaml b/.chloggen/mx-psi_featuregate-localhost.yaml new file mode 100755 index 00000000000..20632157ae8 --- /dev/null +++ b/.chloggen/mx-psi_featuregate-localhost.yaml @@ -0,0 +1,26 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: featuregate + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Add the `featuregate.ErrAlreadyRegistered` error, which is returned by `featuregate.Registry`'s `Register` when adding a feature gate that is already registered." + +# One or more tracking issues or pull requests related to the change +issues: [8622] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + Use `errors.Is` to check for this error. + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/config/internal/go.mod b/config/internal/go.mod index cfa041a557b..05abc943af1 100644 --- a/config/internal/go.mod +++ b/config/internal/go.mod @@ -4,16 +4,30 @@ go 1.20 require ( github.com/stretchr/testify v1.8.4 + go.opentelemetry.io/collector v0.92.0 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.26.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect - github.com/kr/pretty v0.3.1 // indirect + github.com/hashicorp/go-version v1.6.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/rogpeppe/go-internal v1.10.0 // indirect + go.opentelemetry.io/collector/featuregate v1.0.1 // indirect go.uber.org/multierr v1.11.0 // indirect - gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) + +replace go.opentelemetry.io/collector => ../../ + +replace go.opentelemetry.io/collector/featuregate => ../../featuregate + +replace go.opentelemetry.io/collector/confmap => ../../confmap + +replace go.opentelemetry.io/collector/config/configtelemetry => ../configtelemetry + +replace go.opentelemetry.io/collector/pdata => ../../pdata + +replace go.opentelemetry.io/collector/consumer => ../../consumer + +replace go.opentelemetry.io/collector/component => ../../component diff --git a/config/internal/go.sum b/config/internal/go.sum index 7f4f0ed1ebd..d06a0e0bd77 100644 --- a/config/internal/go.sum +++ b/config/internal/go.sum @@ -1,19 +1,12 @@ -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/hashicorp/go-version v1.6.0 h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mOkIeek= +github.com/hashicorp/go-version v1.6.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= -github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= -github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= -github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= -github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= -github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= @@ -24,6 +17,5 @@ go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo= go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= -gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/config/internal/warning.go b/config/internal/warning.go index f403967da2f..28d55bfd4dc 100644 --- a/config/internal/warning.go +++ b/config/internal/warning.go @@ -9,6 +9,8 @@ import ( "strings" "go.uber.org/zap" + + "go.opentelemetry.io/collector/internal/localhostgate" ) func shouldWarn(endpoint string) bool { @@ -38,11 +40,12 @@ func shouldWarn(endpoint string) bool { func WarnOnUnspecifiedHost(logger *zap.Logger, endpoint string) { if shouldWarn(endpoint) { logger.Warn( - "Using the 0.0.0.0 address exposes this server to every network interface, which may facilitate Denial of Service attacks", + "Using the 0.0.0.0 address exposes this server to every network interface, which may facilitate Denial of Service attacks. Enable the feature gate to change the default and remove this warning.", zap.String( "documentation", "https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/security-best-practices.md#safeguards-against-denial-of-service-attacks", ), + zap.String("feature gate ID", localhostgate.UseLocalHostAsDefaultHostID), ) } } diff --git a/docs/security-best-practices.md b/docs/security-best-practices.md index 4101e561177..f68fafe9a73 100644 --- a/docs/security-best-practices.md +++ b/docs/security-best-practices.md @@ -148,6 +148,8 @@ receivers: Generally, `localhost`-like addresses should be preferred over the 0.0.0.0 address. For more information, see [CWE-1327](https://cwe.mitre.org/data/definitions/1327.html). +To change the default endpoint to be `localhost`-bound in all components, enable the `component.UseLocalHostAsDefaultHost` feature gate. This feature gate will be enabled by default in the Collector in a future release. + ## Processors Processors sit between receivers and exporters. They are responsible for diff --git a/featuregate/registry.go b/featuregate/registry.go index b43a33e4466..b2dc2e19cfd 100644 --- a/featuregate/registry.go +++ b/featuregate/registry.go @@ -4,6 +4,7 @@ package featuregate // import "go.opentelemetry.io/collector/featuregate" import ( + "errors" "fmt" "net/url" "regexp" @@ -22,6 +23,11 @@ var ( idRegexp = regexp.MustCompile(`^[0-9a-zA-Z\.]*$`) ) +var ( + // ErrAlreadyRegistered is returned when adding a Gate that is already registered. + ErrAlreadyRegistered = errors.New("gate is already registered") +) + // GlobalRegistry returns the global Registry. func GlobalRegistry() *Registry { return globalRegistry @@ -157,7 +163,7 @@ func (r *Registry) Register(id string, stage Stage, opts ...RegisterOption) (*Ga } if _, loaded := r.gates.LoadOrStore(id, g); loaded { - return nil, fmt.Errorf("attempted to add pre-existing gate %q", id) + return nil, fmt.Errorf("failed to register %q: %w", id, ErrAlreadyRegistered) } return g, nil } diff --git a/featuregate/registry_test.go b/featuregate/registry_test.go index 0149c45b209..b67bd9aaee3 100644 --- a/featuregate/registry_test.go +++ b/featuregate/registry_test.go @@ -33,7 +33,7 @@ func TestRegistry(t *testing.T) { assert.False(t, g.IsEnabled()) _, err = r.Register(id, StageBeta) - assert.Error(t, err) + assert.ErrorIs(t, err, ErrAlreadyRegistered) assert.Panics(t, func() { r.MustRegister(id, StageBeta) }) diff --git a/internal/localhostgate/featuregate.go b/internal/localhostgate/featuregate.go new file mode 100644 index 00000000000..e1b02fca044 --- /dev/null +++ b/internal/localhostgate/featuregate.go @@ -0,0 +1,68 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +// package localhostgate defines a feature gate that controls whether server-like receivers and extensions use localhost as the default host for their endpoints. +// This package is duplicated across core and contrib to avoid exposing the feature gate as part of the public API. +// To do this we define a `registerOrLoad` helper and try to register the gate in both modules. +// IMPORTANT NOTE: ANY CHANGES TO THIS PACKAGE MUST BE MIRRORED IN THE CONTRIB COUNTERPART. +package localhostgate // import "go.opentelemetry.io/collector/internal/localhostgate" + +import ( + "errors" + "fmt" + + "go.uber.org/zap" + + "go.opentelemetry.io/collector/featuregate" +) + +const UseLocalHostAsDefaultHostID = "component.UseLocalHostAsDefaultHost" + +// useLocalHostAsDefaultHostfeatureGate is the feature gate that controls whether +// server-like receivers and extensions such as the OTLP receiver use localhost as the default host for their endpoints. +var useLocalHostAsDefaultHostfeatureGate = mustRegisterOrLoad( + featuregate.GlobalRegistry(), + UseLocalHostAsDefaultHostID, + featuregate.StageAlpha, + featuregate.WithRegisterDescription("controls whether server-like receivers and extensions such as the OTLP receiver use localhost as the default host for their endpoints"), +) + +// mustRegisterOrLoad tries to register the feature gate and loads it if it already exists. +// It panics on any other error. +func mustRegisterOrLoad(reg *featuregate.Registry, id string, stage featuregate.Stage, opts ...featuregate.RegisterOption) *featuregate.Gate { + gate, err := reg.Register(id, stage, opts...) + + if errors.Is(err, featuregate.ErrAlreadyRegistered) { + // Gate is already registered; find it. + // Only a handful of feature gates are registered, so it's fine to iterate over all of them. + reg.VisitAll(func(g *featuregate.Gate) { + if g.ID() == id { + gate = g + return + } + }) + } else if err != nil { + panic(err) + } + + return gate +} + +// EndpointForPort gets the endpoint for a given port using localhost or 0.0.0.0 depending on the feature gate. +func EndpointForPort(port int) string { + host := "localhost" + if !useLocalHostAsDefaultHostfeatureGate.IsEnabled() { + host = "0.0.0.0" + } + return fmt.Sprintf("%s:%d", host, port) +} + +// LogAboutUseLocalHostAsDefault logs about the upcoming change from 0.0.0.0 to localhost on server-like components. +func LogAboutUseLocalHostAsDefault(logger *zap.Logger) { + if !useLocalHostAsDefaultHostfeatureGate.IsEnabled() { + logger.Warn( + "The default endpoints for all servers in components will change to use localhost instead of 0.0.0.0 in a future version. Use the feature gate to preview the new default.", + zap.String("feature gate ID", UseLocalHostAsDefaultHostID), + ) + } +} diff --git a/internal/localhostgate/featuregate_test.go b/internal/localhostgate/featuregate_test.go new file mode 100644 index 00000000000..ef09519ac9c --- /dev/null +++ b/internal/localhostgate/featuregate_test.go @@ -0,0 +1,57 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package localhostgate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/featuregate" +) + +func setFeatureGateForTest(t testing.TB, gate *featuregate.Gate, enabled bool) func() { + originalValue := gate.IsEnabled() + require.NoError(t, featuregate.GlobalRegistry().Set(gate.ID(), enabled)) + return func() { + require.NoError(t, featuregate.GlobalRegistry().Set(gate.ID(), originalValue)) + } +} + +func TestEndpointForPort(t *testing.T) { + tests := []struct { + port int + enabled bool + endpoint string + }{ + { + port: 4317, + enabled: false, + endpoint: "0.0.0.0:4317", + }, + { + port: 4317, + enabled: true, + endpoint: "localhost:4317", + }, + { + port: 0, + enabled: false, + endpoint: "0.0.0.0:0", + }, + { + port: 0, + enabled: true, + endpoint: "localhost:0", + }, + } + + for _, tt := range tests { + t.Run(tt.endpoint, func(t *testing.T) { + defer setFeatureGateForTest(t, useLocalHostAsDefaultHostfeatureGate, tt.enabled)() + assert.Equal(t, EndpointForPort(tt.port), tt.endpoint) + }) + } +} diff --git a/receiver/otlpreceiver/README.md b/receiver/otlpreceiver/README.md index 7d3380ae1dc..0d6e5ef5a2c 100644 --- a/receiver/otlpreceiver/README.md +++ b/receiver/otlpreceiver/README.md @@ -36,7 +36,9 @@ The following settings are configurable: - `endpoint` (default = 0.0.0.0:4317 for grpc protocol, 0.0.0.0:4318 http protocol): host:port to which the receiver is going to receive data. The valid syntax is - described at https://github.com/grpc/grpc/blob/master/doc/naming.md. + described at https://github.com/grpc/grpc/blob/master/doc/naming.md. The + `component.UseLocalHostAsDefaultHost` feature gate changes these to localhost:4317 and + localhost:4318 respectively. This will become the default in a future release. ## Advanced Configuration diff --git a/receiver/otlpreceiver/factory.go b/receiver/otlpreceiver/factory.go index bb8f3bb4f92..169e472b751 100644 --- a/receiver/otlpreceiver/factory.go +++ b/receiver/otlpreceiver/factory.go @@ -11,12 +11,15 @@ import ( "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/confignet" "go.opentelemetry.io/collector/consumer" + "go.opentelemetry.io/collector/internal/localhostgate" "go.opentelemetry.io/collector/internal/sharedcomponent" "go.opentelemetry.io/collector/receiver" "go.opentelemetry.io/collector/receiver/otlpreceiver/internal/metadata" ) const ( + grpcPort = 4317 + httpPort = 4318 defaultGRPCEndpoint = "0.0.0.0:4317" defaultHTTPEndpoint = "0.0.0.0:4318" @@ -42,7 +45,7 @@ func createDefaultConfig() component.Config { Protocols: Protocols{ GRPC: &configgrpc.GRPCServerSettings{ NetAddr: confignet.NetAddr{ - Endpoint: defaultGRPCEndpoint, + Endpoint: localhostgate.EndpointForPort(grpcPort), Transport: "tcp", }, // We almost write 0 bytes, so no need to tune WriteBufferSize. @@ -50,7 +53,7 @@ func createDefaultConfig() component.Config { }, HTTP: &HTTPConfig{ HTTPServerSettings: &confighttp.HTTPServerSettings{ - Endpoint: defaultHTTPEndpoint, + Endpoint: localhostgate.EndpointForPort(httpPort), }, TracesURLPath: defaultTracesURLPath, MetricsURLPath: defaultMetricsURLPath, diff --git a/service/service.go b/service/service.go index cca5e37b3df..4f7b04d42ed 100644 --- a/service/service.go +++ b/service/service.go @@ -19,6 +19,7 @@ import ( "go.opentelemetry.io/collector/connector" "go.opentelemetry.io/collector/exporter" "go.opentelemetry.io/collector/extension" + "go.opentelemetry.io/collector/internal/localhostgate" "go.opentelemetry.io/collector/internal/obsreportconfig" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/processor" @@ -163,6 +164,7 @@ func (srv *Service) Start(ctx context.Context) error { } srv.telemetrySettings.Logger.Info("Everything is ready. Begin running and processing data.") + localhostgate.LogAboutUseLocalHostAsDefault(srv.telemetrySettings.Logger) return nil }