smtp: Customize the SSL/TLS port support (#4757)#4818
Conversation
Spaceman1701
left a comment
There was a problem hiding this comment.
Thanks for opening this PR!
One general comment: Would you mind adding a summary to the PR? I'm aware that this is for #4757, but it helps discoverability if the PR has a descriptive summary.
|
|
||
| // Global Config guarantees RequireTLS is not nil. | ||
| if *n.conf.RequireTLS { | ||
| if *n.conf.RequireTLS && !useImplicitTLS { |
There was a problem hiding this comment.
this subtle change in behavior - It seems like the logic on main will enter this branch regardless of the port (and thus regardless of whether we're using implicit tls). Was this change intentional?
There was a problem hiding this comment.
So on one hand, it's silly to use STARTTLS on an existing secure connection... On the other hand, I'm a little worried about changing this behavior out from under people.
Do you know what the spec says about issuing STARTTLS on a secure connection? If this is defined as a no-op, I'm ok with the behavior change.
There was a problem hiding this comment.
So on one hand, it's silly to use
STARTTLSon an existing secure connection... On the other hand, I'm a little worried about changing this behavior out from under people.Do you know what the spec says about issuing
STARTTLSon a secure connection? If this is defined as a no-op, I'm ok with the behavior change.
Regarding the spec, RFC 3207 Section 4 explicitly forbids this behavior:
"A client MUST NOT attempt to start a TLS session if a TLS session is already active."
There was a problem hiding this comment.
I think RFC 3207 is referring to sending STARTTLS more than once per session, not issuing STARTTLS on an implicit TLS connection.
That being said, RFC 8314 doesn't even seem to require a client which provides STARTTLS over cleartext also implement it on the implicit TLS port, so I'm inclined to agree that the current implementation is wrong.
|
|
||
| // Global Config guarantees RequireTLS is not nil. | ||
| if *n.conf.RequireTLS { | ||
| if *n.conf.RequireTLS && !useImplicitTLS { |
There was a problem hiding this comment.
I think RFC 3207 is referring to sending STARTTLS more than once per session, not issuing STARTTLS on an implicit TLS connection.
That being said, RFC 8314 doesn't even seem to require a client which provides STARTTLS over cleartext also implement it on the implicit TLS port, so I'm inclined to agree that the current implementation is wrong.
config/notifiers.go
Outdated
| // ImplicitTLS controls whether to use implicit TLS (direct TLS connection). | ||
| // true: force use of implicit TLS (direct TLS connection) | ||
| // nil (default): auto-detect based on port (465=implicit, other=explicit) for backward compatibility | ||
| ImplicitTLS *bool `yaml:"implicit_tls,omitempty" json:"implicit_tls,omitempty"` |
There was a problem hiding this comment.
Two small bikesheds: I think this should be called ForceImplicitTLS since implicit TLS is always enabled for port 465.
I also think this should be available as a global setting (like smtp_force_implicit_tls) since I think it's common to configure one smtp configuration for all email receivers.
Lines 984 to 994 in 3a2c66e
There was a problem hiding this comment.
Two small bikesheds: I think this should be called
ForceImplicitTLSsince implicit TLS is always enabled for port 465.I also think this should be available as a global setting (like
smtp_force_implicit_tls) since I think it's common to configure one smtp configuration for all email receivers.Lines 984 to 994 in 3a2c66e
Renamed to ForceImplicitTLS and added global config support as suggested.
Spaceman1701
left a comment
There was a problem hiding this comment.
thanks for making those changes, this lgtm
Signed-off-by: gintom <17498860+Gintoms@users.noreply.github.com>
* [ENHANCEMENT] docs(opsgenie): Fix description of `api_url` field. #4908 * [ENHANCEMENT] docs(slack): Document missing app configs. #4871 * [ENHANCEMENT] docs: Fix `max-silence-size-bytes`. #4805 * [ENHANCEMENT] docs: Update expr for `AlertmanagerClusterFailedToSendAlerts` to exclude value 0. #4872 * [ENHANCEMENT] docs: Use matchers for inhibit rules examples. #4131 * [ENHANCEMENT] docs: add notification integrations. #4901 * [ENHANCEMENT] docs: update `slack_config` attachments documentation links. #4802 * [ENHANCEMENT] docs: update description of filter query params in openapi doc. #4810 * [ENHANCEMENT] provider: Reduce lock contention. #4809 * [FEATURE] slack: Add support for top-level text field in slack notification. #4867 * [FEATURE] smtp: Add support for authsecret from file. #3087 * [FEATURE] smtp: Customize the ssl/tls port support (#4757). #4818 * [FEATURE] smtp: Enhance email notifier configuration validation. #4826 * [FEATURE] telegram: Add `chat_id_file` configuration parameter. #4909 * [FEATURE] telegram: Support global bot token. #4823 * [FEATURE] webhook: Support templating in url fields. #4798 * [FEATURE] wechat: Add config directive to pass api secret via file. #4734 * [FEATURE] provider: Implement per alert limits. #4819 * [BUGFIX] Allow empty `group_by` to override parent route. #4825 * [BUGFIX] Set `spellcheck=false` attribute on silence filter input. #4811 * [BUGFIX] jira: Fix for handling api v3 with ADF. #4756 * [BUGFIX] jira: Prevent hostname corruption in cloud api url replacement. #4892 --------- Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com> Signed-off-by: Ben Kochie <superq@gmail.com> Co-authored-by: Ben Kochie <superq@gmail.com>
|
Hi, just wondering which field name should I use? I saw the doc uses |
Hi! It looks like the docs are wrong - the field is |
Summary
The current email notification implementation has hardcoded logic that only enables direct TLS connections for port 465. This creates a limitation when using TLS on other ports, causing
create SMTP client: EOFerrors when the server expects TLS connections on non-465 ports.Description
In notify/email/email.go:133, there's hardcoded logic that only uses
tls.Dial()for port 465, while all other ports use plain TCP connections. This causes issues when SMTP servers require TLS connections on ports other than 465.However, some SMTP servers may be configured to require TLS connections on non-standard ports but don't support STARTTLS, leading to EOF errors.