Skip to content

Commit

Permalink
[config{grpc,http}] Add warning when using unspecified address (open-…
Browse files Browse the repository at this point in the history
…telemetry#6267)

* [config/config{grpc,http}] Add warning when using a 0.0.0.0 endpoint

* Add warning when using unspecified address

* Add changelog entry

* Fix tests

* Fix HTTP tests

* Apply suggestions from code review

Co-authored-by: Alex Boten <alex@boten.ca>

* Use IsUnspecified method

* no else after return

* Move shared code to internal

Co-authored-by: Alex Boten <alex@boten.ca>
  • Loading branch information
mx-psi and codeboten authored Oct 25, 2022
1 parent db0c250 commit 396964d
Show file tree
Hide file tree
Showing 7 changed files with 246 additions and 2 deletions.
16 changes: 16 additions & 0 deletions .chloggen/mx-psi_add-logging-0.0.0.0.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# 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: receiver/otlp

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add warning when using unspecified (`0.0.0.0`) address on HTTP or gRPC servers

# One or more tracking issues or pull requests related to the change
issues: [6151]

# (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:
9 changes: 9 additions & 0 deletions config/configgrpc/configgrpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"go.opentelemetry.io/collector/config/configcompression"
"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/config/configtls"
"go.opentelemetry.io/collector/config/internal"
)

var errMetadataNotFound = errors.New("no request metadata found")
Expand Down Expand Up @@ -273,6 +274,14 @@ func (gss *GRPCServerSettings) ToListener() (net.Listener, error) {

// ToServerOption maps configgrpc.GRPCServerSettings to a slice of server options for gRPC.
func (gss *GRPCServerSettings) ToServerOption(host component.Host, settings component.TelemetrySettings) ([]grpc.ServerOption, error) {

switch gss.NetAddr.Transport {
case "tcp", "tcp4", "tcp6", "udp", "udp4", "udp6":
if err := internal.WarnOnUnspecifiedHost(settings.Logger, gss.NetAddr.Endpoint); err != nil {
return nil, fmt.Errorf("failed to parse endpoint: %w", err)
}
}

var opts []grpc.ServerOption

if gss.TLSSetting != nil {
Expand Down
65 changes: 63 additions & 2 deletions config/configgrpc/configgrpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
"google.golang.org/grpc"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/peer"
Expand Down Expand Up @@ -162,7 +164,11 @@ func TestAllGrpcClientSettings(t *testing.T) {
}

func TestDefaultGrpcServerSettings(t *testing.T) {
gss := &GRPCServerSettings{}
gss := &GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: "0.0.0.0:1234",
},
}
opts, err := gss.ToServerOption(componenttest.NewNopHost(), componenttest.NewNopTelemetrySettings())
_ = grpc.NewServer(opts...)

Expand Down Expand Up @@ -206,7 +212,11 @@ func TestAllGrpcServerSettingsExceptAuth(t *testing.T) {
}

func TestGrpcServerAuthSettings(t *testing.T) {
gss := &GRPCServerSettings{}
gss := &GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: "0.0.0.0:1234",
},
}

// sanity check
_, err := gss.ToServerOption(componenttest.NewNopHost(), componenttest.NewNopTelemetrySettings())
Expand Down Expand Up @@ -370,6 +380,57 @@ func TestUseSecure(t *testing.T) {
assert.Len(t, dialOpts, 3)
}

func TestGRPCServerWarning(t *testing.T) {
tests := []struct {
name string
settings GRPCServerSettings
len int
}{
{
settings: GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: "0.0.0.0:1234",
Transport: "tcp",
},
},
len: 1,
},
{
settings: GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: "127.0.0.1:1234",
Transport: "tcp",
},
},
len: 0,
},
{
settings: GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: "0.0.0.0:1234",
Transport: "unix",
},
},
len: 0,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
set := componenttest.NewNopTelemetrySettings()
logger, observed := observer.New(zap.DebugLevel)
set.Logger = zap.New(logger)

opts, err := test.settings.ToServerOption(componenttest.NewNopHost(), set)
require.NoError(t, err)
require.NotNil(t, opts)
_ = grpc.NewServer(opts...)

require.Len(t, observed.FilterLevelExact(zap.WarnLevel).All(), test.len)
})
}

}

func TestGRPCServerSettingsError(t *testing.T) {
tests := []struct {
settings GRPCServerSettings
Expand Down
5 changes: 5 additions & 0 deletions config/confighttp/confighttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"go.opentelemetry.io/collector/config/configauth"
"go.opentelemetry.io/collector/config/configcompression"
"go.opentelemetry.io/collector/config/configtls"
"go.opentelemetry.io/collector/config/internal"
)

const headerContentEncoding = "Content-Encoding"
Expand Down Expand Up @@ -259,6 +260,10 @@ func WithErrorHandler(e errorHandler) ToServerOption {

// ToServer creates an http.Server from settings object.
func (hss *HTTPServerSettings) ToServer(host component.Host, settings component.TelemetrySettings, handler http.Handler, opts ...ToServerOption) (*http.Server, error) {
if err := internal.WarnOnUnspecifiedHost(settings.Logger, hss.Endpoint); err != nil {
return nil, err
}

serverOpts := &toServerOptions{}
for _, o := range opts {
o(serverOpts)
Expand Down
44 changes: 44 additions & 0 deletions config/confighttp/confighttp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"

"go.opentelemetry.io/collector/client"
"go.opentelemetry.io/collector/component"
Expand Down Expand Up @@ -392,6 +394,45 @@ func TestHTTPServerSettingsError(t *testing.T) {
}
}

func TestHTTPServerWarning(t *testing.T) {
tests := []struct {
name string
settings HTTPServerSettings
len int
}{
{
settings: HTTPServerSettings{
Endpoint: "0.0.0.0:0",
},
len: 1,
},
{
settings: HTTPServerSettings{
Endpoint: "127.0.0.1:0",
},
len: 0,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
set := componenttest.NewNopTelemetrySettings()
logger, observed := observer.New(zap.DebugLevel)
set.Logger = zap.New(logger)

_, err := test.settings.ToServer(
componenttest.NewNopHost(),
set,
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, errWrite := fmt.Fprint(w, "test")
assert.NoError(t, errWrite)
}))
require.NoError(t, err)
require.Len(t, observed.FilterLevelExact(zap.WarnLevel).All(), test.len)
})
}

}

func TestHttpReception(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -687,6 +728,7 @@ func TestHttpCorsInvalidSettings(t *testing.T) {

func TestHttpCorsWithAuthentication(t *testing.T) {
hss := &HTTPServerSettings{
Endpoint: "localhost:0",
CORS: &CORSSettings{
AllowedOrigins: []string{"*"},
},
Expand Down Expand Up @@ -884,6 +926,7 @@ func TestServerAuth(t *testing.T) {
// prepare
authCalled := false
hss := HTTPServerSettings{
Endpoint: "localhost:0",
Auth: &configauth.Authentication{
AuthenticatorID: config.NewComponentID("mock"),
},
Expand Down Expand Up @@ -931,6 +974,7 @@ func TestInvalidServerAuth(t *testing.T) {
func TestFailedServerAuth(t *testing.T) {
// prepare
hss := HTTPServerSettings{
Endpoint: "localhost:0",
Auth: &configauth.Authentication{
AuthenticatorID: config.NewComponentID("mock"),
},
Expand Down
42 changes: 42 additions & 0 deletions config/internal/warning.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package internal // import "go.opentelemetry.io/collector/config/internal"

import (
"fmt"
"net"

"go.uber.org/zap"
)

// WarnOnUnspecifiedHost emits a warning if an endpoint has an unspecified host.
func WarnOnUnspecifiedHost(logger *zap.Logger, endpoint string) error {
host, _, err := net.SplitHostPort(endpoint)
if err != nil {
return fmt.Errorf("failed to parse endpoint: %w", err)
}

if ip := net.ParseIP(host); ip != nil && ip.IsUnspecified() {
logger.Warn(
"Using the 0.0.0.0 address exposes this server to every network interface, which may facilitate Denial of Service attacks",
zap.String(
"documentation",
"https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/security-best-practices.md#safeguards-against-denial-of-service-attacks",
),
)
}

return nil
}
67 changes: 67 additions & 0 deletions config/internal/warning_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package internal // import "go.opentelemetry.io/collector/config/internal"

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
)

func TestWarnOnUnspecifiedHost(t *testing.T) {
tests := []struct {
endpoint string
warn bool
err string
}{
{
endpoint: "0.0.0.0:0",
warn: true,
},
{
endpoint: "127.0.0.1:0",
},
{
endpoint: "localhost:0",
},
{
endpoint: "localhost::0",
err: "too many colons in address",
},
}
for _, test := range tests {
t.Run(test.endpoint, func(t *testing.T) {
core, observed := observer.New(zap.DebugLevel)
logger := zap.New(core)
err := WarnOnUnspecifiedHost(logger, test.endpoint)
if test.err != "" {
assert.ErrorContains(t, err, test.err)
return
}

require.NoError(t, err)

var len int
if test.warn {
len = 1
}
require.Len(t, observed.FilterLevelExact(zap.WarnLevel).All(), len)
})
}

}

0 comments on commit 396964d

Please sign in to comment.