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

MAIL in Strict mode doesn't support optional extensions #95

Closed
smoores-dev opened this issue May 9, 2020 · 11 comments · Fixed by #96
Closed

MAIL in Strict mode doesn't support optional extensions #95

smoores-dev opened this issue May 9, 2020 · 11 comments · Fixed by #96
Assignees
Labels

Comments

@smoores-dev
Copy link

It looks like the MAIL parsing in strict mode expects the only argument to be the FROM argument, but the SMTP spec allows for optional mail-parameters that are associated with service extensions. See https://tools.ietf.org/html/rfc5321#section-3.3.

It might be safe to ignore these parameters, but right now the server returns a 501 because it expects the argument to end with a >.

Example command from the client:

MAIL FROM:<example@example.com> AUTH=<username>

Response from go-smtp:

501 Was expecting MAIL arg syntax of FROM:<address>

It seems like KMail includes these additional parameters (see https://www.reddit.com/r/ProtonMail/comments/evadaz/need_help_setting_up_outgoing_with_kmail/), as well as emailrelay.

@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

Does RFC 5321 specify how to handle unknown parameters? I do not see anything about it.

@foxcpp foxcpp added the bug label May 9, 2020
@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

protonmail-bridge uses 2018 version of go-smtp via this fork: https://github.com/ProtonMail/go-smtp

I am unable to reproduce the issue this exact error message at latest master.

Here is the test case I am using:

func TestServerStrictESMTP(t *testing.T) {
	_, s, c, scanner := testServerAuthenticated(t)
	s.Strict = true
	defer s.Close()
	defer c.Close()

	io.WriteString(c, "MAIL FROM:<alice@wonderland.book> AUTH=test\r\n")
	scanner.Scan()
	if !strings.HasPrefix(scanner.Text(), "250 ") {
		t.Fatal("Invalid MAIL response:", scanner.Text())
	}

	return
}

@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

Interesting, however, it causes a different error. "Unknown MAIL FROM argument". There is a test that checks for that error.

@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

The proper solution is to recognize AUTH= parameter and pass it to backend.

@foxcpp foxcpp self-assigned this May 9, 2020
@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

https://tools.ietf.org/html/rfc4954

For this reason, servers that advertise support for this
extension MUST support the AUTH parameter to the MAIL FROM
command even when the client has not authenticated itself to the
server.

@smoores-dev
Copy link
Author

Interesting, however, it causes a different error. "Unknown MAIL FROM argument". There is a test that checks for that error.

I'm also confused by this! Reading through the code, what you're describing is absolutely what I'd expect to see. For context, I was interacting with this library through the https://github.com/ProtonMail/proton-bridge application; I wonder if that is being bundled with an older version of this package that handled that responded with the other error message in this scenario? (Full disclosure, I don't know much about Go package management) But it definitely seems to be the AUTH= parameter that's causing this there; dropping it fixes the error message.

@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

(Full disclosure, I don't know much about Go package management)

https://github.com/ProtonMail/proton-bridge/blob/8288a39ff427dccb3db66048240393084db2b23b/go.mod#L76

@smoores-dev
Copy link
Author

Ah, I missed that first bit of your earlier comment, thanks

@smoores-dev
Copy link
Author

Weirdly, looking through that fork, it seems like it shouldn't be responding with an error message at all! But maybe I ought to open an issue on that repo to at least pull in the latest from here (I guess.. after this AUTH= handling is merged)

@foxcpp
Copy link
Collaborator

foxcpp commented May 9, 2020

Problem is resolved in go-smtp. As of proton-bridge issue, I suggest you to raise issue in bridge repo to update used go-smtp version.

@smoores-dev
Copy link
Author

Will do! Thanks so much @foxcpp

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

Successfully merging a pull request may close this issue.

2 participants