* 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.
[not found] ` <D1H4170KM7OL.7HNFYXHGMV1H@proxmox.com>
@ 2024-05-23 15:06 ` Shannon Sterz
0 siblings, 0 replies; 4+ messages in thread
From: Shannon Sterz @ 2024-05-23 15:06 UTC (permalink / raw)
To: Nigel van Keulen, pmg-devel
On 23.05.24 16:56, Shannon Sterz wrote:
> Hi!
>
> On Wed May 22, 2024 at 2:37 PM CEST, Nigel van Keulen wrote:
>> Hey!
>>
>> Thank you for the feedback, I'll change it as soon as I have time!
>>
>> I didn't want to do too much refactoring as I am not very familiar with
>> perl.
>>
>> 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";
>>
>> The postmaster email here basically gets thrown away - it is split by
>> the @ sign and only the domain gets used from here on.
>> It's basically a simple monkey-patch to still send the domain through DKIM
>> (the sender, postmaster does not get used if I am correct).
>>
>
> seems like it, but in that case you can also simply append an "@" here,
> that works too. however, i don't think either is the "right" way of
> going about this.
>
> it would be cleaner for `DKIMSign::sign_entity` to take a domain and not
> the sender email and for `DKIMSign::signing_domain` to do the same. then
> you could parse the sender here or choose an appropriate fallback.
> however, someone with more knowledge of pmg might object to this, as i
> am not super deep into the codebase either.
>
> im also not sure if `$msginfo->{domain}` is ideal here, from what i can
> tell that is the domain of the pmg host and might not corelate with the
> sender domain, afaict.
>
>> The reason it was labeled OOO:
>> Only out of office messages send their e-mails without the sender (as far
>> as I know of). I'll change the PR title next submission anyways! :)
>> Using the dkim-use-domain I don't think will work; me and a few other
>> colleagues have looked extensively through documentation, it doesn't seem
>> possible.
>
> interesting, does your mail server not provide a "MAIL FROM" and also
> not set a `From:` header for such emails?
>
>> The DKIM signing code just exits with an error when there is no valid
>> sender (and thus it cannot split on @ sign). Changing this to `email@<domain>`
>> seems to be the only solution so far.
>> The returning OOO E-mail does show the correct E-mail adress instead of
>> postmaster. I'll have to look through the supplied entity/object to see
>> what fields are available, possibly the envelopfrom could be used.
>>
>
> well i'd be surprised if that worked as `envelopfrom` should be the
> sender from what i can tell. at least pmg should use the envelope from
> or `MAIL FROM` by default for signing mail via DKIM.
>
>> As for the line-length, so I am allowed to submit messages longer than 70
>> chars if there is a newline in between?
>>
>
> so for git you need to keep your commit messages to certain format. we
> keep all lines in commit messages to 70 chars. you can wrap them,
> meaning, you can insert a newline before the word that would go above
> the 70 char limit.
>
> however, the first line is special, as it is treated as a summary line.
> so there is no wrapping there. your commit message should look something
> like this:
>
> ```
> fix: this is a short summary of what this commit does
>
> this is a longer explainer of the commits changes that goes into
> detail. it is hard wrapped at 70 chars, meaning that no line is
> longer than 70 characters and no words are split.
> ```
>
> you also need to add a signed off by line at the end with the
> appropriate switch. otherwise your commit can not be properly
> attributed. please review the "commits and commit message" of the
> developer documentation [1]
>
> [1]: https://pmg.proxmox.com/wiki/index.php/Developer_Documentation#Commits_and_Commit_Messages
>
>> Sorry for taking your time and thank you for the supplied links and
>> information about licensing.
>> I've submitted the Harmony CLA as of today.
>>
>> Kind regards,
>> Nigel van Keulen
>>
>> Op wo 22 mei 2024 om 14:05 schreef Shannon Sterz <s.sterz@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
>>>
>>>
>
oh, btw. please always use reply-all when replying to mailing list
emails. just noticed that my last mail didn't get send to the list
because of that and that means other members and devs can't know what we
are discussing, which is counterproductive. thanks!
_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
^ permalink raw reply [flat|nested] 4+ messages in thread