Exit if there's an error reading users/aliases files on startup

Today, when starting up, if there's an error reading the users or
aliases files, we only log but do not exit. And then those files will
not be attempted to be read on the periodic reload.

We also treat "file does not exist" as an error for users file, but not
aliases file, resulting in inconsistent behaviour between the two.

All of this makes some classes of problems (like permission errors) more
difficult to spot and troubleshoot. For example,
https://github.com/albertito/chasquid/issues/55.

So this patch makes errors reading users/aliases files on startup a
fatal error, and also unifies the "file does not exist" behaviour to
make it not an error in both cases.

Note that the behaviour on the periodic reload is unchanged: treat these
errors as fatal too. This may be changed in future patches.

chasquid.go

@@ -27,7 +27,6 @@ import (
 	"blitiri.com.ar/go/chasquid/internal/normalize"
 	"blitiri.com.ar/go/chasquid/internal/smtpsrv"
 	"blitiri.com.ar/go/chasquid/internal/sts"
-	"blitiri.com.ar/go/chasquid/internal/userdb"
 	"blitiri.com.ar/go/log"
 	"blitiri.com.ar/go/systemd"
 )
@@ -286,18 +285,18 @@ func loadDomain(name, dir string, s *smtpsrv.Server) {
 	log.Infof("  %s", name)
 	s.AddDomain(name)
 
-	udb, err := userdb.Load(dir + "/users")
+	err := s.AddUserDB(name, dir+"/users")
-	if os.IsNotExist(err) {
+	if err != nil {
-		// No users file present, that's okay.
+		// If there is an error loading users, fail hard to make sure this is
-	} else if err != nil {
+		// noticed and fixed as soon as it happens.
-		log.Errorf("    users file error: %v", err)
+		log.Fatalf("    users file error: %v", err)
-	} else {
-		s.AddUserDB(name, udb)
 	}
 
 	err = s.AddAliasesFile(name, dir+"/aliases")
 	if err != nil {
-		log.Errorf("    aliases file error: %v", err)
+		// If there's an error loading aliases, fail hard to make sure this is
+		// noticed and fixed as soon as it happens.
+		log.Fatalf("    aliases file error: %v", err)
 	}
 
 	err = loadDKIM(name, dir, s)

cmd/chasquid-util/chasquid-util.go

@@ -149,7 +149,14 @@ func userDBFromArgs(create bool) (string, string, *userdb.DB) {
 
 // chasquid-util check-userdb <domain>
 func checkUserDB() {
-	_, err := userdb.Load(userDBForDomain(""))
+	path := userDBForDomain("")
+	// Check if the file exists. This is because userdb.Load does not consider
+	// it an error.
+	if _, err := os.Stat(path); os.IsNotExist(err) {
+		Fatalf("Error: file %q does not exist", path)
+	}
+
+	_, err := userdb.Load(path)
 	if err != nil {
 		Fatalf("Error loading database: %v", err)
 	}

cmd/chasquid-util/test_bad_args.cmy

@@ -4,7 +4,7 @@ c <- Unknown argument "blahrarghar"
 c wait 1
 
 c = ./chasquid-util --configdir=.nonono check-userdb
-c <- Error loading database: open .nonono/domains//users: no such file or directory
+c <- Error: file ".nonono/domains//users" does not exist
 c wait 1
 
 c = ./chasquid-util --configdir=.nonono print-config

internal/aliases/aliases.go

@@ -345,7 +345,8 @@ func (v *Resolver) AddDomain(domain string) {
 }
 
 // AddAliasesFile to the resolver. The file will be parsed, and an error
-// returned if it does not exist or parse correctly.
+// returned if it does not parse correctly.  Note that the file not existing
+// does NOT result in an error.
 func (v *Resolver) AddAliasesFile(domain, path string) error {
 	// We unconditionally add the domain and file on our list.
 	// Even if the file does not exist now, it may later. This makes it be

internal/smtpsrv/server.go

@@ -132,9 +132,13 @@ func (s *Server) AddDomain(d string) {
 	s.aliasesR.AddDomain(d)
 }
 
-// AddUserDB adds a userdb.DB instance as backend for the domain.
+// AddUserDB adds a userdb file as backend for the domain.
-func (s *Server) AddUserDB(domain string, db *userdb.DB) {
+func (s *Server) AddUserDB(domain, f string) error {
-	s.authr.Register(domain, auth.WrapNoErrorBackend(db))
+	// Load the userdb, and register it unconditionally (so reload works even
+	// if there are errors right now).
+	udb, err := userdb.Load(f)
+	s.authr.Register(domain, auth.WrapNoErrorBackend(udb))
+	return err
 }
 
 // AddAliasesFile adds an aliases file for the given domain.

internal/smtpsrv/server_test.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"blitiri.com.ar/go/chasquid/internal/aliases"
+	"blitiri.com.ar/go/chasquid/internal/auth"
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
 	"blitiri.com.ar/go/chasquid/internal/maillog"
 	"blitiri.com.ar/go/chasquid/internal/testlib"
@@ -671,8 +672,8 @@ func realMain(m *testing.M) int {
 		udb.AddUser("testuser", "testpasswd")
 		s.aliasesR.AddAliasForTesting(
 			"to@localhost", "testuser@localhost", aliases.EMAIL)
+		s.authr.Register("localhost", auth.WrapNoErrorBackend(udb))
 		s.AddDomain("localhost")
-		s.AddUserDB("localhost", udb)
 
 		s.AddDomain("broken")
 		s.authr.Register("broken", &brokenAuthBE{})

internal/userdb/userdb.go

@@ -33,6 +33,7 @@ import (
 	"crypto/subtle"
 	"errors"
 	"fmt"
+	"os"
 	"sync"
 
 	"golang.org/x/crypto/scrypt"
@@ -59,8 +60,8 @@ func New(fname string) *DB {
 }
 
 // Load the database from the given file.
-// Return the database, and a fatal error if the database could not be
+// Return the database, and an error if the database could not be loaded. If
-// loaded.
+// the file does not exist, that is not considered an error.
 func Load(fname string) (*DB, error) {
 	db := New(fname)
 	err := protoio.ReadTextMessage(fname, db.db)
@@ -72,6 +73,12 @@ func Load(fname string) (*DB, error) {
 		db.db = &ProtoDB{Users: map[string]*Password{}}
 	}
 
+	if os.IsNotExist(err) {
+		// If the file does not exist now, it is not an error, as it might
+		// exist later and we want to be able to read it.
+		err = nil
+	}
+
 	return db, err
 }
 

internal/userdb/userdb_test.go

@@ -293,14 +293,14 @@ func TestReload(t *testing.T) {
 		t.Errorf("expected 2 users, got %d", len(db.db.Users))
 	}
 
-	// Cause an even bigger error loading, check the database is not changed.
+	// Delete the file (which is not considered an error).
-	db.fname = "/does/not/exist"
+	os.Remove(fname)
 	err = db.Reload()
-	if err == nil {
+	if err != nil {
-		t.Errorf("expected error, got nil")
+		t.Errorf("unexpected error: %v", err)
 	}
-	if len(db.db.Users) != 2 {
+	if len(db.db.Users) != 0 {
-		t.Errorf("expected 2 users, got %d", len(db.db.Users))
+		t.Errorf("expected 0 users, got %d", len(db.db.Users))
 	}
 }
 

test/t-20-bad_configs/c-12-bad_users/.expected-error

@@ -0,0 +1 @@
+users file error: open domains/testserver/users: permission denied

test/t-20-bad_configs/c-12-bad_users/chasquid.conf

@@ -0,0 +1,9 @@
+smtp_address: ":1025"
+submission_address: ":1587"
+submission_over_tls_address: ":1465"
+
+mail_delivery_agent_bin: "test-mda"
+mail_delivery_agent_args: "%to%"
+
+data_dir: "../.data"
+mail_log_path: "../.logs/mail_log"

test/t-20-bad_configs/c-12-bad_users/domains/testserver/users

@@ -0,0 +1,26 @@
+users:  {
+  key:  "someone"
+  value:  {
+    scrypt:  {
+      logN:  14
+      r:  8
+      p:  1
+      keyLen:  32
+      salt:  "J\x01\xed7]\x02\n\xe9;z[\x8d˱\x10\xc1"
+      encrypted:  "\xa50宴\xcbb\xc1!r]K\xd1yI\xa2\x99\x8d\xdaQx\x8e69\xac\xf4$\x01\x11\x03\x8d\x10"
+    }
+  }
+}
+users:  {
+  key:  "user"
+  value:  {
+    scrypt:  {
+      logN:  14
+      r:  8
+      p:  1
+      keyLen:  32
+      salt:  "\n\xc6\x1c\x8f\xb2\x0c\x15p\x8d\xa1\xc3\x05U6\xdb\xc4"
+      encrypted:  "\xc3\xe6B2\x84W\x1a\nq{\x07\xe0\x9c\x854\n\xac\xbc\xb7\x9c\x86Kyk\x8dj\x16\x1a\x8c$*N"
+    }
+  }
+}

test/t-20-bad_configs/c-13-bad_aliases/.expected-error

@@ -0,0 +1 @@
+aliases file error: open domains/testserver/aliases: permission denied

test/t-20-bad_configs/c-13-bad_aliases/chasquid.conf

@@ -0,0 +1,9 @@
+smtp_address: ":1025"
+submission_address: ":1587"
+submission_over_tls_address: ":1465"
+
+mail_delivery_agent_bin: "test-mda"
+mail_delivery_agent_args: "%to%"
+
+data_dir: "../.data"
+mail_log_path: "../.logs/mail_log"

test/t-20-bad_configs/c-13-bad_aliases/domains/testserver/aliases

@@ -0,0 +1 @@
+a: b

test/t-20-bad_configs/run.sh

@@ -19,7 +19,7 @@ mkdir -p c-04-no_cert_dirs/certs/
 # Generate certs for the tests that need them.
 for i in c-05-no_addrs c-06-bad_maillog c-07-bad_domain_info \
 	c-08-bad_sts_cache c-09-bad_queue_dir c-10-empty_listening_addr \
-	c-11-bad_dkim_key;
+	c-11-bad_dkim_key c-12-bad_users c-13-bad_aliases;
 do
 	CONFDIR=$i/ generate_certs_for testserver
 done
@@ -30,6 +30,10 @@ done
 cp c-11-bad_dkim_key/domains/testserver/dkim__selector.pem \
 	c-11-bad_dkim_key/domains/testserver/dkim:selector.pem
 
+# For the bad_users and bad_aliases test, make the relevant file unreadable.
+chmod -rw c-12-bad_users/domains/testserver/users
+chmod -rw c-13-bad_aliases/domains/testserver/aliases
+
 for i in c-*; do
 	if chasquid --config_dir="$i" > ".chasquid-$i.out" 2>&1; then
 		echo "$i failed; output:"
@@ -54,4 +58,8 @@ for i in c-*; do
 	fi
 done
 
+# Give permissions back, to avoid annoying git messages.
+chmod +rw c-12-bad_users/domains/testserver/users
+chmod +rw c-13-bad_aliases/domains/testserver/aliases
+
 success