[pmg-devel] [PATCH pmg-api 1/1] fix #2504: do not overwrite existing selector key

[Dominik Csapak]

comments inline

On 12/5/19 2:01 PM, Stoiko Ivanov wrote:
> This patch changes the behavior of DKIM selector creation. Instead of blindly
> overwriting an already present file, add a force parameter to overwrite it (and
> behave like the current code).
> 
> Overwriting an existing selector can potentially be quite destructive (e.g.
> a setup where the admin has already posted the DNS-record for one selector to
> many domains, then wants to quickly experiment with a larger keysize, and tries
> to go back to the existing behavior).
> 
> The new behavior without force set to true, when a private key for the selector
> already exists is to die if the file is either not a private RSA key, or has
> the wrong size, or else just set the selector in pmg.conf
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/PMG/API2/DKIMSign.pm |  7 ++++++-
>   src/PMG/DKIMSign.pm      | 23 ++++++++++++++++++-----
>   2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PMG/API2/DKIMSign.pm b/src/PMG/API2/DKIMSign.pm
> index 8e2d85b..9aad2e2 100644
> --- a/src/PMG/API2/DKIMSign.pm
> +++ b/src/PMG/API2/DKIMSign.pm
> @@ -65,6 +65,10 @@ __PACKAGE__->register_method({
>   		description => "Number of bits for the RSA-Key",
>   		type => 'integer', minimum => 1024
>   	    },
> +	    force => {
> +		description => "Overwrite existing key",
> +		type => 'boolean', optional => 1, default => 1

since the commit says it is by default off, and this
is only cosmetic for the api, i guess the 'default => 1' was
left by mistake

> +	    },
>   	},
>       },
>       returns => { type => 'null' },
> @@ -72,8 +76,9 @@ __PACKAGE__->register_method({
>   	my ($param) = @_;
>   	my $selector = extract_param($param, 'selector');
>   	my $keysize = extract_param($param, 'keysize');
> +	my $force = extract_param($param, 'force');
>   
> -	PMG::DKIMSign::set_selector($selector, $keysize);
> +	PMG::DKIMSign::set_selector($selector, $keysize, $force);
>   
>   	return undef;
>       }});
> diff --git a/src/PMG/DKIMSign.pm b/src/PMG/DKIMSign.pm
> index 5810cea..094e0d9 100644
> --- a/src/PMG/DKIMSign.pm
> +++ b/src/PMG/DKIMSign.pm
> @@ -157,22 +157,35 @@ sub get_selector_info {
>   }
>   
>   sub set_selector {
> -    my ($selector, $keysize) = @_;
> +    my ($selector, $keysize, $force) = @_;
>   
>       die "no selector provided\n" if !defined($selector);
>       die "no keysize provided\n" if !defined($keysize);
>       die "invalid keysize\n" if ($keysize < 1024);
>       my $privkey_file = "/etc/pmg/dkim/$selector.private";
>   
> -    my $code = sub{
> -	my $cmd = ['openssl', 'genrsa', '-out', $privkey_file, $keysize];
> -	PMG::Utils::run_silent_cmd($cmd);
> +    my $code = sub {
> +	my $genkey = $force || (! -e $privkey_file);
> +	if (!$genkey) {
> +	    my ($privkeytext, $privkey, $cursize);
> +	    eval {
> +		$privkeytext = PVE::Tools::file_get_contents($privkey_file);
> +		$privkey =  Crypt::OpenSSL::RSA->new_private_key($privkeytext);
> +		$cursize = $privkey->size() * 8;
> +	    };
> +	    die "$privkey_file exists but is not a valid key.\n" if $@;

nit: i guess file_get_contents can die too? so the error message would 
be confusing, maybe a more generic error would be better?
'error checking $privkey_file: $@' ?

also, you do not use the privkeytext outside the eval, so this could be 
moved in there

> +	    die "$privkey_file already exists, but has different size ($cursize bits)\n"
> +		if $cursize != $keysize;
> +	} else {
> +	    my $cmd = ['openssl', 'genrsa', '-out', $privkey_file, $keysize];
> +	    PMG::Utils::run_silent_cmd($cmd);
> +	}
>   	my $cfg = PMG::Config->new();
>   	$cfg->set('admin', 'dkim_selector', $selector);
>   	$cfg->write();
>   	PMG::Utils::reload_smtp_filter();
>       };
>   
> -    PMG::Config::lock_config($code, "unable to generate DKIM key ($selector - $keysize bits)");
> +    PMG::Config::lock_config($code, "unable to set DKIM key ($selector - $keysize bits)");
>   }
>   1;
>