From 9ed789f67c55d717a43f6b1e4cab9a19975784a5 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 6 Oct 2020 18:04:29 -0400 Subject: [PATCH 1/2] fix #1075 --- conventional.yaml | 45 ++++++------- default.yaml | 45 ++++++------- gencapdefs.py | 6 ++ irc/accounts.go | 22 +++++-- irc/caps/defs.go | 7 +- irc/client.go | 3 +- irc/client_lookup_set.go | 8 ++- irc/commands.go | 10 +++ irc/config.go | 73 ++++++++++++++------- irc/email/email.go | 1 + irc/errors.go | 1 + irc/handlers.go | 138 +++++++++++++++++++++++++++++++++++---- irc/help.go | 10 +++ irc/nickname.go | 2 +- irc/nickserv.go | 22 ++----- 15 files changed, 285 insertions(+), 108 deletions(-) diff --git a/conventional.yaml b/conventional.yaml index 580a2a15..46d47844 100644 --- a/conventional.yaml +++ b/conventional.yaml @@ -313,6 +313,9 @@ accounts: # the `accreg` capability can still create accounts with `/NICKSERV SAREGISTER` enabled: true + # can users use the REGISTER command to register before fully connecting? + allow-before-connect: true + # global throttle on new account creation throttling: enabled: true @@ -327,28 +330,26 @@ accounts: # length of time a user has to verify their account before it can be re-registered verify-timeout: "32h" - # callbacks to allow - enabled-callbacks: - - none # no verification needed, will instantly register successfully - - # example configuration for sending verification emails - # callbacks: - # mailto: - # sender: "admin@my.network" - # require-tls: true - # helo-domain: "my.network" # defaults to server name if unset - # dkim: - # domain: "my.network" - # selector: "20200229" - # key-file: "dkim.pem" - # # to use an MTA/smarthost instead of sending email directly: - # # mta: - # # server: localhost - # # port: 25 - # # username: "admin" - # # password: "hunter2" - # blacklist-regexes: - # # - ".*@mailinator.com" + # options for email verification of account registrations + email-verification: + enabled: false + sender: "admin@my.network" + require-tls: true + helo-domain: "my.network" # defaults to server name if unset + # options to enable DKIM signing of outgoing emails (recommended, but + # requires creating a DNS entry for the public key): + # dkim: + # domain: "my.network" + # selector: "20200229" + # key-file: "dkim.pem" + # to use an MTA/smarthost instead of sending email directly: + # mta: + # server: localhost + # port: 25 + # username: "admin" + # password: "hunter2" + blacklist-regexes: + # - ".*@mailinator.com" # throttle account login attempts (to prevent either password guessing, or DoS # attacks on the server aimed at forcing repeated expensive bcrypt computations) diff --git a/default.yaml b/default.yaml index 6118f86d..25140484 100644 --- a/default.yaml +++ b/default.yaml @@ -341,6 +341,9 @@ accounts: # the `accreg` capability can still create accounts with `/NICKSERV SAREGISTER` enabled: true + # can users use the REGISTER command to register before fully connecting? + allow-before-connect: true + # global throttle on new account creation throttling: enabled: true @@ -355,28 +358,26 @@ accounts: # length of time a user has to verify their account before it can be re-registered verify-timeout: "32h" - # callbacks to allow - enabled-callbacks: - - none # no verification needed, will instantly register successfully - - # example configuration for sending verification emails - # callbacks: - # mailto: - # sender: "admin@my.network" - # require-tls: true - # helo-domain: "my.network" # defaults to server name if unset - # dkim: - # domain: "my.network" - # selector: "20200229" - # key-file: "dkim.pem" - # # to use an MTA/smarthost instead of sending email directly: - # # mta: - # # server: localhost - # # port: 25 - # # username: "admin" - # # password: "hunter2" - # blacklist-regexes: - # # - ".*@mailinator.com" + # options for email verification of account registrations + email-verification: + enabled: false + sender: "admin@my.network" + require-tls: true + helo-domain: "my.network" # defaults to server name if unset + # options to enable DKIM signing of outgoing emails (recommended, but + # requires creating a DNS entry for the public key): + # dkim: + # domain: "my.network" + # selector: "20200229" + # key-file: "dkim.pem" + # to use an MTA/smarthost instead of sending email directly: + # mta: + # server: localhost + # port: 25 + # username: "admin" + # password: "hunter2" + blacklist-regexes: + # - ".*@mailinator.com" # throttle account login attempts (to prevent either password guessing, or DoS # attacks on the server aimed at forcing repeated expensive bcrypt computations) diff --git a/gencapdefs.py b/gencapdefs.py index efd92ccf..26011fa1 100644 --- a/gencapdefs.py +++ b/gencapdefs.py @@ -177,6 +177,12 @@ CAPDEFS = [ url="https://github.com/ircv3/ircv3-specifications/pull/393", standard="proposed IRCv3", ), + CapDef( + identifier="Register", + name="draft/register", + url="https://gist.github.com/edk0/bf3b50fc219fd1bed1aa15d98bfb6495", + standard="proposed IRCv3", + ), ] def validate_defs(): diff --git a/irc/accounts.go b/irc/accounts.go index 2e7345fc..6a2da92b 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -410,10 +410,22 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames // n.b. if ForceGuestFormat, then there's no concern, because you can't // register a guest nickname anyway, and the actual registration system // will prevent any double-register - if client != nil && config.Accounts.NickReservation.Enabled && - !config.Accounts.NickReservation.ForceGuestFormat && - client.NickCasefolded() != casefoldedAccount { - return errAccountMustHoldNick + if client != nil { + if client.registered { + if config.Accounts.NickReservation.Enabled && + !config.Accounts.NickReservation.ForceGuestFormat && + client.NickCasefolded() != casefoldedAccount { + return errAccountMustHoldNick + } + } else { + // XXX this is a REGISTER command from a client who hasn't completed the + // initial handshake ("connection registration"). Do SetNick with dryRun=true, + // testing whether they are able to claim the nick + _, nickAcquireError, _ := am.server.clients.SetNick(client, nil, account, true) + if !(nickAcquireError == nil || nickAcquireError == errNoop) { + return errAccountMustHoldNick + } + } } // can't register a guest nickname @@ -763,7 +775,7 @@ func (am *AccountManager) dispatchCallback(client *Client, account string, callb } func (am *AccountManager) dispatchMailtoCallback(client *Client, account string, callbackValue string) (code string, err error) { - config := am.server.Config().Accounts.Registration.Callbacks.Mailto + config := am.server.Config().Accounts.Registration.EmailVerification code = utils.GenerateSecretToken() subject := config.VerifyMessageSubject diff --git a/irc/caps/defs.go b/irc/caps/defs.go index 70128b56..bc165ce7 100644 --- a/irc/caps/defs.go +++ b/irc/caps/defs.go @@ -7,7 +7,7 @@ package caps const ( // number of recognized capabilities: - numCapabs = 27 + numCapabs = 28 // length of the uint64 array that represents the bitset: bitsetLen = 1 ) @@ -57,6 +57,10 @@ const ( // https://github.com/ircv3/ircv3-specifications/pull/398 Multiline Capability = iota + // Register is the proposed IRCv3 capability named "draft/register": + // https://gist.github.com/edk0/bf3b50fc219fd1bed1aa15d98bfb6495 + Register Capability = iota + // Relaymsg is the proposed IRCv3 capability named "draft/relaymsg": // https://github.com/ircv3/ircv3-specifications/pull/417 Relaymsg Capability = iota @@ -136,6 +140,7 @@ var ( "draft/event-playback", "draft/languages", "draft/multiline", + "draft/register", "draft/relaymsg", "draft/resume-0.5", "echo-message", diff --git a/irc/client.go b/irc/client.go index 603bb11b..788c4ae2 100644 --- a/irc/client.go +++ b/irc/client.go @@ -106,6 +106,7 @@ type Client struct { requireSASLMessage string requireSASL bool registered bool + registerCmdSent bool // already sent the draft/register command, can't send it again registrationTimer *time.Timer resumeID string server *Server @@ -441,7 +442,7 @@ func (server *Server) AddAlwaysOnClient(account ClientAccount, chnames []string, client.resizeHistory(config) - _, err, _ := server.clients.SetNick(client, nil, account.Name) + _, err, _ := server.clients.SetNick(client, nil, account.Name, false) if err != nil { server.logger.Error("internal", "could not establish always-on client", account.Name, err.Error()) return diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index de10f6d3..d7bdaed8 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -104,7 +104,9 @@ func (clients *ClientManager) Resume(oldClient *Client, session *Session) (err e } // SetNick sets a client's nickname, validating it against nicknames in use -func (clients *ClientManager) SetNick(client *Client, session *Session, newNick string) (setNick string, err error, returnedFromAway bool) { +// XXX: dryRun validates a client's ability to claim a nick, without +// actually claiming it +func (clients *ClientManager) SetNick(client *Client, session *Session, newNick string, dryRun bool) (setNick string, err error, returnedFromAway bool) { config := client.server.Config() var newCfNick, newSkeleton string @@ -236,6 +238,10 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick return "", errNicknameInUse, false } + if dryRun { + return "", nil, false + } + formercfnick, formerskeleton := client.uniqueIdentifiers() if changeSuccess := client.SetNick(newNick, newCfNick, newSkeleton); !changeSuccess { return "", errClientDestroyed, false diff --git a/irc/commands.go b/irc/commands.go index 1e58726d..c1ec6f41 100644 --- a/irc/commands.go +++ b/irc/commands.go @@ -256,6 +256,11 @@ func init() { handler: relaymsgHandler, minParams: 3, }, + "REGISTER": { + handler: registerHandler, + minParams: 2, + usablePreReg: true, + }, "RENAME": { handler: renameHandler, minParams: 2, @@ -336,6 +341,11 @@ func init() { "USERS": { handler: usersHandler, }, + "VERIFY": { + handler: verifyHandler, + usablePreReg: true, + minParams: 2, + }, "VERSION": { handler: versionHandler, minParams: 0, diff --git a/irc/config.go b/irc/config.go index f973f8c1..5bb86853 100644 --- a/irc/config.go +++ b/irc/config.go @@ -16,7 +16,6 @@ import ( "os" "path/filepath" "regexp" - "sort" "strconv" "strings" "time" @@ -298,15 +297,18 @@ type AuthScriptConfig struct { // AccountRegistrationConfig controls account registration. type AccountRegistrationConfig struct { - Enabled bool - Throttling ThrottleConfig - EnabledCallbacks []string `yaml:"enabled-callbacks"` - EnabledCredentialTypes []string `yaml:"-"` - VerifyTimeout custime.Duration `yaml:"verify-timeout"` - Callbacks struct { + Enabled bool + AllowBeforeConnect bool `yaml:"allow-before-connect"` + Throttling ThrottleConfig + // new-style (v2.4 email verification config): + EmailVerification email.MailtoConfig `yaml:"email-verification"` + // old-style email verification config, with "callbacks": + LegacyEnabledCallbacks []string `yaml:"enabled-callbacks"` + LegacyCallbacks struct { Mailto email.MailtoConfig - } - BcryptCost uint `yaml:"bcrypt-cost"` + } `yaml:"callbacks"` + VerifyTimeout custime.Duration `yaml:"verify-timeout"` + BcryptCost uint `yaml:"bcrypt-cost"` } type VHostConfig struct { @@ -1049,24 +1051,29 @@ func LoadConfig(filename string) (config *Config, err error) { } config.Logging = newLogConfigs - // hardcode this for now - config.Accounts.Registration.EnabledCredentialTypes = []string{"passphrase", "certfp"} - mailtoEnabled := false - for i, name := range config.Accounts.Registration.EnabledCallbacks { - if name == "none" { - // we store "none" as "*" internally - config.Accounts.Registration.EnabledCallbacks[i] = "*" - } else if name == "mailto" { - mailtoEnabled = true - } - } - sort.Strings(config.Accounts.Registration.EnabledCallbacks) - - if mailtoEnabled { - err := config.Accounts.Registration.Callbacks.Mailto.Postprocess(config.Server.Name) + if config.Accounts.Registration.EmailVerification.Enabled { + err := config.Accounts.Registration.EmailVerification.Postprocess(config.Server.Name) if err != nil { return nil, err } + } else { + // TODO: this processes the legacy "callback" config, clean this up in 2.5 or later + // TODO: also clean up the legacy "inline" MTA config format (from ee05a4324dfde) + mailtoEnabled := false + for _, name := range config.Accounts.Registration.LegacyEnabledCallbacks { + if name == "mailto" { + mailtoEnabled = true + break + } + } + if mailtoEnabled { + config.Accounts.Registration.EmailVerification = config.Accounts.Registration.LegacyCallbacks.Mailto + config.Accounts.Registration.EmailVerification.Enabled = true + err := config.Accounts.Registration.EmailVerification.Postprocess(config.Server.Name) + if err != nil { + return nil, err + } + } } config.Accounts.defaultUserModes = ParseDefaultUserModes(config.Accounts.DefaultUserModes) @@ -1104,6 +1111,24 @@ func LoadConfig(filename string) (config *Config, err error) { config.Server.supportedCaps.Disable(caps.SASL) } + if !config.Accounts.Registration.Enabled { + config.Server.supportedCaps.Disable(caps.Register) + } else { + var registerValues []string + if config.Accounts.Registration.AllowBeforeConnect { + registerValues = append(registerValues, "before-connect") + } + if config.Accounts.Registration.EmailVerification.Enabled { + registerValues = append(registerValues, "email-required") + } + if config.Accounts.RequireSasl.Enabled { + registerValues = append(registerValues, "account-required") + } + if len(registerValues) != 0 { + config.Server.capValues[caps.Register] = strings.Join(registerValues, ",") + } + } + 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/email/email.go b/irc/email/email.go index 212f8fab..4b7554b0 100644 --- a/irc/email/email.go +++ b/irc/email/email.go @@ -31,6 +31,7 @@ type MailtoConfig struct { // so server, port, etc. appear directly at top level // XXX: see https://github.com/go-yaml/yaml/issues/63 MTAConfig `yaml:",inline"` + Enabled bool Sender string HeloDomain string `yaml:"helo-domain"` RequireTLS bool `yaml:"require-tls"` diff --git a/irc/errors.go b/irc/errors.go index 5e3efc74..aa64e760 100644 --- a/irc/errors.go +++ b/irc/errors.go @@ -71,6 +71,7 @@ var ( errWrongChannelKey = errors.New("Cannot join password-protected channel without the password") errInviteOnly = errors.New("Cannot join invite-only channel without an invite") errRegisteredOnly = errors.New("Cannot join registered-only channel without an account") + errValidEmailRequired = errors.New("A valid email address is required for account registration") ) // String Errors diff --git a/irc/handlers.go b/irc/handlers.go index 431a6c7d..940ff2a6 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -33,28 +33,30 @@ import ( ) // helper function to parse ACC callbacks, e.g., mailto:person@example.com, tel:16505551234 -func parseCallback(spec string, config AccountConfig) (callbackNamespace string, callbackValue string) { - callback := strings.ToLower(spec) - if callback == "*" { +func parseCallback(spec string, config *Config) (callbackNamespace string, callbackValue string, err error) { + // XXX if we don't require verification, ignore any callback that was passed here + // (to avoid confusion in the case where the ircd has no mail server configured) + if !config.Accounts.Registration.EmailVerification.Enabled { callbackNamespace = "*" - } else if strings.Contains(callback, ":") { - callbackValues := strings.SplitN(callback, ":", 2) - callbackNamespace, callbackValue = callbackValues[0], callbackValues[1] + return + } + callback := strings.ToLower(spec) + if colonIndex := strings.IndexByte(callback, ':'); colonIndex != -1 { + callbackNamespace, callbackValue = callback[:colonIndex], callback[colonIndex+1:] } else { // "If a callback namespace is not ... provided, the IRC server MUST use mailto"" callbackNamespace = "mailto" callbackValue = callback } - // ensure the callback namespace is valid - // need to search callback list, maybe look at using a map later? - for _, name := range config.Registration.EnabledCallbacks { - if callbackNamespace == name { - return + if config.Accounts.Registration.EmailVerification.Enabled { + if callbackNamespace != "mailto" { + err = errValidEmailRequired + } else if strings.IndexByte(callbackValue, '@') < 1 { + err = errValidEmailRequired } } - // error value - callbackNamespace = "" + return } @@ -2398,6 +2400,116 @@ func quitHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Resp return true } +// REGISTER < email | * > +func registerHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) (exiting bool) { + config := server.Config() + if !config.Accounts.Registration.Enabled { + rb.Add(nil, server.name, "FAIL", "REGISTER", "DISALLOWED", client.t("Account registration is disabled")) + return + } + if !client.registered && !config.Accounts.Registration.AllowBeforeConnect { + rb.Add(nil, server.name, "FAIL", "REGISTER", "DISALLOWED", client.t("You must complete the connection before registering your account")) + return + } + if client.registerCmdSent || client.Account() != "" { + rb.Add(nil, server.name, "FAIL", "REGISTER", "ALREADY_REGISTERED", client.t("You have already registered or attempted to register")) + return + } + + accountName := client.Nick() + if accountName == "*" { + accountName = client.preregNick + } + if accountName == "" || accountName == "*" { + rb.Add(nil, server.name, "FAIL", "REGISTER", "INVALID_USERNAME", client.t("Username invalid or not given")) + return + } + + callbackNamespace, callbackValue, err := parseCallback(msg.Params[0], config) + if err != nil { + rb.Add(nil, server.name, "FAIL", "REGISTER", "INVALID_EMAIL", client.t("A valid e-mail address is required")) + return + } + + err = server.accounts.Register(client, accountName, callbackNamespace, callbackValue, msg.Params[1], rb.session.certfp) + switch err { + case nil: + if callbackNamespace == "*" { + err := server.accounts.Verify(client, accountName, "") + if err == nil { + if client.registered { + if !fixupNickEqualsAccount(client, rb, config) { + err = errNickAccountMismatch + } + } + if err == nil { + rb.Add(nil, server.name, "REGISTER", "SUCCESS", accountName, client.t("Account successfully registered")) + sendSuccessfulRegResponse(client, rb, true) + } + } + if err != nil { + server.logger.Error("internal", "accounts", "failed autoverification", accountName, err.Error()) + rb.Add(nil, server.name, "FAIL", "REGISTER", "UNKNOWN_ERROR", client.t("An error occurred")) + } + } else { + rb.Add(nil, server.name, "REGISTER", "VERIFICATION_REQUIRED", accountName, fmt.Sprintf(client.t("Account created, pending verification; verification code has been sent to %s"), callbackValue)) + client.registerCmdSent = true + } + case errAccountAlreadyRegistered, errAccountAlreadyUnregistered, errAccountMustHoldNick: + rb.Add(nil, server.name, "FAIL", "REGISTER", "USERNAME_EXISTS", client.t("Username is already registered or otherwise unavailable")) + case errAccountBadPassphrase: + rb.Add(nil, server.name, "FAIL", "REGISTER", "INVALID_PASSWORD", client.t("Password was invalid")) + case errCallbackFailed: + // TODO detect this in NS REGISTER as well + rb.Add(nil, server.name, "FAIL", "REGISTER", "UNACCEPTABLE_EMAIL", client.t("Could not dispatch verification e-mail")) + default: + rb.Add(nil, server.name, "FAIL", "REGISTER", "UNKNOWN_ERROR", client.t("Could not register")) + } + return +} + +// VERIFY +func verifyHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) (exiting bool) { + config := server.Config() + if !config.Accounts.Registration.Enabled { + rb.Add(nil, server.name, "FAIL", "VERIFY", "DISALLOWED", client.t("Account registration is disabled")) + return + } + if !client.registered && !config.Accounts.Registration.AllowBeforeConnect { + rb.Add(nil, server.name, "FAIL", "VERIFY", "DISALLOWED", client.t("You must complete the connection before verifying your account")) + return + } + if client.Account() != "" { + rb.Add(nil, server.name, "FAIL", "VERIFY", "ALREADY_REGISTERED", client.t("You have already registered or attempted to register")) + return + } + + accountName, verificationCode := msg.Params[0], msg.Params[1] + err := server.accounts.Verify(client, accountName, verificationCode) + if err == nil && client.registered { + if !fixupNickEqualsAccount(client, rb, config) { + err = errNickAccountMismatch + } + } + switch err { + case nil: + rb.Add(nil, server.name, "VERIFY", "SUCCESS", accountName, client.t("Account successfully registered")) + sendSuccessfulRegResponse(client, rb, true) + case errAccountVerificationInvalidCode: + rb.Add(nil, server.name, "FAIL", "VERIFY", "INVALID_CODE", client.t("Invalid verification code")) + default: + rb.Add(nil, server.name, "FAIL", "VERIFY", "UNKNOWN_ERROR", client.t("Failed to verify account")) + } + + if err != nil && !client.registered { + // XXX pre-registration clients are exempt from fakelag; + // slow the client down to stop them spamming verify attempts + time.Sleep(time.Second) + } + + return +} + // REHASH func rehashHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool { nick := client.Nick() diff --git a/irc/help.go b/irc/help.go index 8e9801a3..9aa63f67 100644 --- a/irc/help.go +++ b/irc/help.go @@ -485,6 +485,11 @@ specs for more info: http://ircv3.net/specs/core/message-tags-3.3.html`, text: `QUIT [reason] Indicates that you're leaving the server, and shows everyone the given reason.`, + }, + "register": { + text: `REGISTER + +Registers an account in accordance with the draft/register capability.`, }, "rehash": { oper: true, @@ -544,6 +549,11 @@ The USERS command is not implemented.`, text: `USERHOST { } Shows information about the given users. Takes up to 10 nicknames.`, + }, + "verify": { + text: `VERIFY + +Verifies an account in accordance with the draft/register capability.`, }, "version": { text: `VERSION [server] diff --git a/irc/nickname.go b/irc/nickname.go index 26847ef0..801354b3 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -33,7 +33,7 @@ func performNickChange(server *Server, client *Client, target *Client, session * origNickMask := details.nickMask isSanick := client != target - assignedNickname, err, back := client.server.clients.SetNick(target, session, nickname) + assignedNickname, err, back := client.server.clients.SetNick(target, session, nickname, false) if err == errNicknameInUse { if !isSanick { rb.Add(nil, server.name, ERR_NICKNAMEINUSE, details.nick, utils.SafeErrorParam(nickname), client.t("Nickname is already in use")) diff --git a/irc/nickserv.go b/irc/nickserv.go index d1e48116..d3334f2b 100644 --- a/irc/nickserv.go +++ b/irc/nickserv.go @@ -853,24 +853,10 @@ func nsRegisterHandler(server *Server, client *Client, command string, params [] account = matches[1] } - var callbackNamespace, callbackValue string - noneCallbackAllowed := false - for _, callback := range config.Accounts.Registration.EnabledCallbacks { - if callback == "*" { - noneCallbackAllowed = true - } - } - // XXX if ACC REGISTER allows registration with the `none` callback, then ignore - // any callback that was passed here (to avoid confusion in the case where the ircd - // has no mail server configured). otherwise, register using the provided callback: - if noneCallbackAllowed { - callbackNamespace = "*" - } else { - callbackNamespace, callbackValue = parseCallback(email, config.Accounts) - if callbackNamespace == "" || callbackValue == "" { - nsNotice(rb, client.t("Registration requires a valid e-mail address")) - return - } + callbackNamespace, callbackValue, validationErr := parseCallback(email, config) + if validationErr != nil { + nsNotice(rb, client.t("Registration requires a valid e-mail address")) + return } err := server.accounts.Register(client, account, callbackNamespace, callbackValue, passphrase, rb.session.certfp) From 754fb79cddf3972a464be9ef64da7559cd02be24 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Wed, 7 Oct 2020 08:54:46 -0400 Subject: [PATCH 2/2] review fixes --- irc/accounts.go | 33 +++++++++++---------------------- irc/client_lookup_set.go | 7 ++++--- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/irc/accounts.go b/irc/accounts.go index 6a2da92b..94e51513 100644 --- a/irc/accounts.go +++ b/irc/accounts.go @@ -403,28 +403,17 @@ func (am *AccountManager) Register(client *Client, account string, callbackNames return errLimitExceeded } - // if nick reservation is enabled, you can only register your current nickname - // as an account; this prevents "land-grab" situations where someone else - // registers your nick out from under you and then NS GHOSTs you - // n.b. client is nil during a SAREGISTER - // n.b. if ForceGuestFormat, then there's no concern, because you can't - // register a guest nickname anyway, and the actual registration system - // will prevent any double-register - if client != nil { - if client.registered { - if config.Accounts.NickReservation.Enabled && - !config.Accounts.NickReservation.ForceGuestFormat && - client.NickCasefolded() != casefoldedAccount { - return errAccountMustHoldNick - } - } else { - // XXX this is a REGISTER command from a client who hasn't completed the - // initial handshake ("connection registration"). Do SetNick with dryRun=true, - // testing whether they are able to claim the nick - _, nickAcquireError, _ := am.server.clients.SetNick(client, nil, account, true) - if !(nickAcquireError == nil || nickAcquireError == errNoop) { - return errAccountMustHoldNick - } + // if nick reservation is enabled, don't let people reserve nicknames + // that they would not be eligible to take, e.g., + // 1. a nickname that someone else is currently holding + // 2. a nickname confusable with an existing reserved nickname + // this has a lot of weird edge cases because of force-guest-format + // and the possibility of registering a nickname on an "unregistered connection" + // (i.e., pre-handshake). + if client != nil && config.Accounts.NickReservation.Enabled { + _, nickAcquireError, _ := am.server.clients.SetNick(client, nil, account, true) + if !(nickAcquireError == nil || nickAcquireError == errNoop) { + return errAccountMustHoldNick } } diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index d7bdaed8..519a3bb5 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -142,7 +142,7 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick return "", errNickMissing, false } - if account == "" && config.Accounts.NickReservation.ForceGuestFormat { + if account == "" && config.Accounts.NickReservation.ForceGuestFormat && !dryRun { newCfNick, err = CasefoldName(newNick) if err != nil { return "", errNicknameInvalid, false @@ -199,9 +199,10 @@ func (clients *ClientManager) SetNick(client *Client, session *Session, newNick currentClient := clients.byNick[newCfNick] // the client may just be changing case - if currentClient != nil && currentClient != client && session != nil { + if currentClient != nil && currentClient != client { // these conditions forbid reattaching to an existing session: - if registered || !bouncerAllowed || account == "" || account != currentClient.Account() { + if registered || !bouncerAllowed || account == "" || account != currentClient.Account() || + dryRun || session == nil { return "", errNicknameInUse, false } // check TLS modes