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

ericrrath · 2022-08-15T22:23:03Z

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.

This is intended to partially address issue #2498

I used the same approach as pull request #2534

joywaves · 2022-08-17T16:33:06Z

Can't wait to see this PR in place, thank you much @ericrrath

valvin1 · 2022-08-31T08:26:32Z

it looks this PR is what I need to avoid to do weird thing such as :

push an smtp password in plain in my config file on a git repository
encrypt my whole alertmanager configuration file
setup a local smtp to manage authentification

What this PR requires to do some progress?

simonpasquier

thanks for doing this!

simonpasquier · 2022-09-09T13:40:09Z

config/notifiers.go

+	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 Secret              `yaml:"auth_password_file,omitempty" json:"auth_password_file,omitempty"`


We don't need the *File fields to be of type Secret, they can be plain string.

No problem, fixed.

simonpasquier · 2022-09-09T13:41:00Z

config/config.go

+	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 Secret     `yaml:"smtp_auth_password_file,omitempty" json:"smtp_auth_password_file,omitempty"`


same here about the field's type.

No problem, fixed.

simonpasquier · 2022-09-09T13:43:29Z

config/config.go

@@ -365,8 +369,13 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 			if ec.AuthUsername == "" {
 				ec.AuthUsername = c.Global.SMTPAuthUsername
 			}
-			if ec.AuthPassword == "" {
+			// require a password only if a username is provided


I don't see how this change relates to the original issue? Can you elaborate?

Another good catch. I thought username-but-no-password was an invalid config, but now see that it's explicitly supported. Simplified.

Ugh, wait, sorry, I was rushing, and misread the code in that last part. no-username is supported, but username-without-password still produces errors. Hold on, let me get this sorted. Sorry for the hassle.

Okay, I finally remembered. Local configs inherit fields from the global config, and now that the SMTP password can come from two different kinds of field -- literal or from-file -- I wanted to avoid having a local config with one kind of field inheriting the other kind of field from global. I.e. if the global config has SMTPAuthPasswordFile set, and a local config has AuthPassword set, that local config should not inherit SMTPAuthPasswordFile.

I updated the code again to enforce that; local configs now only inherit the global SMTP password fields if both of the local fields are unset.

simonpasquier · 2022-09-09T13:57:26Z

notify/email/email.go

+	}
+	content, err := os.ReadFile(string(n.conf.AuthPasswordFile))
+	if err != nil {
+		return ""


we should return an error, rather than silently discarding the issue.

Good catch, fixed. My first instinct was to have this new func treat both the "raw" and "file" fields being unset as an error, and return that error. But the "upstream" code already has a check for that case, so I just had the new func return "" if both fields are unset.

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>