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.

Sign up for GitHub

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

Jump to bottom

net/smtp: don't send a trailing space on AUTH command #17794

Closed
robmccoll opened this issue Nov 4, 2016 · 9 comments
Closed

net/smtp: don't send a trailing space on AUTH command #17794

robmccoll opened this issue Nov 4, 2016 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8Maybe

Comments

@robmccoll
Copy link

What version of Go are you using (go version)?

go version go1.7.3 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/rmccoll/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build035170032=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

Use net/smtp to send mail to an SMTP server that supports LOGIN with a handler mechanism that returns LOGIN bug not a msg.

What did you expect to see?

That the message sent to the server was "AUTH LOGIN\r\n".

What did you see instead?

"AUTH LOGIN \r\n"

If the auth mechanism does not return / use a msg, the code as written will print a trailing space on the end of the auth command "AUTH LOGIN \r\n". When using LOGIN with certain SMTP servers (specifically Nemesis ESMTP Service), this is causing the authentication to fail.

Here is a patch with the fix. I do not have the time to run the gauntlet (it would be great if the community was less of an afterthought - maybe hook the tooling into Github PRs instead of requiring so much external effort?), so if someone else that already has the full suite setup could apply this fix, I would be grateful:

201,203c201,209
< 	resp64 := make([]byte, encoding.EncodedLen(len(resp)))
< 	encoding.Encode(resp64, resp)
< 	code, msg64, err := c.cmd(0, "AUTH %s %s", mech, resp64)

---
> 	var code int
> 	var msg64 string
> 	var resp64 = make([]byte, encoding.EncodedLen(len(resp)))
> 	if len(resp) > 0 {
> 		encoding.Encode(resp64, resp)
> 		code, msg64, err = c.cmd(0, "AUTH %s %s", mech, resp64)
> 	} else {
> 		code, msg64, err = c.cmd(0, "AUTH %s", mech)
> 	}
@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2016

You had such a lovely bug report until it went passive-aggressive on our choice of code review tools. But yes, it would be nice if Github were better and there were better integration. I worked on https://github.com/LetsUseGerrit/gerritbot to sync Github PRs to Gerrit reviews, but it's not done.

@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2016

Also, we can't accept contributions without a CLA. See https://golang.org/doc/contribute.html#cla

@robmccoll
Copy link
Author

robmccoll commented Nov 4, 2016 •
edited
Loading

Haha - fair enough. I have consented to the CLA.

Sorry for being a bit over-the-top, but after going through packet captures of SMTP over TLS to determine what is ultimately a fairly simple change, it's pretty discouraging to see how involved the process of getting the fix in myself would actually be. Is it a concern that developers might be encouraged to make forks internal to their projects rather than contribute fixes back if the barrier to entry is too high?

@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2016

I admit that casual or first-time contributions are difficult, and that our contribute.html page is a wall of text. But that hasn't stopped us from getting over 1,000 contributors. Maybe it could be more, but we're not going to give up Gerrit or CLAs. I have plans to make a tool to guide people through (and automate more of) the process, since that contribution page is too wordy. It's not many steps, though.

@quentinmit
Copy link
Contributor

Incidentally, contribute.html is written from the perspective of people who are new to Git. If you're already familiar with Git (and it sounds like you are), you can use Gerrit without any additional local setup. Just go to the Gerrit UI, create yourself an account, and upload your SSH public key. Then you can submit your change with

git push origin HEAD:refs/for/master

I agree, contribute.html seems too wordy to me. I'm just not sure what to remove to make it simpler without also making it more confusing.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 4, 2016
@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 4, 2016
@bradfitz bradfitz mentioned this issue Nov 4, 2016
Closed
@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2016

Let's move the meta documentation conversation to #17802.

@robmccoll
Copy link
Author

@quentinmit - I'm not seeing the public key management tab under settings. Is the Go Gerrit configured for HTTP-only?

@bradfitz
Copy link
Contributor

bradfitz commented Nov 9, 2016

Yes, Go's Gerrit service only speaks HTTPS (not HTTP or SSH).

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/33143 mentions this issue.

@golang golang locked and limited conversation to collaborators Nov 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
4 participants
@bradfitz @quentinmit @robmccoll @gopherbot