Support the LOGIN method to expand client compatibility #46

erjoalgo · 2023-12-09T00:26:18Z

Some clients do support TLS but use the LOGIN method, where the client
waits for a password prompt before sending the password, as opposed to the
currently-supported PLAIN method, where the client sends both username and
password as part of a single request. This minimal change attempts to support
the LOGIN method by parsing credentials, and then converting them into a
PLAIN-like auth string.

This prevents chasquid from attempting to look for certs under a non-directory, e.g. ./etc/chasquid/domains/.gitignore/certs

Some clients do support TLS but use the LOGIN method, where the client waits for a password prompt before sending the password, as opposed to the currently-supported PLAIN method, where the client sends both username and password as part of a single request. This minimal change attempts to support the LOGIN method by parsing credentials, and then converting them into a PLAIN-like auth string. Example such client: https://salsa.debian.org/debian/ssmtp/-/blob/master/ssmtp.c#L1513

…thentication.

ThinkChaos

I'm definitely not an authority on any of this, but took a quick look cause I'm a user of chasquid and know Go well enough :)
So as a user, thanks for the PR, always nice to have better client compatibility!

ThinkChaos · 2023-12-09T01:09:36Z

internal/smtpsrv/conn.go

@@ -1047,6 +1048,39 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
 	response := ""
 	if len(sp) == 2 {


I think this condition needs to be adjusted cause current code would enter it with LOGIN something.

But I'd say it's probably a better idea to add a Conn.LOGIN method instead to make this kind of bug impossible. Common code could be factored out.

Thanks for the quick response, yes. I'd be happy to refactor this into another method if the maintainer is OK with it.

Thanks to both.

I agree it would be better to refactor this.

I would keep the AUTH function and then split lines 1046-1070 in a separate function for PLAIN, and call either that or a new LOGIN function based on the method the client is using.

Does that seem reasonable to you?

ThinkChaos · 2023-12-09T01:12:02Z

internal/smtpsrv/conn.go

+		plain := []byte{}
+		plain = append(plain, user...)
+		plain = append(plain, '\000')
+		plain = append(plain, user...)
+		plain = append(plain, '\000')
+		plain = append(plain, pass...)
+		response = base64.StdEncoding.EncodeToString(plain)


Similar to the other comment I left, I think this would greatly benefit from splitting the function: it'd be easier and less error prone have the last part of this function be another function that takes user, domain, passwd string arguments.

+1 I think this part can be ommitted if there is a refactor as mentioned in the other comment.

The helper functions can return user, domain and pass, and then AUTH can use that just like it does today.

albertito · 2023-12-11T01:52:08Z

Hi! Thanks a lot for sending this, and to @ThinkChaos for the review!

I just wanted to let you know that I am travelling with limited connectivity, sorry for the delay! I hope to get to this tomorrow!

albertito

Thanks a lot for doing this!

I left comments on this specific code, but will discuss something more general in the "conversation" tab (sorry for the split but github code reviews are quite messy :S).

albertito · 2023-12-12T15:27:13Z

internal/smtpsrv/conn.go

@@ -1035,7 +1036,7 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
 	// response back from the client in the next message.

 	sp := strings.SplitN(params, " ", 2)
-	if len(sp) < 1 || sp[0] != "PLAIN" {
+	if len(sp) < 1 || (sp[0] != "PLAIN" && sp[0] != "LOGIN") {


BTW line 361 also need to be edited to add LOGIN: AUTH PLAIN LOGIN.

albertito · 2023-12-12T15:31:29Z

internal/smtpsrv/conn.go

@@ -1047,6 +1048,39 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
 	response := ""
 	if len(sp) == 2 {


Thanks to both.

I agree it would be better to refactor this.

I would keep the AUTH function and then split lines 1046-1070 in a separate function for PLAIN, and call either that or a new LOGIN function based on the method the client is using.

Does that seem reasonable to you?

albertito · 2023-12-12T15:36:56Z

internal/smtpsrv/conn.go

+		// parameters and convert them into the PLAIN authentication
+		// format, i.e. the base64-encoded string:
+		//	<authorization id> NUL <authentication id> NUL <password>
+		if err := c.writeResponse(334, ""); err != nil {


I think the server needs to write VXNlciBOYW1lAA== (the base64 encoded of User Name\0) here.

https://www.ietf.org/archive/id/draft-murchison-sasl-login-00.txt section 2.2 and 2.3.

albertito · 2023-12-12T15:39:06Z

internal/smtpsrv/conn.go

+			return 554, fmt.Sprintf("5.4.0 Error reading AUTH LOGIN user response: %v", err)
+		} else if user, err = base64.StdEncoding.DecodeString(userb64); err != nil {
+			return 554, fmt.Sprintf("5.4.0 Error parsing AUTH LOGIN user 334: %v", err)
+		} else if err := c.writeResponse(334, ""); err != nil {


I think the server needs to write UGFzc3dvcmQA (the base64 encoded of Password\0) here.

https://www.ietf.org/archive/id/draft-murchison-sasl-login-00.txt section 2.2 and 2.3.

albertito · 2023-12-12T15:40:46Z

internal/smtpsrv/conn.go

+		plain := []byte{}
+		plain = append(plain, user...)
+		plain = append(plain, '\000')
+		plain = append(plain, user...)
+		plain = append(plain, '\000')
+		plain = append(plain, pass...)
+		response = base64.StdEncoding.EncodeToString(plain)


+1 I think this part can be ommitted if there is a refactor as mentioned in the other comment.

The helper functions can return user, domain and pass, and then AUTH can use that just like it does today.

albertito · 2023-12-12T15:50:34Z

I just did a first pass review, overall it looks good, and most of the suggestions are just around reordering code, but I hope it's nothing too problematic.

This still would need a lot of testing but I'm happy to do that part.

All that said, I have to wonder about the benefit of implementing this method vs. the complexity added.

The LOGIN method has been obsolete (IANA SASL mechanisms) for decades.
As far as I can tell it was never really standarized, draft-murchison-sasl-login-00.txt is the closest it got.

And it is basically a more complex variant of the PLAIN method (which is a solid standard and widely used).

So I wonder how many additional "reasonably used clients" this would allow chasquid to interoperate with.

Do you know of any besides ssmtp? If ssmtp is the only one we can find (that is reasonably used), then it might be more practical to send a patch adding PLAIN support to it instead?

What do you think?

Thanks again!

PS: I think the other two patches are useful in any case and I'm inclined to merge them regardless, but let's focus on the LOGIN method first just for clarity of the conversation!

erjoalgo · 2023-12-16T05:23:55Z

Sorry, I've been a little busy, I will take a look next week.

Basically, to test my setup I looked for a few SMTP clients packaged on debian and I initially couldn't get any of them to work out of the box with my chasquid server, so I suspected there was something missing either with my setup or with chasquid, or with my DNS, etc. I tried msmtp, sendemail, and ssmtp. I picked one of these clients. the first one happened to be ssmtp, looked at the source and noticed the problem was the PLAIN vs LOGIN auth, which I fixed and manually tested. Later, I tried fixing msmtp and noticed that it had failed not because of a fundamental incompatibility but because I had failed to specify a number of specific flag values, including: --auth=plain, --tls=on, and --tls-starttls=off. I was never able to get sendemail to work even after installing the requied perl TLS libraries.

I do not know how many clients prefer LOGIN over PLAIN, but in an ideal world most clients should work with most servers out of the box without the need for users to spend too much time tweaking or debugging, especially since most users just care about having a working email server and client, but are probably not willing to dig down and study the sources and compare the server and client to determine why it's not working. If chasquid can be a server that "just works" under most circumstances and make things easy for users and clients then I think it can become even more useful and powerful than it is already.

albertito · 2023-12-20T14:20:04Z

Sorry, I've been a little busy, I will take a look next week.

Of course, no worries, and there's nothing to apologize for!

Basically, to test my setup I looked for a few SMTP clients packaged on debian and I initially couldn't get any of them to work out of the box with my chasquid server, so I suspected there was something missing either with my setup or with chasquid, or with my DNS, etc. I tried msmtp, sendemail, and ssmtp. I picked one of these clients. the first one happened to be ssmtp, looked at the source and noticed the problem was the PLAIN vs LOGIN auth, which I fixed and manually tested.
Later, I tried fixing msmtp and noticed that it had failed not because of a fundamental incompatibility but because I had failed to specify a number of specific flag values, including: --auth=plain, --tls=on, and --tls-starttls=off. I was never able to get sendemail to work even after installing the requied perl TLS libraries.

It's a minor point overall, but for what it's worth, msmtp works fine with auth=on as it can negotiate the method, which is what most clients would do. This is also covered in integration tests (many of them use msmtp).

I've never used sendemail but it seems to support PLAIN. More on this below.

I do not know how many clients prefer LOGIN over PLAIN, but in an ideal world most clients should work with most servers out of the box without the need for users to spend too much time tweaking or debugging, especially since most users just care about having a working email server and client, but are probably not willing to dig down and study the sources and compare the server and client to determine why it's not working. If chasquid can be a server that "just works" under most circumstances and make things easy for users and clients then I think it can become even more useful and powerful than it is already.

I understand this is a frustrating experience, and I really appreciate you sending a patch to make things better.

In practice, I do my best to balance ease of use and flexibility with security, maintainability and reliability. Sometimes the tradeoffs are obvious, but sometimes they are not.

To me, in this case, unfortunately I am not sure the benefits outweigh the costs in terms of complexity.

Another example of a similar case is that chasquid only supports authentication over TLS, even if the client is in localhost, where it could be argued it provides a negligible value. This means client setup on localhost is more complicated, but has the upside of making it less likely to have security bugs around authentication.

However, it is clear from your experience that client set up can be non-intuitive and require more research/trialling than it should.

So how about we add a "Clients" document, where we list different clients and have brief and practical configuration examples, to reduce that friction? And it could include a section on unsupported clients as well. That can help offset some of the difficulty, and cover other aspects you faced, in addition to the login method part.

I'd be happy to write a draft and iterate on that with you to help future users.

What do you think?

Thanks again!

erjoalgo · 2023-12-22T17:37:31Z

Thanks for the thorough reply. I disagree that this change is not worth implementing. I believe LOGIN is a valid SMTP authentication method which valid SMTP clients may choose to employ, and I think it's possible to support it for all clients without compromising security. I do appreciate and respect your work developing and maintaining chasquid, and I respect your opinion so I am closing this PR. I do think providing a client's section would be useful, though I think it would be easier for users if compatibility could be improved on the server side.

albertito · 2023-12-24T01:57:42Z

Thanks for the thorough reply. I disagree that this change is not worth implementing. I believe LOGIN is a valid SMTP authentication method which valid SMTP clients may choose to employ, and I think it's possible to support it for all clients without compromising security. I do appreciate and respect your work developing and maintaining chasquid, and I respect your opinion so I am closing this PR. I do think providing a client's section would be useful, though I think it would be easier for users if compatibility could be improved on the server side.

Thank you so much for your understanding, even though we disagree. I know it is not great to have a change rejected, and I'm sorry about that, but still really appreciate it and the very useful discussion around it.

I would like to merge the other two patches, if that's okay with you?

Also, opened #48 to track the documentation improvements around setting up clients.

Thanks again!

We've had a couple of reported issues about the difficulty of setting up new clients, or confusion due to using broken clients: - #46 - #52 This patch adds the first version of a "Clients" document that includes requirements for all clients, configuration examples, and a list of known-problematic client software. The goal is to help reduce friction and confusion when setting up clients. The document needs more polishing and examples, which hopefully will be added later. Fixes #48.

erjoalgo added 3 commits December 9, 2023 00:09

Skip non-directories like .gitingore in the domains/ directory.

e8e663f

This prevents chasquid from attempting to look for certs under a non-directory, e.g. ./etc/chasquid/domains/.gitignore/certs

Include dovecot-auth-cli in the docker image for debugging dovecot au…

d231048

…thentication.

ThinkChaos reviewed Dec 9, 2023

View reviewed changes

albertito reviewed Dec 12, 2023

View reviewed changes

erjoalgo closed this Dec 22, 2023

albertito mentioned this pull request Dec 24, 2023

Add a document for client configuration #48

Closed

albertito mentioned this pull request Feb 5, 2024

Wording of SMTP error messages #52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the LOGIN method to expand client compatibility #46

Support the LOGIN method to expand client compatibility #46

erjoalgo commented Dec 9, 2023

ThinkChaos left a comment

ThinkChaos Dec 9, 2023

erjoalgo Dec 9, 2023

albertito Dec 12, 2023

ThinkChaos Dec 9, 2023

albertito Dec 12, 2023

albertito commented Dec 11, 2023

albertito left a comment

albertito Dec 12, 2023

albertito Dec 12, 2023

albertito Dec 12, 2023

albertito Dec 12, 2023

albertito Dec 12, 2023

albertito commented Dec 12, 2023 •

edited

erjoalgo commented Dec 16, 2023

albertito commented Dec 20, 2023 •

edited

erjoalgo commented Dec 22, 2023

albertito commented Dec 24, 2023

		@@ -1047,6 +1048,39 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
		response := ""
		if len(sp) == 2 {

Support the LOGIN method to expand client compatibility #46

Support the LOGIN method to expand client compatibility #46

Conversation

erjoalgo commented Dec 9, 2023

ThinkChaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertito commented Dec 11, 2023

albertito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertito commented Dec 12, 2023 • edited

erjoalgo commented Dec 16, 2023

albertito commented Dec 20, 2023 • edited

erjoalgo commented Dec 22, 2023

albertito commented Dec 24, 2023

albertito commented Dec 12, 2023 •

edited

albertito commented Dec 20, 2023 •

edited