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

add SASL login support #11

Closed
wants to merge 1 commit into from
Closed

add SASL login support #11

wants to merge 1 commit into from

Conversation

Habbie
Copy link
Contributor

@Habbie Habbie commented Feb 18, 2019

Tested with freenode

@russss
Copy link
Member

russss commented Feb 21, 2019

Merged this individually, and fixed the caps issue.

@russss russss closed this Feb 21, 2019
@citrin
Copy link

citrin commented Feb 28, 2019

Hi all. I have a problem with using sasl with freenode:

2019/02/28 20:39:22 <-- :weber.freenode.net CAP * LS :account-notify extended-join identify-msg multi-prefix sasl                                                                                                                             
2019/02/28 20:39:22 --> CAP REQ :account-notify
2019/02/28 20:39:22 --> CAP REQ :sasl                                                                                                                                                                                                         
2019/02/28 20:39:22 <-- :weber.freenode.net CAP * ACK :account-notify                                                                                                                                                                         
2019/02/28 20:39:22 <-- :weber.freenode.net CAP * ACK :sasl                                                                                                                                                                                   
2019-02-28 20:39:30 CRITICAL main main.go:69 Error connecting to IRC server: SASL setup timed out. This shouldn't happen.

Any suggestions how to debug this?

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

@citrin what git revision?

@citrin
Copy link

citrin commented Feb 28, 2019

Latest: I've just run go get -u github.com/irccloud/irccat before build.

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

Can you show a (redacted where necessary) config?

@citrin
Copy link

citrin commented Feb 28, 2019

irccat.toml:

[tcp]
listen = "127.0.0.1:10179"

[http]
listen = "127.0.0.1:8045"
tls = false

[irc]
server = "weber.freenode.net:6697"
tls = true
tls_skip_verify = false
nick = "citrin"
sasl_login = "citrin"
sasl_pass = "some_secret"
realname = "Anton"

@citrin
Copy link

citrin commented Feb 28, 2019

I've started to debug and as I can see server reply :weber.freenode.net CAP * ACK :sasl contains space after sasl. We need to strip this space somewhere before calling ACK callbacks (may be in func parseToEvent?).

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

I have reproduced your failure here.

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

The space does not appear to be the issue.

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

The space does not appear to be the issue.

I was wrong. You were right about the space.

This crude patch to go-ircevent makes it work for me. This is not the patch that should be merged!

diff --git a/irc_sasl.go b/irc_sasl.go
index 3346a46..7446788 100644
--- a/irc_sasl.go
+++ b/irc_sasl.go
@@ -20,7 +20,7 @@ func (irc *Connection) setupSASLCallbacks(result chan<- *SASLResult) {
 					result <- &SASLResult{true, errors.New("no SASL capability " + e.Arguments[2])}
 				}
 			}
-			if e.Arguments[1] == "ACK" && e.Arguments[2] == "sasl" {
+			if e.Arguments[1] == "ACK" && strings.TrimSpace(e.Arguments[2]) == "sasl" {
 				if irc.SASLMech != "PLAIN" {
 					result <- &SASLResult{true, errors.New("only PLAIN is supported")}
 				}

I would, however, recommend that this patch be merged:

diff --git a/irc.go b/irc.go
index 45701d3..43d2192 100644
--- a/irc.go
+++ b/irc.go
@@ -71,7 +71,7 @@ func (irc *Connection) readLoop() {
 			}
 
 			if irc.Debug {
-				irc.Log.Printf("<-- %s\n", strings.TrimSpace(msg))
+				irc.Log.Printf("<-- %q\n", msg)
 			}
 
 			irc.lastMessageMutex.Lock()
@@ -167,7 +167,7 @@ func (irc *Connection) writeLoop() {
 			}
 
 			if irc.Debug {
-				irc.Log.Printf("--> %s\n", strings.TrimSpace(b))
+				irc.Log.Printf("--> %q\n", b)
 			}
 
 			// Set a write deadline based on the time out

I might PR some of this tomorrow.

@citrin
Copy link

citrin commented Feb 28, 2019

With this change in irccloud/go-ircevent I can connect to the server:

--- a/irc.go
+++ b/irc.go
@@ -102,6 +102,7 @@ func unescapeTagValue(value string) string {
 func parseToEvent(msg string) (*Event, error) {
        msg = strings.TrimSuffix(msg, "\n") //Remove \r\n
        msg = strings.TrimSuffix(msg, "\r")
+       msg = strings.TrimSuffix(msg, " ")
        event := &Event{Raw: msg}
        if len(msg) < 5 {
                return nil, errors.New("Malformed msg from server")

@citrin
Copy link

citrin commented Feb 28, 2019

if irc.Debug {
- irc.Log.Printf("<-- %s\n", strings.TrimSpace(msg))
+ irc.Log.Printf("<-- %q\n", msg)

This makes debug log less readable - at least we need to strip "\r\n" before logging.

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

This makes debug log less readable - at least we need to strip "\r\n" before logging.

I disagree - the first bug I found in irccat was bare \n being sent without \r

@citrin
Copy link

citrin commented Feb 28, 2019

Yes, it make sense to log line endings in debug mode (I've forgot that with %q they will be escaped before printing).

@Habbie
Copy link
Contributor Author

Habbie commented Feb 28, 2019

(I've forgot that with %q they will be escaped before printing)

Ah yes, looks like this, indeed:

2019/02/28 22:48:51 <-- ":weber.freenode.net CAP * ACK :sasl \r\n"

@citrin
Copy link

citrin commented Feb 28, 2019

It looks like a server bug. But other IRC clients ignore trailing spaces and it will be good if irccat will also ignore then. I would suggest this patch, but I'm not familiar with the code:

--- a/irc.go
+++ b/irc.go
@@ -102,6 +102,7 @@ func unescapeTagValue(value string) string {
 func parseToEvent(msg string) (*Event, error) {
 	msg = strings.TrimSuffix(msg, "\n") //Remove \r\n
 	msg = strings.TrimSuffix(msg, "\r")
+	msg = strings.TrimRight(msg, " ") // workaround; some servers add trailing spaces
 	event := &Event{Raw: msg}
 	if len(msg) < 5 {
 		return nil, errors.New("Malformed msg from server")

I will make a PR to irccloud/go-ircevent.

@russss
Copy link
Member

russss commented Mar 1, 2019

This should be fixed in version 0.4.1.

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

Successfully merging this pull request may close these issues.

3 participants