* [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains
@ 2023-03-09 10:18 Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option Christoph Heiss
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Christoph Heiss @ 2023-03-09 10:18 UTC (permalink / raw)
To: pmg-devel
TL;DR: Implements the approach as laid out by Stoiko in the Bugzilla
ticket [0].
A new API endpoint is added - /api2/json/config/tlsinboundpolicy. This
is used to configure the newly introduced postfix map at
/etc/pmg/tls_inbound_policy, specifying sender domains which get the
`reject_plaintext_session` action [1] set, thus requiring TLS-encrypted
sessions on inbound connections.
On the GUI side, a new panel is added in Configuration -> Mail Proxy ->
TLS, where the domains can be specified for this new policy.
The documentation changes are quite terse, maybe I should expand a bit
more on that topic? (Although the TLS destination policy is only lightly
documented as well, as far as I could see.)
Testing
-------
Tested this to the best of my knowledge, by adding some domains as TLS
inbound policy and using `curl` to send some simple mails:
echo '' | curl -skv smtp://<host> -T - \
--mail-from foo@localhost.localdomain \
--mail-rcpt bar@localhost.localdomain
.. where `localhost.localdomain` is on the new 'TLS Inboud Policy' list.
This will now fail with:
450 4.7.1 Session encryption is required
When additionally adding the `--ssl-reqd` option to curl (instructing it
to require a TLS-encrypted session), the above command will succeed.
(Also tested it with a domain not on the list, checking that no
regressions are introduced.)
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=2437
[1] http://www.postfix.org/postconf.5.html#reject_plaintext_session
---
pmg-api:
Christoph Heiss (1):
fix #2437: config: Add inbound TLS policy option
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(+)
pmg-gui:
Christoph Heiss (1):
fix #2437: proxy: Add 'TLS Inbound Policy' panel
js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++++++++++
js/MailProxyTLSPanel.js | 8 +++-
js/Makefile | 1 +
3 files changed, 101 insertions(+), 1 deletion(-)
pmg-docs:
Christoph Heiss (1):
pmgconfig: Explain new TLS inbound policy configuration
pmgconfig.adoc | 8 ++++++++
1 file changed, 8 insertions(+)
--
2.39.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option
2023-03-09 10:18 [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Christoph Heiss
@ 2023-03-09 10:18 ` Christoph Heiss
2023-03-16 12:50 ` Stoiko Ivanov
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel Christoph Heiss
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2023-03-09 10:18 UTC (permalink / raw)
To: pmg-devel
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.
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*$/) {
+ 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel
2023-03-09 10:18 [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option Christoph Heiss
@ 2023-03-09 10:18 ` Christoph Heiss
2023-03-16 12:32 ` Stoiko Ivanov
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-docs 3/3] pmgconfig: Explain new TLS inbound policy configuration Christoph Heiss
2023-03-16 12:28 ` [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Stoiko Ivanov
3 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2023-03-09 10:18 UTC (permalink / raw)
To: pmg-devel
This panel can be used to configure sender domains for which TLS will be
enforced my postfix. As this takes the usual transport domain format,
either a FQDN or .FQDN (for matching subdomains) can be specified.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++
js/MailProxyTLSPanel.js | 8 ++-
js/Makefile | 1 +
3 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 js/MailProxyTLSInboundPolicy.js
diff --git a/js/MailProxyTLSInboundPolicy.js b/js/MailProxyTLSInboundPolicy.js
new file mode 100644
index 0000000..bc45527
--- /dev/null
+++ b/js/MailProxyTLSInboundPolicy.js
@@ -0,0 +1,93 @@
+Ext.define('pmg-tls-inbound-policy', {
+ extend: 'Ext.data.Model',
+ fields: ['domain'],
+ idProperty: 'domain',
+ proxy: {
+ type: 'proxmox',
+ url: '/api2/json/config/tlsinboundpolicy',
+ },
+ sorters: {
+ property: 'domain',
+ direction: 'ASC',
+ },
+});
+
+Ext.define('PMG.TLSInboundPolicyEdit', {
+ extend: 'Proxmox.window.Edit',
+ xtype: 'pmgTLSInboundPolicyEdit',
+ onlineHelp: 'pmgconfig_mailproxy_tls',
+
+ subject: gettext('TLS Inbound Policy'),
+ url: '/api2/extjs/config/tlsinboundpolicy',
+ method: 'POST',
+
+ items: [
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'domain',
+ fieldLabel: gettext('Domain'),
+ },
+ ],
+});
+
+Ext.define('PMG.MailProxyTLSInboundPolicy', {
+ extend: 'Ext.grid.GridPanel',
+ alias: ['widget.pmgMailProxyTLSInboundPolicy'],
+
+ viewConfig: {
+ trackOver: false,
+ },
+
+ columns: [
+ {
+ header: gettext('Domain'),
+ flex: 1,
+ sortable: true,
+ dataIndex: 'domain',
+ },
+ ],
+
+ initComponent: function() {
+ const me = this;
+
+ const rstore = Ext.create('Proxmox.data.UpdateStore', {
+ model: 'pmg-tls-inbound-policy',
+ storeid: 'pmg-mailproxy-tls-inbound-policy-store-' + ++Ext.idSeed,
+ });
+
+ const store = Ext.create('Proxmox.data.DiffStore', { rstore: rstore });
+ const reload = () => rstore.load();
+ me.selModel = Ext.create('Ext.selection.RowModel', {});
+ Proxmox.Utils.monStoreErrors(me, store, true);
+
+ Ext.apply(me, {
+ store,
+ tbar: [
+ {
+ text: gettext('Create'),
+ handler: () => {
+ Ext.createWidget('pmgTLSInboundPolicyEdit', {
+ autoShow: true,
+ listeners: {
+ destroy: reload,
+ },
+ });
+ },
+ },
+ {
+ xtype: 'proxmoxStdRemoveButton',
+ baseurl: '/config/tlsinboundpolicy',
+ callback: reload,
+ waitMsgTarget: me,
+ },
+ ],
+ listeners: {
+ activate: rstore.startUpdate,
+ destroy: rstore.stopUpdate,
+ deactivate: rstore.stopUpdate,
+ },
+ });
+
+ me.callParent();
+ },
+});
diff --git a/js/MailProxyTLSPanel.js b/js/MailProxyTLSPanel.js
index 82dc3f8..5a5837c 100644
--- a/js/MailProxyTLSPanel.js
+++ b/js/MailProxyTLSPanel.js
@@ -26,11 +26,17 @@ Ext.define('PMG.MailProxyTLSPanel', {
flex: 1,
});
- me.items = [tlsSettings, tlsDestinations];
+ const tlsInboundPolicy = Ext.create('PMG.MailProxyTLSInboundPolicy', {
+ title: gettext('TLS Inbound Policy'),
+ flex: 1,
+ });
+
+ me.items = [tlsSettings, tlsDestinations, tlsInboundPolicy];
me.callParent();
tlsSettings.relayEvents(me, ['activate', 'deactivate', 'destroy']);
tlsDestinations.relayEvents(me, ['activate', 'deactivate', 'destroy']);
+ tlsInboundPolicy.relayEvents(me, ['activate', 'deactivate', 'destroy']);
},
});
diff --git a/js/Makefile b/js/Makefile
index 9a2bcf2..e3b9e78 100644
--- a/js/Makefile
+++ b/js/Makefile
@@ -50,6 +50,7 @@ JSSRC= \
MailProxyTLS.js \
MailProxyTLSPanel.js \
MailProxyTLSDestinations.js \
+ MailProxyTLSInboundPolicy.js \
Transport.js \
MyNetworks.js \
RelayDomains.js \
--
2.39.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [pmg-devel] [PATCH pmg-docs 3/3] pmgconfig: Explain new TLS inbound policy configuration
2023-03-09 10:18 [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel Christoph Heiss
@ 2023-03-09 10:18 ` Christoph Heiss
2023-03-16 12:28 ` [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Stoiko Ivanov
3 siblings, 0 replies; 12+ messages in thread
From: Christoph Heiss @ 2023-03-09 10:18 UTC (permalink / raw)
To: pmg-devel
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
pmgconfig.adoc | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/pmgconfig.adoc b/pmgconfig.adoc
index fea26db..22baef3 100644
--- a/pmgconfig.adoc
+++ b/pmgconfig.adoc
@@ -97,6 +97,10 @@ Stores your subscription key and status.
TLS policy for outbound connections.
+`/etc/pmg/tls_inbound_domains`::
+
+TLS policy for inbound connections.
+
`/etc/pmg/transports`::
Message delivery transport setup.
@@ -495,6 +499,10 @@ This can be used if you need to prevent email delivery without
encryption, or to work around a broken 'STARTTLS' ESMTP implementation. See
{postfix_tls_readme} for details on the supported policies.
+Additionally, TLS can also be enforced on incoming connections for specific
+sender domains by creating a TLS inbound policy. Mails with matching domains
+must use encrypted SMTP session, otherwise they are rejected.
+
Enable TLS logging::
To get additional information about SMTP TLS activity, you can enable
--
2.39.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains
2023-03-09 10:18 [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Christoph Heiss
` (2 preceding siblings ...)
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-docs 3/3] pmgconfig: Explain new TLS inbound policy configuration Christoph Heiss
@ 2023-03-16 12:28 ` Stoiko Ivanov
2023-03-20 8:14 ` Christoph Heiss
3 siblings, 1 reply; 12+ messages in thread
From: Stoiko Ivanov @ 2023-03-16 12:28 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
Thanks for tackling this!!
Works as advertised in general - so I think the approach is fine
One thing that seems odd to me (and sadly I don't have a good and short
answer) is that it's not a policy we're setting - it's a list of domains,
for which the singular policy (in this context) is that we accept mails
from them only via tls...
Inbound TLS Domains ? (at least mentions TLS, and explicitly mentioning
domains might be hint enough that you cannot enter an IP (or network)
there)
Reject Plaintext Domains? (probably only appeals to users who know
`postconf(5)` by heart)
some comments inline (some as reply to the individual patches):
On Thu, 9 Mar 2023 11:18:43 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:
> TL;DR: Implements the approach as laid out by Stoiko in the Bugzilla
> ticket [0].
>
> A new API endpoint is added - /api2/json/config/tlsinboundpolicy. This
> is used to configure the newly introduced postfix map at
> /etc/pmg/tls_inbound_policy, specifying sender domains which get the
> `reject_plaintext_session` action [1] set, thus requiring TLS-encrypted
> sessions on inbound connections.
>
> On the GUI side, a new panel is added in Configuration -> Mail Proxy ->
> TLS, where the domains can be specified for this new policy.
>
> The documentation changes are quite terse, maybe I should expand a bit
> more on that topic? (Although the TLS destination policy is only lightly
> documented as well, as far as I could see.)
I personally am fine with terse documentation - however I always try to
refer to the authoritative source - in this case the relevant postfix
config parameter ([1]) - that way users who want to get more details
actually see what's going on under the hood).
You could rephrase the docs to mention that it sets
reject_plaintext_session for those domains during MAIL FROM)
>
> Testing
> -------
> Tested this to the best of my knowledge, by adding some domains as TLS
> inbound policy and using `curl` to send some simple mails:
>
> echo '' | curl -skv smtp://<host> -T - \
> --mail-from foo@localhost.localdomain \
> --mail-rcpt bar@localhost.localdomain
>
> .. where `localhost.localdomain` is on the new 'TLS Inboud Policy' list.
> This will now fail with:
I use swaks (apt installable) quite extensively for such things - short of
smtputf8 support it should cover most use-cases - but thanks for the tip
with curl being able to speak smtp as well :)
>
> 450 4.7.1 Session encryption is required
>
> When additionally adding the `--ssl-reqd` option to curl (instructing it
> to require a TLS-encrypted session), the above command will succeed.
>
> (Also tested it with a domain not on the list, checking that no
> regressions are introduced.)
>
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=2437
> [1] http://www.postfix.org/postconf.5.html#reject_plaintext_session
>
> ---
> pmg-api:
>
> Christoph Heiss (1):
> fix #2437: config: Add inbound TLS policy option
>
> 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(+)
>
> pmg-gui:
>
> Christoph Heiss (1):
> fix #2437: proxy: Add 'TLS Inbound Policy' panel
>
> js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++++++++++
> js/MailProxyTLSPanel.js | 8 +++-
> js/Makefile | 1 +
> 3 files changed, 101 insertions(+), 1 deletion(-)
>
> pmg-docs:
>
> Christoph Heiss (1):
> pmgconfig: Explain new TLS inbound policy configuration
>
> pmgconfig.adoc | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel Christoph Heiss
@ 2023-03-16 12:32 ` Stoiko Ivanov
2023-03-20 8:36 ` Christoph Heiss
0 siblings, 1 reply; 12+ messages in thread
From: Stoiko Ivanov @ 2023-03-16 12:32 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
only a question/hint - since this is yet another list of domains (with w/o
added value) - any chance to merge this with MailProxyTLSDestinations?
(or DKIMDomains, or Domains)?
OTOH the component is small enough and refactoring can very well happen at
another time as well) - so feel free to leave that out :)
On Thu, 9 Mar 2023 11:18:45 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:
> This panel can be used to configure sender domains for which TLS will be
> enforced my postfix. As this takes the usual transport domain format,
> either a FQDN or .FQDN (for matching subdomains) can be specified.
>
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++
> js/MailProxyTLSPanel.js | 8 ++-
> js/Makefile | 1 +
> 3 files changed, 101 insertions(+), 1 deletion(-)
> create mode 100644 js/MailProxyTLSInboundPolicy.js
>
> diff --git a/js/MailProxyTLSInboundPolicy.js b/js/MailProxyTLSInboundPolicy.js
> new file mode 100644
> index 0000000..bc45527
> --- /dev/null
> +++ b/js/MailProxyTLSInboundPolicy.js
> @@ -0,0 +1,93 @@
> +Ext.define('pmg-tls-inbound-policy', {
> + extend: 'Ext.data.Model',
> + fields: ['domain'],
> + idProperty: 'domain',
> + proxy: {
> + type: 'proxmox',
> + url: '/api2/json/config/tlsinboundpolicy',
> + },
> + sorters: {
> + property: 'domain',
> + direction: 'ASC',
> + },
> +});
> +
> +Ext.define('PMG.TLSInboundPolicyEdit', {
> + extend: 'Proxmox.window.Edit',
> + xtype: 'pmgTLSInboundPolicyEdit',
> + onlineHelp: 'pmgconfig_mailproxy_tls',
> +
> + subject: gettext('TLS Inbound Policy'),
> + url: '/api2/extjs/config/tlsinboundpolicy',
> + method: 'POST',
> +
> + items: [
> + {
> + xtype: 'proxmoxtextfield',
> + name: 'domain',
> + fieldLabel: gettext('Domain'),
> + },
> + ],
> +});
> +
> +Ext.define('PMG.MailProxyTLSInboundPolicy', {
> + extend: 'Ext.grid.GridPanel',
> + alias: ['widget.pmgMailProxyTLSInboundPolicy'],
> +
> + viewConfig: {
> + trackOver: false,
> + },
> +
> + columns: [
> + {
> + header: gettext('Domain'),
> + flex: 1,
> + sortable: true,
> + dataIndex: 'domain',
> + },
> + ],
> +
> + initComponent: function() {
> + const me = this;
> +
> + const rstore = Ext.create('Proxmox.data.UpdateStore', {
> + model: 'pmg-tls-inbound-policy',
> + storeid: 'pmg-mailproxy-tls-inbound-policy-store-' + ++Ext.idSeed,
> + });
> +
> + const store = Ext.create('Proxmox.data.DiffStore', { rstore: rstore });
> + const reload = () => rstore.load();
> + me.selModel = Ext.create('Ext.selection.RowModel', {});
> + Proxmox.Utils.monStoreErrors(me, store, true);
> +
> + Ext.apply(me, {
> + store,
> + tbar: [
> + {
> + text: gettext('Create'),
> + handler: () => {
> + Ext.createWidget('pmgTLSInboundPolicyEdit', {
> + autoShow: true,
> + listeners: {
> + destroy: reload,
> + },
> + });
> + },
> + },
> + {
> + xtype: 'proxmoxStdRemoveButton',
> + baseurl: '/config/tlsinboundpolicy',
> + callback: reload,
> + waitMsgTarget: me,
> + },
> + ],
> + listeners: {
> + activate: rstore.startUpdate,
> + destroy: rstore.stopUpdate,
> + deactivate: rstore.stopUpdate,
> + },
> + });
> +
> + me.callParent();
> + },
> +});
> diff --git a/js/MailProxyTLSPanel.js b/js/MailProxyTLSPanel.js
> index 82dc3f8..5a5837c 100644
> --- a/js/MailProxyTLSPanel.js
> +++ b/js/MailProxyTLSPanel.js
> @@ -26,11 +26,17 @@ Ext.define('PMG.MailProxyTLSPanel', {
> flex: 1,
> });
>
> - me.items = [tlsSettings, tlsDestinations];
> + const tlsInboundPolicy = Ext.create('PMG.MailProxyTLSInboundPolicy', {
> + title: gettext('TLS Inbound Policy'),
> + flex: 1,
> + });
> +
> + me.items = [tlsSettings, tlsDestinations, tlsInboundPolicy];
>
> me.callParent();
>
> tlsSettings.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> tlsDestinations.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> + tlsInboundPolicy.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> },
> });
> diff --git a/js/Makefile b/js/Makefile
> index 9a2bcf2..e3b9e78 100644
> --- a/js/Makefile
> +++ b/js/Makefile
> @@ -50,6 +50,7 @@ JSSRC= \
> MailProxyTLS.js \
> MailProxyTLSPanel.js \
> MailProxyTLSDestinations.js \
> + MailProxyTLSInboundPolicy.js \
> Transport.js \
> MyNetworks.js \
> RelayDomains.js \
> --
> 2.39.2
>
>
>
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option Christoph Heiss
@ 2023-03-16 12:50 ` Stoiko Ivanov
2023-03-20 8:21 ` Christoph Heiss
0 siblings, 1 reply; 12+ messages in thread
From: Stoiko Ivanov @ 2023-03-16 12:50 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
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
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains
2023-03-16 12:28 ` [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Stoiko Ivanov
@ 2023-03-20 8:14 ` Christoph Heiss
2023-03-20 8:36 ` Stoiko Ivanov
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2023-03-20 8:14 UTC (permalink / raw)
To: Stoiko Ivanov; +Cc: pmg-devel
Thanks for the review on the whole series!
On Thu, Mar 16, 2023 at 01:28:42PM +0100, Stoiko Ivanov wrote:
> Thanks for tackling this!!
>
> Works as advertised in general - so I think the approach is fine
>
> One thing that seems odd to me (and sadly I don't have a good and short
> answer) is that it's not a policy we're setting - it's a list of domains,
> for which the singular policy (in this context) is that we accept mails
> from them only via tls...
I'm bad at naming as per usual :^) I agree that 'policy' might be the
wrong word for that ..
>
> Inbound TLS Domains ? (at least mentions TLS, and explicitly mentioning
> domains might be hint enough that you cannot enter an IP (or network)
> there)
> Reject Plaintext Domains? (probably only appeals to users who know
> `postconf(5)` by heart)
From the two suggestions above I'd IMHO go with 'Inbound TLS Domains' -
it mostly says what it does on the tin and is probably one of the better
options in general.
The latter - as you say - leans very much on postfix nomenclature and
might be unclear to (some) users.
If that's okay with you I'll rename it for v2 and than see how it looks.
>
> some comments inline (some as reply to the individual patches):
> On Thu, 9 Mar 2023 11:18:43 +0100
> Christoph Heiss <c.heiss@proxmox.com> wrote:
>
> > [..]
> > The documentation changes are quite terse, maybe I should expand a bit
> > more on that topic? (Although the TLS destination policy is only lightly
> > documented as well, as far as I could see.)
> I personally am fine with terse documentation - however I always try to
> refer to the authoritative source - in this case the relevant postfix
> config parameter ([1]) - that way users who want to get more details
> actually see what's going on under the hood).
> You could rephrase the docs to mention that it sets
> reject_plaintext_session for those domains during MAIL FROM)
Ack, I'll add a link to the postfix config parameter and mention that it
sets `reject_plaintext_session`.
>
>
> >
> > Testing
> > -------
> > [..]
> I use swaks (apt installable) quite extensively for such things - short of
> smtputf8 support it should cover most use-cases - but thanks for the tip
> with curl being able to speak smtp as well :)
Haven't heard of or used swaks before, looks very useful - thanks for the
tip as well!
curl speaks ~every protocol that exists, so it was simply the first tool
that came to my mind :)
>
> >
> > 450 4.7.1 Session encryption is required
> >
> > When additionally adding the `--ssl-reqd` option to curl (instructing it
> > to require a TLS-encrypted session), the above command will succeed.
> >
> > (Also tested it with a domain not on the list, checking that no
> > regressions are introduced.)
> >
> > [0] https://bugzilla.proxmox.com/show_bug.cgi?id=2437
> > [1] http://www.postfix.org/postconf.5.html#reject_plaintext_session
> >
> > ---
> > pmg-api:
> >
> > Christoph Heiss (1):
> > fix #2437: config: Add inbound TLS policy option
> >
> > 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(+)
> >
> > pmg-gui:
> >
> > Christoph Heiss (1):
> > fix #2437: proxy: Add 'TLS Inbound Policy' panel
> >
> > js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++++++++++
> > js/MailProxyTLSPanel.js | 8 +++-
> > js/Makefile | 1 +
> > 3 files changed, 101 insertions(+), 1 deletion(-)
> >
> > pmg-docs:
> >
> > Christoph Heiss (1):
> > pmgconfig: Explain new TLS inbound policy configuration
> >
> > pmgconfig.adoc | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > --
> > 2.39.2
> >
> >
> >
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option
2023-03-16 12:50 ` Stoiko Ivanov
@ 2023-03-20 8:21 ` Christoph Heiss
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Heiss @ 2023-03-20 8:21 UTC (permalink / raw)
To: Stoiko Ivanov; +Cc: pmg-devel
On Thu, Mar 16, 2023 at 01:50:41PM +0100, Stoiko Ivanov wrote:
> 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.
Ack.
>
> One thing that is missing is adding the new file to the cluster sync (`git
> grep tls_policy`).
Thanks, didn't know that - will fix this for the next revision!
>
>
> > [..]
> > +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?
I mostly copied this straight from read_tls_policy(), so that's why ..
>
> 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))
That seems _very_ sensible, especially erroring out on entries with
anything other than `reject_plaintext_session` set to prevent users from
mis-using this file.
I will rework this for v2.
>
> [..]
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains
2023-03-20 8:14 ` Christoph Heiss
@ 2023-03-20 8:36 ` Stoiko Ivanov
0 siblings, 0 replies; 12+ messages in thread
From: Stoiko Ivanov @ 2023-03-20 8:36 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
On Mon, 20 Mar 2023 09:14:06 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:
> Thanks for the review on the whole series!
>
> On Thu, Mar 16, 2023 at 01:28:42PM +0100, Stoiko Ivanov wrote:
> > Thanks for tackling this!!
> >
> > Works as advertised in general - so I think the approach is fine
> >
> > One thing that seems odd to me (and sadly I don't have a good and short
> > answer) is that it's not a policy we're setting - it's a list of domains,
> > for which the singular policy (in this context) is that we accept mails
> > from them only via tls...
> I'm bad at naming as per usual :^) I agree that 'policy' might be the
> wrong word for that ..
>
> >
> > Inbound TLS Domains ? (at least mentions TLS, and explicitly mentioning
> > domains might be hint enough that you cannot enter an IP (or network)
> > there)
> > Reject Plaintext Domains? (probably only appeals to users who know
> > `postconf(5)` by heart)
> From the two suggestions above I'd IMHO go with 'Inbound TLS Domains' -
> it mostly says what it does on the tin and is probably one of the better
> options in general.
> The latter - as you say - leans very much on postfix nomenclature and
> might be unclear to (some) users.
>
> If that's okay with you I'll rename it for v2 and than see how it looks.
Sounds good! - Thanks for reworking this!
>
> >
> > some comments inline (some as reply to the individual patches):
> > On Thu, 9 Mar 2023 11:18:43 +0100
> > Christoph Heiss <c.heiss@proxmox.com> wrote:
> >
> > > [..]
> > > The documentation changes are quite terse, maybe I should expand a bit
> > > more on that topic? (Although the TLS destination policy is only lightly
> > > documented as well, as far as I could see.)
> > I personally am fine with terse documentation - however I always try to
> > refer to the authoritative source - in this case the relevant postfix
> > config parameter ([1]) - that way users who want to get more details
> > actually see what's going on under the hood).
> > You could rephrase the docs to mention that it sets
> > reject_plaintext_session for those domains during MAIL FROM)
> Ack, I'll add a link to the postfix config parameter and mention that it
> sets `reject_plaintext_session`.
>
> >
> >
> > >
> > > Testing
> > > -------
> > > [..]
> > I use swaks (apt installable) quite extensively for such things - short of
> > smtputf8 support it should cover most use-cases - but thanks for the tip
> > with curl being able to speak smtp as well :)
> Haven't heard of or used swaks before, looks very useful - thanks for the
> tip as well!
>
> curl speaks ~every protocol that exists, so it was simply the first tool
> that came to my mind :)
>
> >
> > >
> > > 450 4.7.1 Session encryption is required
> > >
> > > When additionally adding the `--ssl-reqd` option to curl (instructing it
> > > to require a TLS-encrypted session), the above command will succeed.
> > >
> > > (Also tested it with a domain not on the list, checking that no
> > > regressions are introduced.)
> > >
> > > [0] https://bugzilla.proxmox.com/show_bug.cgi?id=2437
> > > [1] http://www.postfix.org/postconf.5.html#reject_plaintext_session
> > >
> > > ---
> > > pmg-api:
> > >
> > > Christoph Heiss (1):
> > > fix #2437: config: Add inbound TLS policy option
> > >
> > > 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(+)
> > >
> > > pmg-gui:
> > >
> > > Christoph Heiss (1):
> > > fix #2437: proxy: Add 'TLS Inbound Policy' panel
> > >
> > > js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++++++++++
> > > js/MailProxyTLSPanel.js | 8 +++-
> > > js/Makefile | 1 +
> > > 3 files changed, 101 insertions(+), 1 deletion(-)
> > >
> > > pmg-docs:
> > >
> > > Christoph Heiss (1):
> > > pmgconfig: Explain new TLS inbound policy configuration
> > >
> > > pmgconfig.adoc | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > --
> > > 2.39.2
> > >
> > >
> > >
> > > _______________________________________________
> > > pmg-devel mailing list
> > > pmg-devel@lists.proxmox.com
> > > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> > >
> > >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel
2023-03-16 12:32 ` Stoiko Ivanov
@ 2023-03-20 8:36 ` Christoph Heiss
2023-03-20 8:42 ` Stoiko Ivanov
0 siblings, 1 reply; 12+ messages in thread
From: Christoph Heiss @ 2023-03-20 8:36 UTC (permalink / raw)
To: Stoiko Ivanov; +Cc: pmg-devel
On Thu, Mar 16, 2023 at 01:32:03PM +0100, Stoiko Ivanov wrote:
> only a question/hint - since this is yet another list of domains (with w/o
> added value) - any chance to merge this with MailProxyTLSDestinations?
> (or DKIMDomains, or Domains)?
>
> OTOH the component is small enough and refactoring can very well happen at
> another time as well) - so feel free to leave that out :)
I would do it as a follow-up patch(-series) to this, as to not block
this series on that needlessly :^)
Didn't know there were more such instance other than
MailProxyTLSDestinations, I'll have a look at it and unify them as best
as possible.
>
> On Thu, 9 Mar 2023 11:18:45 +0100
> Christoph Heiss <c.heiss@proxmox.com> wrote:
>
> > This panel can be used to configure sender domains for which TLS will be
> > enforced my postfix. As this takes the usual transport domain format,
> > either a FQDN or .FQDN (for matching subdomains) can be specified.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> > js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++
> > js/MailProxyTLSPanel.js | 8 ++-
> > js/Makefile | 1 +
> > 3 files changed, 101 insertions(+), 1 deletion(-)
> > create mode 100644 js/MailProxyTLSInboundPolicy.js
> >
> > diff --git a/js/MailProxyTLSInboundPolicy.js b/js/MailProxyTLSInboundPolicy.js
> > new file mode 100644
> > index 0000000..bc45527
> > --- /dev/null
> > +++ b/js/MailProxyTLSInboundPolicy.js
> > @@ -0,0 +1,93 @@
> > +Ext.define('pmg-tls-inbound-policy', {
> > + extend: 'Ext.data.Model',
> > + fields: ['domain'],
> > + idProperty: 'domain',
> > + proxy: {
> > + type: 'proxmox',
> > + url: '/api2/json/config/tlsinboundpolicy',
> > + },
> > + sorters: {
> > + property: 'domain',
> > + direction: 'ASC',
> > + },
> > +});
> > +
> > +Ext.define('PMG.TLSInboundPolicyEdit', {
> > + extend: 'Proxmox.window.Edit',
> > + xtype: 'pmgTLSInboundPolicyEdit',
> > + onlineHelp: 'pmgconfig_mailproxy_tls',
> > +
> > + subject: gettext('TLS Inbound Policy'),
> > + url: '/api2/extjs/config/tlsinboundpolicy',
> > + method: 'POST',
> > +
> > + items: [
> > + {
> > + xtype: 'proxmoxtextfield',
> > + name: 'domain',
> > + fieldLabel: gettext('Domain'),
> > + },
> > + ],
> > +});
> > +
> > +Ext.define('PMG.MailProxyTLSInboundPolicy', {
> > + extend: 'Ext.grid.GridPanel',
> > + alias: ['widget.pmgMailProxyTLSInboundPolicy'],
> > +
> > + viewConfig: {
> > + trackOver: false,
> > + },
> > +
> > + columns: [
> > + {
> > + header: gettext('Domain'),
> > + flex: 1,
> > + sortable: true,
> > + dataIndex: 'domain',
> > + },
> > + ],
> > +
> > + initComponent: function() {
> > + const me = this;
> > +
> > + const rstore = Ext.create('Proxmox.data.UpdateStore', {
> > + model: 'pmg-tls-inbound-policy',
> > + storeid: 'pmg-mailproxy-tls-inbound-policy-store-' + ++Ext.idSeed,
> > + });
> > +
> > + const store = Ext.create('Proxmox.data.DiffStore', { rstore: rstore });
> > + const reload = () => rstore.load();
> > + me.selModel = Ext.create('Ext.selection.RowModel', {});
> > + Proxmox.Utils.monStoreErrors(me, store, true);
> > +
> > + Ext.apply(me, {
> > + store,
> > + tbar: [
> > + {
> > + text: gettext('Create'),
> > + handler: () => {
> > + Ext.createWidget('pmgTLSInboundPolicyEdit', {
> > + autoShow: true,
> > + listeners: {
> > + destroy: reload,
> > + },
> > + });
> > + },
> > + },
> > + {
> > + xtype: 'proxmoxStdRemoveButton',
> > + baseurl: '/config/tlsinboundpolicy',
> > + callback: reload,
> > + waitMsgTarget: me,
> > + },
> > + ],
> > + listeners: {
> > + activate: rstore.startUpdate,
> > + destroy: rstore.stopUpdate,
> > + deactivate: rstore.stopUpdate,
> > + },
> > + });
> > +
> > + me.callParent();
> > + },
> > +});
> > diff --git a/js/MailProxyTLSPanel.js b/js/MailProxyTLSPanel.js
> > index 82dc3f8..5a5837c 100644
> > --- a/js/MailProxyTLSPanel.js
> > +++ b/js/MailProxyTLSPanel.js
> > @@ -26,11 +26,17 @@ Ext.define('PMG.MailProxyTLSPanel', {
> > flex: 1,
> > });
> >
> > - me.items = [tlsSettings, tlsDestinations];
> > + const tlsInboundPolicy = Ext.create('PMG.MailProxyTLSInboundPolicy', {
> > + title: gettext('TLS Inbound Policy'),
> > + flex: 1,
> > + });
> > +
> > + me.items = [tlsSettings, tlsDestinations, tlsInboundPolicy];
> >
> > me.callParent();
> >
> > tlsSettings.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > tlsDestinations.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > + tlsInboundPolicy.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > },
> > });
> > diff --git a/js/Makefile b/js/Makefile
> > index 9a2bcf2..e3b9e78 100644
> > --- a/js/Makefile
> > +++ b/js/Makefile
> > @@ -50,6 +50,7 @@ JSSRC= \
> > MailProxyTLS.js \
> > MailProxyTLSPanel.js \
> > MailProxyTLSDestinations.js \
> > + MailProxyTLSInboundPolicy.js \
> > Transport.js \
> > MyNetworks.js \
> > RelayDomains.js \
> > --
> > 2.39.2
> >
> >
> >
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel
2023-03-20 8:36 ` Christoph Heiss
@ 2023-03-20 8:42 ` Stoiko Ivanov
0 siblings, 0 replies; 12+ messages in thread
From: Stoiko Ivanov @ 2023-03-20 8:42 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
On Mon, 20 Mar 2023 09:36:36 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:
> On Thu, Mar 16, 2023 at 01:32:03PM +0100, Stoiko Ivanov wrote:
> > only a question/hint - since this is yet another list of domains (with w/o
> > added value) - any chance to merge this with MailProxyTLSDestinations?
> > (or DKIMDomains, or Domains)?
> >
> > OTOH the component is small enough and refactoring can very well happen at
> > another time as well) - so feel free to leave that out :)
> I would do it as a follow-up patch(-series) to this, as to not block
> this series on that needlessly :^)
Definitely! - it's a separate step after all - if you like to look into it
later - it would be appreciated!
>
> Didn't know there were more such instance other than
> MailProxyTLSDestinations, I'll have a look at it and unify them as best
> as possible.
did not look into it in detail, so am not sure if it's sensible/better to
unify them - if it makes things more complicated keeping it as is is also
an option.
>
> >
> > On Thu, 9 Mar 2023 11:18:45 +0100
> > Christoph Heiss <c.heiss@proxmox.com> wrote:
> >
> > > This panel can be used to configure sender domains for which TLS will be
> > > enforced my postfix. As this takes the usual transport domain format,
> > > either a FQDN or .FQDN (for matching subdomains) can be specified.
> > >
> > > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > > ---
> > > js/MailProxyTLSInboundPolicy.js | 93 +++++++++++++++++++++++++++++++++
> > > js/MailProxyTLSPanel.js | 8 ++-
> > > js/Makefile | 1 +
> > > 3 files changed, 101 insertions(+), 1 deletion(-)
> > > create mode 100644 js/MailProxyTLSInboundPolicy.js
> > >
> > > diff --git a/js/MailProxyTLSInboundPolicy.js b/js/MailProxyTLSInboundPolicy.js
> > > new file mode 100644
> > > index 0000000..bc45527
> > > --- /dev/null
> > > +++ b/js/MailProxyTLSInboundPolicy.js
> > > @@ -0,0 +1,93 @@
> > > +Ext.define('pmg-tls-inbound-policy', {
> > > + extend: 'Ext.data.Model',
> > > + fields: ['domain'],
> > > + idProperty: 'domain',
> > > + proxy: {
> > > + type: 'proxmox',
> > > + url: '/api2/json/config/tlsinboundpolicy',
> > > + },
> > > + sorters: {
> > > + property: 'domain',
> > > + direction: 'ASC',
> > > + },
> > > +});
> > > +
> > > +Ext.define('PMG.TLSInboundPolicyEdit', {
> > > + extend: 'Proxmox.window.Edit',
> > > + xtype: 'pmgTLSInboundPolicyEdit',
> > > + onlineHelp: 'pmgconfig_mailproxy_tls',
> > > +
> > > + subject: gettext('TLS Inbound Policy'),
> > > + url: '/api2/extjs/config/tlsinboundpolicy',
> > > + method: 'POST',
> > > +
> > > + items: [
> > > + {
> > > + xtype: 'proxmoxtextfield',
> > > + name: 'domain',
> > > + fieldLabel: gettext('Domain'),
> > > + },
> > > + ],
> > > +});
> > > +
> > > +Ext.define('PMG.MailProxyTLSInboundPolicy', {
> > > + extend: 'Ext.grid.GridPanel',
> > > + alias: ['widget.pmgMailProxyTLSInboundPolicy'],
> > > +
> > > + viewConfig: {
> > > + trackOver: false,
> > > + },
> > > +
> > > + columns: [
> > > + {
> > > + header: gettext('Domain'),
> > > + flex: 1,
> > > + sortable: true,
> > > + dataIndex: 'domain',
> > > + },
> > > + ],
> > > +
> > > + initComponent: function() {
> > > + const me = this;
> > > +
> > > + const rstore = Ext.create('Proxmox.data.UpdateStore', {
> > > + model: 'pmg-tls-inbound-policy',
> > > + storeid: 'pmg-mailproxy-tls-inbound-policy-store-' + ++Ext.idSeed,
> > > + });
> > > +
> > > + const store = Ext.create('Proxmox.data.DiffStore', { rstore: rstore });
> > > + const reload = () => rstore.load();
> > > + me.selModel = Ext.create('Ext.selection.RowModel', {});
> > > + Proxmox.Utils.monStoreErrors(me, store, true);
> > > +
> > > + Ext.apply(me, {
> > > + store,
> > > + tbar: [
> > > + {
> > > + text: gettext('Create'),
> > > + handler: () => {
> > > + Ext.createWidget('pmgTLSInboundPolicyEdit', {
> > > + autoShow: true,
> > > + listeners: {
> > > + destroy: reload,
> > > + },
> > > + });
> > > + },
> > > + },
> > > + {
> > > + xtype: 'proxmoxStdRemoveButton',
> > > + baseurl: '/config/tlsinboundpolicy',
> > > + callback: reload,
> > > + waitMsgTarget: me,
> > > + },
> > > + ],
> > > + listeners: {
> > > + activate: rstore.startUpdate,
> > > + destroy: rstore.stopUpdate,
> > > + deactivate: rstore.stopUpdate,
> > > + },
> > > + });
> > > +
> > > + me.callParent();
> > > + },
> > > +});
> > > diff --git a/js/MailProxyTLSPanel.js b/js/MailProxyTLSPanel.js
> > > index 82dc3f8..5a5837c 100644
> > > --- a/js/MailProxyTLSPanel.js
> > > +++ b/js/MailProxyTLSPanel.js
> > > @@ -26,11 +26,17 @@ Ext.define('PMG.MailProxyTLSPanel', {
> > > flex: 1,
> > > });
> > >
> > > - me.items = [tlsSettings, tlsDestinations];
> > > + const tlsInboundPolicy = Ext.create('PMG.MailProxyTLSInboundPolicy', {
> > > + title: gettext('TLS Inbound Policy'),
> > > + flex: 1,
> > > + });
> > > +
> > > + me.items = [tlsSettings, tlsDestinations, tlsInboundPolicy];
> > >
> > > me.callParent();
> > >
> > > tlsSettings.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > > tlsDestinations.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > > + tlsInboundPolicy.relayEvents(me, ['activate', 'deactivate', 'destroy']);
> > > },
> > > });
> > > diff --git a/js/Makefile b/js/Makefile
> > > index 9a2bcf2..e3b9e78 100644
> > > --- a/js/Makefile
> > > +++ b/js/Makefile
> > > @@ -50,6 +50,7 @@ JSSRC= \
> > > MailProxyTLS.js \
> > > MailProxyTLSPanel.js \
> > > MailProxyTLSDestinations.js \
> > > + MailProxyTLSInboundPolicy.js \
> > > Transport.js \
> > > MyNetworks.js \
> > > RelayDomains.js \
> > > --
> > > 2.39.2
> > >
> > >
> > >
> > > _______________________________________________
> > > pmg-devel mailing list
> > > pmg-devel@lists.proxmox.com
> > > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> > >
> > >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-20 8:43 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-09 10:18 [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-api 1/3] fix #2437: config: Add inbound TLS policy option Christoph Heiss
2023-03-16 12:50 ` Stoiko Ivanov
2023-03-20 8:21 ` Christoph Heiss
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-gui 2/3] fix #2437: proxy: Add 'TLS Inbound Policy' panel Christoph Heiss
2023-03-16 12:32 ` Stoiko Ivanov
2023-03-20 8:36 ` Christoph Heiss
2023-03-20 8:42 ` Stoiko Ivanov
2023-03-09 10:18 ` [pmg-devel] [PATCH pmg-docs 3/3] pmgconfig: Explain new TLS inbound policy configuration Christoph Heiss
2023-03-16 12:28 ` [pmg-devel] [PATCH pmg-{api, gui, docs} 0/3] fix #2437: Add TLS inbound policy for sender domains Stoiko Ivanov
2023-03-20 8:14 ` Christoph Heiss
2023-03-20 8:36 ` Stoiko Ivanov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox