From a25d748da372dc4583e8588654d311aefaf1aa11 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 7 Feb 2019 15:11:13 +0100 Subject: [PATCH 1/6] Fix the assets make target Presumable, it broke with the introduction of Go modules Signed-off-by: beorn7 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e3ae23cf..34c34730 100644 --- a/Makefile +++ b/Makefile @@ -28,7 +28,7 @@ PRECHECK_OPTIONS_bzr = version build-all: assets apiv2 build assets: ui/app/script.js ui/app/index.html ui/app/lib template/default.tmpl - cd $(PREFIX)/asset && $(GO) generate + GO111MODULE=$(GO111MODULE) $(GO) generate ./asset @$(GOFMT) -w ./asset ui/app/script.js: $(shell find ui/app/src -iname *.elm) api/v2/openapi.yaml From c70e97c9edb8de29aad6aa39902c35a291178908 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 7 Feb 2019 15:43:23 +0100 Subject: [PATCH 2/6] Update scripts/errcheck_excludes.txt With Go modules, the path appears un-vendored. Plus, we are not calling AllowedLevel.Set anywhere anymore. Signed-off-by: beorn7 --- scripts/errcheck_excludes.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/errcheck_excludes.txt b/scripts/errcheck_excludes.txt index caef015e..8fb66258 100644 --- a/scripts/errcheck_excludes.txt +++ b/scripts/errcheck_excludes.txt @@ -3,8 +3,8 @@ fmt.Fprintf fmt.Fprintln +// Exclude both vendored and un-vendored to cover both vendor mode and module mode. (github.com/prometheus/alertmanager/vendor/github.com/go-kit/kit/log.Logger).Log -// Allowed log levels are enforced via kingpin -(*github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/promlog.AllowedLevel).Set +(github.com/go-kit/kit/log.Logger).Log // Generated via go-swagger (*github.com/prometheus/alertmanager/api/v2/restapi.Server).Shutdown From 03f7459a30398e5e955e2d75c6c5d66d18448b29 Mon Sep 17 00:00:00 2001 From: Jo Walsh <369631+metazool@users.noreply.github.com> Date: Fri, 1 Mar 2019 14:53:18 +0000 Subject: [PATCH 3/6] Allow sending of unauthenticated SMTP requests when smtp_auth_username is not supplied (#1739) * try a more complicated but clearer approach explicitly returning a no-auth stmp.Auth when no username is supplied in config Signed-off-by: Jo Walsh * fix test to expect no error from auth if username is not supplied Signed-off-by: Jo Walsh * clean up some formatting errors in surplus comments Signed-off-by: Jo Walsh * keep noAuth / loginAuth functions all together Signed-off-by: Jo Walsh * Address latest comments Co-Authored-By: Jo Walsh Signed-off-by: Simon Pasquier --- notify/impl.go | 7 +++++++ notify/impl_test.go | 14 ++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 2fbf49c0..92a278ec 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -221,6 +221,13 @@ func NewEmail(c *config.EmailConfig, t *template.Template, l log.Logger) *Email // auth resolves a string of authentication mechanisms. func (n *Email) auth(mechs string) (smtp.Auth, error) { username := n.conf.AuthUsername + + // If no username is set, keep going without authentication. + if n.conf.AuthUsername == "" { + level.Debug(n.logger).Log("msg", "smtp_auth_username is not configured. Attempting to send email without authenticating") + return nil, nil + } + err := &types.MultiError{} for _, mech := range strings.Split(mechs, " ") { switch mech { diff --git a/notify/impl_test.go b/notify/impl_test.go index a123fa35..84a380b5 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -295,7 +295,7 @@ func TestOpsGenie(t *testing.T) { func TestEmailConfigNoAuthMechs(t *testing.T) { email := &Email{ - conf: &config.EmailConfig{}, tmpl: &template.Template{}, logger: log.NewNopLogger(), + conf: &config.EmailConfig{AuthUsername: "test"}, tmpl: &template.Template{}, logger: log.NewNopLogger(), } _, err := email.auth("") require.Error(t, err) @@ -304,8 +304,9 @@ func TestEmailConfigNoAuthMechs(t *testing.T) { func TestEmailConfigMissingAuthParam(t *testing.T) { + conf := &config.EmailConfig{AuthUsername: "test"} email := &Email{ - conf: &config.EmailConfig{}, tmpl: &template.Template{}, logger: log.NewNopLogger(), + conf: conf, tmpl: &template.Template{}, logger: log.NewNopLogger(), } _, err := email.auth("CRAM-MD5") require.Error(t, err) @@ -324,6 +325,15 @@ func TestEmailConfigMissingAuthParam(t *testing.T) { require.Equal(t, err.Error(), "missing password for PLAIN auth mechanism; missing password for LOGIN auth mechanism") } +func TestEmailNoUsernameStillOk(t *testing.T) { + email := &Email{ + conf: &config.EmailConfig{}, tmpl: &template.Template{}, logger: log.NewNopLogger(), + } + a, err := email.auth("CRAM-MD5") + require.NoError(t, err) + require.Nil(t, a) +} + func TestVictorOpsCustomFields(t *testing.T) { logger := log.NewNopLogger() tmpl := createTmpl(t) From 2addaf5272dee530453cc807097302f230f6bb90 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Wed, 3 Apr 2019 17:44:32 +0200 Subject: [PATCH 4/6] notify: redact Slack webhook URL from logs Signed-off-by: Simon Pasquier --- notify/impl.go | 15 +++++++++++++-- notify/impl_test.go | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 92a278ec..1deddd0a 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -845,9 +845,10 @@ func (n *Slack) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { return false, err } - resp, err := post(ctx, c, n.conf.APIURL.String(), contentTypeJSON, &buf) + u := n.conf.APIURL.String() + resp, err := post(ctx, c, u, contentTypeJSON, &buf) if err != nil { - return true, err + return true, redactURL(err) } resp.Body.Close() @@ -1566,6 +1567,16 @@ func hashKey(s string) string { return fmt.Sprintf("%x", h.Sum(nil)) } +// redactURL removes the URL part from an error of *url.Error type. +func redactURL(err error) error { + e, ok := err.(*url.Error) + if !ok { + return err + } + e.URL = "" + return e +} + func post(ctx context.Context, client *http.Client, url string, bodyType string, body io.Reader) (*http.Response, error) { req, err := http.NewRequest("POST", url, body) if err != nil { diff --git a/notify/impl_test.go b/notify/impl_test.go index 84a380b5..9879bee9 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -19,17 +19,19 @@ import ( "fmt" "io/ioutil" "net/http" + "net/http/httptest" "net/url" "testing" "time" "github.com/go-kit/kit/log" + commoncfg "github.com/prometheus/common/config" + "github.com/prometheus/common/model" "github.com/stretchr/testify/require" "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/template" "github.com/prometheus/alertmanager/types" - "github.com/prometheus/common/model" ) func TestWebhookRetry(t *testing.T) { @@ -75,6 +77,37 @@ func TestSlackRetry(t *testing.T) { } } +func TestSlackRedactedURL(t *testing.T) { + done := make(chan struct{}) + ctx, cancel := context.WithCancel(context.Background()) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + cancel() + <-done + })) + defer func() { + close(done) + srv.Close() + }() + + u, err := url.Parse(srv.URL) + require.NoError(t, err) + + notifier := NewSlack( + &config.SlackConfig{ + APIURL: &config.SecretURL{URL: u}, + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + ok, err := notifier.Notify(ctx, []*types.Alert{}...) + require.True(t, ok) + require.Error(t, err) + require.NotContains(t, err.Error(), srv.URL) +} + func TestHipchatRetry(t *testing.T) { notifier := new(Hipchat) retryCodes := append(defaultRetryCodes(), http.StatusTooManyRequests) From b71488e162cd39afde1c2186dace1b02f3a3e1de Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Thu, 4 Apr 2019 18:27:13 +0200 Subject: [PATCH 5/6] notify: redact more secret data from logs (#1825) Follow-up of #1822 Signed-off-by: Simon Pasquier --- notify/impl.go | 36 +++---- notify/impl_test.go | 225 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 225 insertions(+), 36 deletions(-) diff --git a/notify/impl.go b/notify/impl.go index 1deddd0a..2764c4c3 100644 --- a/notify/impl.go +++ b/notify/impl.go @@ -471,11 +471,16 @@ type PagerDuty struct { conf *config.PagerdutyConfig tmpl *template.Template logger log.Logger + apiV1 string // for tests. } // NewPagerDuty returns a new PagerDuty notifier. func NewPagerDuty(c *config.PagerdutyConfig, t *template.Template, l log.Logger) *PagerDuty { - return &PagerDuty{conf: c, tmpl: t, logger: l} + n := &PagerDuty{conf: c, tmpl: t, logger: l} + if c.ServiceKey != "" { + n.apiV1 = "https://events.pagerduty.com/generic/2010-04-15/create_event.json" + } + return n } const ( @@ -540,12 +545,6 @@ func (n *PagerDuty) notifyV1( Details: details, } - apiURL, err := url.Parse("https://events.pagerduty.com/generic/2010-04-15/create_event.json") - if err != nil { - return false, err - } - n.conf.URL = &config.URL{apiURL} - if eventType == pagerDutyEventTrigger { msg.Client = tmpl(n.conf.Client) msg.ClientURL = tmpl(n.conf.ClientURL) @@ -560,7 +559,7 @@ func (n *PagerDuty) notifyV1( return false, err } - resp, err := post(ctx, c, n.conf.URL.String(), contentTypeJSON, &buf) + resp, err := post(ctx, c, n.apiV1, contentTypeJSON, &buf) if err != nil { return true, err } @@ -671,7 +670,7 @@ func (n *PagerDuty) Notify(ctx context.Context, as ...*types.Alert) (bool, error return false, err } - if n.conf.ServiceKey != "" { + if n.apiV1 != "" { return n.notifyV1(ctx, c, eventType, key, data, details, as...) } return n.notifyV2(ctx, c, eventType, key, data, details, as...) @@ -935,7 +934,7 @@ func (n *Hipchat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) resp, err := post(ctx, c, apiURL.String(), contentTypeJSON, &buf) if err != nil { - return true, err + return true, redactURL(err) } defer resp.Body.Close() @@ -1036,7 +1035,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { resp, err := c.Do(req.WithContext(ctx)) if err != nil { - return true, err + return true, redactURL(err) } defer resp.Body.Close() @@ -1087,7 +1086,7 @@ func (n *Wechat) Notify(ctx context.Context, as ...*types.Alert) (bool, error) { resp, err := c.Do(req.WithContext(ctx)) if err != nil { - return true, err + return true, redactURL(err) } defer resp.Body.Close() @@ -1316,7 +1315,7 @@ func (n *VictorOps) Notify(ctx context.Context, as ...*types.Alert) (bool, error resp, err := post(ctx, c, apiURL.String(), contentTypeJSON, buf) if err != nil { - return true, err + return true, redactURL(err) } defer resp.Body.Close() @@ -1404,11 +1403,12 @@ type Pushover struct { conf *config.PushoverConfig tmpl *template.Template logger log.Logger + apiURL string // for tests. } // NewPushover returns a new Pushover notifier. func NewPushover(c *config.PushoverConfig, t *template.Template, l log.Logger) *Pushover { - return &Pushover{conf: c, tmpl: t, logger: l} + return &Pushover{conf: c, tmpl: t, logger: l, apiURL: "https://api.pushover.net/1/messages.json"} } // Notify implements the Notifier interface. @@ -1473,13 +1473,13 @@ func (n *Pushover) Notify(ctx context.Context, as ...*types.Alert) (bool, error) return false, err } - apiURL := "https://api.pushover.net/1/messages.json" - u, err := url.Parse(apiURL) + u, err := url.Parse(n.apiURL) if err != nil { return false, err } u.RawQuery = parameters.Encode() - level.Debug(n.logger).Log("msg", "Sending Pushover message", "incident", key, "url", u.String()) + // Don't log the URL as it contains secret data (see #1825). + level.Debug(n.logger).Log("msg", "Sending Pushover message", "incident", key) c, err := commoncfg.NewClientFromConfig(*n.conf.HTTPConfig, "pushover") if err != nil { @@ -1488,7 +1488,7 @@ func (n *Pushover) Notify(ctx context.Context, as ...*types.Alert) (bool, error) resp, err := post(ctx, c, u.String(), "text/plain", nil) if err != nil { - return true, err + return true, redactURL(err) } defer resp.Body.Close() diff --git a/notify/impl_test.go b/notify/impl_test.go index 9879bee9..1fda654c 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -34,6 +34,61 @@ import ( "github.com/prometheus/alertmanager/types" ) +// getContextWithCancelingURL returns a context that gets canceled when a +// client does a GET request to the returned URL. +// Handlers passed to the function will be invoked in order before the context gets canceled. +func getContextWithCancelingURL(h ...func(w http.ResponseWriter, r *http.Request)) (context.Context, *url.URL, func()) { + done := make(chan struct{}) + ctx, cancel := context.WithCancel(context.Background()) + i := 0 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if i < len(h) { + h[i](w, r) + } else { + cancel() + <-done + } + i++ + })) + + // No need to check the error since httptest.NewServer always return a valid URL. + u, _ := url.Parse(srv.URL) + + return ctx, u, func() { + close(done) + srv.Close() + } +} + +// assertNotifyLeaksNoSecret calls the Notify() method of the notifier, expects +// it to fail because the context is canceled by the server and checks that no +// secret data is leaked in the error message returned by Notify(). +func assertNotifyLeaksNoSecret(t *testing.T, ctx context.Context, n Notifier, secret ...string) { + t.Helper() + require.NotEmpty(t, secret) + + ctx = WithGroupKey(ctx, "1") + ok, err := n.Notify(ctx, []*types.Alert{ + &types.Alert{ + Alert: model.Alert{ + Labels: model.LabelSet{ + "lbl1": "val1", + }, + StartsAt: time.Now(), + EndsAt: time.Now().Add(time.Hour), + }, + }, + }...) + + require.Error(t, err) + require.Contains(t, err.Error(), context.Canceled.Error()) + for _, s := range secret { + require.NotContains(t, err.Error(), s) + } + require.True(t, ok) +} + func TestWebhookRetry(t *testing.T) { u, err := url.Parse("http://example.com") if err != nil { @@ -69,6 +124,42 @@ func TestPagerDutyRetryV2(t *testing.T) { } } +func TestPagerDutyRedactedURLV1(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + key := "01234567890123456789012345678901" + notifier := NewPagerDuty( + &config.PagerdutyConfig{ + ServiceKey: config.Secret(key), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + notifier.apiV1 = u.String() + + assertNotifyLeaksNoSecret(t, ctx, notifier, key) +} + +func TestPagerDutyRedactedURLV2(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + key := "01234567890123456789012345678901" + notifier := NewPagerDuty( + &config.PagerdutyConfig{ + URL: &config.URL{u}, + RoutingKey: config.Secret(key), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, key) +} + func TestSlackRetry(t *testing.T) { notifier := new(Slack) for statusCode, expected := range retryTests(defaultRetryCodes()) { @@ -78,20 +169,8 @@ func TestSlackRetry(t *testing.T) { } func TestSlackRedactedURL(t *testing.T) { - done := make(chan struct{}) - ctx, cancel := context.WithCancel(context.Background()) - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - cancel() - <-done - })) - defer func() { - close(done) - srv.Close() - }() - - u, err := url.Parse(srv.URL) - require.NoError(t, err) + ctx, u, fn := getContextWithCancelingURL() + defer fn() notifier := NewSlack( &config.SlackConfig{ @@ -102,10 +181,7 @@ func TestSlackRedactedURL(t *testing.T) { log.NewNopLogger(), ) - ok, err := notifier.Notify(ctx, []*types.Alert{}...) - require.True(t, ok) - require.Error(t, err) - require.NotContains(t, err.Error(), srv.URL) + assertNotifyLeaksNoSecret(t, ctx, notifier, u.String()) } func TestHipchatRetry(t *testing.T) { @@ -117,6 +193,24 @@ func TestHipchatRetry(t *testing.T) { } } +func TestHipchatRedactedURL(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + token := "secret_token" + notifier := NewHipchat( + &config.HipchatConfig{ + APIURL: &config.URL{URL: u}, + AuthToken: config.Secret(token), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, token) +} + func TestOpsGenieRetry(t *testing.T) { notifier := new(OpsGenie) @@ -127,6 +221,24 @@ func TestOpsGenieRetry(t *testing.T) { } } +func TestOpsGenieRedactedURL(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + key := "key" + notifier := NewOpsGenie( + &config.OpsGenieConfig{ + APIURL: &config.URL{URL: u}, + APIKey: config.Secret(key), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, key) +} + func TestVictorOpsRetry(t *testing.T) { notifier := new(VictorOps) for statusCode, expected := range retryTests(defaultRetryCodes()) { @@ -135,6 +247,24 @@ func TestVictorOpsRetry(t *testing.T) { } } +func TestVictorOpsRedactedURL(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + secret := "secret" + notifier := NewVictorOps( + &config.VictorOpsConfig{ + APIURL: &config.URL{URL: u}, + APIKey: config.Secret(secret), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, secret) +} + func TestPushoverRetry(t *testing.T) { notifier := new(Pushover) for statusCode, expected := range retryTests(defaultRetryCodes()) { @@ -143,6 +273,25 @@ func TestPushoverRetry(t *testing.T) { } } +func TestPushoverRedactedURL(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + key, token := "user_key", "token" + notifier := NewPushover( + &config.PushoverConfig{ + UserKey: config.Secret(key), + Token: config.Secret(token), + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + notifier.apiURL = u.String() + + assertNotifyLeaksNoSecret(t, ctx, notifier, key, token) +} + func retryTests(retryCodes []int) map[int]bool { tests := map[int]bool{ // 1xx @@ -414,3 +563,43 @@ func TestVictorOpsCustomFields(t *testing.T) { // Verify that a custom field was added to the payload and templatized. require.Equal(t, "message", m["Field_A"]) } + +func TestWechatRedactedURLOnInitialAuthentication(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + secret := "secret_key" + notifier := NewWechat( + &config.WechatConfig{ + APIURL: &config.URL{URL: u}, + HTTPConfig: &commoncfg.HTTPClientConfig{}, + CorpID: "corpid", + APISecret: config.Secret(secret), + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, secret) +} + +func TestWechatRedactedURLOnNotify(t *testing.T) { + secret, token := "secret", "token" + ctx, u, fn := getContextWithCancelingURL(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"access_token":"%s"}`, token) + }) + defer fn() + + notifier := NewWechat( + &config.WechatConfig{ + APIURL: &config.URL{URL: u}, + HTTPConfig: &commoncfg.HTTPClientConfig{}, + CorpID: "corpid", + APISecret: config.Secret(secret), + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, secret, token) +} From fb326296af59864465f546d07deffda1e2871620 Mon Sep 17 00:00:00 2001 From: Max Leonard Inden Date: Wed, 3 Apr 2019 19:02:24 +0200 Subject: [PATCH 6/6] *: Cut v0.16.2 Signed-off-by: Max Leonard Inden --- CHANGELOG.md | 9 +++++++++ VERSION | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1510aead..ed7dd68a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +## 0.16.2 / 2019-04-03 + +Updating to v0.16.2 is recommended for all users using the Slack, Pagerduty, +Hipchat, Wechat, VictorOps and Pushover notifier, as connection errors could +leak secrets embedded in the notifier's URL to stdout. + +* [BUGFIX] Redact notifier URL from logs to not leak secrets embedded in the URL (#1822, #1825) +* [BUGFIX] Allow sending of unauthenticated SMTP requests when `smtp_auth_username` is not supplied (#1739) + ## 0.16.1 / 2019-01-31 * [BUGFIX] Do not populate cluster info if clustering is disabled in API v2 (#1726) diff --git a/VERSION b/VERSION index 2a0970ca..201a22c8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.16.1 +0.16.2