Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move SMTP auth to the config file #308

Merged
merged 1 commit into from Apr 16, 2016
Merged

Conversation

@mpchadwick
Copy link
Contributor

@mpchadwick mpchadwick commented Apr 15, 2016

Implements enhancement proposed in #306.

Note: I have left support for environment variables as a fallback since we probably don't want people's alerts to break if they pull down master and don't move their credentials from env variables to the config file.

Edit: Environment variables are no longer supported

Documentation additionally updated to reflect the fact that the config file is the preferred place to store creds.

func smtpAuthParam(key string, gc *GlobalConfig) (string, Secret) {
// Try to pull it from the global config
globalKey := "SMTPAuth" + key
reflected := reflect.Indirect(reflect.ValueOf(gc))
Copy link
Contributor

@brian-brazil brian-brazil Apr 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting a bit magic, I think it'd be okay to just switch to config file for this.

Loading

@mpchadwick
Copy link
Contributor Author

@mpchadwick mpchadwick commented Apr 16, 2016

@brian-brazil updated per your feedback. I can squash as needed if you're OK with this (all into 1 commit or into 2 commits, one for code changes and the other for documentation).

Loading

@@ -26,7 +26,7 @@ import (
"gopkg.in/yaml.v2"
)

var patAuthLine = regexp.MustCompile(`((?:api_key|service_key|api_url|token|user_key):\s+)(".+"|'.+'|[^\s]+)`)
var patAuthLine = regexp.MustCompile(`((?:api_key|service_key|api_url|token|user_key|auth_password|auth_secret):\s+)(".+"|'.+'|[^\s]+)`)
Copy link
Contributor

@fabxc fabxc Apr 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making that password and secret will be enough. We are just so specific for key and url because it could be something normal.

Loading

@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Apr 16, 2016

That looks good, can you squash please?

Loading

To string `yaml:"to"`
From string `yaml:"from"`
Smarthost string `yaml:"smarthost,omitempty"`
AuthUsername string `yaml:"auth_username"`
Copy link
Contributor

@fabxc fabxc Apr 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These all have the same prefix. Would it be worthwhile to move them into their own YAML block?

Loading

@fabxc
Copy link
Contributor

@fabxc fabxc commented Apr 16, 2016

@mpchadwick Thanks a lot! Added two comments.

Loading

@mpchadwick
Copy link
Contributor Author

@mpchadwick mpchadwick commented Apr 16, 2016

@brian-brazil @fabxc

Rebased to a single commit.

Just making that password and secret will be enough. We are just so specific for key and url because it could be something normal.

I updated the RegEx to be more loose. I agree that probably we'd always want to obscure anything matching these terms.

These all have the same prefix. Would it be worthwhile to move them into their own YAML block?

I guess it would be something like this?

global:
  # The smarthost and SMTP sender used for mail notifications.
  smtp_smarthost: 'localhost:25'
  smtp_from: 'alertmanager@example.org'
  smtp_auth:
  - username: 'alertmanager'
    password: 'password'  

I didn't address it because your comment was a question, but if you'd prefer that syntax I can.

Loading

@fabxc
Copy link
Contributor

@fabxc fabxc commented Apr 16, 2016

Yes, that's what I had in mind. I would say grouping things together in a block is a good idea if you have a common prefix. We did it similar to TLS configs in Prometheus.

Of course any objections can be discussed.

Loading

@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Apr 16, 2016

For TLS there were 20-30 potential fields as things expand, there's only 4 here. I'd see this along the same lines as us not having an auth field in Prometheus for basic auth vs. token vs tls.

Loading

@fabxc
Copy link
Contributor

@fabxc fabxc commented Apr 16, 2016

Loading

@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Apr 16, 2016

We have a basic_auth, a bearer_token and a bearer_token_file all of which are auth related. These are not however nested under an auth.

Loading

@fabxc
Copy link
Contributor

@fabxc fabxc commented Apr 16, 2016

Ah, I see what you mean.
So it would be symmetric to put username and password in a separate block. However, don't see strong need in general. We can leave it as it is.

Loading

@brian-brazil
Copy link
Contributor

@brian-brazil brian-brazil commented Apr 16, 2016

Yeah. There are a few other possible auth options, but if they come up (sound to be most likely in a full-Windows environment) we can refactor this a bit.

Loading

@fabxc
Copy link
Contributor

@fabxc fabxc commented Apr 16, 2016

Okay. Thanks @mpchadwick 👍

Loading

@fabxc fabxc merged commit b52e71e into prometheus:master Apr 16, 2016
1 check passed
Loading
kaihendry
Copy link

kaihendry commented on 4cb3874 Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auth secret and & auth password?

I'm using :latest and I noticed things have broken with 530 Authentication required.

Loading

mpchadwick
Copy link
Contributor

mpchadwick commented on 4cb3874 Apr 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaihendry Environment variables are no longer supported. Those should be supplied in the global configuration

Loading

kaihendry
Copy link

kaihendry commented on 4cb3874 Apr 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but:

global:
   smtp_smarthost: 'email-smtp.us-west-2.amazonaws.com:587'
   smtp_from: 'Alertmanager <dev@example.com>'
   smtp_auth_username: 'AWSID'
   smtp_auth_password: 'AWSSECRET'

Is not working for me.

Loading

mpchadwick
Copy link
Contributor

mpchadwick commented on 4cb3874 Apr 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you previously using SMTP_AUTH_USERNAME and SMTP_AUTH_PASSWORD environment variables? SMTP_AUTH_IDENTITY was mapped to smtp_auth_identity and SMTP_AUTH_SECRET was mapped to smtp_auth_secret.

Loading

mpchadwick
Copy link
Contributor

mpchadwick commented on 4cb3874 Apr 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested again to be sure. I've been testing with Mandrill using simple auth and am not having any issues.

Loading

kaihendry
Copy link

kaihendry commented on 4cb3874 Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was previously using SMTP_AUTH_USERNAME & SMTP_AUTH_PASSWORD and it's not working when it's mapped to smtp_auth_username & smtp_auth_password

Loading

mpchadwick
Copy link
Contributor

mpchadwick commented on 4cb3874 Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to say. I just tested with Mailgun and that's working too. Not sure how there could be something different with SES. You appear to be using simple. I've used that as well for both Mailgun and Mandrill. It should function no differently than it did with environment variables.

Loading

kaihendry
Copy link

kaihendry commented on 4cb3874 Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the noise. smtp_auth_username & smtp_auth_password do work. I think I was confused between smtp_auth_identity & smtp_auth_secret and to make matters worse I rolled back alertmanager to prom/alertmanager:latest from prom/alertmanager:master without thinking.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants