Re: [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option

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

On Thu,  9 Mar 2023 11:18:44 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:

> Add a new configuration file /etc/pmg/tls_inbound_policy, which is a
> postfix map containing all domains having `reject_plaintext_session`
> action set, which is then used in smtpd_sender_restriction in the
> main.cf template.
> 
> Also add the accompanying API endpoint for modifying it.
I usually split this out into a patch of its own.

One thing that is missing is adding the new file to the cluster sync (`git
grep tls_policy`).


> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  src/Makefile                     |   1 +
>  src/PMG/API2/Config.pm           |   7 ++
>  src/PMG/API2/InboundTLSPolicy.pm | 127 +++++++++++++++++++++++++++++++
>  src/PMG/Config.pm                |  56 ++++++++++++++
>  src/templates/main.cf.in         |   1 +
>  5 files changed, 192 insertions(+)
>  create mode 100644 src/PMG/API2/InboundTLSPolicy.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index 49c7974..139a4b5 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -130,6 +130,7 @@ LIBSOURCES =				\
>  	PMG/API2/DKIMSignDomains.pm	\
>  	PMG/API2/DKIMSign.pm		\
>  	PMG/API2/Fetchmail.pm		\
> +	PMG/API2/InboundTLSPolicy.pm	\
>  	PMG/API2/Users.pm		\
>  	PMG/API2/Transport.pm		\
>  	PMG/API2/MyNetworks.pm		\
> diff --git a/src/PMG/API2/Config.pm b/src/PMG/API2/Config.pm
> index 37da096..8a8a469 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::InboundTLSPolicy;
>  use PMG::API2::DKIMSign;
>  use PMG::API2::SACustom;
>  use PMG::API2::PBS::Remote;
> @@ -86,6 +87,11 @@ __PACKAGE__->register_method ({
>      path => 'tlspolicy',
>  });
> 
> +__PACKAGE__->register_method ({
> +    subclass => "PMG::API2::InboundTLSPolicy",
> +    path => 'tlsinboundpolicy',
> +});
> +
>  __PACKAGE__->register_method({
>      subclass => "PMG::API2::DKIMSign",
>      path => 'dkim',
> @@ -146,6 +152,7 @@ __PACKAGE__->register_method ({
>  	push @$res, { section => 'ruledb' };
>  	push @$res, { section => 'tfa' };
>  	push @$res, { section => 'tlspolicy' };
> +	push @$res, { section => 'tlsinboundpolicy' };
>  	push @$res, { section => 'transport' };
>  	push @$res, { section => 'users' };
>  	push @$res, { section => 'whitelist' };
> diff --git a/src/PMG/API2/InboundTLSPolicy.pm b/src/PMG/API2/InboundTLSPolicy.pm
> new file mode 100644
> index 0000000..74fb16a
> --- /dev/null
> +++ b/src/PMG/API2/InboundTLSPolicy.pm
> @@ -0,0 +1,127 @@
> +package PMG::API2::InboundTLSPolicy;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::RESTHandler;
> +use PVE::INotify;
> +use PVE::Exception qw(raise_param_exc);
> +
> +use PMG::Config;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => 'List tls_inbound_policy entries.',
> +    proxyto => 'master',
> +    permissions => { check => [ 'admin', 'audit' ] },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => 'string',
> +	    format => 'transport-domain',
> +	},
> +	description => 'List of domains for which TLS will be enforced on incoming connections',
> +	links => [ { rel => 'child', href => '{domain}' } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $res = [];
> +
> +	my $policies = PVE::INotify::read_file('tls_inbound_policy');
> +
> +	foreach my $domain (sort keys %$policies) {
> +	    push @$res, { domain => $domain };
> +	}
> +
> +	return $res;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'create',
> +    path => '',
> +    method => 'POST',
> +    proxyto => 'master',
> +    protected => 1,
> +    permissions => { check => [ 'admin' ] },
> +    description => 'Add new tls_inbound_policy entry.',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    domain => {
> +		type => 'string',
> +		format => 'transport-domain',
> +		description => 'Domain for which TLS should be enforced on incoming connections',
> +	    },
> +	},
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +	my $domain = $param->{domain};
> +
> +	my $code = sub {
> +	    my $policies = PVE::INotify::read_file('tls_inbound_policy');
> +	    raise_param_exc({ domain => "InboundTLSPolicy entry for '$domain' already exists" })
> +		if $policies->{$domain};
> +
> +	    $policies->{$domain} = 1;
> +
> +	    PVE::INotify::write_file('tls_inbound_policy', $policies);
> +	    PMG::Config::postmap_tls_inbound_policy();
> +	};
> +
> +	PMG::Config::lock_config($code, 'adding tls_inbound_policy entry failed');
> +
> +	return undef;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'delete',
> +    path => '{domain}',
> +    method => 'DELETE',
> +    description => 'Delete a tls_inbound_policy entry',
> +    protected => 1,
> +    permissions => { check => [ 'admin' ] },
> +    proxyto => 'master',
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    domain => {
> +		type => 'string',
> +		format => 'transport-domain',
> +		description => 'Domain which should be removed from tls_inbound_policy',
> +	    },
> +	}
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	my ($param) = @_;
> +	my $domain = $param->{domain};
> +
> +	my $code = sub {
> +	    my $policies = PVE::INotify::read_file('tls_inbound_policy');
> +
> +	    raise_param_exc({ domain => "tls_inbound_policy entry for '$domain' does not exist" })
> +		if !$policies->{$domain};
> +
> +	    delete $policies->{$domain};
> +
> +	    PVE::INotify::write_file('tls_inbound_policy', $policies);
> +	    PMG::Config::postmap_tls_inbound_policy();
> +	};
> +
> +	PMG::Config::lock_config($code, 'deleting tls_inbound_policy entry failed');
> +
> +	return undef;
> +    }});
> +
> +1;
> diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
> index a0b1866..45a4b3a 100755
> --- a/src/PMG/Config.pm
> +++ b/src/PMG/Config.pm
> @@ -1154,6 +1154,61 @@ sub postmap_tls_policy {
>      PMG::Utils::run_postmap($tls_policy_map_filename);
>  }
> 
> +sub read_tls_inbound_policy {
> +    my ($filename, $fh) = @_;
> +
> +    return {} if !defined($fh);
> +
> +    my $tls_policy = {};
> +
> +    while (defined(my $line = <$fh>)) {
> +	chomp $line;
> +	next if $line =~ m/^\s*$/;
> +	next if $line =~ m/^#(.*)\s*$/;
> +
> +	my $parse_error = sub {
> +	    my ($err) = @_;
> +	    die "parse error in '$filename': $line - $err";
> +	};
> +
> +	if ($line =~ m/^(\S+)\s+.+\s*$/) {
The matching seems odd - IIRC + is greedy so '.+' above would match
everything anyways - making \s* superfluous?

Why not explicitly match for 'reject_plain_text_session'? - since we write
this literally into the file it should be there.
(erroring out on unexpected content is better than to clobber it and
replace what the users wrote there with 'reject_plaintext_session' upon
any next update (and hopefully motivates the users to not use this
particular file for other unrelated ACL entries))



> +	    my $domain = $1;
> +
> +	    eval { pmg_verify_transport_domain($domain) };
> +	    if (my $err = $@) {
> +		$parse_error->($err);
> +		next;
> +	    }
> +
> +	    $tls_policy->{$domain} = 1;
> +	} else {
> +	    $parse_error->('wrong format');
> +	}
> +    }
> +
> +    return $tls_policy;
> +}
> +
> +sub write_tls_inbound_policy {
> +    my ($filename, $fh, $tls_policy) = @_;
> +
> +    return if !$tls_policy;
> +
> +    foreach my $domain (sort keys %$tls_policy) {
> +	PVE::Tools::safe_print($filename, $fh, "$domain reject_plaintext_session\n");
> +    }
> +}
> +
> +my $tls_inbound_policy_map_filename = "/etc/pmg/tls_inbound_policy";
> +PVE::INotify::register_file('tls_inbound_policy', $tls_inbound_policy_map_filename,
> +			    \&read_tls_inbound_policy,
> +			    \&write_tls_inbound_policy,
> +			    undef, always_call_parser => 1);
> +
> +sub postmap_tls_inbound_policy {
> +    PMG::Utils::run_postmap($tls_inbound_policy_map_filename);
> +}
> +
>  my $transport_map_filename = "/etc/pmg/transport";
> 
>  sub postmap_pmg_transport {
> @@ -1684,6 +1739,7 @@ sub rewrite_config_postfix {
>      postmap_pmg_domains();
>      postmap_pmg_transport();
>      postmap_tls_policy();
> +    postmap_tls_inbound_policy();
> 
>      rewrite_postfix_whitelist($rulecache) if $rulecache;
> 
> diff --git a/src/templates/main.cf.in b/src/templates/main.cf.in
> index 190c913..4905eeb 100644
> --- a/src/templates/main.cf.in
> +++ b/src/templates/main.cf.in
> @@ -79,6 +79,7 @@ smtpd_sender_restrictions =
>          reject_non_fqdn_sender
>          check_client_access     cidr:/etc/postfix/clientaccess
>          check_sender_access     regexp:/etc/postfix/senderaccess
> +        check_sender_access     hash:/etc/pmg/tls_inbound_policy
>          check_recipient_access  regexp:/etc/postfix/rcptaccess
>  [%- IF pmg.mail.rejectunknown %] reject_unknown_client_hostname[% END %]
>  [%- IF pmg.mail.rejectunknownsender %] reject_unknown_sender_domain[% END %]
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
>