From 5738d9e7b8dcd488ccf9db1f955921d8cc0d8ae7 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Tue, 5 Mar 2024 10:40:40 +0100 Subject: [PATCH] when auth fails due to missing derived secrets, don't hold it against connection smtp & imap can only indicate which mechanisms the server software supports. individual accounts may not have derived secrets for all those mechanisms. imap & smtp cannot indicate that a client should try another (specific) mechanism. but at least we shouldn't slow the connection down due to failed auth attempts in that case. heard from ben that this is a common source for trouble when setting up email accounts. --- imapserver/server.go | 26 ++++++++++++++++++++------ smtpserver/server.go | 25 +++++++++++++++++++------ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/imapserver/server.go b/imapserver/server.go index ee1c23a..8e8a534 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -1495,8 +1495,17 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { if c.authFailed > 3 && authFailDelay > 0 { mox.Sleep(mox.Context, time.Duration(c.authFailed-3)*authFailDelay) } + + // If authentication fails due to missing derived secrets, we don't hold it against + // the connection. There is no way to indicate server support for an authentication + // mechanism, but that a mechanism won't work for an account. + var missingDerivedSecrets bool + c.authFailed++ // Compensated on success. defer func() { + if missingDerivedSecrets { + c.authFailed-- + } // On the 3rd failed authentication, start responding slowly. Successful auth will // cause fast responses again. if c.authFailed >= 3 { @@ -1508,10 +1517,9 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { authResult := "error" defer func() { metrics.AuthenticationInc("imap", authVariant, authResult) - switch authResult { - case "ok": + if authResult == "ok" { mox.LimiterFailedAuth.Reset(c.remoteIP, time.Now()) - default: + } else if !missingDerivedSecrets { mox.LimiterFailedAuth.Add(c.remoteIP, time.Now(), 1) } }() @@ -1648,6 +1656,7 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { if ipadhash == nil || opadhash == nil { c.log.Info("cram-md5 auth attempt without derived secrets set, save password again to store secrets", slog.String("username", addr)) c.log.Info("failed authentication attempt", slog.String("username", addr), slog.Any("remote", c.remoteIP)) + missingDerivedSecrets = true xusercodeErrorf("AUTHENTICATIONFAILED", "bad credentials") } @@ -1716,6 +1725,11 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { acc.WithRLock(func() { err := acc.DB.Read(context.TODO(), func(tx *bstore.Tx) error { password, err := bstore.QueryTx[store.Password](tx).Get() + if err == bstore.ErrAbsent { + c.log.Info("failed authentication attempt", slog.String("username", ss.Authentication), slog.Any("remote", c.remoteIP)) + xusercodeErrorf("AUTHENTICATIONFAILED", "bad credentials") + } + xcheckf(err, "fetching credentials") switch authVariant { case "scram-sha-1", "scram-sha-1-plus": xscram = password.SCRAMSHA1 @@ -1724,12 +1738,12 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { default: xserverErrorf("missing case for scram credentials") } - if err == bstore.ErrAbsent || err == nil && (len(xscram.Salt) == 0 || xscram.Iterations == 0 || len(xscram.SaltedPassword) == 0) { + if len(xscram.Salt) == 0 || xscram.Iterations == 0 || len(xscram.SaltedPassword) == 0 { + missingDerivedSecrets = true c.log.Info("scram auth attempt without derived secrets set, save password again to store secrets", slog.String("address", ss.Authentication)) xuserErrorf("scram not possible") } - xcheckf(err, "fetching credentials") - return err + return nil }) xcheckf(err, "read tx") }) diff --git a/smtpserver/server.go b/smtpserver/server.go index a3ec423..5314908 100644 --- a/smtpserver/server.go +++ b/smtpserver/server.go @@ -959,6 +959,11 @@ func (c *conn) cmdAuth(p *parser) { // todo future: we may want to normalize usernames and passwords, see stringprep in ../rfc/4013:38 and possibly newer mechanisms (though they are opt-in and that may not have happened yet). + // If authentication fails due to missing derived secrets, we don't hold it against + // the connection. There is no way to indicate server support for an authentication + // mechanism, but that a mechanism won't work for an account. + var missingDerivedSecrets bool + // For many failed auth attempts, slow down verification attempts. // Dropping the connection could also work, but more so when we have a connection rate limiter. // ../rfc/4954:770 @@ -968,6 +973,9 @@ func (c *conn) cmdAuth(p *parser) { } c.authFailed++ // Compensated on success. defer func() { + if missingDerivedSecrets { + c.authFailed-- + } // On the 3rd failed authentication, start responding slowly. Successful auth will // cause fast responses again. if c.authFailed >= 3 { @@ -979,10 +987,9 @@ func (c *conn) cmdAuth(p *parser) { authResult := "error" defer func() { metrics.AuthenticationInc("submission", authVariant, authResult) - switch authResult { - case "ok": + if authResult == "ok" { mox.LimiterFailedAuth.Reset(c.remoteIP, time.Now()) - default: + } else if !missingDerivedSecrets { mox.LimiterFailedAuth.Add(c.remoteIP, time.Now(), 1) } }() @@ -1179,6 +1186,7 @@ func (c *conn) cmdAuth(p *parser) { xcheckf(err, "tx read") }) if ipadhash == nil || opadhash == nil { + missingDerivedSecrets = true c.log.Info("cram-md5 auth attempt without derived secrets set, save password again to store secrets", slog.String("username", addr)) c.log.Info("failed authentication attempt", slog.String("username", addr), slog.Any("remote", c.remoteIP)) xsmtpUserErrorf(smtp.C535AuthBadCreds, smtp.SePol7AuthBadCreds8, "bad user/pass") @@ -1254,6 +1262,11 @@ func (c *conn) cmdAuth(p *parser) { acc.WithRLock(func() { err := acc.DB.Read(context.TODO(), func(tx *bstore.Tx) error { password, err := bstore.QueryTx[store.Password](tx).Get() + if err == bstore.ErrAbsent { + c.log.Info("failed authentication attempt", slog.String("username", ss.Authentication), slog.Any("remote", c.remoteIP)) + xsmtpUserErrorf(smtp.C535AuthBadCreds, smtp.SePol7AuthBadCreds8, "bad user/pass") + } + xcheckf(err, "fetching credentials") switch authVariant { case "scram-sha-1", "scram-sha-1-plus": xscram = password.SCRAMSHA1 @@ -1262,13 +1275,13 @@ func (c *conn) cmdAuth(p *parser) { default: xsmtpServerErrorf(codes{smtp.C554TransactionFailed, smtp.SeSys3Other0}, "missing scram auth credentials case") } - if err == bstore.ErrAbsent || err == nil && (len(xscram.Salt) == 0 || xscram.Iterations == 0 || len(xscram.SaltedPassword) == 0) { + if len(xscram.Salt) == 0 || xscram.Iterations == 0 || len(xscram.SaltedPassword) == 0 { + missingDerivedSecrets = true c.log.Info("scram auth attempt without derived secrets set, save password again to store secrets", slog.String("address", ss.Authentication)) c.log.Info("failed authentication attempt", slog.String("username", ss.Authentication), slog.Any("remote", c.remoteIP)) xsmtpUserErrorf(smtp.C454TempAuthFail, smtp.SeSys3Other0, "scram not possible") } - xcheckf(err, "fetching credentials") - return err + return nil }) xcheckf(err, "read tx") })