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

[Stoiko Ivanov <s.ivanov@proxmox.com>]

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);