From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pmg-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 91B8D1FF396
for <inbox@lore.proxmox.com>; Wed, 22 May 2024 14:05:48 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 069CFB6DB;
Wed, 22 May 2024 14:06:07 +0200 (CEST)
Mime-Version: 1.0
Date: Wed, 22 May 2024 14:05:32 +0200
Message-Id: <D1G5RK3KIX8M.S76SRDH42RGJ@proxmox.com>
From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Nigel van Keulen" <nigel2392@gmail.com>, <pmg-devel@lists.proxmox.com>
X-Mailer: aerc 0.17.0-69-g65571b67d7d3-dirty
References: <20240518145855.670-1-nigel2392@gmail.com>
<20240518145855.670-2-nigel2392@gmail.com>
In-Reply-To: <20240518145855.670-2-nigel2392@gmail.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.066 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [accept.pm, proxmox.com]
Subject: Re: [pmg-devel] SPAM: [PATCH pmg-api 1/1] fix: 2971,
to DKIM signing for OOO messages by using postmaster@<domain> as a
sender if it is not present.
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
<pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pmg-devel-bounces@lists.proxmox.com
Sender: "pmg-devel" <pmg-devel-bounces@lists.proxmox.com>
hey thanks for contributing to PMG!
i just wanted to provide some quick feedback on the code you sent in,
but please make sure to follow our development guidelines and also note
that if you haven't already, you need to sign a harmony CLA [1].
some notes on the commit message:
a) it is to long, we generally enforce a limit of 70 characters per
line. please provide a short summary in the first line and if needed
more context in a second paragraph.
b) i am assumig OOO stands for "out of office", however your patch
applies to all mails as long as they don't have a sender. so this
commit message is somewhat confusing.
On Sat May 18, 2024 at 4:58 PM CEST, Nigel van Keulen wrote:
> ---
> src/PMG/RuleDB/Accept.pm | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/PMG/RuleDB/Accept.pm b/src/PMG/RuleDB/Accept.pm
> index e3e39a7..8a6261f 100644
> --- a/src/PMG/RuleDB/Accept.pm
> +++ b/src/PMG/RuleDB/Accept.pm
> @@ -101,8 +101,16 @@ sub execute {
> PMG::Utils::remove_marks($entity);
>
> if ($dkim->{sign}) {
> +
> + my $mailSender = $msginfo->{sender};
> + if ($mailSender eq '') {
> + my $mailDomain = $msginfo->{domain};
> + $mailSender = "postmaster\@$mailDomain";
i wonder if more or less hardcoding a sender for mails here makes sense.
it might be neater to make this configurable somehow.
i also wonder if this adds much functionality in reality. most users may
be able to just use the `dkim-use-domain` to use the sender from the
envelop instead.
> + syslog('info', "%s: No sender found, using default sender: %s", $queue->{logid}, $mailSender);
note that this line is too long. lines should not be longer than 100
characters. you can wrap this line like so to fit that limit
```pl
syslog(
'info',
"%s: No sender found, using default sender: %s",
$queue->{logid},
$mailSender
);
```
this would be in accordance with our style guide [2]. also note that
you did not correctly indent your code. please follow the indentation
guidelines in the style guide as well.
> + }
> +
> eval {
> - $entity = PMG::DKIMSign::sign_entity($entity, $dkim, $msginfo->{sender});
> + $entity = PMG::DKIMSign::sign_entity($entity, $dkim, $mailSender);
> };
> if ($@) {
> syslog('warning',
[1]: https://pmg.proxmox.com/wiki/index.php/Developer_Documentation#Software_License_and_Copyright
[2]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings
_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel