@rombobeorn I would very much like your input in this PR.

I've decided to announce rejection if the sender email is invalid but also if the destination email is. To be honest, I'm not entirely sure about the later, maybe we could announce only invalid "sender" email and keep silently drop the others (I would not want to leak information giving a way to someone, patient, to brute force the system).

Let's not announce invalid destination by default, though I can see some people wanting to be able to do that.

If you don't want to make this configurable, I'd just drop announcing invalid destinations.

If you don't want to make this configurable, I'd just drop announcing invalid destinations.

Not sure it's worth.

Let's wait on @rombobeorn if there are more comments, but otherwise, I'll revert that section.

Pierre-YvesChibon wrote:

I've decided to announce rejection if the sender email is invalid

This part looks good.

but also if the destination email is.

I'm confused about this part. If an address doesn't exist then the MTA
will reject it, so there's no need for a milter to do that. Surely a
server where Pagure runs can also receive mail to other addresses? The
"pagure" address may receive bounce messages, and it wouldn't be smart
to reject those as invalid. Other addresses may also exist, for example
those specified by RFC 2142.

Normally a milter will see all messages, and if it's only interested in
a certain category of messages then it should refrain from touching any
others. That's what the test for "reply+" seems to be for in the
current version. It stops processing the message if "reply" isn't among
the recipients, telling Postfix to continue normally. (It could be done
more efficiently though, by setting a flag if PagureMilter.envrcpt sees
"reply" and returning ACCEPT from PagureMilter.data if the flag is
absent.)

Or does this milter somehow only receive mail addressed to "reply"?
Then the test for "reply+" would only be meant to check that the plus
sign is present, but there seems to be little need for that.

It would make more sense to return REJECT from the tohash check, from
handle_ticket_email and handle_request_email, and from the final else
clause =E2=80=93 and also if "In-Reply-To" is missing, but only after it has
been verified that the message is addressed to "reply".

Anyway, where did the status code 510 come from? x1z codes are
specified as "replies to requests for information, such as status or
help".

It would make more sense to return REJECT from the tohash check, from
handle_ticket_email and handle_request_email, and from the final else
clause =E2=80=93 and also if "In-Reply-To" is missing, but only after it has
been verified that the message is addressed to "reply".

I'll drop the change for the destination email and adjust the REJECT here.

Anyway, where did the status code 510 come from? x1z codes are
specified as "replies to requests for information, such as status or
help".

I took it from https://en.wikipedia.org/wiki/List_of_SMTP_server_return_codes#5xx_Permanent_errors

There is one thing that I'm not entirely sure is: what is the behaviour of postfix when we return REJECT? Does it discard the message entirely? Or set it to be re-processed later?

Pierre-YvesChibon wrote:

I took it from https://en.wikipedia.org/wiki/List_of_SMTP_server_return_c=
odes#5xx_Permanent_errors

Wow, what a crappy article. It lacks references for all the codes it
lists that aren't in RFC 5321, and links to several irrelevant HTTP
resources.

what is the behaviour of postfix when we return REJECT? Does it discard t=
he message entirely? Or set it to be re-processed later?

It refuses the message (or recipient, or connection) with a permanent
failure code (5yz). The client MTA is then supposed to inform the
sender that the message couldn't be delivered.

If there is a transient problem =E2=80=93 for example if handle_ticket_email
fails to contact the web server =E2=80=93 then you can return TEMPFAIL to s=
end
a transient failure code (4yz) to the client. The client MTA (not your
local Postfix) is then supposed to try again later.

See /usr/share/doc/sendmail-milter-devel/api.html and smfi_setreply.html
in the package sendmail-milter-devel.

return TEMPFAIL to send a transient failure code (4yz) to the client. The client MTA (not your
local Postfix) is then supposed to try again later.

Looking at the code server side, I wonder if we want to do this as the server may return error code in a number of (valid) cases, for example if the PR was deleted from the DB.
I'm thinking that REJECT may be cleaner.

It would be best if the milter could distinguish between transient and
permanent errors from DNS, web and database requests. If it's uncertain
whether an error is permanent, then it's probably best to treat it as
permanent so the sender gets an error message quickly.

Thanks for all your help @rombobeorn, much appreciated!

I think this is ready for review now :)

:thumbsup:

Since this code isn't actually covered in the tests and 2 of the 3 environments passed, I'm going to merge it now.

Thanks for the reviews! :)