From 5892a3110140ad28f6ba9e4ccf33c005974d8c70 Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:09 +0000 Subject: [PATCH 1/7] Send appropriate SMTP status codes and error messages. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index 3627d70..ada670d 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -158,6 +158,11 @@ class PagureMilter(Milter.Base): msg_id = msg.get("In-Reply-To", None) if msg_id is None: self.log("No In-Reply-To, can't process this message.") + self.setreply( + "554", + xcode="5.5.0", + msg="Replies to Pagure must have an In-Reply-To header field." + ) return Milter.REJECT # Ensure we don't get extra lines in the message-id @@ -203,6 +208,9 @@ class PagureMilter(Milter.Base): self.log("tohash: %s" % tohash) self.log("Hash does not correspond to the destination") session.remove() + self.setreply("550", + xcode="5.7.1", + msg="Reply authentication failed.") return Milter.REJECT msg_id = clean_item(msg_id) @@ -249,6 +257,11 @@ class PagureMilter(Milter.Base): self.log("Could not add the comment to ticket to pagure") self.log(req.text) + self.setreply("554", + xcode="5.3.0", + msg=("The comment couldn't be added to the issue. " + + "HTTP status: %d %s." % + (req.status_code, req.reason))) return Milter.REJECT def handle_request_email(self, emailobj, msg_id): @@ -280,6 +293,11 @@ class PagureMilter(Milter.Base): self.log("Could not add the comment to PR to pagure") self.log(req.text) + self.setreply("554", + xcode="5.3.0", + msg=("The comment couldn't be added to the pull " + + "request. HTTP status: %d %s." % + (req.status_code, req.reason))) return Milter.REJECT From afa1b66d47768a25a1924b073797529f9a8e112e Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:09 +0000 Subject: [PATCH 2/7] Added exception handling for HTTP requests. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index ada670d..cedd586 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -215,18 +215,34 @@ class PagureMilter(Milter.Base): msg_id = clean_item(msg_id) - if msg_id and "-ticket-" in msg_id: - self.log("Processing issue") - session.remove() - return self.handle_ticket_email(msg, msg_id) - elif msg_id and "-pull-request-" in msg_id: - self.log("Processing pull-request") - session.remove() - return self.handle_request_email(msg, msg_id) - else: - self.log("Not a pagure ticket or pull-request email, let it go") - session.remove() - return Milter.CONTINUE + try: + if msg_id and "-ticket-" in msg_id: + self.log("Processing issue") + session.remove() + return self.handle_ticket_email(msg, msg_id) + elif msg_id and "-pull-request-" in msg_id: + self.log("Processing pull-request") + session.remove() + return self.handle_request_email(msg, msg_id) + else: + self.log("Not a pagure ticket or pull-request email, let it go") + session.remove() + return Milter.CONTINUE + except requests.ReadTimeout as e: + self.setreply("451", + xcode="4.4.2", + msg="The comment couldn't be added: "+str(e)) + return Milter.TEMPFAIL + except requests.ConnectionError as e: + self.setreply("451", + xcode="4.4.1", + msg="The comment couldn't be added: "+str(e)) + return Milter.TEMPFAIL + except requests.RequestException as e: + self.setreply("554", + xcode="5.3.0", + msg="The comment couldn't be added: "+str(e)) + return Milter.REJECT def handle_ticket_email(self, emailobj, msg_id): """ Add the email as a comment on a ticket. """ From bab29b616bd920e4e15644591b04396f145f3d1d Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:10 +0000 Subject: [PATCH 3/7] coding style --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index cedd586..17e4354 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -208,9 +208,9 @@ class PagureMilter(Milter.Base): self.log("tohash: %s" % tohash) self.log("Hash does not correspond to the destination") session.remove() - self.setreply("550", - xcode="5.7.1", - msg="Reply authentication failed.") + self.setreply( + "550", xcode="5.7.1", msg="Reply authentication failed." + ) return Milter.REJECT msg_id = clean_item(msg_id) @@ -225,23 +225,31 @@ class PagureMilter(Milter.Base): session.remove() return self.handle_request_email(msg, msg_id) else: - self.log("Not a pagure ticket or pull-request email, let it go") + self.log( + "Not a pagure ticket or pull-request email, let it go" + ) session.remove() return Milter.CONTINUE except requests.ReadTimeout as e: - self.setreply("451", - xcode="4.4.2", - msg="The comment couldn't be added: "+str(e)) + self.setreply( + "451", + xcode="4.4.2", + msg="The comment couldn't be added: " + str(e) + ) return Milter.TEMPFAIL except requests.ConnectionError as e: - self.setreply("451", - xcode="4.4.1", - msg="The comment couldn't be added: "+str(e)) + self.setreply( + "451", + xcode="4.4.1", + msg="The comment couldn't be added: " + str(e) + ) return Milter.TEMPFAIL except requests.RequestException as e: - self.setreply("554", - xcode="5.3.0", - msg="The comment couldn't be added: "+str(e)) + self.setreply( + "554", + xcode="5.3.0", + msg="The comment couldn't be added: " + str(e) + ) return Milter.REJECT def handle_ticket_email(self, emailobj, msg_id): @@ -273,11 +281,14 @@ class PagureMilter(Milter.Base): self.log("Could not add the comment to ticket to pagure") self.log(req.text) - self.setreply("554", - xcode="5.3.0", - msg=("The comment couldn't be added to the issue. " + - "HTTP status: %d %s." % - (req.status_code, req.reason))) + self.setreply( + "554", + xcode="5.3.0", + msg=( + "The comment couldn't be added to the issue. " + + "HTTP status: %d %s." % (req.status_code, req.reason) + ) + ) return Milter.REJECT def handle_request_email(self, emailobj, msg_id): @@ -309,11 +320,14 @@ class PagureMilter(Milter.Base): self.log("Could not add the comment to PR to pagure") self.log(req.text) - self.setreply("554", - xcode="5.3.0", - msg=("The comment couldn't be added to the pull " + - "request. HTTP status: %d %s." % - (req.status_code, req.reason))) + self.setreply( + "554", + xcode="5.3.0", + msg=( + "The comment couldn't be added to the pull request. " + + "HTTP status: %d %s." % (req.status_code, req.reason) + ) + ) return Milter.REJECT From 94be235f4f98fafb10947964cbe11d51fa44f8ff Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:10 +0000 Subject: [PATCH 4/7] Report an error if a message ID isn't recognized. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index 17e4354..a01e47e 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -225,11 +225,20 @@ class PagureMilter(Milter.Base): session.remove() return self.handle_request_email(msg, msg_id) else: + # msg_id passed the hash check, and yet wasn't recognized as + # a message ID generated by Pagure. This is probably a bug, + # because it should be impossible unless an attacker has + # acquired the secret "salt" or broken the hash algorithm. self.log( - "Not a pagure ticket or pull-request email, let it go" + "Not a pagure ticket or pull-request email, rejecting it." ) session.remove() - return Milter.CONTINUE + self.setreply( + "554", + xcode="5.3.5", + msg="Pagure couldn't determine how to handle the message." + ) + return Milter.REJECT except requests.ReadTimeout as e: self.setreply( "451", From 086803a33e538602a0df2e4a7f5ecbd432ba372a Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:10 +0000 Subject: [PATCH 5/7] Drop messages after delivering them. Given that email replies aren't processed further after the milter has delivered them to the web server, the MTA doesn't need to deliver them to any mailbox, so the milter should tell the MTA to discard them. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index a01e47e..86f177b 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -286,7 +286,9 @@ class PagureMilter(Milter.Base): req = requests.put(url, data=data) if req.status_code == 200: self.log("Comment added") - return Milter.ACCEPT + # The message is now effectively delivered. Tell the MTA to accept + # and discard it. + return Milter.DROP self.log("Could not add the comment to ticket to pagure") self.log(req.text) @@ -325,7 +327,9 @@ class PagureMilter(Milter.Base): req = requests.put(url, data=data) if req.status_code == 200: self.log("Comment added on PR") - return Milter.ACCEPT + # The message is now effectively delivered. Tell the MTA to accept + # and discard it. + return Milter.DROP self.log("Could not add the comment to PR to pagure") self.log(req.text) From 7aa7c1f781f45abbb51cfa0b0a03d8f08c98999d Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:10 +0000 Subject: [PATCH 6/7] Don't return CONTINUE from eom. Because eom is the last callback function called for an email message, returning CONTINUE effectively means ACCEPT. Returning ACCEPT explicitly makes the meaning clearer. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index 86f177b..3ea0171 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -149,7 +149,7 @@ class PagureMilter(Milter.Base): self.log( "No valid recipient email found in To/Cc: %s" % email_address ) - return Milter.CONTINUE + return Milter.ACCEPT if msg["From"] and msg["From"] == _config.get("FROM_EMAIL"): self.log("Let's not process the email we send") From 08fbe3d66f420cddbd59ed3c7603c46f60a50a5b Mon Sep 17 00:00:00 2001 From: Björn Persson Date: Sep 16 2020 10:14:10 +0000 Subject: [PATCH 7/7] Expanded comments. --- diff --git a/pagure-milters/comment_email_milter.py b/pagure-milters/comment_email_milter.py index 3ea0171..ed06758 100644 --- a/pagure-milters/comment_email_milter.py +++ b/pagure-milters/comment_email_milter.py @@ -288,6 +288,9 @@ class PagureMilter(Milter.Base): self.log("Comment added") # The message is now effectively delivered. Tell the MTA to accept # and discard it. + # If you want the message to be processed by another milter after + # this one, or delivered to a mailbox the usual way, then change + # DROP to ACCEPT. return Milter.DROP self.log("Could not add the comment to ticket to pagure") self.log(req.text) @@ -329,6 +332,9 @@ class PagureMilter(Milter.Base): self.log("Comment added on PR") # The message is now effectively delivered. Tell the MTA to accept # and discard it. + # If you want the message to be processed by another milter after + # this one, or delivered to a mailbox the usual way, then change + # DROP to ACCEPT. return Milter.DROP self.log("Could not add the comment to PR to pagure") self.log(req.text)