From efc956c7f79ef3cd02d52104fb997a2f69235f06 Mon Sep 17 00:00:00 2001 From: "Eric R. Rath" <4080262+ericrrath@users.noreply.github.com> Date: Fri, 16 Sep 2022 05:36:57 -0700 Subject: [PATCH] SMTP config: add global and local password file fields (#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 #2534 "Add support to set the Slack URL in the file" in the upstream repo. Signed-off-by: Eric R. Rath * changed *AuthPasswordFile field types to string per review feedback Signed-off-by: Eric R. Rath * added error to getPassword() retval per review feedback Signed-off-by: Eric R. Rath * simplified conf.smtp-* files Signed-off-by: Eric R. Rath * update docs to reflect field type change Signed-off-by: Eric R. Rath * don't treat username-without-password as invalid Signed-off-by: Eric R. Rath * test cleanup Signed-off-by: Eric R. Rath * Apply suggestions from code review Co-authored-by: Simon Pasquier Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com> * Updated per review feedback Signed-off-by: Eric R. Rath * added sub-test per review feedback Signed-off-by: Eric R. Rath * added test on Email.getPassword() per feedback Signed-off-by: Eric R. Rath * only inherit global SMTP passwords if neither local password field is set Signed-off-by: Eric R. Rath * removed blank line caught by gofumpt Signed-off-by: Eric R. Rath Signed-off-by: Eric R. Rath Signed-off-by: Eric R. Rath <4080262+ericrrath@users.noreply.github.com> Co-authored-by: Simon Pasquier --- config/config.go | 48 ++++++----- config/config_test.go | 33 ++++++++ config/notifiers.go | 27 +++--- .../conf.smtp-both-password-and-file.yml | 13 +++ .../conf.smtp-no-username-or-password.yml | 10 +++ .../conf.smtp-password-global-and-local.yml | 21 +++++ docs/configuration.md | 4 + notify/email/email.go | 23 ++++- notify/email/email_test.go | 83 +++++++++++++++++++ 9 files changed, 226 insertions(+), 36 deletions(-) create mode 100644 config/testdata/conf.smtp-both-password-and-file.yml create mode 100644 config/testdata/conf.smtp-no-username-or-password.yml create mode 100644 config/testdata/conf.smtp-password-global-and-local.yml diff --git a/config/config.go b/config/config.go index efc1a9f3..356ecccd 100644 --- 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 index 7e7a1fee..10a3272f 100644 --- 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 index 95df28db..28aaa780 100644 --- 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 new file mode 100644 index 00000000..98511b16 --- /dev/null +++ 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 new file mode 100644 index 00000000..52ec672a --- /dev/null +++ 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 new file mode 100644 index 00000000..ac077b4a --- /dev/null +++ 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 index 2f468d7a..d769ad4d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -71,6 +71,8 @@ global: [ smtp_auth_username: ] # SMTP Auth using LOGIN and PLAIN. [ smtp_auth_password: ] + # SMTP Auth using LOGIN and PLAIN. + [ smtp_auth_password_file: ] # SMTP Auth using PLAIN. [ smtp_auth_identity: ] # SMTP Auth using CRAM-MD5. @@ -514,8 +516,10 @@ to: [ hello: | default = global.smtp_hello ] # SMTP authentication information. +# auth_password and auth_password_file are mutually exclusive. [ auth_username: | default = global.smtp_auth_username ] [ auth_password: | default = global.smtp_auth_password ] +[ auth_password_file: | default = global.smtp_auth_password_file ] [ auth_secret: | default = global.smtp_auth_secret ] [ auth_identity: | default = global.smtp_auth_identity ] diff --git a/notify/email/email.go b/notify/email/email.go index eebf20c7..944aabdf 100644 --- 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 index e55246ab..a57faf2d 100644 --- 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) + } + }) + } +}