From bca3081d8977bd15b12be0d5f80c151f8d213fb9 Mon Sep 17 00:00:00 2001 From: Jorik van der Werf Date: Fri, 18 Oct 2019 11:07:47 +0200 Subject: [PATCH 1/4] hide license key in logs --- cmd/nri-prometheus/config.go | 2 +- internal/cmd/scraper/scraper.go | 21 ++++++++++++-- internal/cmd/scraper/scraper_test.go | 43 ++++++++++++++++++++++++++++ internal/integration/fetcher.go | 2 +- 4 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 internal/cmd/scraper/scraper_test.go diff --git a/cmd/nri-prometheus/config.go b/cmd/nri-prometheus/config.go index 83f4acb3..fe2e4630 100644 --- a/cmd/nri-prometheus/config.go +++ b/cmd/nri-prometheus/config.go @@ -36,7 +36,7 @@ func loadConfig() (*scraper.Config, error) { } if scraperCfg.MetricAPIURL == "" { - scraperCfg.MetricAPIURL = determineMetricAPIURL(scraperCfg.LicenseKey) + scraperCfg.MetricAPIURL = determineMetricAPIURL(string(scraperCfg.LicenseKey)) } return &scraperCfg, nil diff --git a/internal/cmd/scraper/scraper.go b/internal/cmd/scraper/scraper.go index e06cbb21..d60564e0 100644 --- a/internal/cmd/scraper/scraper.go +++ b/internal/cmd/scraper/scraper.go @@ -23,7 +23,7 @@ import ( type Config struct { ConfigFile string MetricAPIURL string `mapstructure:"metric_api_url"` - LicenseKey string `mapstructure:"license_key"` + LicenseKey LicenseKey `mapstructure:"license_key"` ClusterName string `mapstructure:"cluster_name"` Debug bool `mapstructure:"debug"` Verbose bool `mapstructure:"verbose"` @@ -48,6 +48,21 @@ type Config struct { EmitterInsecureSkipVerify bool `mapstructure:"emitter_insecure_skip_verify" default:"false"` } +const maskedLicenseKey = "****" + +// LicenseKey is a New Relic license key that will be masked when printed using standard formatters +type LicenseKey string + +// String ensures that the LicenseKey will be masked in functions like fmt.Println(licenseKey) +func (l LicenseKey) String() string { + return maskedLicenseKey +} + +// GoString ensures that the LicenseKey will be masked in functions like fmt.Printf("%#v", licenseKey) +func (l LicenseKey) GoString() string { + return maskedLicenseKey +} + // Number of /metrics targets that can be fetched in parallel const maxTargetConnections = 4 @@ -186,7 +201,7 @@ func Run(cfg *Config) error { } harvesterOpts := []func(*telemetry.Config){ - telemetry.ConfigAPIKey(cfg.LicenseKey), + telemetry.ConfigAPIKey(string(cfg.LicenseKey)), telemetry.ConfigBasicErrorLogger(os.Stdout), integration.TelemetryHarvesterWithMetricsURL(cfg.MetricAPIURL), integration.TelemetryHarvesterWithHarvestPeriod(hTime), @@ -218,7 +233,7 @@ func Run(cfg *Config) error { // Transport to `integration.licenseKeyRoundTripper`. harvesterOpts = append( harvesterOpts, - integration.TelemetryHarvesterWithLicenseKeyRoundTripper(cfg.LicenseKey), + integration.TelemetryHarvesterWithLicenseKeyRoundTripper(string(cfg.LicenseKey)), ) if cfg.Verbose { diff --git a/internal/cmd/scraper/scraper_test.go b/internal/cmd/scraper/scraper_test.go new file mode 100644 index 00000000..5caddb5d --- /dev/null +++ b/internal/cmd/scraper/scraper_test.go @@ -0,0 +1,43 @@ +// Package scraper ... +// Copyright 2019 New Relic Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package scraper + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLicenseKeyMasking(t *testing.T) { + + const licenseKeyString = "secret" + licenseKey := LicenseKey(licenseKeyString) + + t.Run("Masks licenseKey in fmt.Sprintf (which uses same logic as Printf)", func(t *testing.T) { + masked := fmt.Sprintf("%s", licenseKey) + assert.Equal(t, masked, maskedLicenseKey) + }) + + t.Run("Masks licenseKey in fmt.Sprint (which uses same logic as Print)", func(t *testing.T) { + masked := fmt.Sprint(licenseKey) + assert.Equal(t, masked, maskedLicenseKey) + }) + + t.Run("Masks licenseKey in %#v formatting", func(t *testing.T) { + masked := fmt.Sprintf("%#v", licenseKey) + if strings.Contains(masked, licenseKeyString) { + t.Error("found licenseKey in formatted string") + } + if !strings.Contains(masked, maskedLicenseKey) { + t.Error("could not find masked password in formatted string") + } + }) + + t.Run("Able to convert licenseKey back to string", func(t *testing.T) { + unmasked := string(licenseKey) + assert.Equal(t, licenseKeyString, unmasked) + }) +} diff --git a/internal/integration/fetcher.go b/internal/integration/fetcher.go index 95267798..e9e4163f 100644 --- a/internal/integration/fetcher.go +++ b/internal/integration/fetcher.go @@ -279,7 +279,7 @@ const ( metricType_GAUGE metricType = "gauge" metricType_SUMMARY metricType = "summary" metricType_HISTOGRAM metricType = "histogram" - metricType_UNTYPED metricType = "untyped" + //metricType_UNTYPED metricType = "untyped" ) // Metric represents a Prometheus metric. From 775c74df57c3891d1d13a361384af351d689e938 Mon Sep 17 00:00:00 2001 From: Jorik van der Werf Date: Fri, 18 Oct 2019 11:26:34 +0200 Subject: [PATCH 2/4] test whether logrus printing also masks license keys --- internal/cmd/scraper/scraper_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/internal/cmd/scraper/scraper_test.go b/internal/cmd/scraper/scraper_test.go index 5caddb5d..b979e91d 100644 --- a/internal/cmd/scraper/scraper_test.go +++ b/internal/cmd/scraper/scraper_test.go @@ -4,10 +4,12 @@ package scraper import ( + "bytes" "fmt" "strings" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" ) @@ -41,3 +43,26 @@ func TestLicenseKeyMasking(t *testing.T) { assert.Equal(t, licenseKeyString, unmasked) }) } + +func TestLogrusDebugPrintMasksLicenseKey(t *testing.T) { + + const licenseKey = "SECRET_LICENSE_KEY" + + cfg := Config{ + LicenseKey: licenseKey, + } + + var b bytes.Buffer + + logrus.SetOutput(&b) + logrus.SetLevel(logrus.DebugLevel) + logrus.Debugf("Config: %#v", cfg) + + msg := b.String() + if strings.Contains(msg, licenseKey) { + t.Error("Log output contains the license key") + } + if !strings.Contains(msg, maskedLicenseKey) { + t.Error("Log output does not contain the masked licenseKey") + } +} From 16377d4b4c4910333ba152e35f003329566126bc Mon Sep 17 00:00:00 2001 From: Jorik van der Werf Date: Fri, 18 Oct 2019 12:07:34 +0200 Subject: [PATCH 3/4] remove unused variable --- internal/integration/fetcher.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/integration/fetcher.go b/internal/integration/fetcher.go index e9e4163f..154f9ecf 100644 --- a/internal/integration/fetcher.go +++ b/internal/integration/fetcher.go @@ -279,7 +279,6 @@ const ( metricType_GAUGE metricType = "gauge" metricType_SUMMARY metricType = "summary" metricType_HISTOGRAM metricType = "histogram" - //metricType_UNTYPED metricType = "untyped" ) // Metric represents a Prometheus metric. From 829fb3c2400d87c2ac497d3484e20c9a6915d270 Mon Sep 17 00:00:00 2001 From: Jorik van der Werf Date: Fri, 18 Oct 2019 12:21:32 +0200 Subject: [PATCH 4/4] Add test for parsing custom string type --- internal/cmd/scraper/scraper_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/cmd/scraper/scraper_test.go b/internal/cmd/scraper/scraper_test.go index b979e91d..7350d291 100644 --- a/internal/cmd/scraper/scraper_test.go +++ b/internal/cmd/scraper/scraper_test.go @@ -9,7 +9,10 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + "github.com/sirupsen/logrus" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -66,3 +69,20 @@ func TestLogrusDebugPrintMasksLicenseKey(t *testing.T) { t.Error("Log output does not contain the masked licenseKey") } } + +func TestConfigParseWithCustomType(t *testing.T) { + + const licenseKey = "MY_LICENSE_KEY" + cfgStr := []byte(fmt.Sprintf(`LICENSE_KEY: %s`, licenseKey)) + + vip := viper.New() + vip.SetConfigType("yaml") + err := vip.ReadConfig(bytes.NewBuffer(cfgStr)) + require.NoError(t, err) + + var cfg Config + err = vip.Unmarshal(&cfg) + require.NoError(t, err) + + assert.Equal(t, licenseKey, string(cfg.LicenseKey)) +}