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

Support the LOGIN method to expand client compatibility #46

Closed
wants to merge 3 commits into from
Closed

Support the LOGIN method to expand client compatibility #46

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8e663f
Skip non-directories like .gitingore in the domains/ directory.
erjoalgo Dec 8, 2023
9002042
Support the LOGIN method over TLS to support more clients like ssmtp.
erjoalgo Dec 8, 2023
d231048
Include dovecot-auth-cli in the docker image for debugging dovecot au…
erjoalgo Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 35 additions & 1 deletion internal/smtpsrv/conn.go
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import (
"bytes"
"context"
"crypto/tls"
"encoding/base64"
"flag"
"fmt"
"io"
Expand Down Expand Up @@ -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") {
Copy link
Owner

Choose a reason for hiding this comment

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

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

// As we only offer plain, this should not really happen.
return 534, "5.7.9 Asmodeus demands 534 zorkmids for safe passage"
}
Expand All @@ -1047,6 +1048,39 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
response := ""
if len(sp) == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

response = sp[1]
} else if sp[0] == "LOGIN" {
// With the LOGIN method, the user password and domain are
// passed in separate messages. Here we prompt for the LOGIN
// 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 {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

return 554, fmt.Sprintf("5.4.0 Error writing AUTH 334: %v", err)
}
user := []byte{}
pass := []byte{}

if userb64, err := c.readLine(); err != nil {
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 {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

return 554, fmt.Sprintf("5.4.0 Error writing AUTH 334: %v", err)
}

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

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)
Comment on lines +1077 to +1083
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

+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.

} else {
// Reply 334 and expect the user to provide it.
// In this case, the text IS relevant, as it is taken as the
Expand Down