-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support the LOGIN method to expand client compatibility #46
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"bytes" | ||
"context" | ||
"crypto/tls" | ||
"encoding/base64" | ||
"flag" | ||
"fmt" | ||
"io" | ||
|
@@ -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") { | ||
// As we only offer plain, this should not really happen. | ||
return 534, "5.7.9 Asmodeus demands 534 zorkmids for safe passage" | ||
} | ||
|
@@ -1047,6 +1048,39 @@ func (c *Conn) AUTH(params string) (code int, msg string) { | |
response := "" | ||
if len(sp) == 2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 But I'd say it's probably a better idea to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the server needs to write 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the server needs to write 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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
.