SMTP config: add global and local password file fields (prometheus#3038)

* SMTP config: add global and local password file fields Add config fields (for both global email config and route-specific email config) that specify path to file containing SMTP password. We don't want the password in the config file itself, and reading the password from a k8s-secret-backed file keeps the password itself "encrypted at rest" in etcd, and cleanly separated from the rest of the AM config. I used the same approach as pull request prometheus#2534 "Add support to set the Slack URL in the file" <https://github.com/prometheus/alertmanager/pull/2534/files> in the upstream repo. Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * changed *AuthPasswordFile field types to string per review feedback Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * added error to getPassword() retval per review feedback Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * simplified conf.smtp-* files Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * update docs to reflect field type change Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * don't treat username-without-password as invalid Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * test cleanup Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * Apply suggestions from code review Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com> * Updated per review feedback Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * added sub-test per review feedback Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * added test on Email.getPassword() per feedback Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * only inherit global SMTP passwords if neither local password field is set Signed-off-by: Eric R. Rath <eric.rath@oracle.com> * removed blank line caught by gofumpt Signed-off-by: Eric R. Rath <eric.rath@oracle.com> Signed-off-by: Eric R. Rath <eric.rath@oracle.com> Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com> Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>
qinxx108 · Dec 13, 2022 · 28ba3b5 · 28ba3b5
1 parent 664e022
commit 28ba3b5
Show file tree

Hide file tree

Showing 9 changed files with 226 additions and 36 deletions.
diff --git a/config/config.go b/config/config.go
@@ -335,6 +335,10 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return fmt.Errorf("at most one of opsgenie_api_key & opsgenie_api_key_file must be configured")
 	}
 
