From ec4f1c189a8d07adc58a48fd36cf5eaa5f9eb037 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Mon, 31 Dec 2018 11:37:58 -0500 Subject: [PATCH 01/10] pointless optimizations to the logger --- irc/logger/logger.go | 64 +++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 22 deletions(-) diff --git a/irc/logger/logger.go b/irc/logger/logger.go index f3e98c4b..38090c28 100644 --- a/irc/logger/logger.go +++ b/irc/logger/logger.go @@ -5,6 +5,7 @@ package logger import ( "bufio" + "bytes" "fmt" "os" "time" @@ -32,6 +33,20 @@ const ( LogError ) +var ( + colorTimeGrey = ansi.ColorFunc("243") + colorGrey = ansi.ColorFunc("8") + colorAlert = ansi.ColorFunc("232+b:red") + colorWarn = ansi.ColorFunc("black:214") + colorInfo = ansi.ColorFunc("117") + colorDebug = ansi.ColorFunc("78") + colorSection = ansi.ColorFunc("229") + separator = colorGrey(":") + + colorableStdout = colorable.NewColorableStdout() + colorableStderr = colorable.NewColorableStderr() +) + var ( // LogLevelNames takes a config name and gives the real log level. LogLevelNames = map[string]Level{ @@ -230,51 +245,56 @@ func (logger *singleLogger) Log(level Level, logType string, messageParts ...str } // assemble full line - timeGrey := ansi.ColorFunc("243") - grey := ansi.ColorFunc("8") - alert := ansi.ColorFunc("232+b:red") - warn := ansi.ColorFunc("black:214") - info := ansi.ColorFunc("117") - debug := ansi.ColorFunc("78") - section := ansi.ColorFunc("229") levelDisplay := LogLevelDisplayNames[level] if level == LogError { - levelDisplay = alert(levelDisplay) + levelDisplay = colorAlert(levelDisplay) } else if level == LogWarning { - levelDisplay = warn(levelDisplay) + levelDisplay = colorWarn(levelDisplay) } else if level == LogInfo { - levelDisplay = info(levelDisplay) + levelDisplay = colorInfo(levelDisplay) } else if level == LogDebug { - levelDisplay = debug(levelDisplay) + levelDisplay = colorDebug(levelDisplay) } - sep := grey(":") - fullStringFormatted := fmt.Sprintf("%s %s %s %s %s %s ", timeGrey(time.Now().UTC().Format("2006-01-02T15:04:05.000Z")), sep, levelDisplay, sep, section(logType), sep) - fullStringRaw := fmt.Sprintf("%s : %s : %s : ", time.Now().UTC().Format("2006-01-02T15:04:05Z"), LogLevelDisplayNames[level], logType) + var formattedBuf, rawBuf bytes.Buffer + fmt.Fprintf(&formattedBuf, "%s %s %s %s %s %s ", colorTimeGrey(time.Now().UTC().Format("2006-01-02T15:04:05.000Z")), separator, levelDisplay, separator, colorSection(logType), separator) + if logger.MethodFile.Enabled { + fmt.Fprintf(&rawBuf, "%s : %s : %s : ", time.Now().UTC().Format("2006-01-02T15:04:05Z"), LogLevelDisplayNames[level], logType) + } for i, p := range messageParts { - fullStringFormatted += p - fullStringRaw += p - if i != len(messageParts)-1 { - fullStringFormatted += " " + sep + " " - fullStringRaw += " : " + formattedBuf.WriteString(p) + if logger.MethodFile.Enabled { + rawBuf.WriteString(p) } + if i != len(messageParts)-1 { + formattedBuf.WriteRune(' ') + formattedBuf.WriteString(separator) + formattedBuf.WriteRune(' ') + if logger.MethodFile.Enabled { + rawBuf.WriteString(" : ") + } + } + } + formattedBuf.WriteRune('\n') + if logger.MethodFile.Enabled { + rawBuf.WriteRune('\n') } // output if logger.MethodSTDOUT { logger.stdoutWriteLock.Lock() - fmt.Fprintln(colorable.NewColorableStdout(), fullStringFormatted) + colorableStdout.Write(formattedBuf.Bytes()) logger.stdoutWriteLock.Unlock() } if logger.MethodSTDERR { logger.stdoutWriteLock.Lock() - fmt.Fprintln(colorable.NewColorableStderr(), fullStringFormatted) + colorableStderr.Write(formattedBuf.Bytes()) logger.stdoutWriteLock.Unlock() } if logger.MethodFile.Enabled { logger.fileWriteLock.Lock() - logger.MethodFile.Writer.WriteString(fullStringRaw + "\n") + logger.MethodFile.Writer.Write(rawBuf.Bytes()) logger.MethodFile.Writer.Flush() logger.fileWriteLock.Unlock() } From c2b2559ab460be2ca1e17777f0c9d1b69130a789 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Mon, 31 Dec 2018 11:33:42 -0500 Subject: [PATCH 02/10] avoid some uses of Sprintf for loglines --- irc/accounts.go | 12 ++++++------ irc/channel.go | 2 +- irc/client_lookup_set.go | 2 +- irc/handlers.go | 2 +- irc/nickserv.go | 2 +- irc/server.go | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/irc/accounts.go b/irc/accounts.go index c4c86e17..f676cba2 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -99,7 +99,7 @@ func (am *AccountManager) buildNickToAccountIndex() { }) if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("couldn't read reserved nicks: %v", err)) + am.server.logger.Error("internal", "couldn't read reserved nicks", err.Error()) } else { am.Lock() am.nickToAccount = result @@ -286,14 +286,14 @@ func (am *AccountManager) serializeCredentials(passphrase string, certfp string) bcryptCost := int(am.server.Config().Accounts.Registration.BcryptCost) creds.PassphraseHash, err = passwd.GenerateFromPassword([]byte(passphrase), bcryptCost) if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not hash password: %v", err)) + am.server.logger.Error("internal", "could not hash password", err.Error()) return "", errAccountCreation } } credText, err := json.Marshal(creds) if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not marshal credentials: %v", err)) + am.server.logger.Error("internal", "could not marshal credentials", err.Error()) return "", errAccountCreation } return string(credText), nil @@ -367,7 +367,7 @@ func (am *AccountManager) dispatchMailtoCallback(client *Client, casefoldedAccou // config.TLS.InsecureSkipVerify err = smtp.SendMail(addr, auth, config.Sender, []string{callbackValue}, message) if err != nil { - am.server.logger.Error("internal", fmt.Sprintf("Failed to dispatch e-mail: %v", err)) + am.server.logger.Error("internal", "Failed to dispatch e-mail", err.Error()) } return } @@ -619,7 +619,7 @@ func (am *AccountManager) deserializeRawAccount(raw rawClientAccount) (result Cl result.RegisteredAt = time.Unix(regTimeInt, 0) e := json.Unmarshal([]byte(raw.Credentials), &result.Credentials) if e != nil { - am.server.logger.Error("internal", fmt.Sprintf("could not unmarshal credentials: %v", e)) + am.server.logger.Error("internal", "could not unmarshal credentials", e.Error()) err = errAccountDoesNotExist return } @@ -628,7 +628,7 @@ func (am *AccountManager) deserializeRawAccount(raw rawClientAccount) (result Cl if raw.VHost != "" { e := json.Unmarshal([]byte(raw.VHost), &result.VHost) if e != nil { - am.server.logger.Warning("internal", fmt.Sprintf("could not unmarshal vhost for account %s: %v", result.Name, e)) + am.server.logger.Warning("internal", "could not unmarshal vhost for account", result.Name, e.Error()) // pretend they have no vhost and move on } } diff --git a/irc/channel.go b/irc/channel.go index 7144da9b..579d3fd6 100644 --- a/irc/channel.go +++ b/irc/channel.go @@ -48,7 +48,7 @@ type Channel struct { func NewChannel(s *Server, name string, regInfo *RegisteredChannel) *Channel { casefoldedName, err := CasefoldChannel(name) if err != nil { - s.logger.Error("internal", fmt.Sprintf("Bad channel name %s: %v", name, err)) + s.logger.Error("internal", "Bad channel name", name, err.Error()) return nil } diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index 65ed3a4b..e54e29b6 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -72,7 +72,7 @@ func (clients *ClientManager) removeInternal(client *Client) (err error) { delete(clients.byNick, oldcfnick) } else { // this shouldn't happen, but we can ignore it - client.server.logger.Warning("internal", fmt.Sprintf("clients for nick %s out of sync", oldcfnick)) + client.server.logger.Warning("internal", "clients for nick out of sync", oldcfnick) err = errNickMissing } } diff --git a/irc/handlers.go b/irc/handlers.go index d65d0280..6aca801f 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -367,7 +367,7 @@ func authErrorToMessage(server *Server, err error) (msg string) { if err == errAccountDoesNotExist || err == errAccountUnverified || err == errAccountInvalidCredentials { msg = err.Error() } else { - server.logger.Error("internal", fmt.Sprintf("sasl authentication failure: %v", err)) + server.logger.Error("internal", "sasl authentication failure", err.Error()) msg = "Unknown" } return diff --git a/irc/nickserv.go b/irc/nickserv.go index 72ca453e..37973e90 100644 --- a/irc/nickserv.go +++ b/irc/nickserv.go @@ -443,7 +443,7 @@ func nsPasswdHandler(server *Server, client *Client, command, params string, rb if err == nil { nsNotice(rb, client.t("Password changed")) } else { - server.logger.Error("internal", fmt.Sprintf("could not upgrade user password: %v", err)) + server.logger.Error("internal", "could not upgrade user password:", err.Error()) nsNotice(rb, client.t("Password could not be changed due to server error")) } } diff --git a/irc/server.go b/irc/server.go index 1afee925..5970f9b6 100644 --- a/irc/server.go +++ b/irc/server.go @@ -836,7 +836,7 @@ func (server *Server) setupPprofListener(config *Config) { } go func() { if err := ps.ListenAndServe(); err != nil { - server.logger.Error("rehash", fmt.Sprintf("pprof listener failed: %v", err)) + server.logger.Error("rehash", "pprof listener failed", err.Error()) } }() server.pprofServer = &ps From 960d51159c09a5a86a94b35e579376c149f0eaa5 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 1 Jan 2019 13:00:16 -0500 Subject: [PATCH 03/10] add ClientDetails struct for getting a snapshot of client state --- irc/client.go | 22 +++++++++++++++++++++- irc/getters.go | 18 +++++++++++------- irc/handlers.go | 2 +- irc/nickname.go | 2 +- irc/server.go | 11 +++++------ irc/whowas.go | 11 +---------- irc/whowas_test.go | 26 +++++++++++++------------- 7 files changed, 53 insertions(+), 39 deletions(-) diff --git a/irc/client.go b/irc/client.go index ebcaa066..849d69ea 100644 --- a/irc/client.go +++ b/irc/client.go @@ -52,7 +52,7 @@ type ResumeDetails struct { // Client is an IRC client. type Client struct { account string - accountName string + accountName string // display name of the account: uncasefolded, '*' if not logged in atime time.Time authorized bool awayMessage string @@ -100,6 +100,25 @@ type Client struct { history *history.Buffer } +// WhoWas is the subset of client details needed to answer a WHOWAS query +type WhoWas struct { + nick string + nickCasefolded string + username string + hostname string + realname string +} + +// ClientDetails is a standard set of details about a client +type ClientDetails struct { + WhoWas + + nickMask string + nickMaskCasefolded string + account string + accountName string +} + // NewClient sets up a new client and starts its goroutine. func NewClient(server *Server, conn net.Conn, isTLS bool) { now := time.Now() @@ -117,6 +136,7 @@ func NewClient(server *Server, conn net.Conn, isTLS bool) { flags: modes.NewModeSet(), server: server, socket: socket, + accountName: "*", nick: "*", // * is used until actual nick is given nickCasefolded: "*", nickMaskString: "*", // * is used until actual nick is given diff --git a/irc/getters.go b/irc/getters.go index fcaab836..b2ac01c3 100644 --- a/irc/getters.go +++ b/irc/getters.go @@ -147,9 +147,6 @@ func (client *Client) Account() string { func (client *Client) AccountName() string { client.stateMutex.RLock() defer client.stateMutex.RUnlock() - if client.accountName == "" { - return "*" - } return client.accountName } @@ -217,15 +214,22 @@ func (client *Client) Channels() (result []*Channel) { } func (client *Client) WhoWas() (result WhoWas) { + return client.Details().WhoWas +} + +func (client *Client) Details() (result ClientDetails) { client.stateMutex.RLock() defer client.stateMutex.RUnlock() - result.nicknameCasefolded = client.nickCasefolded - result.nickname = client.nick + result.nick = client.nick + result.nickCasefolded = client.nickCasefolded result.username = client.username - result.hostname = client.hostname + result.hostname = client.username result.realname = client.realname - + result.nickMask = client.nickMaskString + result.nickMaskCasefolded = client.nickMaskCasefolded + result.account = client.account + result.accountName = client.accountName return } diff --git a/irc/handlers.go b/irc/handlers.go index 6aca801f..0537382f 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -2541,7 +2541,7 @@ func whowasHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Re } } else { for _, whoWas := range results { - rb.Add(nil, server.name, RPL_WHOWASUSER, cnick, whoWas.nickname, whoWas.username, whoWas.hostname, "*", whoWas.realname) + rb.Add(nil, server.name, RPL_WHOWASUSER, cnick, whoWas.nick, whoWas.username, whoWas.hostname, "*", whoWas.realname) } } if len(nickname) > 0 { diff --git a/irc/nickname.go b/irc/nickname.go index c72de556..d1df3153 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -59,7 +59,7 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nickname, cfnick)) if hadNick { - target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), whowas.nickname, nickname)) + target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), whowas.nick, nickname)) target.server.whoWas.Append(whowas) for friend := range target.Friends() { friend.Send(nil, origNickMask, "NICK", nickname) diff --git a/irc/server.go b/irc/server.go index 5970f9b6..9beecb85 100644 --- a/irc/server.go +++ b/irc/server.go @@ -500,9 +500,9 @@ func (client *Client) WhoisChannelsNames(target *Client) []string { func (client *Client) getWhoisOf(target *Client, rb *ResponseBuffer) { cnick := client.Nick() - targetInfo := target.WhoWas() - rb.Add(nil, client.server.name, RPL_WHOISUSER, cnick, targetInfo.nickname, targetInfo.username, targetInfo.hostname, "*", targetInfo.realname) - tnick := targetInfo.nickname + targetInfo := target.Details() + rb.Add(nil, client.server.name, RPL_WHOISUSER, cnick, targetInfo.nick, targetInfo.username, targetInfo.hostname, "*", targetInfo.realname) + tnick := targetInfo.nick whoischannels := client.WhoisChannelsNames(target) if whoischannels != nil { @@ -518,9 +518,8 @@ func (client *Client) getWhoisOf(target *Client, rb *ResponseBuffer) { if target.HasMode(modes.TLS) { rb.Add(nil, client.server.name, RPL_WHOISSECURE, cnick, tnick, client.t("is using a secure connection")) } - taccount := target.AccountName() - if taccount != "*" { - rb.Add(nil, client.server.name, RPL_WHOISACCOUNT, cnick, tnick, taccount, client.t("is logged in as")) + if targetInfo.accountName != "*" { + rb.Add(nil, client.server.name, RPL_WHOISACCOUNT, cnick, tnick, targetInfo.accountName, client.t("is logged in as")) } if target.HasMode(modes.Bot) { rb.Add(nil, client.server.name, RPL_WHOISBOT, cnick, tnick, ircfmt.Unescape(fmt.Sprintf(client.t("is a $bBot$b on %s"), client.server.Config().Network.Name))) diff --git a/irc/whowas.go b/irc/whowas.go index 3ecf870d..e12b587c 100644 --- a/irc/whowas.go +++ b/irc/whowas.go @@ -22,15 +22,6 @@ type WhoWasList struct { accessMutex sync.RWMutex // tier 1 } -// WhoWas is an entry in the WhoWasList. -type WhoWas struct { - nicknameCasefolded string - nickname string - username string - hostname string - realname string -} - // NewWhoWasList returns a new WhoWasList func NewWhoWasList(size int) *WhoWasList { return &WhoWasList{ @@ -82,7 +73,7 @@ func (list *WhoWasList) Find(nickname string, limit int) (results []WhoWas) { // iterate backwards through the ring buffer pos := list.prev(list.end) for limit == 0 || len(results) < limit { - if casefoldedNickname == list.buffer[pos].nicknameCasefolded { + if casefoldedNickname == list.buffer[pos].nickCasefolded { results = append(results, list.buffer[pos]) } if pos == list.start { diff --git a/irc/whowas_test.go b/irc/whowas_test.go index 4f011660..2ee76536 100644 --- a/irc/whowas_test.go +++ b/irc/whowas_test.go @@ -13,11 +13,11 @@ func makeTestWhowas(nick string) WhoWas { panic(err) } return WhoWas{ - nicknameCasefolded: cfnick, - nickname: nick, - username: "user", - hostname: "oragono.io", - realname: "Real Name", + nickCasefolded: cfnick, + nick: nick, + username: "user", + hostname: "oragono.io", + realname: "Real Name", } } @@ -36,48 +36,48 @@ func TestWhoWas(t *testing.T) { t.Fatalf("incorrect whowas results: %v", results) } results = wwl.Find("dan-", 10) - if len(results) != 1 || results[0].nickname != "dan-" { + if len(results) != 1 || results[0].nick != "dan-" { t.Fatalf("incorrect whowas results: %v", results) } wwl.Append(makeTestWhowas("slingamn")) results = wwl.Find("slingamN", 10) - if len(results) != 1 || results[0].nickname != "slingamn" { + if len(results) != 1 || results[0].nick != "slingamn" { t.Fatalf("incorrect whowas results: %v", results) } wwl.Append(makeTestWhowas("Dan-")) results = wwl.Find("dan-", 10) // reverse chronological order - if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" { + if len(results) != 2 || results[0].nick != "Dan-" || results[1].nick != "dan-" { t.Fatalf("incorrect whowas results: %v", results) } // 0 means no limit results = wwl.Find("dan-", 0) - if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" { + if len(results) != 2 || results[0].nick != "Dan-" || results[1].nick != "dan-" { t.Fatalf("incorrect whowas results: %v", results) } // a limit of 1 should return the most recent entry only results = wwl.Find("dan-", 1) - if len(results) != 1 || results[0].nickname != "Dan-" { + if len(results) != 1 || results[0].nick != "Dan-" { t.Fatalf("incorrect whowas results: %v", results) } wwl.Append(makeTestWhowas("moocow")) results = wwl.Find("moocow", 10) - if len(results) != 1 || results[0].nickname != "moocow" { + if len(results) != 1 || results[0].nick != "moocow" { t.Fatalf("incorrect whowas results: %v", results) } results = wwl.Find("dan-", 10) // should have overwritten the original entry, leaving the second - if len(results) != 1 || results[0].nickname != "Dan-" { + if len(results) != 1 || results[0].nick != "Dan-" { t.Fatalf("incorrect whowas results: %v", results) } // overwrite the second entry wwl.Append(makeTestWhowas("enckse")) results = wwl.Find("enckse", 10) - if len(results) != 1 || results[0].nickname != "enckse" { + if len(results) != 1 || results[0].nick != "enckse" { t.Fatalf("incorrect whowas results: %v", results) } results = wwl.Find("slingamn", 10) From 3cd3601a305d232ffc11670d42cb0876ee508664 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 1 Jan 2019 13:15:38 -0500 Subject: [PATCH 04/10] refactor join/part --- irc/channel.go | 64 +++++++++++++++++++++++++------------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/irc/channel.go b/irc/channel.go index 579d3fd6..b3d69db7 100644 --- a/irc/channel.go +++ b/irc/channel.go @@ -346,8 +346,7 @@ func (channel *Channel) IsEmpty() bool { // Join joins the given client to this channel (if they can be joined). func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *ResponseBuffer) { - account := client.Account() - nickMaskCasefolded := client.NickMaskCasefolded() + details := client.Details() channel.stateMutex.RLock() chname := channel.name @@ -357,7 +356,7 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp limit := channel.userLimit chcount := len(channel.members) _, alreadyJoined := channel.members[client] - persistentMode := channel.accountToUMode[account] + persistentMode := channel.accountToUMode[details.account] channel.stateMutex.RUnlock() if alreadyJoined { @@ -367,7 +366,7 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp // the founder can always join (even if they disabled auto +q on join); // anyone who automatically receives halfop or higher can always join - hasPrivs := isSajoin || (founder != "" && founder == account) || (persistentMode != 0 && persistentMode != modes.Voice) + hasPrivs := isSajoin || (founder != "" && founder == details.account) || (persistentMode != 0 && persistentMode != modes.Voice) if !hasPrivs && limit != 0 && chcount >= limit { rb.Add(nil, client.server.name, ERR_CHANNELISFULL, chname, fmt.Sprintf(client.t("Cannot join channel (+%s)"), "l")) @@ -379,20 +378,20 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp return } - isInvited := client.CheckInvited(chcfname) || channel.lists[modes.InviteMask].Match(nickMaskCasefolded) + isInvited := client.CheckInvited(chcfname) || channel.lists[modes.InviteMask].Match(details.nickMaskCasefolded) if !hasPrivs && channel.flags.HasMode(modes.InviteOnly) && !isInvited { rb.Add(nil, client.server.name, ERR_INVITEONLYCHAN, chname, fmt.Sprintf(client.t("Cannot join channel (+%s)"), "i")) return } - if !hasPrivs && channel.lists[modes.BanMask].Match(nickMaskCasefolded) && + if !hasPrivs && channel.lists[modes.BanMask].Match(details.nickMaskCasefolded) && !isInvited && - !channel.lists[modes.ExceptMask].Match(nickMaskCasefolded) { + !channel.lists[modes.ExceptMask].Match(details.nickMaskCasefolded) { rb.Add(nil, client.server.name, ERR_BANNEDFROMCHAN, chname, fmt.Sprintf(client.t("Cannot join channel (+%s)"), "b")) return } - client.server.logger.Debug("join", fmt.Sprintf("%s joined channel %s", client.nick, chname)) + client.server.logger.Debug("join", fmt.Sprintf("%s joined channel %s", details.nick, chname)) newChannel, givenMode := func() (newChannel bool, givenMode modes.Mode) { channel.joinPartMutex.Lock() @@ -416,15 +415,19 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp }() channel.regenerateMembersCache() + + channel.history.Add(history.Item{ + Type: history.Join, + Nick: details.nickMask, + AccountName: details.accountName, + Msgid: details.realname, + }) + return }() client.addChannel(channel) - nick := client.Nick() - nickmask := client.NickMaskString() - realname := client.Realname() - accountName := client.AccountName() var modestr string if givenMode != 0 { modestr = fmt.Sprintf("+%v", givenMode) @@ -435,19 +438,19 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp continue } if member.capabilities.Has(caps.ExtendedJoin) { - member.Send(nil, nickmask, "JOIN", chname, accountName, realname) + member.Send(nil, details.nickMask, "JOIN", chname, details.accountName, details.realname) } else { - member.Send(nil, nickmask, "JOIN", chname) + member.Send(nil, details.nickMask, "JOIN", chname) } if givenMode != 0 { - member.Send(nil, client.server.name, "MODE", chname, modestr, nick) + member.Send(nil, client.server.name, "MODE", chname, modestr, details.nick) } } if client.capabilities.Has(caps.ExtendedJoin) { - rb.Add(nil, nickmask, "JOIN", chname, accountName, realname) + rb.Add(nil, details.nickMask, "JOIN", chname, details.accountName, details.realname) } else { - rb.Add(nil, nickmask, "JOIN", chname) + rb.Add(nil, details.nickMask, "JOIN", chname) } // don't send topic when it's an entirely new channel @@ -458,22 +461,19 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp channel.Names(client, rb) if givenMode != 0 { - rb.Add(nil, client.server.name, "MODE", chname, modestr, nick) + rb.Add(nil, client.server.name, "MODE", chname, modestr, details.nick) } - channel.history.Add(history.Item{ - Type: history.Join, - Nick: nickmask, - AccountName: accountName, - Msgid: realname, - }) - // TODO #259 can be implemented as Flush(false) (i.e., nonblocking) while holding joinPartMutex rb.Flush(true) replayLimit := channel.server.Config().History.AutoreplayOnJoin if replayLimit > 0 { - items := channel.history.Latest(replayLimit) + // don't replay the client's own events + matcher := func(item history.Item) bool { + return item.Nick != details.nickMask + } + items := channel.history.Match(matcher, replayLimit) channel.replayHistoryItems(rb, items) rb.Flush(true) } @@ -489,20 +489,20 @@ func (channel *Channel) Part(client *Client, message string, rb *ResponseBuffer) channel.Quit(client) - nickmask := client.NickMaskString() + details := client.Details() for _, member := range channel.Members() { - member.Send(nil, nickmask, "PART", chname, message) + member.Send(nil, details.nickMask, "PART", chname, message) } - rb.Add(nil, nickmask, "PART", chname, message) + rb.Add(nil, details.nickMask, "PART", chname, message) channel.history.Add(history.Item{ Type: history.Part, - Nick: nickmask, - AccountName: client.AccountName(), + Nick: details.nickMask, + AccountName: details.accountName, Message: utils.MakeSplitMessage(message, true), }) - client.server.logger.Debug("part", fmt.Sprintf("%s left channel %s", client.nick, chname)) + client.server.logger.Debug("part", fmt.Sprintf("%s left channel %s", details.nick, chname)) } // Resume is called after a successful global resume to: From f94f737b319461b6f681ffc39e8136e3d4db98ad Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 1 Jan 2019 16:45:37 -0500 Subject: [PATCH 05/10] add support for login throttling --- Makefile | 1 + irc/client.go | 22 ++++--- irc/config.go | 17 +++-- irc/connection_limits/throttler.go | 63 ++++++++++++++---- irc/connection_limits/throttler_test.go | 86 +++++++++++++++++++++++++ irc/handlers.go | 38 +++++++---- irc/nickserv.go | 19 +++++- oragono.yaml | 11 ++++ 8 files changed, 220 insertions(+), 37 deletions(-) create mode 100644 irc/connection_limits/throttler_test.go diff --git a/Makefile b/Makefile index 4d6cd4b9..864031f0 100644 --- a/Makefile +++ b/Makefile @@ -20,6 +20,7 @@ test: python3 ./gencapdefs.py | diff - ${capdef_file} cd irc && go test . && go vet . cd irc/caps && go test . && go vet . + cd irc/connection_limits && go test . && go vet . cd irc/history && go test . && go vet . cd irc/isupport && go test . && go vet . cd irc/modes && go test . && go vet . diff --git a/irc/client.go b/irc/client.go index 849d69ea..b48176e8 100644 --- a/irc/client.go +++ b/irc/client.go @@ -19,6 +19,7 @@ import ( "github.com/goshuirc/irc-go/ircmsg" ident "github.com/oragono/go-ident" "github.com/oragono/oragono/irc/caps" + "github.com/oragono/oragono/irc/connection_limits" "github.com/oragono/oragono/irc/history" "github.com/oragono/oragono/irc/modes" "github.com/oragono/oragono/irc/sno" @@ -73,6 +74,7 @@ type Client struct { isDestroyed bool isQuitting bool languages []string + loginThrottle connection_limits.GenericThrottle maxlenTags uint32 maxlenRest uint32 nick string @@ -126,14 +128,18 @@ func NewClient(server *Server, conn net.Conn, isTLS bool) { fullLineLenLimit := config.Limits.LineLen.Tags + config.Limits.LineLen.Rest socket := NewSocket(conn, fullLineLenLimit*2, config.Server.MaxSendQBytes) client := &Client{ - atime: now, - authorized: server.Password() == nil, - capabilities: caps.NewSet(), - capState: caps.NoneState, - capVersion: caps.Cap301, - channels: make(ChannelSet), - ctime: now, - flags: modes.NewModeSet(), + atime: now, + authorized: server.Password() == nil, + capabilities: caps.NewSet(), + capState: caps.NoneState, + capVersion: caps.Cap301, + channels: make(ChannelSet), + ctime: now, + flags: modes.NewModeSet(), + loginThrottle: connection_limits.GenericThrottle{ + Duration: config.Accounts.LoginThrottling.Duration, + Limit: config.Accounts.LoginThrottling.MaxAttempts, + }, server: server, socket: socket, accountName: "*", diff --git a/irc/config.go b/irc/config.go index dcf30b7b..8003eb5b 100644 --- a/irc/config.go +++ b/irc/config.go @@ -54,10 +54,15 @@ func (conf *TLSListenConfig) Config() (*tls.Config, error) { type AccountConfig struct { Registration AccountRegistrationConfig - AuthenticationEnabled bool `yaml:"authentication-enabled"` - SkipServerPassword bool `yaml:"skip-server-password"` - NickReservation NickReservationConfig `yaml:"nick-reservation"` - VHosts VHostConfig + AuthenticationEnabled bool `yaml:"authentication-enabled"` + LoginThrottling struct { + Enabled bool + Duration time.Duration + MaxAttempts int `yaml:"max-attempts"` + } `yaml:"login-throttling"` + SkipServerPassword bool `yaml:"skip-server-password"` + NickReservation NickReservationConfig `yaml:"nick-reservation"` + VHosts VHostConfig } // AccountRegistrationConfig controls account registration. @@ -558,6 +563,10 @@ func LoadConfig(filename string) (config *Config, err error) { config.Accounts.VHosts.ValidRegexp = defaultValidVhostRegex } + if !config.Accounts.LoginThrottling.Enabled { + config.Accounts.LoginThrottling.MaxAttempts = 0 // limit of 0 means disabled + } + maxSendQBytes, err := bytefmt.ToBytes(config.Server.MaxSendQString) if err != nil { return nil, fmt.Errorf("Could not parse maximum SendQ size (make sure it only contains whole numbers): %s", err.Error()) diff --git a/irc/connection_limits/throttler.go b/irc/connection_limits/throttler.go index aa29d241..505f08a5 100644 --- a/irc/connection_limits/throttler.go +++ b/irc/connection_limits/throttler.go @@ -26,8 +26,45 @@ type ThrottlerConfig struct { // ThrottleDetails holds the connection-throttling details for a subnet/IP. type ThrottleDetails struct { - Start time.Time - ClientCount int + Start time.Time + Count int +} + +// GenericThrottle allows enforcing limits of the form +// "at most X events per time window of duration Y" +type GenericThrottle struct { + ThrottleDetails // variable state: what events have been seen + // these are constant after creation: + Duration time.Duration // window length to consider + Limit int // number of events allowed per window +} + +// Touch checks whether an additional event is allowed: +// it either denies it (by returning false) or allows it (by returning true) +// and records it +func (g *GenericThrottle) Touch() (throttled bool, remainingTime time.Duration) { + return g.touch(time.Now()) +} + +func (g *GenericThrottle) touch(now time.Time) (throttled bool, remainingTime time.Duration) { + if g.Limit == 0 { + return // limit of 0 disables throttling + } + + elapsed := now.Sub(g.Start) + if elapsed > g.Duration { + // reset window, record the operation + g.Start = now + g.Count = 1 + return false, 0 + } else if g.Count >= g.Limit { + // we are throttled + return true, g.Start.Add(g.Duration).Sub(now) + } else { + // we are not throttled, record the operation + g.Count += 1 + return false, 0 + } } // Throttler manages automated client connection throttling. @@ -102,21 +139,21 @@ func (ct *Throttler) AddClient(addr net.IP) error { ct.maskAddr(addr) addrString := addr.String() - details, exists := ct.population[addrString] - if !exists || details.Start.Add(ct.duration).Before(time.Now()) { - details = ThrottleDetails{ - Start: time.Now(), - } + details := ct.population[addrString] // retrieve mutable throttle state from the map + // add in constant state to process the limiting operation + g := GenericThrottle{ + ThrottleDetails: details, + Duration: ct.duration, + Limit: ct.subnetLimit, } + throttled, _ := g.Touch() // actually check the limit + ct.population[addrString] = g.ThrottleDetails // store modified mutable state - if details.ClientCount+1 > ct.subnetLimit { + if throttled { return errTooManyClients + } else { + return nil } - - details.ClientCount++ - ct.population[addrString] = details - - return nil } func (ct *Throttler) BanDuration() time.Duration { diff --git a/irc/connection_limits/throttler_test.go b/irc/connection_limits/throttler_test.go new file mode 100644 index 00000000..b7862375 --- /dev/null +++ b/irc/connection_limits/throttler_test.go @@ -0,0 +1,86 @@ +// Copyright (c) 2018 Shivaram Lingamneni +// released under the MIT license + +package connection_limits + +import ( + "net" + "reflect" + "testing" + "time" +) + +func assertEqual(supplied, expected interface{}, t *testing.T) { + if !reflect.DeepEqual(supplied, expected) { + t.Errorf("expected %v but got %v", expected, supplied) + } +} + +func TestGenericThrottle(t *testing.T) { + minute, _ := time.ParseDuration("1m") + second, _ := time.ParseDuration("1s") + zero, _ := time.ParseDuration("0s") + + throttler := GenericThrottle{ + Duration: minute, + Limit: 2, + } + + now := time.Now() + throttled, remaining := throttler.touch(now) + assertEqual(throttled, false, t) + assertEqual(remaining, zero, t) + + now = now.Add(second) + throttled, remaining = throttler.touch(now) + assertEqual(throttled, false, t) + assertEqual(remaining, zero, t) + + now = now.Add(second) + throttled, remaining = throttler.touch(now) + assertEqual(throttled, true, t) + assertEqual(remaining, 58*second, t) + + now = now.Add(minute) + throttled, remaining = throttler.touch(now) + assertEqual(throttled, false, t) + assertEqual(remaining, zero, t) +} + +func TestGenericThrottleDisabled(t *testing.T) { + minute, _ := time.ParseDuration("1m") + throttler := GenericThrottle{ + Duration: minute, + Limit: 0, + } + + for i := 0; i < 1024; i += 1 { + throttled, _ := throttler.Touch() + if throttled { + t.Error("disabled throttler should not throttle") + } + } +} + +func TestConnectionThrottle(t *testing.T) { + minute, _ := time.ParseDuration("1m") + maxConnections := 3 + config := ThrottlerConfig{ + Enabled: true, + CidrLenIPv4: 32, + CidrLenIPv6: 64, + ConnectionsPerCidr: maxConnections, + Duration: minute, + } + throttler := NewThrottler() + throttler.ApplyConfig(config) + + addr := net.ParseIP("8.8.8.8") + + for i := 0; i < maxConnections; i += 1 { + err := throttler.AddClient(addr) + assertEqual(err, nil, t) + } + err := throttler.AddClient(addr) + assertEqual(err, errTooManyClients, t) +} diff --git a/irc/handlers.go b/irc/handlers.go index 0537382f..8ef2d45b 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -83,9 +83,10 @@ func parseCallback(spec string, config *AccountConfig) (callbackNamespace string // ACC REGISTER [callback_namespace:] [cred_type] : func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool { + nick := client.Nick() // clients can't reg new accounts if they're already logged in if client.LoggedIntoAccount() { - rb.Add(nil, server.name, ERR_REG_UNSPECIFIED_ERROR, client.nick, "*", client.t("You're already logged into an account")) + rb.Add(nil, server.name, ERR_REG_UNSPECIFIED_ERROR, nick, "*", client.t("You're already logged into an account")) return false } @@ -94,12 +95,12 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r casefoldedAccount, err := CasefoldName(account) // probably don't need explicit check for "*" here... but let's do it anyway just to make sure if err != nil || msg.Params[1] == "*" { - rb.Add(nil, server.name, ERR_REG_UNSPECIFIED_ERROR, client.nick, account, client.t("Account name is not valid")) + rb.Add(nil, server.name, ERR_REG_UNSPECIFIED_ERROR, nick, account, client.t("Account name is not valid")) return false } if len(msg.Params) < 4 { - rb.Add(nil, server.name, ERR_NEEDMOREPARAMS, client.nick, msg.Command, client.t("Not enough parameters")) + rb.Add(nil, server.name, ERR_NEEDMOREPARAMS, nick, msg.Command, client.t("Not enough parameters")) return false } @@ -107,7 +108,7 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r callbackNamespace, callbackValue := parseCallback(callbackSpec, server.AccountConfig()) if callbackNamespace == "" { - rb.Add(nil, server.name, ERR_REG_INVALID_CALLBACK, client.nick, account, callbackSpec, client.t("Callback namespace is not supported")) + rb.Add(nil, server.name, ERR_REG_INVALID_CALLBACK, nick, account, callbackSpec, client.t("Callback namespace is not supported")) return false } @@ -131,12 +132,12 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r } } if credentialType == "certfp" && client.certfp == "" { - rb.Add(nil, server.name, ERR_REG_INVALID_CRED_TYPE, client.nick, credentialType, callbackNamespace, client.t("You are not using a TLS certificate")) + rb.Add(nil, server.name, ERR_REG_INVALID_CRED_TYPE, nick, credentialType, callbackNamespace, client.t("You are not using a TLS certificate")) return false } if !credentialValid { - rb.Add(nil, server.name, ERR_REG_INVALID_CRED_TYPE, client.nick, credentialType, callbackNamespace, client.t("Credential type is not supported")) + rb.Add(nil, server.name, ERR_REG_INVALID_CRED_TYPE, nick, credentialType, callbackNamespace, client.t("Credential type is not supported")) return false } @@ -146,6 +147,13 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r } else if credentialType == "passphrase" { passphrase = credentialValue } + + throttled, remainingTime := client.loginThrottle.Touch() + if throttled { + rb.Add(nil, server.name, ERR_REG_UNSPECIFIED_ERROR, nick, fmt.Sprintf(client.t("Please wait at least %v and try again"), remainingTime)) + return false + } + err = server.accounts.Register(client, account, callbackNamespace, callbackValue, passphrase, certfp) if err != nil { msg := "Unknown" @@ -161,7 +169,7 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r if err == errAccountAlreadyRegistered || err == errAccountCreation || err == errCertfpAlreadyExists { msg = err.Error() } - rb.Add(nil, server.name, code, client.nick, "ACC", "REGISTER", client.t(msg)) + rb.Add(nil, server.name, code, nick, "ACC", "REGISTER", client.t(msg)) return false } @@ -175,7 +183,7 @@ func accRegisterHandler(server *Server, client *Client, msg ircmsg.IrcMessage, r } else { messageTemplate := client.t("Account created, pending verification; verification code has been sent to %s:%s") message := fmt.Sprintf(messageTemplate, callbackNamespace, callbackValue) - rb.Add(nil, server.name, RPL_REG_VERIFICATION_REQUIRED, client.nick, casefoldedAccount, message) + rb.Add(nil, server.name, RPL_REG_VERIFICATION_REQUIRED, nick, casefoldedAccount, message) } return false @@ -336,6 +344,8 @@ func authPlainHandler(server *Server, client *Client, mechanism string, value [] var accountKey, authzid string + nick := client.Nick() + if len(splitValue) == 3 { accountKey = string(splitValue[0]) authzid = string(splitValue[1]) @@ -343,11 +353,17 @@ func authPlainHandler(server *Server, client *Client, mechanism string, value [] if accountKey == "" { accountKey = authzid } else if accountKey != authzid { - rb.Add(nil, server.name, ERR_SASLFAIL, client.nick, client.t("SASL authentication failed: authcid and authzid should be the same")) + rb.Add(nil, server.name, ERR_SASLFAIL, nick, client.t("SASL authentication failed: authcid and authzid should be the same")) return false } } else { - rb.Add(nil, server.name, ERR_SASLFAIL, client.nick, client.t("SASL authentication failed: Invalid auth blob")) + rb.Add(nil, server.name, ERR_SASLFAIL, nick, client.t("SASL authentication failed: Invalid auth blob")) + return false + } + + throttled, remainingTime := client.loginThrottle.Touch() + if throttled { + rb.Add(nil, server.name, ERR_SASLFAIL, nick, fmt.Sprintf(client.t("Please wait at least %v and try again"), remainingTime)) return false } @@ -355,7 +371,7 @@ func authPlainHandler(server *Server, client *Client, mechanism string, value [] err := server.accounts.AuthenticateByPassphrase(client, accountKey, password) if err != nil { msg := authErrorToMessage(server, err) - rb.Add(nil, server.name, ERR_SASLFAIL, client.nick, fmt.Sprintf("%s: %s", client.t("SASL authentication failed"), client.t(msg))) + rb.Add(nil, server.name, ERR_SASLFAIL, nick, fmt.Sprintf("%s: %s", client.t("SASL authentication failed"), client.t(msg))) return false } diff --git a/irc/nickserv.go b/irc/nickserv.go index 37973e90..c5a85617 100644 --- a/irc/nickserv.go +++ b/irc/nickserv.go @@ -200,6 +200,15 @@ func nsGroupHandler(server *Server, client *Client, command, params string, rb * } } +func nsLoginThrottleCheck(client *Client, rb *ResponseBuffer) (success bool) { + throttled, remainingTime := client.loginThrottle.Touch() + if throttled { + nsNotice(rb, fmt.Sprintf(client.t("Please wait at least %v and try again"), remainingTime)) + return false + } + return true +} + func nsIdentifyHandler(server *Server, client *Client, command, params string, rb *ResponseBuffer) { loginSuccessful := false @@ -207,6 +216,9 @@ func nsIdentifyHandler(server *Server, client *Client, command, params string, r // try passphrase if username != "" && passphrase != "" { + if !nsLoginThrottleCheck(client, rb) { + return + } err := server.accounts.AuthenticateByPassphrase(client, username, passphrase) loginSuccessful = (err == nil) } @@ -407,10 +419,15 @@ func nsPasswdHandler(server *Server, client *Client, command, params string, rb var newPassword string var errorMessage string + hasPrivs := client.HasRoleCapabs("accreg") + if !hasPrivs && !nsLoginThrottleCheck(client, rb) { + return + } + fields := strings.Fields(params) switch len(fields) { case 2: - if !client.HasRoleCapabs("accreg") { + if !hasPrivs { errorMessage = "Insufficient privileges" } else { target, newPassword = fields[0], fields[1] diff --git a/oragono.yaml b/oragono.yaml index 03ce36d4..0ff89d78 100644 --- a/oragono.yaml +++ b/oragono.yaml @@ -179,6 +179,17 @@ accounts: # is account authentication enabled? authentication-enabled: true + # throttle account login attempts (to prevent either password guessing, or DoS + # attacks on the server aimed at forcing repeated expensive bcrypt computations) + login-throttling: + enabled: true + + # window + duration: 1m + + # number of attempts allowed within the window + max-attempts: 3 + # some clients (notably Pidgin and Hexchat) offer only a single password field, # which makes it impossible to specify a separate server password (for the PASS # command) and SASL password. if this option is set to true, a client that From 9a2117f75d61e4a3b05b3238067d7d8ea25b6ab4 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 1 Jan 2019 23:45:47 -0500 Subject: [PATCH 06/10] preregNick doesn't need synchronization (since it's only accessed from the client's own goroutine) --- irc/getters.go | 12 ------------ irc/handlers.go | 2 +- irc/server.go | 7 +++---- 3 files changed, 4 insertions(+), 17 deletions(-) diff --git a/irc/getters.go b/irc/getters.go index b2ac01c3..c1f65afa 100644 --- a/irc/getters.go +++ b/irc/getters.go @@ -179,18 +179,6 @@ func (client *Client) SetAuthorized(authorized bool) { client.authorized = authorized } -func (client *Client) PreregNick() string { - client.stateMutex.RLock() - defer client.stateMutex.RUnlock() - return client.preregNick -} - -func (client *Client) SetPreregNick(preregNick string) { - client.stateMutex.Lock() - defer client.stateMutex.Unlock() - client.preregNick = preregNick -} - func (client *Client) HasMode(mode modes.Mode) bool { // client.flags has its own synch return client.flags.HasMode(mode) diff --git a/irc/handlers.go b/irc/handlers.go index 8ef2d45b..81af9c45 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -1662,7 +1662,7 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Resp if client.Registered() { performNickChange(server, client, client, msg.Params[0], rb) } else { - client.SetPreregNick(msg.Params[0]) + client.preregNick = msg.Params[0] } return false } diff --git a/irc/server.go b/irc/server.go index 9beecb85..bf810bd6 100644 --- a/irc/server.go +++ b/irc/server.go @@ -386,8 +386,7 @@ func (server *Server) tryRegister(c *Client) { return } - preregNick := c.PreregNick() - if preregNick == "" || !c.HasUsername() || c.capState == caps.NegotiatingState { + if c.preregNick == "" || !c.HasUsername() || c.capState == caps.NegotiatingState { return } @@ -400,10 +399,10 @@ func (server *Server) tryRegister(c *Client) { } rb := NewResponseBuffer(c) - nickAssigned := performNickChange(server, c, c, preregNick, rb) + nickAssigned := performNickChange(server, c, c, c.preregNick, rb) rb.Send(true) if !nickAssigned { - c.SetPreregNick("") + c.preregNick = "" return } From d0ded906d4ac8f91c453f6ee544cc7638365a851 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 1 Jan 2019 21:16:29 -0500 Subject: [PATCH 07/10] fix a fairly bad bug where nicks could get out of sync during nick change, removeInternal(client) was being called even before checking whether the new nick was in use or reserved. Reproduction steps: 1. Log in a client 'alice' 2. Log in a client 'bob' 3. bob issues /nick alice, which fails (correctly) with: :oragono.test 433 bob alice :Nickname is already in use 4. alice issues /msg bob hi, which fails (incorrectly) with: :oragono.test 401 alice bob :No such nick --- irc/client_lookup_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index e54e29b6..6a3a749c 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -129,7 +129,6 @@ func (clients *ClientManager) SetNick(client *Client, newNick string) error { clients.Lock() defer clients.Unlock() - clients.removeInternal(client) currentNewEntry := clients.byNick[newcfnick] // the client may just be changing case if currentNewEntry != nil && currentNewEntry != client { @@ -138,6 +137,7 @@ func (clients *ClientManager) SetNick(client *Client, newNick string) error { if method == NickReservationStrict && reservedAccount != client.Account() { return errNicknameReserved } + clients.removeInternal(client) clients.byNick[newcfnick] = client client.updateNickMask(newNick) return nil From 2ee89b15b398099d4c84d68296f63dd7e051137d Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 2 Jan 2019 10:08:44 -0500 Subject: [PATCH 08/10] per-user settings for nickname enforcement --- irc/accounts.go | 111 +++++++++++++++++++++++++++++++++++---- irc/client_lookup_set.go | 7 +-- irc/config.go | 65 ++++++++++++++++++----- irc/errors.go | 1 + irc/idletimer.go | 13 ++--- irc/nickserv.go | 43 +++++++++++++++ oragono.yaml | 15 ++++-- 7 files changed, 216 insertions(+), 39 deletions(-) diff --git a/irc/accounts.go b/irc/accounts.go index f676cba2..612e0b14 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -30,6 +30,7 @@ const ( keyAccountRegTime = "account.registered.time %s" keyAccountCredentials = "account.credentials %s" keyAccountAdditionalNicks = "account.additionalnicks %s" + keyAccountEnforcement = "account.customenforcement %s" keyAccountVHost = "account.vhost %s" keyCertToAccount = "account.creds.certfp %s" @@ -53,12 +54,14 @@ type AccountManager struct { // track clients logged in to accounts accountToClients map[string][]*Client nickToAccount map[string]string + accountToMethod map[string]NickReservationMethod } func NewAccountManager(server *Server) *AccountManager { am := AccountManager{ accountToClients: make(map[string][]*Client), nickToAccount: make(map[string]string), + accountToMethod: make(map[string]NickReservationMethod), server: server, } @@ -72,7 +75,8 @@ func (am *AccountManager) buildNickToAccountIndex() { return } - result := make(map[string]string) + nickToAccount := make(map[string]string) + accountToMethod := make(map[string]NickReservationMethod) existsPrefix := fmt.Sprintf(keyAccountExists, "") am.serialCacheUpdateMutex.Lock() @@ -83,14 +87,22 @@ func (am *AccountManager) buildNickToAccountIndex() { if !strings.HasPrefix(key, existsPrefix) { return false } - accountName := strings.TrimPrefix(key, existsPrefix) - if _, err := tx.Get(fmt.Sprintf(keyAccountVerified, accountName)); err == nil { - result[accountName] = accountName + + account := strings.TrimPrefix(key, existsPrefix) + if _, err := tx.Get(fmt.Sprintf(keyAccountVerified, account)); err == nil { + nickToAccount[account] = account } - if rawNicks, err := tx.Get(fmt.Sprintf(keyAccountAdditionalNicks, accountName)); err == nil { + if rawNicks, err := tx.Get(fmt.Sprintf(keyAccountAdditionalNicks, account)); err == nil { additionalNicks := unmarshalReservedNicks(rawNicks) for _, nick := range additionalNicks { - result[nick] = accountName + nickToAccount[nick] = account + } + } + + if methodStr, err := tx.Get(fmt.Sprintf(keyAccountEnforcement, account)); err == nil { + method, err := nickReservationFromString(methodStr) + if err == nil { + accountToMethod[account] = method } } return true @@ -102,7 +114,8 @@ func (am *AccountManager) buildNickToAccountIndex() { am.server.logger.Error("internal", "couldn't read reserved nicks", err.Error()) } else { am.Lock() - am.nickToAccount = result + am.nickToAccount = nickToAccount + am.accountToMethod = accountToMethod am.Unlock() } } @@ -156,6 +169,84 @@ func (am *AccountManager) NickToAccount(nick string) string { return am.nickToAccount[cfnick] } +// Given a nick, looks up the account that owns it and the method (none/timeout/strict) +// used to enforce ownership. +func (am *AccountManager) EnforcementStatus(nick string) (account string, method NickReservationMethod) { + cfnick, err := CasefoldName(nick) + if err != nil { + return + } + + config := am.server.Config() + if !config.Accounts.NickReservation.Enabled { + method = NickReservationNone + return + } + + am.RLock() + defer am.RUnlock() + + account = am.nickToAccount[cfnick] + method = am.accountToMethod[account] + // if they don't have a custom setting, or customization is disabled, use the default + if method == NickReservationOptional || !config.Accounts.NickReservation.AllowCustomEnforcement { + method = config.Accounts.NickReservation.Method + } + if method == NickReservationOptional { + // enforcement was explicitly enabled neither in the config or by the user + method = NickReservationNone + } + return +} + +// Looks up the enforcement method stored in the database for an account +// (typically you want EnforcementStatus instead, which respects the config) +func (am *AccountManager) getStoredEnforcementStatus(account string) string { + am.RLock() + defer am.RUnlock() + return nickReservationToString(am.accountToMethod[account]) +} + +// Sets a custom enforcement method for an account and stores it in the database. +func (am *AccountManager) SetEnforcementStatus(account string, method NickReservationMethod) (err error) { + config := am.server.Config() + if !(config.Accounts.NickReservation.Enabled && config.Accounts.NickReservation.AllowCustomEnforcement) { + return errFeatureDisabled + } + + var serialized string + if method == NickReservationOptional { + serialized = "" // normally this is "default", but we're going to delete the key + } else { + serialized = nickReservationToString(method) + } + + key := fmt.Sprintf(keyAccountEnforcement, account) + + am.Lock() + defer am.Unlock() + + currentMethod := am.accountToMethod[account] + if method != currentMethod { + if method == NickReservationOptional { + delete(am.accountToMethod, account) + } else { + am.accountToMethod[account] = method + } + + return am.server.store.Update(func(tx *buntdb.Tx) (err error) { + if serialized != "" { + _, _, err = tx.Set(key, nickReservationToString(method), nil) + } else { + _, err = tx.Delete(key) + } + return + }) + } + + return nil +} + func (am *AccountManager) AccountToClients(account string) (result []*Client) { cfaccount, err := CasefoldName(account) if err != nil { @@ -992,10 +1083,12 @@ func (am *AccountManager) applyVhostToClients(account string, result VHostInfo) func (am *AccountManager) Login(client *Client, account ClientAccount) { changed := client.SetAccountName(account.Name) - if changed { - go client.nickTimer.Touch() + if !changed { + return } + client.nickTimer.Touch() + am.applyVHostInfo(client, account.VHost) casefoldedAccount := client.Account() diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index 6a3a749c..387a357a 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -119,12 +119,7 @@ func (clients *ClientManager) SetNick(client *Client, newNick string) error { return err } - var reservedAccount string - var method NickReservationMethod - if client.server.AccountConfig().NickReservation.Enabled { - reservedAccount = client.server.accounts.NickToAccount(newcfnick) - method = client.server.AccountConfig().NickReservation.Method - } + reservedAccount, method := client.server.accounts.EnforcementStatus(newcfnick) clients.Lock() defer clients.Unlock() diff --git a/irc/config.go b/irc/config.go index 8003eb5b..272b91bc 100644 --- a/irc/config.go +++ b/irc/config.go @@ -8,7 +8,6 @@ package irc import ( "crypto/tls" "encoding/json" - "errors" "fmt" "io/ioutil" "log" @@ -105,10 +104,50 @@ type VHostConfig struct { type NickReservationMethod int const ( - NickReservationWithTimeout NickReservationMethod = iota + // NickReservationOptional is the zero value; it serializes to + // "optional" in the yaml config, and "default" as an arg to `NS ENFORCE`. + // in both cases, it means "defer to the other source of truth", i.e., + // in the config, defer to the user's custom setting, and as a custom setting, + // defer to the default in the config. if both are NickReservationOptional then + // there is no enforcement. + NickReservationOptional NickReservationMethod = iota + NickReservationNone + NickReservationWithTimeout NickReservationStrict ) +func nickReservationToString(method NickReservationMethod) string { + switch method { + case NickReservationOptional: + return "default" + case NickReservationNone: + return "none" + case NickReservationWithTimeout: + return "timeout" + case NickReservationStrict: + return "strict" + default: + return "" + } +} + +func nickReservationFromString(method string) (NickReservationMethod, error) { + switch method { + case "default": + return NickReservationOptional, nil + case "optional": + return NickReservationOptional, nil + case "none": + return NickReservationNone, nil + case "timeout": + return NickReservationWithTimeout, nil + case "strict": + return NickReservationStrict, nil + default: + return NickReservationOptional, fmt.Errorf("invalid nick-reservation.method value: %s", method) + } +} + func (nr *NickReservationMethod) UnmarshalYAML(unmarshal func(interface{}) error) error { var orig, raw string var err error @@ -118,22 +157,20 @@ func (nr *NickReservationMethod) UnmarshalYAML(unmarshal func(interface{}) error if raw, err = Casefold(orig); err != nil { return err } - if raw == "timeout" { - *nr = NickReservationWithTimeout - } else if raw == "strict" { - *nr = NickReservationStrict - } else { - return errors.New(fmt.Sprintf("invalid nick-reservation.method value: %s", orig)) + method, err := nickReservationFromString(raw) + if err == nil { + *nr = method } - return nil + return err } type NickReservationConfig struct { - Enabled bool - AdditionalNickLimit int `yaml:"additional-nick-limit"` - Method NickReservationMethod - RenameTimeout time.Duration `yaml:"rename-timeout"` - RenamePrefix string `yaml:"rename-prefix"` + Enabled bool + AdditionalNickLimit int `yaml:"additional-nick-limit"` + Method NickReservationMethod + AllowCustomEnforcement bool `yaml:"allow-custom-enforcement"` + RenameTimeout time.Duration `yaml:"rename-timeout"` + RenamePrefix string `yaml:"rename-prefix"` } // ChannelRegistrationConfig controls channel registration. diff --git a/irc/errors.go b/irc/errors.go index db969071..a906a391 100644 --- a/irc/errors.go +++ b/irc/errors.go @@ -40,6 +40,7 @@ var ( errSaslFail = errors.New("SASL failed") errResumeTokenAlreadySet = errors.New("Client was already assigned a resume token") errInvalidUsername = errors.New("Invalid username") + errFeatureDisabled = errors.New("That feature is disabled") ) // Socket Errors diff --git a/irc/idletimer.go b/irc/idletimer.go index f4850a80..2aa32a58 100644 --- a/irc/idletimer.go +++ b/irc/idletimer.go @@ -189,14 +189,14 @@ type NickTimer struct { // NewNickTimer sets up a new nick timer (returning nil if timeout enforcement is not enabled) func NewNickTimer(client *Client) *NickTimer { config := client.server.AccountConfig().NickReservation - if !(config.Enabled && config.Method == NickReservationWithTimeout) { + if !(config.Enabled && (config.Method == NickReservationWithTimeout || config.AllowCustomEnforcement)) { return nil } - nt := NickTimer{ + + return &NickTimer{ client: client, timeout: config.RenameTimeout, } - return &nt } // Touch records a nick change and updates the timer as necessary @@ -207,7 +207,8 @@ func (nt *NickTimer) Touch() { nick := nt.client.NickCasefolded() account := nt.client.Account() - accountForNick := nt.client.server.accounts.NickToAccount(nick) + accountForNick, method := nt.client.server.accounts.EnforcementStatus(nick) + enforceTimeout := method == NickReservationWithTimeout var shouldWarn bool @@ -227,11 +228,11 @@ func (nt *NickTimer) Touch() { nt.accountForNick = accountForNick delinquent := accountForNick != "" && accountForNick != account - if nt.timer != nil && (!delinquent || accountChanged) { + if nt.timer != nil && (!enforceTimeout || !delinquent || accountChanged) { nt.timer.Stop() nt.timer = nil } - if delinquent && accountChanged { + if enforceTimeout && delinquent && accountChanged { nt.timer = time.AfterFunc(nt.timeout, nt.processTimeout) shouldWarn = true } diff --git a/irc/nickserv.go b/irc/nickserv.go index c5a85617..936f0819 100644 --- a/irc/nickserv.go +++ b/irc/nickserv.go @@ -25,6 +25,11 @@ func nsGroupEnabled(server *Server) bool { return conf.Accounts.AuthenticationEnabled && conf.Accounts.NickReservation.Enabled } +func nsEnforceEnabled(server *Server) bool { + config := server.Config() + return config.Accounts.NickReservation.Enabled && config.Accounts.NickReservation.AllowCustomEnforcement +} + const nickservHelp = `NickServ lets you register and login to an account. To see in-depth help for a specific NickServ command, try: @@ -44,6 +49,22 @@ DROP de-links the given (or your current) nickname from your user account.`, enabled: servCmdRequiresAccreg, authRequired: true, }, + "enforce": { + handler: nsEnforceHandler, + help: `Syntax: $bENFORCE [method]$b + +ENFORCE lets you specify a custom enforcement mechanism for your registered +nicknames. Your options are: +1. 'none' [no enforcement, overriding the server default] +2. 'timeout' [anyone using the nick must authenticate before a deadline, + or else they will be renamed] +3. 'strict' [you must already be authenticated to use the nick] +4. 'default' [use the server default] +With no arguments, queries your current enforcement status.`, + helpShort: `$bENFORCE$b lets you change how your nicknames are reserved.`, + authRequired: true, + enabled: nsEnforceEnabled, + }, "ghost": { handler: nsGhostHandler, help: `Syntax: $bGHOST $b @@ -464,3 +485,25 @@ func nsPasswdHandler(server *Server, client *Client, command, params string, rb nsNotice(rb, client.t("Password could not be changed due to server error")) } } + +func nsEnforceHandler(server *Server, client *Client, command, params string, rb *ResponseBuffer) { + arg := strings.TrimSpace(params) + + if arg == "" { + status := server.accounts.getStoredEnforcementStatus(client.Account()) + nsNotice(rb, fmt.Sprintf(client.t("Your current nickname enforcement is: %s"), status)) + } else { + method, err := nickReservationFromString(arg) + if err != nil { + nsNotice(rb, client.t("Invalid parameters")) + return + } + err = server.accounts.SetEnforcementStatus(client.Account(), method) + if err == nil { + nsNotice(rb, client.t("Enforcement method set")) + } else { + server.logger.Error("internal", "couldn't store NS ENFORCE data", err.Error()) + nsNotice(rb, client.t("An error occurred")) + } + } +} diff --git a/oragono.yaml b/oragono.yaml index 0ff89d78..3d9966ce 100644 --- a/oragono.yaml +++ b/oragono.yaml @@ -206,12 +206,19 @@ accounts: additional-nick-limit: 2 # method describes how nickname reservation is handled - # timeout: let the user change to the registered nickname, give them X seconds - # to login and then rename them if they haven't done so - # strict: don't let the user change to the registered nickname unless they're - # already logged-in using SASL or NickServ + # already logged-in using SASL or NickServ + # timeout: let the user change to the registered nickname, give them X seconds + # to login and then rename them if they haven't done so + # strict: don't let the user change to the registered nickname unless they're + # already logged-in using SASL or NickServ + # optional: no enforcement by default, but allow users to opt in to + # the enforcement level of their choice method: timeout + # allow users to set their own nickname enforcement status, e.g., + # to opt in to strict enforcement + allow-custom-enforcement: true + # rename-timeout - this is how long users have 'til they're renamed rename-timeout: 30s From f20abf414f6b0a07c520deb07e4df7489ad80e03 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 2 Jan 2019 10:29:42 -0500 Subject: [PATCH 09/10] don't log an error logline for an incorrect SASL password --- irc/accounts.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/irc/accounts.go b/irc/accounts.go index 612e0b14..a8cf8d0e 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -667,7 +667,9 @@ func (am *AccountManager) checkPassphrase(accountName, passphrase string) (accou case 0: err = handleLegacyPasswordV0(am.server, accountName, account.Credentials, passphrase) case 1: - err = passwd.CompareHashAndPassword(account.Credentials.PassphraseHash, []byte(passphrase)) + if passwd.CompareHashAndPassword(account.Credentials.PassphraseHash, []byte(passphrase)) != nil { + err = errAccountInvalidCredentials + } default: err = errAccountInvalidCredentials } From 501bb1e5c5c3b07c9db4e56a528391f760a17187 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 2 Jan 2019 17:52:36 -0500 Subject: [PATCH 10/10] replay JOIN/PART/QUIT/KICK as PRIVMSG from HistServ see https://github.com/ircv3/ircv3-specifications/issues/293 --- irc/channel.go | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/irc/channel.go b/irc/channel.go index b3d69db7..374bb57f 100644 --- a/irc/channel.go +++ b/irc/channel.go @@ -9,6 +9,7 @@ import ( "bytes" "fmt" "strconv" + "strings" "time" "sync" @@ -469,11 +470,7 @@ func (channel *Channel) Join(client *Client, key string, isSajoin bool, rb *Resp replayLimit := channel.server.Config().History.AutoreplayOnJoin if replayLimit > 0 { - // don't replay the client's own events - matcher := func(item history.Item) bool { - return item.Nick != details.nickMask - } - items := channel.history.Match(matcher, replayLimit) + items := channel.history.Latest(replayLimit) channel.replayHistoryItems(rb, items) rb.Flush(true) } @@ -591,10 +588,17 @@ func (channel *Channel) replayHistoryForResume(newClient *Client, after time.Tim rb.Send(true) } +func stripMaskFromNick(nickMask string) (nick string) { + index := strings.Index(nickMask, "!") + if index == -1 { + return + } + return nickMask[0:index] +} + func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.Item) { chname := channel.Name() client := rb.target - extendedJoin := client.capabilities.Has(caps.ExtendedJoin) serverTime := client.capabilities.Has(caps.ServerTime) for _, item := range items { @@ -609,21 +613,27 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I case history.Notice: rb.AddSplitMessageFromClient(item.Msgid, item.Nick, item.AccountName, tags, "NOTICE", chname, item.Message) case history.Join: - if extendedJoin { - // XXX Msgid is the realname in this case - rb.Add(tags, item.Nick, "JOIN", chname, item.AccountName, item.Msgid) + nick := stripMaskFromNick(item.Nick) + var message string + if item.AccountName == "*" { + message = fmt.Sprintf(client.t("%s joined the channel"), nick) } else { - rb.Add(tags, item.Nick, "JOIN", chname) + message = fmt.Sprintf(client.t("%s [account: %s] joined the channel"), nick, item.AccountName) } - case history.Quit: - // XXX: send QUIT as PART to avoid having to correctly deduplicate and synchronize - // QUIT messages across channels - fallthrough + rb.Add(tags, "HistServ", "PRIVMSG", chname, message) case history.Part: - rb.Add(tags, item.Nick, "PART", chname, item.Message.Original) + nick := stripMaskFromNick(item.Nick) + message := fmt.Sprintf(client.t("%s left the channel (%s)"), nick, item.Message.Original) + rb.Add(tags, "HistServ", "PRIVMSG", chname, message) + case history.Quit: + nick := stripMaskFromNick(item.Nick) + message := fmt.Sprintf(client.t("%s quit (%s)"), nick, item.Message.Original) + rb.Add(tags, "HistServ", "PRIVMSG", chname, message) case history.Kick: + nick := stripMaskFromNick(item.Nick) // XXX Msgid is the kick target - rb.Add(tags, item.Nick, "KICK", chname, item.Msgid, item.Message.Original) + message := fmt.Sprintf(client.t("%s kicked %s (%s)"), nick, item.Msgid, item.Message.Original) + rb.Add(tags, "HistServ", "PRIVMSG", chname, message) } } }