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/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 diff --git a/VERSION b/VERSION index 2a0970ca..201a22c8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.16.1 +0.16.2 diff --git a/notify/impl.go b/notify/impl.go index 2fbf49c0..2764c4c3 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 { @@ -464,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 ( @@ -533,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) @@ -553,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 } @@ -664,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...) @@ -838,9 +844,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() @@ -927,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() @@ -1028,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() @@ -1079,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() @@ -1308,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() @@ -1396,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. @@ -1465,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 { @@ -1480,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() @@ -1559,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 a123fa35..1fda654c 100644 --- a/notify/impl_test.go +++ b/notify/impl_test.go @@ -19,19 +19,76 @@ 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" ) +// 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 { @@ -67,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()) { @@ -75,6 +168,22 @@ func TestSlackRetry(t *testing.T) { } } +func TestSlackRedactedURL(t *testing.T) { + ctx, u, fn := getContextWithCancelingURL() + defer fn() + + notifier := NewSlack( + &config.SlackConfig{ + APIURL: &config.SecretURL{URL: u}, + HTTPConfig: &commoncfg.HTTPClientConfig{}, + }, + createTmpl(t), + log.NewNopLogger(), + ) + + assertNotifyLeaksNoSecret(t, ctx, notifier, u.String()) +} + func TestHipchatRetry(t *testing.T) { notifier := new(Hipchat) retryCodes := append(defaultRetryCodes(), http.StatusTooManyRequests) @@ -84,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) @@ -94,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()) { @@ -102,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()) { @@ -110,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 @@ -295,7 +477,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 +486,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 +507,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) @@ -371,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) +} 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