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

feat: new email auth type: LOGIN #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

exfly
Copy link

@exfly exfly commented Mar 11, 2020

fix: #104

@jordan-wright
Copy link
Owner

I'm good adding this, and this is from gomail which is MIT licensed so it should be fine.

All I'd ask is that we please change error messages to appropriately identify this library.

Comment on lines +22 to +36
if !server.TLS {
advertised := false
for _, mechanism := range server.Auth {
if mechanism == "LOGIN" {
advertised = true
break
}
}
if !advertised {
return "", nil, errors.New("gomail: unencrypted connection")
}
}
if server.Name != a.host {
return "", nil, errors.New("gomail: wrong host name")
}
Copy link

Choose a reason for hiding this comment

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

I feel I've seen this implementation before (perhaps when perusing gomail when I've needed to implement this myself) but it does not make sense to me. I'd expect something more like:

Suggested change
if !server.TLS {
advertised := false
for _, mechanism := range server.Auth {
if mechanism == "LOGIN" {
advertised = true
break
}
}
if !advertised {
return "", nil, errors.New("gomail: unencrypted connection")
}
}
if server.Name != a.host {
return "", nil, errors.New("gomail: wrong host name")
}
if !server.TLS {
return "", nil, errors.New("unencrypted connection")
}
if server.Name != a.Host {
return "", nil, errors.New("wrong host name")
}
advertised := false
for _, mechanism := range server.Auth {
if mechanism == "LOGIN" {
advertised = true
break
}
}
if !advertised {
return "", nil, errors.New("login auth not supported")
}

I don't see how whether it advertises LOGIN has anything to do with an unencrypted connection. In a comment in the net/smtp source it even mentions:

Note: If TLS is not true, then we can't trust ANYTHING in ServerInfo.

And they use something similar to what I propose above for the smtp.PlainAuth implementation.

Unless I'm missing something?

@clairmont32
Copy link

Is there anything further that needs to be done for this to be merged? Just poking through PRs after starting to utilize the package and came across this, which seems complete.

@leucos
Copy link
Contributor

leucos commented Jan 12, 2022

Well, this project last commit is 1 yo, and author hasn't committed since last march. I guess nothing will happen on the project ?

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

Successfully merging this pull request may close these issues.

golang std lib: stmp no auth LOGIN
5 participants