From a7beedf74ac76982baa45097a365986ad062ccb6 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Mon, 8 Aug 2022 11:42:34 -0700 Subject: [PATCH] Enforce scheme name restrictions to all confmap.Provider implementations. Currently was documented that the format should be compatible with the URI definition (see https://datatracker.ietf.org/doc/html/rfc3986), but the scheme name restriction was not copied from the RFC, and no tests to enforce. This PR clearly documents the characters allowed in the scheme name and add confmaptest helper func to test for valid scheme names. Signed-off-by: Bogdan --- CHANGELOG.md | 1 + confmap/confmaptest/configtest.go | 37 +++++++++++++++-- confmap/confmaptest/configtest_test.go | 40 ++++++++++++++++++- confmap/confmaptest/testdata/invalid.yaml | 1 + confmap/provider.go | 10 +++-- confmap/provider/envprovider/provider_test.go | 5 +++ .../provider/fileprovider/provider_test.go | 5 +++ .../provider/yamlprovider/provider_test.go | 6 +++ 8 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 confmap/confmaptest/testdata/invalid.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f8b65f6c2a..0183abf3a36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ - Bump to opentelemetry-proto v0.19.0. (#5823) - Expose `Scope.Attributes` in pdata (#5826) - Add support to handle 404, 405 http error code as permanent errors in OTLP exporter (#5827) +- Enforce scheme name restrictions to all `confmap.Provider` implementations. (#5861) ### 🧰 Bug fixes 🧰 diff --git a/confmap/confmaptest/configtest.go b/confmap/confmaptest/configtest.go index 4f9a52c98d2..64571e6667e 100644 --- a/confmap/confmaptest/configtest.go +++ b/confmap/confmaptest/configtest.go @@ -15,17 +15,46 @@ package confmaptest // import "go.opentelemetry.io/collector/confmap/confmaptest" import ( - "context" + "fmt" + "io/ioutil" + "path/filepath" + "regexp" + + "gopkg.in/yaml.v2" "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/provider/fileprovider" ) // LoadConf loads a confmap.Conf from file, and does NOT validate the configuration. func LoadConf(fileName string) (*confmap.Conf, error) { - ret, err := fileprovider.New().Retrieve(context.Background(), "file:"+fileName, nil) + // Clean the path before using it. + content, err := ioutil.ReadFile(filepath.Clean(fileName)) if err != nil { + return nil, fmt.Errorf("unable to read the file %v: %w", fileName, err) + } + + var rawConf map[string]interface{} + if err = yaml.Unmarshal(content, &rawConf); err != nil { return nil, err } - return ret.AsConf() + + return confmap.NewFromStringMap(rawConf), nil +} + +var schemeValidator = regexp.MustCompile("^[A-Za-z][A-Za-z0-9+.-]+$") + +// ValidateProviderScheme enforces that given confmap.Provider.Scheme() object is following the restriction defined by the collector: +// - Checks that the scheme name follows the restrictions defined https://datatracker.ietf.org/doc/html/rfc3986#section-3.1 +// - Checks that the scheme name has at leas two characters per the confmap.Provider.Scheme() comment. +func ValidateProviderScheme(p confmap.Provider) error { + scheme := p.Scheme() + if len(scheme) < 2 { + return fmt.Errorf("scheme must be at least 2 characters long: %q", scheme) + } + + if !schemeValidator.MatchString(scheme) { + return fmt.Errorf("scheme names consist of a sequence of characters beginning with a letter and followed by any combination of letters, digits, \"+\", \".\", or \"-\": %q", scheme) + } + + return nil } diff --git a/confmap/confmaptest/configtest_test.go b/confmap/confmaptest/configtest_test.go index ae1e592dec1..8175ffaa194 100644 --- a/confmap/confmaptest/configtest_test.go +++ b/confmap/confmaptest/configtest_test.go @@ -15,20 +15,56 @@ package confmaptest import ( + "context" "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" ) -func TestLoadConfigMap_FileNotFound(t *testing.T) { +func TestLoadConfFileNotFound(t *testing.T) { _, err := LoadConf("file/not/found") assert.Error(t, err) } -func TestLoadConfigMap(t *testing.T) { +func TestLoadConfInvalidYAML(t *testing.T) { + _, err := LoadConf(filepath.Join("testdata", "invalid.yaml")) + require.Error(t, err) +} + +func TestLoadConf(t *testing.T) { cfg, err := LoadConf(filepath.Join("testdata", "simple.yaml")) require.NoError(t, err) assert.Equal(t, map[string]interface{}{"floating": 3.14}, cfg.ToStringMap()) } + +func TestValidateProviderScheme(t *testing.T) { + assert.NoError(t, ValidateProviderScheme(&schemeProvider{scheme: "file"})) + assert.NoError(t, ValidateProviderScheme(&schemeProvider{scheme: "s3"})) + assert.NoError(t, ValidateProviderScheme(&schemeProvider{scheme: "a.l-l+"})) + // Too short. + assert.Error(t, ValidateProviderScheme(&schemeProvider{scheme: "a"})) + // Invalid first character. + assert.Error(t, ValidateProviderScheme(&schemeProvider{scheme: "3s"})) + // Invalid underscore character. + assert.Error(t, ValidateProviderScheme(&schemeProvider{scheme: "all_"})) +} + +type schemeProvider struct { + scheme string +} + +func (s schemeProvider) Retrieve(context.Context, string, confmap.WatcherFunc) (confmap.Retrieved, error) { + return confmap.Retrieved{}, nil +} + +func (s schemeProvider) Scheme() string { + return s.scheme +} + +func (s schemeProvider) Shutdown(ctx context.Context) error { + return nil +} diff --git a/confmap/confmaptest/testdata/invalid.yaml b/confmap/confmaptest/testdata/invalid.yaml new file mode 100644 index 00000000000..848469f9a11 --- /dev/null +++ b/confmap/confmaptest/testdata/invalid.yaml @@ -0,0 +1 @@ +[invalid, \ No newline at end of file diff --git a/confmap/provider.go b/confmap/provider.go index 653853a28bf..e6f2587d579 100644 --- a/confmap/provider.go +++ b/confmap/provider.go @@ -41,9 +41,13 @@ type Provider interface { // // `uri` must follow the ":" format. This format is compatible // with the URI definition (see https://datatracker.ietf.org/doc/html/rfc3986). The "" - // must be always included in the `uri`. The scheme supported by any provider MUST be at - // least 2 characters long to avoid conflicting with a driver-letter identifier as specified - // in https://tools.ietf.org/id/draft-kerwin-file-scheme-07.html#syntax. + // must be always included in the `uri`. The "" supported by any provider: + // - MUST consist of a sequence of characters beginning with a letter and followed by any + // combination of letters, digits, plus ("+"), period ("."), or hyphen ("-"). + // See https://datatracker.ietf.org/doc/html/rfc3986#section-3.1. + // - MUST be at least 2 characters long to avoid conflicting with a driver-letter identifier as specified + // in https://tools.ietf.org/id/draft-kerwin-file-scheme-07.html#syntax. + // - For testing, all implementation MUST check that confmaptest.ValidateProviderScheme returns no error. // // `watcher` callback is called when the config changes. watcher may be called from // a different go routine. After watcher is called Retrieved.Get should be called diff --git a/confmap/provider/envprovider/provider_test.go b/confmap/provider/envprovider/provider_test.go index 35c611bed00..60af1357740 100644 --- a/confmap/provider/envprovider/provider_test.go +++ b/confmap/provider/envprovider/provider_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" ) const envSchemePrefix = schemeName + ":" @@ -34,6 +35,10 @@ exporters: endpoint: "localhost:4317" ` +func TestValidateProviderScheme(t *testing.T) { + assert.NoError(t, confmaptest.ValidateProviderScheme(New())) +} + func TestEmptyName(t *testing.T) { env := New() _, err := env.Retrieve(context.Background(), "", nil) diff --git a/confmap/provider/fileprovider/provider_test.go b/confmap/provider/fileprovider/provider_test.go index 59f3043df48..c401585f968 100644 --- a/confmap/provider/fileprovider/provider_test.go +++ b/confmap/provider/fileprovider/provider_test.go @@ -24,10 +24,15 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" ) const fileSchemePrefix = schemeName + ":" +func TestValidateProviderScheme(t *testing.T) { + assert.NoError(t, confmaptest.ValidateProviderScheme(New())) +} + func TestEmptyName(t *testing.T) { fp := New() _, err := fp.Retrieve(context.Background(), "", nil) diff --git a/confmap/provider/yamlprovider/provider_test.go b/confmap/provider/yamlprovider/provider_test.go index 35a2548aa3b..0cf6a49be35 100644 --- a/confmap/provider/yamlprovider/provider_test.go +++ b/confmap/provider/yamlprovider/provider_test.go @@ -19,8 +19,14 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/collector/confmap/confmaptest" ) +func TestValidateProviderScheme(t *testing.T) { + assert.NoError(t, confmaptest.ValidateProviderScheme(New())) +} + func TestEmpty(t *testing.T) { sp := New() _, err := sp.Retrieve(context.Background(), "", nil)