deprecate having only localparts in an Account's Destinations, it sho…

…uld always be a full email address current behaviour isn't intuitive. it's not great to have to attempt parsing the strings as both localpart and email address. so we deprecate the localpart-only behaviour. when we load the config file, and it has localpart-only Destinations keys, we'll change them to full addresses in memory. when an admin causes a write of domains.conf, it'll automatically be fixed. we log an error with a deprecated notice for each localpart-only destinations key. sometime in the future, we can remove the old localpart-only destination support. will be in the release notes then. also start keeping track of update notes that need to make it in the release notes of the next release. for issue #18
mjl- · Mar 9, 2023 · 2c07645 · 2c07645
1 parent 5742ed1
commit 2c07645
Show file tree

Hide file tree

Showing 20 changed files with 57 additions and 41 deletions.
diff --git a/config/config.go b/config/config.go
@@ -225,9 +225,9 @@ type DKIM struct {
 }
 
 type Account struct {
-	Domain       string                 `sconf-doc:"Default domain for addresses specified in Destinations. An address can specify a domain override."`
+	Domain       string                 `sconf-doc:"Default domain for account. Deprecated behaviour: If a destination is not a full address but only a localpart, this domain is added to form a full address."`
 	Description  string                 `sconf:"optional" sconf-doc:"Free form description, e.g. full name or alternative contact info."`
-	Destinations map[string]Destination `sconf-doc:"Destinations, specified as (encoded) localpart for Domain, or a full address including domain override."`
+	Destinations map[string]Destination `sconf-doc:"Destinations, keys are email addresses (with IDNA domains). Deprecated behaviour: If the address is not a full address but a localpart, it is combined with Domain to form a full address."`
 	SubjectPass  struct {
 		Period time.Duration `sconf-doc:"How long unique values are accepted after generating, e.g. 12h."` // todo: have a reasonable default for this?
 	} `sconf:"optional" sconf-doc:"If configured, messages classified as weakly spam are rejected with instructions to retry delivery, but this time with a signed token added to the subject. During the next delivery attempt, the signed token will bypass the spam filter. Messages with a clear spam signal, such as a known bad reputation, are rejected/delayed without a signed token."`

diff --git a/config/doc.go b/config/doc.go
@@ -436,15 +436,16 @@ describe-static" and "mox config describe-domains":
 	Accounts:
 		x:
 
-			# Default domain for addresses specified in Destinations. An address can specify a
-			# domain override.
+			# Default domain for account. Deprecated behaviour: If a destination is not a full
+			# address but only a localpart, this domain is added to form a full address.
 			Domain:
 
 			# Free form description, e.g. full name or alternative contact info. (optional)
 			Description:
 
-			# Destinations, specified as (encoded) localpart for Domain, or a full address
-			# including domain override.
+			# Destinations, keys are email addresses (with IDNA domains). Deprecated
+			# behaviour: If the address is not a full address but a localpart, it is combined
+			# with Domain to form a full address.
 			Destinations:
 				x:
 

diff --git a/http/account_test.go b/http/account_test.go
@@ -67,7 +67,7 @@ func TestAccount(t *testing.T) {
 	test(authBad, "")
 
 	_, dests := Account{}.Destinations(authCtx)
-	Account{}.DestinationSave(authCtx, "mjl", dests["mjl"], dests["mjl"]) // todo: save modified value and compare it afterwards
+	Account{}.DestinationSave(authCtx, "mjl@mox.example", dests["mjl@mox.example"], dests["mjl@mox.example"]) // todo: save modified value and compare it afterwards
 
 	go importManage()
 

diff --git a/mox-/admin.go b/mox-/admin.go
@@ -115,7 +115,7 @@ func MakeAccountConfig(addr smtp.Address) config.Account {
 	account := config.Account{
 		Domain: addr.Domain.Name(),
 		Destinations: map[string]config.Destination{
-			addr.Localpart.String(): {},
+			addr.String(): {},
 		},
 		RejectsMailbox: "Rejects",
 		JunkFilter: &config.JunkFilter{
@@ -676,13 +676,7 @@ func AddressAdd(ctx context.Context, address, account string) (rerr error) {
 	for name, d := range a.Destinations {
 		nd[name] = d
 	}
-	var k string
-	if addr.Domain == a.DNSDomain {
-		k = addr.Localpart.String()
-	} else {
-		k = addr.String()
-	}
-	nd[k] = config.Destination{}
+	nd[addr.String()] = config.Destination{}
 	a.Destinations = nd
 	nc.Accounts[account] = a
 
@@ -727,6 +721,7 @@ func AddressRemove(ctx context.Context, address string) (rerr error) {
 	na.Destinations = map[string]config.Destination{}
 	var dropped bool
 	for name, d := range a.Destinations {
+		// todo deprecated: remove support for localpart-only with default domain as destination address.
 		if !(name == addr.Localpart.String() && a.DNSDomain == addr.Domain || name == addrStr) {
 			na.Destinations[name] = d
 		} else {

diff --git a/mox-/config.go b/mox-/config.go
@@ -847,6 +847,9 @@ func prepareDynamicConfig(ctx context.Context, dynamicPath string, static config
 		}
 		c.Accounts[accName] = acc
 
+		// todo deprecated: only localpart as keys for Destinations, we are replacing them with full addresses. if domains.conf is written, we won't have to do this again.
+		replaceLocalparts := map[string]string{}
+
 		for addrName, dest := range acc.Destinations {
 			checkMailboxNormf(dest.Mailbox, "account %q, destination %q", accName, addrName)
 
@@ -906,6 +909,7 @@ func prepareDynamicConfig(ctx context.Context, dynamicPath string, static config
 				}
 			}
 
+			// todo deprecated: remove support for parsing destination as just a localpart instead full address.
 			var address smtp.Address
 			localpart, err := smtp.ParseLocalpart(addrName)
 			if err != nil && errors.Is(err, smtp.ErrBadLocalpart) {
@@ -927,13 +931,25 @@ func prepareDynamicConfig(ctx context.Context, dynamicPath string, static config
 					addErrorf("unknown domain %s for account %q", acc.DNSDomain.Name(), accName)
 					continue
 				}
+				replaceLocalparts[addrName] = address.Pack(true)
 			}
 			addrFull := address.Pack(true)
 			if _, ok := accDests[addrFull]; ok {
 				addErrorf("duplicate destination address %q", addrFull)
 			}
 			accDests[addrFull] = AccountDestination{address.Localpart, accName, dest}
 		}
+
+		for lp, addr := range replaceLocalparts {
+			dest, ok := acc.Destinations[lp]
+			if !ok {
+				addErrorf("could not find localpart %q to replace with address in destinations", lp)
+			} else {
+				log.Error("deprecated: destination with localpart-only key will be removed in the future, replace it with a full email address, by appending the default domain", mlog.Field("localpart", lp), mlog.Field("address", addr), mlog.Field("account", accName))
+				acc.Destinations[addr] = dest
+				delete(acc.Destinations, lp)
+			}
+		}
 	}
 
 	// Set DMARC destinations.

diff --git a/quickstart.go b/quickstart.go
@@ -125,16 +125,16 @@ webhandlers".
 	if err != nil {
 		fatalf("parsing email address: %s", err)
 	}
-	username := addr.Localpart.String()
+	accountName := addr.Localpart.String()
 	domain := addr.Domain
 
-	for _, c := range username {
+	for _, c := range accountName {
 		if c > 0x7f {
 			fmt.Printf(`NOTE: Username %q is not ASCII-only. It is recommended you also configure an
 ASCII-only alias. Both for delivery of email from other systems, and for
 logging in with IMAP.
 
-`, username)
+`, accountName)
 			break
 		}
 	}
@@ -538,7 +538,7 @@ listed in more DNS block lists, visit:
 		"public":   public,
 		"internal": internal,
 	}
-	sc.Postmaster.Account = username
+	sc.Postmaster.Account = accountName
 	sc.Postmaster.Mailbox = "Postmaster"
 
 	mox.ConfigStaticPath = "config/mox.conf"
@@ -548,7 +548,7 @@ listed in more DNS block lists, visit:
 
 	accountConf := mox.MakeAccountConfig(addr)
 	const withMTASTS = true
-	confDomain, keyPaths, err := mox.MakeDomainConfig(context.Background(), domain, dnshostname, username, withMTASTS)
+	confDomain, keyPaths, err := mox.MakeDomainConfig(context.Background(), domain, dnshostname, accountName, withMTASTS)
 	if err != nil {
 		fatalf("making domain config: %s", err)
 	}
@@ -558,7 +558,7 @@ listed in more DNS block lists, visit:
 		domain.Name(): confDomain,
 	}
 	dc.Accounts = map[string]config.Account{
-		username: accountConf,
+		accountName: accountConf,
 	}
 
 	// Build config in memory, so we can easily comment out the DNSBLs config.
@@ -584,12 +584,12 @@ listed in more DNS block lists, visit:
 	// This approach is a bit horrible, but it generates a convenient
 	// example that includes the comments. Though it is gone by the first
 	// write of the file by mox.
-	odests := fmt.Sprintf("\t\tDestinations:\n\t\t\t%s: nil\n", addr.Localpart.String())
+	odests := fmt.Sprintf("\t\tDestinations:\n\t\t\t%s: nil\n", addr.String())
 	var destsExample = struct {
 		Destinations map[string]config.Destination
 	}{
 		Destinations: map[string]config.Destination{
-			addr.Localpart.String(): {
+			addr.String(): {
 				Rulesets: []config.Ruleset{
 					{
 						VerifiedDomain: "list.example.org",
@@ -641,7 +641,7 @@ listed in more DNS block lists, visit:
 	if err != nil {
 		fatalf("open account: %s", err)
 	}
-	cleanupPaths = append(cleanupPaths, dataDir, filepath.Join(dataDir, "accounts"), filepath.Join(dataDir, "accounts", username), filepath.Join(dataDir, "accounts", username, "index.db"))
+	cleanupPaths = append(cleanupPaths, dataDir, filepath.Join(dataDir, "accounts"), filepath.Join(dataDir, "accounts", accountName), filepath.Join(dataDir, "accounts", accountName, "index.db"))
 
 	password := pwgen()
 	if err := acc.SetPassword(password); err != nil {

diff --git a/testdata/dsn/domains.conf b/testdata/dsn/domains.conf
@@ -23,4 +23,4 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
diff --git a/testdata/httpaccount/domains.conf b/testdata/httpaccount/domains.conf
@@ -4,7 +4,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl:
+			mjl@mox.example:
 				Mailbox: Inbox
 				Rulesets:
 					-
@@ -15,7 +15,7 @@ Accounts:
 						HeadersRegexp:
 								subject: .*
 						Mailbox: Catchall
-			other:
+			other@mox.example:
 				Mailbox: Other
 		JunkFilter:
 			Threshold: 0.950000

diff --git a/testdata/imap/domains.conf b/testdata/imap/domains.conf
@@ -5,7 +5,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 		JunkFilter:
 			Threshold: 0.95
 			Params:

diff --git a/testdata/imaptest/config/domains.conf b/testdata/imaptest/config/domains.conf
@@ -5,7 +5,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 		JunkFilter:
 			Threshold: 0.95
 			Params:

diff --git a/testdata/integration/config/domains.conf b/testdata/integration/config/domains.conf
@@ -39,7 +39,7 @@ Accounts:
 	moxtest1:
 		Domain: mox1.example
 		Destinations:
-			moxtest1: nil
+			moxtest1@mox1.example: nil
 		JunkFilter:
 			Threshold: 0.9999
 			Params:
@@ -53,7 +53,7 @@ Accounts:
 	moxtest2:
 		Domain: mox2.example
 		Destinations:
-			moxtest2: nil
+			moxtest2@mox2.example: nil
 		JunkFilter:
 			Threshold: 0.9999
 			Params:
@@ -67,7 +67,7 @@ Accounts:
 	moxtest3:
 		Domain: mox3.example
 		Destinations:
-			moxtest3: nil
+			moxtest3@mox3.example: nil
 		SubjectPass:
 			Period: 1h
 		RejectsMailbox: rejects

diff --git a/testdata/queue/domains.conf b/testdata/queue/domains.conf
@@ -4,4 +4,4 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
diff --git a/testdata/smtp/dmarcreport/domains.conf b/testdata/smtp/dmarcreport/domains.conf
@@ -8,4 +8,4 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
diff --git a/testdata/smtp/domains.conf b/testdata/smtp/domains.conf
@@ -4,7 +4,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 		JunkFilter:
 			Threshold: 0.9
 			Params:

diff --git a/testdata/smtp/junk/domains.conf b/testdata/smtp/junk/domains.conf
@@ -4,7 +4,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 		RejectsMailbox: Rejects
 		JunkFilter:
 			# Spamminess score between 0 and 1 above which emails are rejected as spam. E.g.

diff --git a/testdata/smtp/tlsrpt/domains.conf b/testdata/smtp/tlsrpt/domains.conf
@@ -8,4 +8,4 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
diff --git a/testdata/store/domains.conf b/testdata/store/domains.conf
@@ -4,7 +4,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl:
+			mjl@mox.example:
 				Mailbox: Inbox
 				Rulesets:
 					-
@@ -15,7 +15,7 @@ Accounts:
 						HeadersRegexp:
 								subject: .*
 						Mailbox: Catchall
-			other:
+			other@mox.example:
 				Mailbox: Other
 		JunkFilter:
 			Threshold: 0.95

diff --git a/testdata/web/domains.conf b/testdata/web/domains.conf
@@ -5,7 +5,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 WebDomainRedirects:
 	redir.mox.example: mox.example
 WebHandlers:

diff --git a/testdata/webserver/domains.conf b/testdata/webserver/domains.conf
@@ -5,7 +5,7 @@ Accounts:
 	mjl:
 		Domain: mox.example
 		Destinations:
-			mjl: nil
+			mjl@mox.example: nil
 WebDomainRedirects:
 	redir.mox.example: mox.example
 WebHandlers:

diff --git a/updating.txt b/updating.txt
@@ -0,0 +1,4 @@
+work in progress: update instructions for the next release
+
+- In domains.conf, for an account, the Destinations map will now always use full email addresses, no longer localparts relative to the Domain configured for the account. The old form with just a localpart is still accepted. When writing domains.conf through the cli commands or admin web pages, the destinations will automatically be written with full email addresses. In the future, support for the localpart-only form will be removed.
+- If you run mox behind a NAT, you can now specify "IPsNATed: true" in the SMTP listener to skip a few DNS checks that previously would always fail due to the IPs being NATed.