From 8cc795b2ecbbc130370281c7e1647bde8474f4c6 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sun, 28 Apr 2024 13:18:25 +0200 Subject: [PATCH] in smtp submission, if a fromid is present in the mailfrom command, use it when queueing it's the responsibility of the sender to use unique fromid's. we do check if that's the case, and return an error if not. also make it more clear that "unique smtp mail from addresses" map to the "FromIDLoginAddresses" account config field. based on feedback from cuu508 for #31, thanks! --- config/config.go | 2 +- config/doc.go | 6 +++-- queue/queue.go | 20 ++++++++++++++ smtpserver/server.go | 25 +++++++++++++++--- smtpserver/server_test.go | 45 ++++++++++++++++++++++++++++++++ testdata/smtpfromid/domains.conf | 10 +++++++ testdata/smtpfromid/mox.conf | 9 +++++++ webaccount/account.js | 2 +- webaccount/account.ts | 2 +- webapi/doc.go | 10 ++++--- webapi/gendoc.sh | 10 ++++--- website/features/index.md | 4 +-- 12 files changed, 126 insertions(+), 19 deletions(-) create mode 100644 testdata/smtpfromid/domains.conf create mode 100644 testdata/smtpfromid/mox.conf diff --git a/config/config.go b/config/config.go index 7ebc411..35a7f6d 100644 --- a/config/config.go +++ b/config/config.go @@ -410,7 +410,7 @@ type AutomaticJunkFlags struct { type Account struct { OutgoingWebhook *OutgoingWebhook `sconf:"optional" sconf-doc:"Webhooks for events about outgoing deliveries."` IncomingWebhook *IncomingWebhook `sconf:"optional" sconf-doc:"Webhooks for events about incoming deliveries over SMTP."` - FromIDLoginAddresses []string `sconf:"optional" sconf-doc:"Login addresses that cause outgoing email to be sent with SMTP MAIL FROM addresses with a unique id after the localpart catchall separator (which must be enabled when addresses are specified here). Any delivery status notifications (DSN, e.g. for bounces), can be related to the original message and recipient with unique id's. You can login to an account with any valid email address, including variants with the localpart catchall separator. You can use this mechanism to both send outgoing messages both with and without unique fromid for a given address."` + FromIDLoginAddresses []string `sconf:"optional" sconf-doc:"Login addresses that cause outgoing email to be sent with SMTP MAIL FROM addresses with a unique id after the localpart catchall separator (which must be enabled when addresses are specified here). Any delivery status notifications (DSN, e.g. for bounces), can be related to the original message and recipient with unique id's. You can login to an account with any valid email address, including variants with the localpart catchall separator. You can use this mechanism to both send outgoing messages with and without unique fromid for a given email address. With the webapi and webmail, a unique id will be generated. For submission, the id from the SMTP MAIL FROM command is used if present, and a unique id is generated otherwise."` KeepRetiredMessagePeriod time.Duration `sconf:"optional" sconf-doc:"Period to keep messages retired from the queue (delivered or failed) around. Keeping retired messages is useful for maintaining the suppression list for transactional email, for matching incoming DSNs to sent messages, and for debugging. The time at which to clean up (remove) is calculated at retire time. E.g. 168h (1 week)."` KeepRetiredWebhookPeriod time.Duration `sconf:"optional" sconf-doc:"Period to keep webhooks retired from the queue (delivered or failed) around. Useful for debugging. The time at which to clean up (remove) is calculated at retire time. E.g. 168h (1 week)."` diff --git a/config/doc.go b/config/doc.go index 8b3d1e9..fdb31fe 100644 --- a/config/doc.go +++ b/config/doc.go @@ -975,8 +975,10 @@ See https://pkg.go.dev/github.com/mjl-/sconf for details. # (DSN, e.g. for bounces), can be related to the original message and recipient # with unique id's. You can login to an account with any valid email address, # including variants with the localpart catchall separator. You can use this - # mechanism to both send outgoing messages both with and without unique fromid for - # a given address. (optional) + # mechanism to both send outgoing messages with and without unique fromid for a + # given email address. With the webapi and webmail, a unique id will be generated. + # For submission, the id from the SMTP MAIL FROM command is used if present, and a + # unique id is generated otherwise. (optional) FromIDLoginAddresses: - diff --git a/queue/queue.go b/queue/queue.go index e6b40f4..ec1d8b8 100644 --- a/queue/queue.go +++ b/queue/queue.go @@ -41,6 +41,10 @@ import ( "github.com/mjl-/mox/webhook" ) +// ErrFromID indicate a fromid was present when adding a message to the queue, but +// it wasn't unique. +var ErrFromID = errors.New("fromid not unique") + var ( metricConnection = promauto.NewCounterVec( prometheus.CounterOpts{ @@ -662,6 +666,22 @@ func Add(ctx context.Context, log mlog.Log, senderAccount string, msgFile *os.Fi // message inserted. var baseID int64 for i := range qml { + // FromIDs must be unique if present. We don't have a unique index because values + // can be the empty string. We check in both Msg and MsgRetired, both are relevant + // for uniquely identifying a message sent in the past. + if fromID := qml[i].FromID; fromID != "" { + if exists, err := bstore.QueryTx[Msg](tx).FilterNonzero(Msg{FromID: fromID}).Exists(); err != nil { + return fmt.Errorf("looking up fromid: %v", err) + } else if exists { + return fmt.Errorf("%w: fromid %q already present in message queue", ErrFromID, fromID) + } + if exists, err := bstore.QueryTx[MsgRetired](tx).FilterNonzero(MsgRetired{FromID: fromID}).Exists(); err != nil { + return fmt.Errorf("looking up fromid: %v", err) + } else if exists { + return fmt.Errorf("%w: fromid %q already present in retired message queue", ErrFromID, fromID) + } + } + qml[i].SenderAccount = senderAccount qml[i].BaseID = baseID for _, hr := range holdRules { diff --git a/smtpserver/server.go b/smtpserver/server.go index 0bb6e07..38fa205 100644 --- a/smtpserver/server.go +++ b/smtpserver/server.go @@ -2097,8 +2097,20 @@ func (c *conn) submit(ctx context.Context, recvHdrFor func(string) string, msgWr xcheckf(err, "parsing login address") useFromID := slices.Contains(accConf.ParsedFromIDLoginAddresses, loginAddr) var localpartBase string + var fromID string + var genFromID bool if useFromID { - localpartBase = strings.SplitN(string(c.mailFrom.Localpart), confDom.LocalpartCatchallSeparator, 2)[0] + // With submission, user can bring their own fromid. + t := strings.SplitN(string(c.mailFrom.Localpart), confDom.LocalpartCatchallSeparator, 2) + localpartBase = t[0] + if len(t) == 2 { + fromID = t[1] + if fromID != "" && len(c.recipients) > 1 { + xsmtpServerErrorf(codes{smtp.C554TransactionFailed, smtp.SeProto5TooManyRcpts3}, "cannot send to multiple recipients with chosen fromid") + } + } else { + genFromID = true + } } now := time.Now() qml := make([]queue.Msg, len(c.recipients)) @@ -2116,9 +2128,10 @@ func (c *conn) submit(ctx context.Context, recvHdrFor func(string) string, msgWr } fp := *c.mailFrom - var fromID string if useFromID { - fromID = xrandomID(16) + if genFromID { + fromID = xrandomID(16) + } fp.Localpart = smtp.Localpart(localpartBase + confDom.LocalpartCatchallSeparator + fromID) } @@ -2142,7 +2155,11 @@ func (c *conn) submit(ctx context.Context, recvHdrFor func(string) string, msgWr } // todo: it would be good to have a limit on messages (count and total size) a user has in the queue. also/especially with futurerelease. ../rfc/4865:387 - if err := queue.Add(ctx, c.log, c.account.Name, dataFile, qml...); err != nil { + if err := queue.Add(ctx, c.log, c.account.Name, dataFile, qml...); err != nil && errors.Is(err, queue.ErrFromID) && !genFromID { + // todo: should we return this error during the "rcpt to" command? + // secode is not an exact match, but seems closest. + xsmtpServerErrorf(errCodes(smtp.C554TransactionFailed, smtp.SeAddr1SenderSyntax7, err), "bad fromid in smtp mail from address: %s", err) + } else if err != nil { // Aborting the transaction is not great. But continuing and generating DSNs will // probably result in errors as well... metricSubmission.WithLabelValues("queueerror").Inc() diff --git a/smtpserver/server_test.go b/smtpserver/server_test.go index 7697b31..db48b89 100644 --- a/smtpserver/server_test.go +++ b/smtpserver/server_test.go @@ -1898,3 +1898,48 @@ test email ts.smtpErr(err, &smtpclient.Error{Permanent: true, Code: smtp.C554TransactionFailed, Secode: smtp.SeMsg6Other0}) }) } + +// FromID can be specified during submission, but must be unique, with single recipient. +func TestUniqueFromID(t *testing.T) { + ts := newTestServer(t, filepath.FromSlash("../testdata/smtpfromid/mox.conf"), dns.MockResolver{}) + defer ts.close() + + ts.user = "mjl+fromid@mox.example" + ts.pass = password0 + ts.submission = true + + extraMsg := strings.ReplaceAll(`From: +To: +Subject: test + +test email +`, "\n", "\r\n") + + // Specify our own unique id when queueing. + ts.run(func(err error, client *smtpclient.Client) { + tcheck(t, err, "init client") + mailFrom := "mjl+unique@mox.example" + rcptTo := "mjl@mox.example" + err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(extraMsg)), strings.NewReader(extraMsg), true, true, false) + ts.smtpErr(err, nil) + }) + + // But we can only use it once. + ts.run(func(err error, client *smtpclient.Client) { + tcheck(t, err, "init client") + mailFrom := "mjl+unique@mox.example" + rcptTo := "mjl@mox.example" + err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(extraMsg)), strings.NewReader(extraMsg), true, true, false) + ts.smtpErr(err, &smtpclient.Error{Permanent: true, Code: smtp.C554TransactionFailed, Secode: smtp.SeAddr1SenderSyntax7}) + }) + + // We cannot use our own fromid with multiple recipients. + ts.run(func(err error, client *smtpclient.Client) { + tcheck(t, err, "init client") + mailFrom := "mjl+unique2@mox.example" + rcptTo := []string{"mjl@mox.example", "mjl@mox.example"} + _, err = client.DeliverMultiple(ctxbg, mailFrom, rcptTo, int64(len(extraMsg)), strings.NewReader(extraMsg), true, true, false) + ts.smtpErr(err, &smtpclient.Error{Permanent: true, Code: smtp.C554TransactionFailed, Secode: smtp.SeProto5TooManyRcpts3}) + }) + +} diff --git a/testdata/smtpfromid/domains.conf b/testdata/smtpfromid/domains.conf new file mode 100644 index 0000000..defcf96 --- /dev/null +++ b/testdata/smtpfromid/domains.conf @@ -0,0 +1,10 @@ +Domains: + mox.example: + LocalpartCatchallSeparator: + +Accounts: + mjl: + Domain: mox.example + Destinations: + mjl@mox.example: nil + FromIDLoginAddresses: + - mjl+fromid@mox.example diff --git a/testdata/smtpfromid/mox.conf b/testdata/smtpfromid/mox.conf new file mode 100644 index 0000000..e1286db --- /dev/null +++ b/testdata/smtpfromid/mox.conf @@ -0,0 +1,9 @@ +DataDir: data +User: 1000 +LogLevel: trace +Hostname: mox.example +Postmaster: + Account: mjl + Mailbox: postmaster +Listeners: + local: nil diff --git a/webaccount/account.js b/webaccount/account.js index 52a738b..2177084 100644 --- a/webaccount/account.js +++ b/webaccount/account.js @@ -1449,7 +1449,7 @@ const index = async () => { e.preventDefault(); e.stopPropagation(); await check(keepRetiredPeriodsFieldset, (async () => await client.KeepRetiredPeriodsSave(parseDuration(keepRetiredMessagePeriod.value), parseDuration(keepRetiredWebhookPeriod.value)))()); - }, keepRetiredPeriodsFieldset = dom.fieldset(dom.div(style({ display: 'flex', gap: '1em', alignItems: 'flex-end' }), dom.div(dom.label('Messages deliveries', dom.br(), keepRetiredMessagePeriod = dom.input(attr.value(formatDuration(acc.KeepRetiredMessagePeriod))))), dom.div(dom.label('Webhook deliveries', dom.br(), keepRetiredWebhookPeriod = dom.input(attr.value(formatDuration(acc.KeepRetiredWebhookPeriod))))), dom.div(dom.submitbutton('Save'))))), dom.br(), dom.h2('Unique SMTP MAIL FROM login addresses', attr.title('Outgoing messages are normally sent using your email address in the SMTP MAIL FROM command. By using unique addresses (by using the localpart catchall separator, e.g. addresses of the form "localpart+@domain"), future incoming DSNs can be related to the original outgoing messages and recipients, which allows for automatic management of recipient suppression lists when keeping retired messages for as long as you expect DSNs to come in as configured above. Configure the addresses used for logging in with SMTP submission, the webapi or webmail for which unique SMTP MAIL FROM addesses should be enabled. Note: These are addresses used for authenticating, not the address in the message "From" header.')), (() => { + }, keepRetiredPeriodsFieldset = dom.fieldset(dom.div(style({ display: 'flex', gap: '1em', alignItems: 'flex-end' }), dom.div(dom.label('Messages deliveries', dom.br(), keepRetiredMessagePeriod = dom.input(attr.value(formatDuration(acc.KeepRetiredMessagePeriod))))), dom.div(dom.label('Webhook deliveries', dom.br(), keepRetiredWebhookPeriod = dom.input(attr.value(formatDuration(acc.KeepRetiredWebhookPeriod))))), dom.div(dom.submitbutton('Save'))))), dom.br(), dom.h2('Unique SMTP MAIL FROM login addresses ("FromID")', attr.title('Login addresses that cause outgoing email to be sent with SMTP MAIL FROM addresses with a unique id after the localpart catchall separator (which must be enabled when addresses are specified here). Any delivery status notifications (DSN, e.g. for bounces), can be related to the original message and recipient with unique id\'s. You can login to an account with any valid email address, including variants with the localpart catchall separator. You can use this mechanism to both send outgoing messages with and without unique fromid for a given email address. With the webapi and webmail, a unique id will be generated. For submission, the id from the SMTP MAIL FROM command is used if present, and a unique id is generated otherwise. Corresponds to field FromIDLoginAddresses in the Account configuration in domains.conf.')), (() => { let inputs = []; let elem; const render = () => { diff --git a/webaccount/account.ts b/webaccount/account.ts index 5d071c6..3880b01 100644 --- a/webaccount/account.ts +++ b/webaccount/account.ts @@ -1063,7 +1063,7 @@ const index = async () => { ), dom.br(), - dom.h2('Unique SMTP MAIL FROM login addresses', attr.title('Outgoing messages are normally sent using your email address in the SMTP MAIL FROM command. By using unique addresses (by using the localpart catchall separator, e.g. addresses of the form "localpart+@domain"), future incoming DSNs can be related to the original outgoing messages and recipients, which allows for automatic management of recipient suppression lists when keeping retired messages for as long as you expect DSNs to come in as configured above. Configure the addresses used for logging in with SMTP submission, the webapi or webmail for which unique SMTP MAIL FROM addesses should be enabled. Note: These are addresses used for authenticating, not the address in the message "From" header.')), + dom.h2('Unique SMTP MAIL FROM login addresses ("FromID")', attr.title('Login addresses that cause outgoing email to be sent with SMTP MAIL FROM addresses with a unique id after the localpart catchall separator (which must be enabled when addresses are specified here). Any delivery status notifications (DSN, e.g. for bounces), can be related to the original message and recipient with unique id\'s. You can login to an account with any valid email address, including variants with the localpart catchall separator. You can use this mechanism to both send outgoing messages with and without unique fromid for a given email address. With the webapi and webmail, a unique id will be generated. For submission, the id from the SMTP MAIL FROM command is used if present, and a unique id is generated otherwise. Corresponds to field FromIDLoginAddresses in the Account configuration in domains.conf.')), (() => { let inputs: HTMLInputElement[] = [] let elem: HTMLElement diff --git a/webapi/doc.go b/webapi/doc.go index 23edaa7..d7df748 100644 --- a/webapi/doc.go +++ b/webapi/doc.go @@ -27,10 +27,12 @@ An HTTP POST to /webapi/v0/ calls a method.The form can be either "application/x-www-form-urlencoded" or "multipart/form-data". Form field "request" must contain the request parameters, encoded as JSON. -HTTP basic authentication is required for calling methods, with an email -address as user name. Use a login address configured for "unique SMTP MAIL -FROM" addresses, and configure a period to "keep retired messages delivered -from the queue" for automatic suppression list management. +HTTP basic authentication is required for calling methods, with an email address +as user name. Use a login address configured for "unique SMTP MAIL FROM" +addresses ("FromIDLoginAddresses" in the account configuration), and configure +an interval to "keep retired messages delivered from the queue". This allows +incoming DSNs to be matched to the original outgoing messages, and enables +automatic suppression list management. HTTP response status 200 OK indicates a successful method call, status 400 indicates an error. The response body of an error is a JSON object with a diff --git a/webapi/gendoc.sh b/webapi/gendoc.sh index a5831b4..60dfa97 100755 --- a/webapi/gendoc.sh +++ b/webapi/gendoc.sh @@ -37,10 +37,12 @@ An HTTP POST to /webapi/v0/ calls a method.The form can be either "application/x-www-form-urlencoded" or "multipart/form-data". Form field "request" must contain the request parameters, encoded as JSON. -HTTP basic authentication is required for calling methods, with an email -address as user name. Use a login address configured for "unique SMTP MAIL -FROM" addresses, and configure a period to "keep retired messages delivered -from the queue" for automatic suppression list management. +HTTP basic authentication is required for calling methods, with an email address +as user name. Use a login address configured for "unique SMTP MAIL FROM" +addresses ("FromIDLoginAddresses" in the account configuration), and configure +an interval to "keep retired messages delivered from the queue". This allows +incoming DSNs to be matched to the original outgoing messages, and enables +automatic suppression list management. HTTP response status 200 OK indicates a successful method call, status 400 indicates an error. The response body of an error is a JSON object with a diff --git a/website/features/index.md b/website/features/index.md index aa59ff1..867d04d 100644 --- a/website/features/index.md +++ b/website/features/index.md @@ -315,8 +315,8 @@ composing email messages (internet message format, IMF), SMTP for submission, and IMAP for handling incoming messages including delivery status notifications (DSNs). -Outgoing webhooks notify about events for outgoing deliveries (such as -"delivered", "delayed", "failed", "suppressed"). +Outgoing webhooks notify about events for outgoing deliveries, such as +"delivered", "delayed", "failed", "suppressed". Incoming webhooks notify about incoming deliveries.