Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new DELIVERY config parameter. #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iskunk
Copy link

@iskunk iskunk commented Apr 23, 2023

Implementation of the feature discussed in #121.

Some remarks:

  • I shortened "DELIVERYMODE" to "DELIVERY" for brevity;

  • I fixed one unrelated bug along the way: specifying FINGERPRINT without an argument led to a segfault instead of an error;

  • NULLCLIENT remains recognized in the config file, but I removed mention of it from the documentation as DELIVERY REMOTEONLY supersedes it.

Between this new parameter, use/absence of a smarthost, and a local (user) vs. remote (user@host) e-mail recipient address, there are a number of permutations of operation. The table below summarizes them, and the result:

Address Smarthost? DELIVERY Result
local no LOCALANDREMOTE delivered locally
local no LOCALONLY delivered locally
local no REMOTEONLY error (SMARTHOST required, rc=78)
local yes LOCALANDREMOTE delivered locally
local yes LOCALONLY delivered locally
local yes REMOTEONLY sent to SMARTHOST, bounce likely (rc=70)
remote no LOCALANDREMOTE sent directly to address host
remote no LOCALONLY error (invalid recipient, rc=65)
remote no REMOTEONLY error (SMARTHOST required, rc=78)
remote yes LOCALANDREMOTE sent to SMARTHOST
remote yes LOCALONLY error (invalid recipient, rc=65)
remote yes REMOTEONLY sent to SMARTHOST

I used a script to test the various cases, which you might find useful for that purpose: dma-delivery-test.sh.txt

This allows dma to support three modes of delivery: local-only,
remote-only, and local + remote (the default). It supersedes the
NULLCLIENT directive, which is equivalent to the remote-only mode.

Fixes: corecode#121
@corecode
Copy link
Owner

I'm a bit puzzled by: remote address + REMOTEONLY will fail without SMARTHOST. Feels to violate POLA. Maybe we should call it SMARTHOSTONLY?

@iskunk
Copy link
Author

iskunk commented May 4, 2023

Note that the REMOTEONLY behavior is the same as NULLCLIENT (in fact, the latter is now synonymous with the former, for the sake of backward compatibility), and the failure when specifying REMOTEONLY without SMARTHOST occurs on any invocation of dma.

Renaming it to SMARTHOSTONLY would complicate the current [mental] model of "local" and "remote" effectively being checkboxes, such that you can choose one, or the other, or both (but not neither), hence the LOCALANDREMOTE option. And the direct-delivery use case is, IMO, fairly niche---in practical terms, it is only usable with private mail servers, and if you have one of those, then a suitable smarthost is usually part of the setup.

@corecode
Copy link
Owner

corecode commented May 4, 2023

I see. Still REMOTEONLY sounds like it should be able to deliver to remote addresses, which isn't the case - it can only deliver to a smarthost. I think the patch itself is great, but we need to figure out a meaningful name that won't confuse.

@iskunk
Copy link
Author

iskunk commented May 4, 2023

If we went with SMARTHOSTONLY, then the other two keywords would presumably be LOCALONLY and LOCALANDSMARTHOST. But then LOCALANDSMARTHOST without SMARTHOST would be, on its face, an invalid config.

If you had LOCALANDREMOTE, LOCALONLY, and SMARTHOSTONLY, then users might wonder why there is no REMOTEONLY option.

We could fix the delivery code to allow REMOTEONLY to work without SMARTHOST, thus allowing the "remote" descriptor to apply to both smarthost and direct delivery mechanisms. But that's a bit beyond the scope of this PR.

Or instead of having a DELIVERY <option> parameter, we could have three parameters to enable/disable the three different kinds of delivery, e.g.

# Uncomment to enable local delivery
DELIVERLOCAL

# Uncomment to enable direct delivery to remote mail servers
# (ignored if DELIVERSMARTHOST is set)
DELIVERDIRECT

# Uncomment to relay messages to SMARTHOST
DELIVERSMARTHOST

This would also require some changes to the delivery code, however.

@tedkotz
Copy link

tedkotz commented Sep 3, 2023

Doesn't having a SMARTHOST configured imply DELIVERSMARTHOST?

And I think it might make more sense to invert the logic on the other 2: NOLOCALDELIVERY and NODIRECTDELIVERY.

This way "everything off" is still "normal" behavior and it doesn't break peoples configs on upgrade.

Then:

  1. a NULLCLIENT is just a synonym for NOLOCALDELIVERY with a SMARTHOST defined.
  2. a LOCALONLY Support for local-only delivery? #62 would be a synonym for NODIRECTDELIVERY without a SMARTHOST defined.
  3. a SMARTHOST definition would imply NODIRECTDELIVERY

@@ -255,8 +254,10 @@ add_recp(struct queue *queue, const char *str, int expand)
endpwent();
}
}
} else {
} else if (config.features & DELIVERY_REMOTE) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you made this check down in deliver() where deliver_remote is called you can use bounce on the email which might be better behavior if local delivery is allowed. Of course if both local and remote delivery are disabled we probably want to catch that at configuration time.

@iskunk
Copy link
Author

iskunk commented Sep 11, 2023

Doesn't having a SMARTHOST configured imply DELIVERSMARTHOST?

Yes, I'm not keen on that semantic redundancy.

