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

Re-do #163: smtp: STARTTLS before querying auth mechanisms #266

Merged
merged 1 commit into from Apr 1, 2016

Conversation

Projects
None yet
4 participants
@stapelberg
Copy link
Contributor

commented Mar 4, 2016

This was not ported over with the rewrite.

@grobie

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

How will this work if TLS is not available? See comments in #193

@fabxc

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

To clarify, this change did not carry over from the old AM because people reported issues with it. See #193.

@stapelberg stapelberg force-pushed the stapelberg:starttls branch from 9a1bb9a to e3551ef Mar 4, 2016

@stapelberg

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2016

How will this work if TLS is not available?

I’ve modified the pull request to check whether STARTTLS is announced by the server.

To clarify, this change did not carry over from the old AM because people reported issues with it.

The next time people report issues with features that I contribute, I’d be happier if you could notify me and give me a chance to fix the issues instead of effectively reverting my contributions.

I think what #193 is asking for is an option for specifying InsecureSkipVerify in the TLS config, which is orthogonal to what I’m implementing here. Of course, I do realize that the change I’m making has the side-effect of exposing broken configurations that used to work before.

How do you suggest we proceed? Should #193 be fixed before this can go in, or can we fix #193 independently?

@fabxc

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

By the time of your contribution the rewrite was already in process and I only carried over very few changes – and none that might have been source of issues.
So it was no intentional revert of your changes and following up on it drowned in a lot of other work – sorry about that.

I'll take a look at the details tomorrow.

return fmt.Errorf("invalid address: %s", err)
}

if ok, _ := c.Extension("STARTTLS"); ok {

This comment has been minimized.

Copy link
@grobie

grobie Mar 4, 2016

Member

Doesn't this open a downgrade attack vector? An attacker could just strip the STARTTLS field from the extension list and trick alertmanager into sending messages unencrypted.

What do you think about using a flag instead, so that alertmanager would send emails either always with TLS (and failing if it can't) or always without?

This comment has been minimized.

Copy link
@stapelberg

stapelberg Mar 4, 2016

Author Contributor

Yeah, having a flag which defaults to always enforcing TLS sounds good to me.

This comment has been minimized.

Copy link
@fabxc

fabxc Mar 7, 2016

Member

This sounds like something we should add within this PR then.

@fabxc

This comment has been minimized.

Copy link
Member

commented Mar 7, 2016

We can handle the other issue independently. It should just be done before the next release.

Re-do #163: smtp: STARTTLS before querying auth mechanisms
This was not ported over with the rewrite.

@stapelberg stapelberg force-pushed the stapelberg:starttls branch from e3551ef to 5158926 Apr 1, 2016

@stapelberg

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2016

Added the flag. PTAL.

@grobie

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

👍

@juliusv

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

👍 Thanks, Herr @stapelberg :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.