notify: redact more secret data from logs (#1825)

Follow-up of #1822

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

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(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 {
@@ -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()
 

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, u, fn := getContextWithCancelingURL()
-	ctx, cancel := context.WithCancel(context.Background())
+	defer fn()
-
-	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{
@@ -102,10 +181,7 @@ func TestSlackRedactedURL(t *testing.T) {
 		log.NewNopLogger(),
 	)
 
-	ok, err := notifier.Notify(ctx, []*types.Alert{}...)
+	assertNotifyLeaksNoSecret(t, ctx, notifier, u.String())
-	require.True(t, ok)
-	require.Error(t, err)
-	require.NotContains(t, err.Error(), srv.URL)
 }
 
 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)
+}