And I think it might make more sense to invert the logic on the other 2: NOLOCALDELIVERY and NODIRECTDELIVERY.

That seems like a reasonable approach. Between those two keywords and SMARTHOST, the invalid combinations would be...

  • no SMARTHOST + NOLOCALDELIVERY + NODIRECTDELIVERY (nowhere for mail to go)
  • no SMARTHOST + NOLOCALDELIVERY + local address (can't direct-deliver without a host)
  • no SMARTHOST + NODIRECTDELIVERY + remote address (no way to reach remote destination)

I think that's everything... please comment if you see other examples. (SMARTHOST + NOLOCALDELIVERY + local address is a borderline case, depending on how the smarthost qualifies the address.)

For brevity, I would suggest shortening the new keywords to NOLOCAL and NODIRECT. IMO, the DELIVERY suffix doesn't help much, and the shorter forms fit in better with the existing keywords.

2. a LOCALONLY (#62) would be a synonym for NODIRECTDELIVERY without a SMARTHOST defined.

Note that LOCALONLY exists only in PR-space, so there is no need to support it. NULLCLIENT is a different story, of course.

@corecode, what are your thoughts on this? I think we're all in agreement on what we want to make possible in dma; it is just a matter of articulating the configuration for it in as clear a manner as possible.

@corecode
Copy link
Owner

And I think it might make more sense to invert the logic on the other 2: NOLOCALDELIVERY and NODIRECTDELIVERY.

That seems like a reasonable approach. Between those two keywords and SMARTHOST, the invalid combinations would be...

I keep getting confused, it's not obvious how things work.

If I only set SMARTHOST, shouldn't mail only get delivered to the smarthost (I think that's how e.g. postfix does it)?

So NOLOCAL and NODIRECT are implied if I set SMARTHOST?

Can we do something like:

DELIVERY smarthost.example.org   # only use smarthost
DELIVERY LOCAL # only deliver locally
DELIVERY MX # only deliver to MX, no local
DELIVERY ALL # deliver to local or MX

@corecode, what are your thoughts on this? I think we're all in agreement on what we want to make possible in dma; it is just a matter of articulating the configuration for it in as clear a manner as possible.

Absolutely. What do you think about my suggestion? Seems like we had this before, but it also feels new?

@iskunk
Copy link
Author

iskunk commented Sep 11, 2023

If I only set SMARTHOST, shouldn't mail only get delivered to the smarthost (I think that's how e.g. postfix does it)?

So NOLOCAL and NODIRECT are implied if I set SMARTHOST?

Not necessarily. There are [at least] two exception cases when SMARTHOST is set and the other two keywords aren't:

  • A local-only address (i.e. no @domain part): This should be delivered locally. It is possible for a smarthost to interpret such an address, but that is unusual, and likely to fail. (It certainly failed in my big org's mail server, despite there being an obvious domain that could have been inferred.)
  • Dead smarthost + remote address: dma could fall back to direct/MX delivery.

Can we do something like:

That wouldn't cover smarthost + local, or local + MX (both reasonable scenarios), and ALL doesn't really mean "all" as it refers to only two of the three cases.

@corecode
Copy link
Owner

  • A local-only address (i.e. no @domain part): This should be delivered locally. It is possible for a smarthost to interpret such an address, but that is unusual, and likely to fail. (It certainly failed in my big org's mail server, despite there being an obvious domain that could have been inferred.)

How do you send mail from a MUA? Usually delivery to a smarthost is configured in a way to accept all locally originating mails, and the smarthost does masquerading, etc.

  • Dead smarthost + remote address: dma could fall back to direct/MX delivery.

No, that makes no sense. Smarthost is used when you never want to do direct MX delivery.

That wouldn't cover smarthost + local, or local + MX (both reasonable scenarios), and ALL doesn't really mean "all" as it refers to only two of the three cases.

Maybe LOCAL,MX is better than ALL?

then we could also do LOCAL,myhost.example.org.

@iskunk
Copy link
Author

iskunk commented Sep 11, 2023

How do you send mail from a MUA? Usually delivery to a smarthost is configured in a way to accept all locally originating mails, and the smarthost does masquerading, etc.

Traditionally, MUAs would invoke /usr/sbin/sendmail to send mail, on a system with a full-featured MTA+MDA. While most mail would be queued for remote delivery, you could have local recipients, too. Consider the following two mailx examples:

echo hello | mail -s "Greetings" bob

echo hello | mail -s "Greetings" [email protected]

Typically, the first message would never leave the system. Why would it? You're already at the destination.

That said, if you do want even local addresses to go to the smarthost, this can be specified with the NOLOCAL keyword. So both cases are covered.

No, that makes no sense. Smarthost is used when you never want to do direct MX delivery.

It's conceivable to have MX fallback as a resiliency measure (which can be opted out of with NODIRECT). But I have no interest in MX mode myself, as it breaks SPF/DKIM/everything.

Maybe LOCAL,MX is better than ALL?

then we could also do LOCAL,myhost.example.org.

LOCAL,MX is better, but the comma would be new to the dma config syntax. Also, I'm not keen on the approach of specifying a hostname where a keyword might go, since then you can run into "but my smarthost's hostname is 'MX' actually" situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants