[pmg-devel] [PATCH pmg-api 09/12] add DKIM API paths

[Dominik Csapak]

some comments inline

On 10/7/19 9:28 PM, Stoiko Ivanov wrote:
> A new path is added at /config/dkim with 2 subpaths:
> * /config/dkim/domains gives access to the dkimdomains
> * /config/dkim/selector gives access to the private key for the configured
>    selector
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/Makefile             |   1 +
>   src/PMG/API2/Config.pm   |   7 +++
>   src/PMG/API2/DKIMSign.pm | 125 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 133 insertions(+)
>   create mode 100644 src/PMG/API2/DKIMSign.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index 5cf9d27..a640d8a 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -119,6 +119,7 @@ LIBSOURCES =				\
>   	PMG/API2/DestinationTLSPolicy.pm\
>   	PMG/API2/Domains.pm		\
>   	PMG/API2/DKIMSignDomains.pm	\
> +	PMG/API2/DKIMSign.pm		\
>   	PMG/API2/Fetchmail.pm		\
>   	PMG/API2/Users.pm		\
>   	PMG/API2/Transport.pm		\
> diff --git a/src/PMG/API2/Config.pm b/src/PMG/API2/Config.pm
> index 3b688fa..43653e4 100644
> --- a/src/PMG/API2/Config.pm
> +++ b/src/PMG/API2/Config.pm
> @@ -23,6 +23,7 @@ use PMG::API2::SMTPWhitelist;
>   use PMG::API2::MimeTypes;
>   use PMG::API2::Fetchmail;
>   use PMG::API2::DestinationTLSPolicy;
> +use PMG::API2::DKIMSign;
>   
>   use base qw(PVE::RESTHandler);
>   
> @@ -81,6 +82,11 @@ __PACKAGE__->register_method ({
>       path => 'tlspolicy',
>   });
>   
> +__PACKAGE__->register_method({
> +    subclass => "PMG::API2::DKIMSign",
> +    path => 'dkim',
> +});
> +
>   __PACKAGE__->register_method ({
>       name => 'index',
>       path => '',
> @@ -118,6 +124,7 @@ __PACKAGE__->register_method ({
>   	push @$res, { section => 'whitelist' };
>   	push @$res, { section => 'regextest' };
>   	push @$res, { section => 'tlspolicy' };
> +	push @$res, { section => 'dkim' };
>   
>   	return $res;
>       }});
> diff --git a/src/PMG/API2/DKIMSign.pm b/src/PMG/API2/DKIMSign.pm
> new file mode 100644
> index 0000000..f21f91a
> --- /dev/null
> +++ b/src/PMG/API2/DKIMSign.pm
> @@ -0,0 +1,125 @@
> +package PMG::API2::DKIMSign;

nit: please leave an empty line here (for consistency)
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(extract_param);
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::Exception qw(raise_param_exc);
> +use PVE::RESTHandler;
> +
> +use PMG::Config;
> +use PMG::DKIMSign;
> +
> +use PMG::API2::DKIMSignDomains;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    subclass => "PMG::API2::DKIMSignDomains",
> +    path => 'domains',
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "Directory index.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => { section => { type => 'string'} },
> +	},
> +	links => [ { rel => 'child', href => "{section}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	return [ { section => 'domains'},
> +	    { section => 'selector'} ];

please put every object on its own line, this
way we can add new ones without touching the surround lines

e.g.
return [
  {},
  {},
...
];

> +    }});
> +
> +__PACKAGE__->register_method({
> +    name => 'set_selector',
> +    path => 'selector',
> +    method => 'POST',
> +    description => "Generate a new private key for selector. This will make all future mail be signed with the new key!",

how about: All future mails will be signed with the new key.

altough it would probably better fit as a hint in the gui

> +    protected => 1,
> +    permissions => { check => [ 'admin' ] },
> +    proxyto => 'master',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    selector => {
> +		description => "DKIM Selector",
> +		type => 'string', format => 'dns-name',
> +	    },
> +	    keysize => {
> +		description => "Number of bits for the RSA-Key",
> +		type => 'integer', maximum => 8129, minimum => 1024

should probably be '8192' ?

> +	    },
> +	},
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +	my $selector = extract_param($param, 'selector');
> +	my $keysize = extract_param($param, 'keysize');
> +
> +	eval { PMG::DKIMSign::set_selector($selector, $keysize); };

we do check the keysize there (see my answer to that patch)
but would it not be better to do it here and do a 'raise_param_exc'?
(so it does not get attributed falsely to the 'selector' parameter)

> +
> +	raise_param_exc({ selector => "could not generate private key: $@" }) if $@;
> +
> +	return undef;
> +    }});
> +
> +sub pmg_verify_dkim_pubkey_record {
> +    my ($rec, $noerr) = @_;
> +
> +    if ($rec !~ /\._domainkey\tIN\tTXT\t\( "v=DKIM1; h=sha256; k=rsa; ".+ \)  ; ----- DKIM key/ms ) {
> +	return undef if $noerr;
> +       die "value does not look like a valid DKIM TXT record\n";

indentation is wrong here

> +    }
> +
> +    return $rec
> +}
> +
> +PVE::JSONSchema::register_format(
> +    'pmg-dkim-record', \&pmg_verify_dkim_pubkey_record);
> +
> +__PACKAGE__->register_method({
> +    name => 'get_selector_info',
> +    path => 'selector',
> +    method => 'GET',
> +    description => "Get the public key for the configured selector, prepared as DKIM TXT record",
> +    protected => 1,
> +    permissions => { check => [ 'admin' ] },
> +    proxyto => 'master',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => { },
> +    },
> +    returns => {
> +	type => 'object',
> +	properties => {
> +		selector => { type => 'string', format => 'dns-name' },
> +		keysize => { type => 'integer', maximum => 8129, minimum => 1024 },

8192

> +		record => { type => 'string', format => 'pmg-dkim-record'},

wrong indentation

> +	},
> +    },
> +    code => sub {
> +	my $cfg = PMG::Config->new();
> +	my $selector = $cfg->get('admin', 'dkim_selector');
> +
> +	my ($record, $size);
> +	eval { ($record, $size) = PMG::DKIMSign::get_selector_info($selector); };
> +
> +	raise_param_exc({ selector => "could find key for $selector: $@"}) if $@;

does it work that way if the api call does not even have this parameter?
i would simply die in this case (so no eval)

> +
> +	return { selector => $selector, keysize => $size, record => $record };
> +    }});
> +1;
>