[pmg-devel] [PATCH pmg-api 0/1] DKIM, first check for exact domain match

[pmg-devel] [PATCH pmg-api 0/1] DKIM, first check for exact domain match

[Daniel Berteaud]

When selecting the sending domain for the DKIM signature, we should first check for an exact match. If none is found, look for parent domains. This fixes the case where wrong signing domain can be added if sign_all is disabled and we sign both a parent and a child domain.

This fixes #3098

Daniel Berteaud (1):
  [pmg-api]: fix #3098 : first check for exact domain match

 src/PMG/DKIMSign.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.26.2

[pmg-devel] [PATCH pmg-api 1/1] [pmg-api]: fix #3098 : first check for exact domain match

[Daniel Berteaud]

When selecting the sending domain for the DKIM signature, we should first check for an exact match. If none is found, look for parent domains. This fixes the case where wrong signing domain can be added if sign_all is disabled and we sign both a parent and a child domain.

Signed-off-by: Daniel Berteaud <daniel@firewall-services.com>
---
 src/PMG/DKIMSign.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PMG/DKIMSign.pm b/src/PMG/DKIMSign.pm
index 7cb06a6..8fd9eed 100644
--- a/src/PMG/DKIMSign.pm
+++ b/src/PMG/DKIMSign.pm
@@ -69,6 +69,14 @@ sub signing_domain {
     my $dkimdomains = PVE::INotify::read_file('dkimdomains');
     $dkimdomains = PVE::INotify::read_file('domains') if !scalar(%$dkimdomains);
 
+    # First check for an exact match in the domain list
+    foreach my $domain (sort keys %$dkimdomains) {
+	if ( $input_domain eq $domain ) {
+	    $self->domain($domain);
+	    return 1;
+	}
+    }
+    # If no exact match is found, check for parent/child domains
     foreach my $domain (sort keys %$dkimdomains) {
 	if ( $input_domain =~ /\Q$domain\E$/i ) {
 	    $self->domain($domain);
-- 
2.26.2

Re: [pmg-devel] [PATCH pmg-api 1/1] [pmg-api]: fix #3098 : first check for exact domain match

[Stoiko Ivanov]

Hi,

Thank you very much for catching this, opening a report and directly
providing a patch!

The patch works as advertised (gave it a quick spin on my test-setup).

However I wonder if it wouldn't be better to address this by sorting
properly - instead of differentiating between exact match and first
matching subdomain we could match for the longest matching subdomain
(suggestion inline).
This should cover your issue as well as far as I can tell, and provide
fewer suprises regarding the chosen signing domain.

What do you think?


On Mon, 26 Oct 2020 11:50:46 +0100
Daniel Berteaud <daniel@firewall-services.com> wrote:

> When selecting the sending domain for the DKIM signature, we should first check for an exact match. If none is found, look for parent domains. This fixes the case where wrong signing domain can be added if sign_all is disabled and we sign both a parent and a child domain.
> 
> Signed-off-by: Daniel Berteaud <daniel@firewall-services.com>
> ---
>  src/PMG/DKIMSign.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/PMG/DKIMSign.pm b/src/PMG/DKIMSign.pm
> index 7cb06a6..8fd9eed 100644
> --- a/src/PMG/DKIMSign.pm
> +++ b/src/PMG/DKIMSign.pm
> @@ -69,6 +69,14 @@ sub signing_domain {
>      my $dkimdomains = PVE::INotify::read_file('dkimdomains');
>      $dkimdomains = PVE::INotify::read_file('domains') if !scalar(%$dkimdomains);
>  
> +    # First check for an exact match in the domain list
> +    foreach my $domain (sort keys %$dkimdomains) {
> +	if ( $input_domain eq $domain ) {
> +	    $self->domain($domain);
> +	    return 1;
> +	}
> +    }
> +    # If no exact match is found, check for parent/child domains
>      foreach my $domain (sort keys %$dkimdomains) {
by replacing `sort keys %$dkimdomains` with:
`sort { length($b) <=> length($a) || $a cmp $b} keys %$dkimdomains`
(from the top of my head - without thorough tests) - sort longest domains
first and if the length is equal sort lexically



>  	if ( $input_domain =~ /\Q$domain\E$/i ) {
>  	    $self->domain($domain);

Re: [pmg-devel] [PATCH pmg-api 1/1] [pmg-api]: fix #3098 : first check for exact domain match

[Daniel Berteaud]

----- Le 27 Oct 20, à 12:39, Stoiko Ivanov s.ivanov@proxmox.com a écrit :

> Hi,
> 
> Thank you very much for catching this, opening a report and directly
> providing a patch!
> 
> The patch works as advertised (gave it a quick spin on my test-setup).
> 
> However I wonder if it wouldn't be better to address this by sorting
> properly - instead of differentiating between exact match and first
> matching subdomain we could match for the longest matching subdomain
> (suggestion inline).
> This should cover your issue as well as far as I can tell, and provide
> fewer suprises regarding the chosen signing domain.
> 
> What do you think?

I'll give your proposal a try and report back (my perl sort-fu is a bit rusty :-) )

Cheers,
Daniel


-- 
[ https://www.firewall-services.com/ ] 	
Daniel Berteaud 
FIREWALL-SERVICES SAS, La sécurité des réseaux 
Société de Services en Logiciels Libres 
Tél : +33.5 56 64 15 32 
Matrix: @dani:fws.fr 
[ https://www.firewall-services.com/ | https://www.firewall-services.com ]