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

pchaploutskii · 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 <[email protected]>

Signed-off-by: Eric R. Rath <[email protected]>

docs/configuration.md

Co-authored-by: Simon Pasquier <[email protected]> Signed-off-by: Eric R. Rath <[email protected]>

Signed-off-by: Eric R. Rath <[email protected]>

simonpasquier

could you add sub-tests here that verify the happy (password file configured with good value) and sad (password file configured with wrong value) paths?

Signed-off-by: Eric R. Rath <[email protected]>

ericrrath · 2022-09-14T17:31:28Z

could you add sub-tests here that verify the happy (password file configured with good value) and sad (password file configured with wrong value) paths?

Another good catch; done!

simonpasquier

Very cool! I'm being a bit nit-picky but could you also add a test that would configure an invalid file name and trigger an error when getPassword() gets called?

Signed-off-by: Eric R. Rath <[email protected]>

… set Signed-off-by: Eric R. Rath <[email protected]>

Signed-off-by: Eric R. Rath <[email protected]>

ericrrath · 2022-09-15T18:43:37Z

Very cool! I'm being a bit nit-picky but could you also add a test that would configure an invalid file name and trigger an error when getPassword() gets called?

Done!

simonpasquier

thanks!

simonpasquier · 2022-09-16T08:09:59Z

config/config.go

@@ -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 == "" {


* 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 <[email protected]> * changed *AuthPasswordFile field types to string per review feedback Signed-off-by: Eric R. Rath <[email protected]> * added error to getPassword() retval per review feedback Signed-off-by: Eric R. Rath <[email protected]> * simplified conf.smtp-* files Signed-off-by: Eric R. Rath <[email protected]> * update docs to reflect field type change Signed-off-by: Eric R. Rath <[email protected]> * don't treat username-without-password as invalid Signed-off-by: Eric R. Rath <[email protected]> * test cleanup Signed-off-by: Eric R. Rath <[email protected]> * Apply suggestions from code review Co-authored-by: Simon Pasquier <[email protected]> Signed-off-by: Eric R. Rath <[email protected]> * Updated per review feedback Signed-off-by: Eric R. Rath <[email protected]> * added sub-test per review feedback Signed-off-by: Eric R. Rath <[email protected]> * added test on Email.getPassword() per feedback Signed-off-by: Eric R. Rath <[email protected]> * only inherit global SMTP passwords if neither local password field is set Signed-off-by: Eric R. Rath <[email protected]> * removed blank line caught by gofumpt Signed-off-by: Eric R. Rath <[email protected]> Signed-off-by: Eric R. Rath <[email protected]> Signed-off-by: Eric R. Rath <[email protected]> Co-authored-by: Simon Pasquier <[email protected]> Signed-off-by: Yijie Qin <[email protected]>

ericrrath force-pushed the add-smtp-password-file branch from a93e4eb to aa036db Compare August 15, 2022 22:24

valvin1 mentioned this pull request Aug 31, 2022

Generally enable reading secrets from files #2498

Open

simonpasquier reviewed Sep 9, 2022

View reviewed changes

ericrrath added 2 commits September 12, 2022 15:44

changed *AuthPasswordFile field types to string per review feedback

397db16

Signed-off-by: Eric R. Rath <[email protected]>

ericrrath force-pushed the add-smtp-password-file branch from aaedd7b to 397db16 Compare September 12, 2022 22:44

ericrrath added 5 commits September 12, 2022 16:15

added error to getPassword() retval per review feedback

c66e4ea

Signed-off-by: Eric R. Rath <[email protected]>

simplified conf.smtp-* files

751ecd3

Signed-off-by: Eric R. Rath <[email protected]>

update docs to reflect field type change

936a7de

Signed-off-by: Eric R. Rath <[email protected]>

don't treat username-without-password as invalid

803d8bb

Signed-off-by: Eric R. Rath <[email protected]>

test cleanup

4ad5936

Signed-off-by: Eric R. Rath <[email protected]>

simonpasquier reviewed Sep 13, 2022

View reviewed changes

docs/configuration.md Outdated Show resolved Hide resolved

docs/configuration.md Show resolved Hide resolved

docs/configuration.md Outdated Show resolved Hide resolved

ericrrath and others added 2 commits September 13, 2022 09:25

Apply suggestions from code review

114b26c

Co-authored-by: Simon Pasquier <[email protected]> Signed-off-by: Eric R. Rath <[email protected]>

Updated per review feedback

9091654

Signed-off-by: Eric R. Rath <[email protected]>

simonpasquier reviewed Sep 14, 2022

View reviewed changes

added sub-test per review feedback

ba8a3bb

Signed-off-by: Eric R. Rath <[email protected]>

simonpasquier reviewed Sep 15, 2022

View reviewed changes

ericrrath added 3 commits September 15, 2022 09:41

added test on Email.getPassword() per feedback

da48e29

Signed-off-by: Eric R. Rath <[email protected]>

only inherit global SMTP passwords if neither local password field is…

d51d1b3

… set Signed-off-by: Eric R. Rath <[email protected]>

removed blank line caught by gofumpt

1444875

Signed-off-by: Eric R. Rath <[email protected]>

simonpasquier reviewed Sep 16, 2022

View reviewed changes

simonpasquier merged commit efc956c into prometheus:main Sep 16, 2022

simonpasquier mentioned this pull request Sep 16, 2022

notify/email: refactor tests #3078

Merged

gciria mentioned this pull request Jan 3, 2023

[ kube-prometheus-stack ] Alertmanager with error 'field smtp_auth_password_file not found in type config.plain' prometheus-community/helm-charts#2881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ericrrath commented Aug 15, 2022

pchaploutskii commented Aug 17, 2022

valvin1 commented Aug 31, 2022

simonpasquier left a comment

simonpasquier Sep 9, 2022

ericrrath Sep 12, 2022

simonpasquier Sep 9, 2022

ericrrath Sep 12, 2022

simonpasquier Sep 9, 2022

ericrrath Sep 13, 2022

ericrrath Sep 13, 2022

ericrrath Sep 15, 2022

simonpasquier Sep 9, 2022

ericrrath Sep 12, 2022

simonpasquier left a comment

ericrrath commented Sep 14, 2022

simonpasquier left a comment

ericrrath commented Sep 15, 2022

simonpasquier left a comment

simonpasquier Sep 16, 2022

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

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

Conversation

ericrrath commented Aug 15, 2022

pchaploutskii commented Aug 17, 2022

valvin1 commented Aug 31, 2022

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

ericrrath commented Sep 14, 2022

simonpasquier left a comment

Choose a reason for hiding this comment

ericrrath commented Sep 15, 2022

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment