-
-
Notifications
You must be signed in to change notification settings - Fork 751
XOAUTH2 Authentication Mechanism for SMTP #1432
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
base: main
Are you sure you want to change the base?
Conversation
mattwoberts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @FreakIsTea !
It's a good solution for working with XOAUTH2 that doesn't involve swapping out to another smtp package.
One issue - the authenticate method is called for every email sent, so I think this current code will go and fetch a new token for each email sent. We'd need to implement token caching with this, presumably with the TokenSource method..
|
Hi @mattwoberts Sorry for only getting back to you now. This is a good idea, I'll implement this. Please then check my code for code smells etc. as this is my first time coding in go. |
|
I added the token caching by doing the following:
If there are more inputs, please tell me. I'd be happy to get this change into the project 😁 |
mattwoberts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some good code, thanks very much for doing this - as a non-go developer it's very impressive!
I've got some suggestions for a few minor things to change. In addition to those would you mind adding some tests too - there are some smtp tests in the codebase, if you could add some, to test the caching behavior, that kind of thing.
Thanks!
| if !server.TLS { | ||
| return "", nil, errors.New("smtp: XOAUTH2 requires TLS") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XOAUTH2 requires TLS, but EMAIL_SMTP_ENABLE_STARTTLS can be disabled.
Would be nice to verify this all in env.go, in the reload method:
case "smtp":
mustBeSet("EMAIL_SMTP_HOST")
mustBeSet("EMAIL_SMTP_PORT")
// Validate auth mechanism if explicitly set
authMech := strings.ToUpper(strings.TrimSpace(Config.Email.SMTP.AuthMechanism))
switch authMech {
case "", "AGNOSTIC":
// Username/password are optional - supports unauthenticated SMTP
case "XOAUTH2":
// If explicitly choosing XOAUTH2, OAuth credentials are required
mustBeSet("EMAIL_SMTP_OAUTH_CLIENT_ID")
mustBeSet("EMAIL_SMTP_OAUTH_CLIENT_SECRET")
mustBeSet("EMAIL_SMTP_OAUTH_TOKEN_URL")
mustBeSet("EMAIL_SMTP_USERNAME") // Used as the OAuth user
if !Config.Email.SMTP.EnableStartTLS {
panic("XOAUTH2 requires STARTTLS to be enabled (set EMAIL_SMTP_ENABLE_STARTTLS=true)")
}
default:
// Fail fast on typos like "XOATH2" or "OAUTH2"
panic(fmt.Sprintf("invalid EMAIL_SMTP_AUTH_MECHANISM '%s' (valid options: AGNOSTIC, XOAUTH2)", authMech))
}
}| ClientId string `env:"EMAIL_SMTP_OAUTH_CLIENT_ID"` | ||
| ClientSecret string `env:"EMAIL_SMTP_OAUTH_CLIENT_SECRET"` | ||
| TokenUrl string `env:"EMAIL_SMTP_OAUTH_TOKEN_URL"` | ||
| Scopes string `env:"EMAIL_SMTP_OAUTH_SCOPES"` // comma-separated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind changing the suffix to be upper case, e.g ClientID and TokenURL - for consistency, ta
|
Thanks for the feedback! I'll implement it as soon as I have time, just started working again so I probably will not be able to do the changes anytime soon. I'll try my best though 😊 |
With upcoming changes on cloud providers, SMTP will not be working with AgnosticAuthentication. I added configuration to enable XOAUTH2 authentication with the OAUTH2 client credentials flow. The AgnosticAuthentication will still be the default, and current configs will work the same (no breaking changes). With the following config, the XOAUTH2 authentication can be enabled:
EMAIL_SMTP_AUTH_MECHANISM=> Sets the authentication mechanism. Options:AGNOSTIC,XOAUTH2. Default:AGNOSTIC.EMAIL_SMTP_OAUTH_CLIENT_ID=> Client ID for the OAUTH2 client credentials flowEMAIL_SMTP_OAUTH_CLIENT_SECRET=> Client Secret for the OAUTH2 client credentials flowEMAIL_SMTP_OAUTH_TOKEN_URL=> Token Endpoint for OAUTH2 (IdP specific)EMAIL_SMTP_OAUTH_SCOPES=> Scopes to fetch for the tokenRight now, I haven't added any documentation yet. I'd like for the PR to be checked first and then finalize the PR with the documentation after all necessary changes have been made.
Feel free to suggest changes and ask questions if something's not clear!