+	if len(c.Global.SMTPAuthPassword) > 0 && len(c.Global.SMTPAuthPasswordFile) > 0 {
+		return fmt.Errorf("at most one of smtp_auth_password & smtp_auth_password_file must be configured")
+	}
+
 	names := map[string]struct{}{}
 
 	for _, rcv := range c.Receivers {
@@ -365,8 +369,9 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 			if ec.AuthUsername == "" {
 				ec.AuthUsername = c.Global.SMTPAuthUsername
 			}
-			if ec.AuthPassword == "" {
+			if ec.AuthPassword == "" && ec.AuthPasswordFile == "" {
 				ec.AuthPassword = c.Global.SMTPAuthPassword
+				ec.AuthPasswordFile = c.Global.SMTPAuthPasswordFile
 			}
 			if ec.AuthSecret == "" {
 				ec.AuthSecret = c.Global.SMTPAuthSecret
@@ -693,26 +698,27 @@ type GlobalConfig struct {
 
 	HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"`
 
-	SMTPFrom           string     `yaml:"smtp_from,omitempty" json:"smtp_from,omitempty"`
-	SMTPHello          string     `yaml:"smtp_hello,omitempty" json:"smtp_hello,omitempty"`
-	SMTPSmarthost      HostPort   `yaml:"smtp_smarthost,omitempty" json:"smtp_smarthost,omitempty"`
-	SMTPAuthUsername   string     `yaml:"smtp_auth_username,omitempty" json:"smtp_auth_username,omitempty"`
-	SMTPAuthPassword   Secret     `yaml:"smtp_auth_password,omitempty" json:"smtp_auth_password,omitempty"`
-	SMTPAuthSecret     Secret     `yaml:"smtp_auth_secret,omitempty" json:"smtp_auth_secret,omitempty"`
-	SMTPAuthIdentity   string     `yaml:"smtp_auth_identity,omitempty" json:"smtp_auth_identity,omitempty"`
-	SMTPRequireTLS     bool       `yaml:"smtp_require_tls" json:"smtp_require_tls,omitempty"`
-	SlackAPIURL        *SecretURL `yaml:"slack_api_url,omitempty" json:"slack_api_url,omitempty"`
-	SlackAPIURLFile    string     `yaml:"slack_api_url_file,omitempty" json:"slack_api_url_file,omitempty"`
-	PagerdutyURL       *URL       `yaml:"pagerduty_url,omitempty" json:"pagerduty_url,omitempty"`
-	OpsGenieAPIURL     *URL       `yaml:"opsgenie_api_url,omitempty" json:"opsgenie_api_url,omitempty"`
-	OpsGenieAPIKey     Secret     `yaml:"opsgenie_api_key,omitempty" json:"opsgenie_api_key,omitempty"`
-	OpsGenieAPIKeyFile string     `yaml:"opsgenie_api_key_file,omitempty" json:"opsgenie_api_key_file,omitempty"`
-	WeChatAPIURL       *URL       `yaml:"wechat_api_url,omitempty" json:"wechat_api_url,omitempty"`
-	WeChatAPISecret    Secret     `yaml:"wechat_api_secret,omitempty" json:"wechat_api_secret,omitempty"`
-	WeChatAPICorpID    string     `yaml:"wechat_api_corp_id,omitempty" json:"wechat_api_corp_id,omitempty"`
-	VictorOpsAPIURL    *URL       `yaml:"victorops_api_url,omitempty" json:"victorops_api_url,omitempty"`
-	VictorOpsAPIKey    Secret     `yaml:"victorops_api_key,omitempty" json:"victorops_api_key,omitempty"`
-	TelegramAPIUrl     *URL       `yaml:"telegram_api_url,omitempty" json:"telegram_api_url,omitempty"`
+	SMTPFrom             string     `yaml:"smtp_from,omitempty" json:"smtp_from,omitempty"`
+	SMTPHello            string     `yaml:"smtp_hello,omitempty" json:"smtp_hello,omitempty"`
+	SMTPSmarthost        HostPort   `yaml:"smtp_smarthost,omitempty" json:"smtp_smarthost,omitempty"`
+	SMTPAuthUsername     string     `yaml:"smtp_auth_username,omitempty" json:"smtp_auth_username,omitempty"`
+	SMTPAuthPassword     Secret     `yaml:"smtp_auth_password,omitempty" json:"smtp_auth_password,omitempty"`
+	SMTPAuthPasswordFile string     `yaml:"smtp_auth_password_file,omitempty" json:"smtp_auth_password_file,omitempty"`
+	SMTPAuthSecret       Secret     `yaml:"smtp_auth_secret,omitempty" json:"smtp_auth_secret,omitempty"`
+	SMTPAuthIdentity     string     `yaml:"smtp_auth_identity,omitempty" json:"smtp_auth_identity,omitempty"`
+	SMTPRequireTLS       bool       `yaml:"smtp_require_tls" json:"smtp_require_tls,omitempty"`
+	SlackAPIURL          *SecretURL `yaml:"slack_api_url,omitempty" json:"slack_api_url,omitempty"`
+	SlackAPIURLFile      string     `yaml:"slack_api_url_file,omitempty" json:"slack_api_url_file,omitempty"`
+	PagerdutyURL         *URL       `yaml:"pagerduty_url,omitempty" json:"pagerduty_url,omitempty"`
+	OpsGenieAPIURL       *URL       `yaml:"opsgenie_api_url,omitempty" json:"opsgenie_api_url,omitempty"`
+	OpsGenieAPIKey       Secret     `yaml:"opsgenie_api_key,omitempty" json:"opsgenie_api_key,omitempty"`
+	OpsGenieAPIKeyFile   string     `yaml:"opsgenie_api_key_file,omitempty" json:"opsgenie_api_key_file,omitempty"`
+	WeChatAPIURL         *URL       `yaml:"wechat_api_url,omitempty" json:"wechat_api_url,omitempty"`
+	WeChatAPISecret      Secret     `yaml:"wechat_api_secret,omitempty" json:"wechat_api_secret,omitempty"`
+	WeChatAPICorpID      string     `yaml:"wechat_api_corp_id,omitempty" json:"wechat_api_corp_id,omitempty"`
+	VictorOpsAPIURL      *URL       `yaml:"victorops_api_url,omitempty" json:"victorops_api_url,omitempty"`
+	VictorOpsAPIKey      Secret     `yaml:"victorops_api_key,omitempty" json:"victorops_api_key,omitempty"`
+	TelegramAPIUrl       *URL       `yaml:"telegram_api_url,omitempty" json:"telegram_api_url,omitempty"`
 }
 
 // UnmarshalYAML implements the yaml.Unmarshaler interface for GlobalConfig.

diff --git a/config/config_test.go b/config/config_test.go
@@ -965,6 +965,39 @@ func TestSMTPHello(t *testing.T) {
 	}
 }
 
+func TestSMTPBothPasswordAndFile(t *testing.T) {
+	_, err := LoadFile("testdata/conf.smtp-both-password-and-file.yml")
+	if err == nil {
+		t.Fatalf("Expected an error parsing %s: %s", "testdata/conf.smtp-both-password-and-file.yml", err)
+	}
+	if err.Error() != "at most one of smtp_auth_password & smtp_auth_password_file must be configured" {
+		t.Errorf("Expected: %s\nGot: %s", "at most one of auth_password & auth_password_file must be configured", err.Error())
+	}
+}
+
+func TestSMTPNoUsernameOrPassword(t *testing.T) {
+	_, err := LoadFile("testdata/conf.smtp-no-username-or-password.yml")
+	if err != nil {
+		t.Fatalf("Error parsing %s: %s", "testdata/conf.smtp-no-username-or-password.yml", err)
+	}
+}
+
+func TestGlobalAndLocalSMTPPassword(t *testing.T) {
+	config, err := LoadFile("testdata/conf.smtp-password-global-and-local.yml")
+	if err != nil {
+		t.Fatalf("Error parsing %s: %s", "testdata/conf.smtp-password-global-and-local.yml", err)
+	}
+
+	require.Equal(t, "/tmp/globaluserpassword", config.Receivers[0].EmailConfigs[0].AuthPasswordFile, "first email should use password file /tmp/globaluserpassword")
+	require.Emptyf(t, config.Receivers[0].EmailConfigs[0].AuthPassword, "password field should be empty when file provided")
+
+	require.Equal(t, "/tmp/localuser1password", config.Receivers[0].EmailConfigs[1].AuthPasswordFile, "second email should use password file /tmp/localuser1password")
+	require.Emptyf(t, config.Receivers[0].EmailConfigs[1].AuthPassword, "password field should be empty when file provided")
+
+	require.Equal(t, Secret("mysecret"), config.Receivers[0].EmailConfigs[2].AuthPassword, "third email should use password mysecret")
+	require.Emptyf(t, config.Receivers[0].EmailConfigs[2].AuthPasswordFile, "file field should be empty when password provided")
+}
+
 func TestGroupByAll(t *testing.T) {
 	c, err := LoadFile("testdata/conf.group-by-all.yml")
 	if err != nil {

diff --git a/config/notifiers.go b/config/notifiers.go
@@ -165,19 +165,20 @@ type EmailConfig struct {
 	NotifierConfig `yaml:",inline" json:",inline"`
 
 	// Email address to notify.
-	To           string              `yaml:"to,omitempty" json:"to,omitempty"`
-	From         string              `yaml:"from,omitempty" json:"from,omitempty"`
-	Hello        string              `yaml:"hello,omitempty" json:"hello,omitempty"`
-	Smarthost    HostPort            `yaml:"smarthost,omitempty" json:"smarthost,omitempty"`
-	AuthUsername string              `yaml:"auth_username,omitempty" json:"auth_username,omitempty"`
-	AuthPassword Secret              `yaml:"auth_password,omitempty" json:"auth_password,omitempty"`
-	AuthSecret   Secret              `yaml:"auth_secret,omitempty" json:"auth_secret,omitempty"`
-	AuthIdentity string              `yaml:"auth_identity,omitempty" json:"auth_identity,omitempty"`
-	Headers      map[string]string   `yaml:"headers,omitempty" json:"headers,omitempty"`
-	HTML         string              `yaml:"html,omitempty" json:"html,omitempty"`
-	Text         string              `yaml:"text,omitempty" json:"text,omitempty"`
-	RequireTLS   *bool               `yaml:"require_tls,omitempty" json:"require_tls,omitempty"`
-	TLSConfig    commoncfg.TLSConfig `yaml:"tls_config,omitempty" json:"tls_config,omitempty"`
+	To               string              `yaml:"to,omitempty" json:"to,omitempty"`
+	From             string              `yaml:"from,omitempty" json:"from,omitempty"`
+	Hello            string              `yaml:"hello,omitempty" json:"hello,omitempty"`
+	Smarthost        HostPort            `yaml:"smarthost,omitempty" json:"smarthost,omitempty"`
+	AuthUsername     string              `yaml:"auth_username,omitempty" json:"auth_username,omitempty"`
+	AuthPassword     Secret              `yaml:"auth_password,omitempty" json:"auth_password,omitempty"`
+	AuthPasswordFile string              `yaml:"auth_password_file,omitempty" json:"auth_password_file,omitempty"`
+	AuthSecret       Secret              `yaml:"auth_secret,omitempty" json:"auth_secret,omitempty"`
+	AuthIdentity     string              `yaml:"auth_identity,omitempty" json:"auth_identity,omitempty"`
+	Headers          map[string]string   `yaml:"headers,omitempty" json:"headers,omitempty"`
+	HTML             string              `yaml:"html,omitempty" json:"html,omitempty"`
+	Text             string              `yaml:"text,omitempty" json:"text,omitempty"`
+	RequireTLS       *bool               `yaml:"require_tls,omitempty" json:"require_tls,omitempty"`
+	TLSConfig        commoncfg.TLSConfig `yaml:"tls_config,omitempty" json:"tls_config,omitempty"`
 }
 
 // UnmarshalYAML implements the yaml.Unmarshaler interface.

diff --git a/config/testdata/conf.smtp-both-password-and-file.yml b/config/testdata/conf.smtp-both-password-and-file.yml
@@ -0,0 +1,13 @@
+global:
+  smtp_smarthost: 'localhost:25'
+  smtp_from: 'alertmanager@example.org'
+  smtp_auth_username: 'alertmanager'
+  smtp_auth_password: "multiline\nmysecret"
+  smtp_auth_password_file: "/tmp/global"
+  smtp_hello: "host.example.org"
+route:
+  receiver: 'email-notifications'
+receivers:
+  - name: 'email-notifications'
+    email_configs:
+      - to: 'one@example.org'
diff --git a/config/testdata/conf.smtp-no-username-or-password.yml b/config/testdata/conf.smtp-no-username-or-password.yml
@@ -0,0 +1,10 @@
+global:
+  smtp_smarthost: 'localhost:25'
+  smtp_from: 'alertmanager@example.org'
+  smtp_hello: "host.example.org"
+route:
+  receiver: 'email-notifications'
+receivers:
+  - name: 'email-notifications'
+    email_configs:
+      - to: 'one@example.org'
diff --git a/config/testdata/conf.smtp-password-global-and-local.yml b/config/testdata/conf.smtp-password-global-and-local.yml
@@ -0,0 +1,21 @@
+global:
+  smtp_smarthost: 'localhost:25'
+  smtp_from: 'alertmanager@example.org'
+  smtp_auth_username: 'globaluser'
+  smtp_auth_password_file: '/tmp/globaluserpassword'
+  smtp_hello: "host.example.org"
+route:
+  receiver: 'email-notifications'
+receivers:
+  - name: 'email-notifications'
+    email_configs:
+      # Use global
+      - to: 'one@example.org'
+      # Override global with other file
+      - to: 'two@example.org'
+        auth_username: 'localuser1'
+        auth_password_file: '/tmp/localuser1password'
+      # Override global with inline password
+      - to: 'three@example.org'
+        auth_username: 'localuser2'
+        auth_password: 'mysecret'
diff --git a/docs/configuration.md b/docs/configuration.md
@@ -71,6 +71,8 @@ global:
   [ smtp_auth_username: <string> ]
   # SMTP Auth using LOGIN and PLAIN.
   [ smtp_auth_password: <secret> ]
+  # SMTP Auth using LOGIN and PLAIN.
+  [ smtp_auth_password_file: <string> ]
   # SMTP Auth using PLAIN.
   [ smtp_auth_identity: <string> ]
   # SMTP Auth using CRAM-MD5.
@@ -514,8 +516,10 @@ to: <tmpl_string>
 [ hello: <string> | default = global.smtp_hello ]
 
 # SMTP authentication information.
+# auth_password and auth_password_file are mutually exclusive.
 [ auth_username: <string> | default = global.smtp_auth_username ]
 [ auth_password: <secret> | default = global.smtp_auth_password ]
+[ auth_password_file: <string> | default = global.smtp_auth_password_file ]
 [ auth_secret: <secret> | default = global.smtp_auth_secret ]
 [ auth_identity: <string> | default = global.smtp_auth_identity ]
 

diff --git a/notify/email/email.go b/notify/email/email.go
@@ -91,7 +91,11 @@ func (n *Email) auth(mechs string) (smtp.Auth, error) {
 			return smtp.CRAMMD5Auth(username, secret), nil
 
 		case "PLAIN":
-			password := string(n.conf.AuthPassword)
+			password, passwordErr := n.getPassword()
+			if passwordErr != nil {
+				err.Add(passwordErr)
+				continue
+			}
 			if password == "" {
 				err.Add(errors.New("missing password for PLAIN auth mechanism"))
 				continue
@@ -100,7 +104,11 @@ func (n *Email) auth(mechs string) (smtp.Auth, error) {
 
 			return smtp.PlainAuth(identity, username, password, n.conf.Smarthost.Host), nil
 		case "LOGIN":
-			password := string(n.conf.AuthPassword)
+			password, passwordErr := n.getPassword()
+			if passwordErr != nil {
+				err.Add(passwordErr)
+				continue
+			}
 			if password == "" {
 				err.Add(errors.New("missing password for LOGIN auth mechanism"))
 				continue
@@ -353,3 +361,14 @@ func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) {
 	}
 	return nil, nil
 }
+
+func (n *Email) getPassword() (string, error) {
+	if len(n.conf.AuthPasswordFile) > 0 {
+		content, err := os.ReadFile(n.conf.AuthPasswordFile)
+		if err != nil {
+			return "", fmt.Errorf("could not read %s: %w", n.conf.AuthPasswordFile, err)
+		}
+		return string(content), nil
+	}
+	return string(n.conf.AuthPassword), nil
+}
diff --git a/notify/email/email_test.go b/notify/email/email_test.go
@@ -427,6 +427,16 @@ func TestEmailNotifyWithAuthentication(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	fileWithCorrectPassword, err := os.CreateTemp("", "smtp-password-correct")
+	require.NoError(t, err, "creating temp file failed")
+	_, err = fileWithCorrectPassword.WriteString(c.Password)
+	require.NoError(t, err, "writing to temp file failed")
+
+	fileWithIncorrectPassword, err := os.CreateTemp("", "smtp-password-incorrect")
+	require.NoError(t, err, "creating temp file failed")
+	_, err = fileWithIncorrectPassword.WriteString(c.Password + "wrong")
+	require.NoError(t, err, "writing to temp file failed")
+
 	for _, tc := range []struct {
 		title     string
 		updateCfg func(*config.EmailConfig)
@@ -441,6 +451,13 @@ func TestEmailNotifyWithAuthentication(t *testing.T) {
 				cfg.AuthPassword = config.Secret(c.Password)
 			},
 		},
+		{
+			title: "email with authentication (password from file)",
+			updateCfg: func(cfg *config.EmailConfig) {
+				cfg.AuthUsername = c.Username
+				cfg.AuthPasswordFile = fileWithCorrectPassword.Name()
+			},
+		},
 		{
 			title: "HTML-only email",
 			updateCfg: func(cfg *config.EmailConfig) {
@@ -486,6 +503,16 @@ func TestEmailNotifyWithAuthentication(t *testing.T) {
 			errMsg: "Invalid username or password",
 			retry:  true,
 		},
+		{
+			title: "wrong credentials (password from file)",
+			updateCfg: func(cfg *config.EmailConfig) {
+				cfg.AuthUsername = c.Username
+				cfg.AuthPasswordFile = fileWithIncorrectPassword.Name()
+			},
+
+			errMsg: "Invalid username or password",
+			retry:  true,
+		},
 		{
 			title:  "no credentials",
 			errMsg: "authentication Required",
@@ -606,3 +633,59 @@ func TestEmailNoUsernameStillOk(t *testing.T) {
 	require.NoError(t, err)
 	require.Nil(t, a)
 }
+
+func TestEmailGetPassword(t *testing.T) {
+	passwordFile, err := os.CreateTemp("", "smtp-password")
+	require.NoError(t, err, "creating temp file failed")
+	_, err = passwordFile.WriteString("secret")
+	require.NoError(t, err, "writing to temp file failed")
+
+	for _, tc := range []struct {
+		title     string
+		updateCfg func(*config.EmailConfig)
+
+		errMsg string
+	}{
+		{
+			title: "password from field",
+			updateCfg: func(cfg *config.EmailConfig) {
+				cfg.AuthPassword = "secret"
+				cfg.AuthPasswordFile = ""
+			},
+		},
+		{
+			title: "password from file field",
+			updateCfg: func(cfg *config.EmailConfig) {
+				cfg.AuthPassword = ""
+				cfg.AuthPasswordFile = passwordFile.Name()
+			},
+		},
+		{
+			title: "password file path incorrect",
+			updateCfg: func(cfg *config.EmailConfig) {
+				cfg.AuthPassword = ""
+				cfg.AuthPasswordFile = "/does/not/exist"
+			},
+			errMsg: "could not read",
+		},
+	} {
+		tc := tc
+		t.Run(tc.title, func(t *testing.T) {
+			email := &Email{
+				conf: &config.EmailConfig{},
+			}
+
+			tc.updateCfg(email.conf)
+
+			password, err := email.getPassword()
+			if len(tc.errMsg) > 0 {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.errMsg)
+				require.Equal(t, "", password)
+			} else {
+				require.Nil(t, err)
+				require.Equal(t, "secret", password)
+			}
+		})
+	}
+}