Merge pull request #1823 from mxinden/cut-v0.16.2

*: Cut v0.16.2

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)

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

VERSION

@@ -1 +1 @@
-0.16.1
+0.16.2

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(n.apiURL)
-	u, err := url.Parse(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 = "<redacted>"
+	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 {

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)
+}

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/go-kit/kit/log.Logger).Log
-(*github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/promlog.AllowedLevel).Set
 // Generated via go-swagger
 (*github.com/prometheus/alertmanager/api/v2/restapi.Server).Shutdown