email: Code cleanup, address reviews

- View: security.html -> email_security.html
- Setup: will not install Redis
- Setup: put clamav packages on hold
- Crash recovery: opening firewall ports becomes the last post action
- Crash recovery: group postconf.set_many into small transactions
- Crash recovery: safer postconf.set_master_cf_options

plinth/modules/email_server/__init__.py

@@ -16,26 +16,51 @@ from . import audit
 from . import manifest
 
 version = 1
-managed_packages = ['postfix-ldap', 'dovecot-pop3d', 'dovecot-imapd',
-                    'dovecot-ldap', 'dovecot-lmtpd', 'dovecot-managesieved',
-                    'rspamd', 'clamav', 'clamav-daemon']
-managed_services = ['postfix', 'dovecot', 'rspamd', 'redis', 'clamav-daemon',
-                    'clamav-freshclam']
+
+packages = [
+    'postfix-ldap', 'dovecot-pop3d', 'dovecot-imapd',
+    'dovecot-ldap', 'dovecot-lmtpd', 'dovecot-managesieved',
+]
+
+packages_bloat = ['rspamd']
+
+clamav_packages = ['clamav', 'clamav-daemon']
+clamav_daemons = ['clamav-daemon', 'clamav-freshclam']
+
+port_info = {
+    'postfix': ('smtp', 25, 'smtps', 465, 'smtp-submission', 587),
+    'dovecot': ('imaps', 993, 'pop3s', 995),
+}
+
+managed_services = ['postfix', 'dovecot', 'rspamd']
+
+managed_packages = packages + packages_bloat
 app = None
 
 
 class EmailServerApp(plinth.app.App):
     """FreedomBox email server app"""
     app_id = 'email_server'
+    app_name = _('Email Server')
 
     def __init__(self):
         """The app's constructor"""
         super().__init__()
+        self._add_ui_components()
+        self._add_daemons()
+        self._add_firewall_ports()
 
+        # /rspamd location
+        webserver = Webserver('webserver-email',  # unique id
+                              'email-server-freedombox',  # config file name
+                              urls=['https://{host}/rspamd'])
+        self.add(webserver)
+
+    def _add_ui_components(self):
         info = plinth.app.Info(
             app_id=self.app_id,
             version=version,
-            name=_('Email Server'),
+            name=self.app_name,
             short_description=_('Powered by Postfix, Dovecot & Rspamd'),
             manual_page='EmailServer',
             clients=manifest.clients,
@@ -53,11 +78,6 @@ class EmailServerApp(plinth.app.App):
         )
         self.add(menu_item)
 
-        # /rspamd location
-        webserver = Webserver('webserver-email', 'email-server-freedombox',
-                              urls=['https://{host}/rspamd'])
-        self.add(webserver)
-
         shortcut = plinth.frontpage.Shortcut(
             'shortcut_' + self.app_id,
             name=info.name,
@@ -69,30 +89,28 @@ class EmailServerApp(plinth.app.App):
         )
         self.add(shortcut)
 
-        postfix_ports = []
-        dovecot_ports = []
-        all_firewalld_ports = []
-        for port in (25, 465, 587):
-            postfix_ports.extend([(port, 'tcp4'), (port, 'tcp6')])
-        for port in (993, 995):
-            dovecot_ports.extend([(port, 'tcp4'), (port, 'tcp6')])
-        all_firewalld_ports.extend(['smtp', 'smtps', 'smtp-submission'])
-        all_firewalld_ports.extend(['pop3s', 'imaps'])
-
-        # Manage daemons
-        postfixd = plinth.daemon.Daemon('daemon-postfix', 'postfix',
-                                        listen_ports=postfix_ports)
-        dovecotd = plinth.daemon.Daemon('daemon-dovecot', 'dovecot',
-                                        listen_ports=dovecot_ports)
-        self.add(postfixd)
-        self.add(dovecotd)
-        for name in ('rspamd', 'redis', 'clamav-daemon', 'clamav-freshclam'):
-            daemon = plinth.daemon.Daemon('daemon-' + name, name)
+    def _add_daemons(self):
+        for srvname in managed_services:
+            # Construct `listen_ports` parameter for the daemon
+            mixed = port_info.get(srvname, ())
+            port_numbers = [v for v in mixed if isinstance(v, int)]
+            listen = []
+            for n in port_numbers:
+                listen.append((n, 'tcp4'))
+                listen.append((n, 'tcp6'))
+            # Add daemon
+            daemon = plinth.daemon.Daemon('daemon-' + srvname, srvname,
+                                          listen_ports=listen)
             self.add(daemon)
 
-        # Ports
-        firewall = Firewall('firewall-email', info.name,
-                            ports=all_firewalld_ports, is_external=True)
+    def _add_firewall_ports(self):
+        all_port_names = []
+        for mixed in port_info.values():
+            port_names = [v for v in mixed if isinstance(v, str)]
+            all_port_names.extend(port_names)
+
+        firewall = Firewall('firewall-email', self.app_name,
+                            ports=all_port_names, is_external=True)
         self.add(firewall)
 
     def diagnose(self):
@@ -106,9 +124,11 @@ class EmailServerApp(plinth.app.App):
 
 def setup(helper, old_version=None):
     """Installs and configures module"""
-    helper.install(managed_packages)
+    helper.install(packages)
+    helper.install(packages_bloat, skip_recommends=True)
     helper.call('post', audit.ldap.repair)
     helper.call('post', audit.spam.repair)
+    for srvname in managed_services:
+        actions.superuser_run('service', ['reload', srvname])
+    # Final step: expose service daemons to public internet
     helper.call('post', app.enable)
-    for service_name in managed_services:
-        actions.superuser_run('service', ['reload', service_name])

plinth/modules/email_server/audit/ldap.py

@@ -102,11 +102,8 @@ def action_set_sasl():
 
 def action_set_submission():
     """Handles email_server -i ldap set_submission"""
-    logger.info('Set postfix service: %r', default_submission_options)
     postconf.set_master_cf_options(service_flags=submission_flags,
                                    options=default_submission_options)
-
-    logger.info('Set postfix service: %r', default_smtps_options)
     postconf.set_master_cf_options(service_flags=smtps_flags,
                                    options=default_smtps_options)
 
@@ -139,7 +136,7 @@ def fix_alias_maps(diagnosis):
         for i in range(len(analysis.parsed)):
             if analysis.parsed[i] in (BEFORE_ALIASES, AFTER_ALIASES):
                 analysis.parsed[i] = ''
-        # Does hash:/etc/aliases exist?
+        # Does hash:/etc/aliases exist in list?
         if analysis.isystem >= 0:
             # Put the maps around hash:/etc/aliases
             val = '%s %s %s' % (BEFORE_ALIASES, ETC_ALIASES, AFTER_ALIASES)

plinth/modules/email_server/postconf.py

@@ -1,15 +1,20 @@
 """Postconf wrapper providing thread-safe operations"""
 # SPDX-License-Identifier: AGPL-3.0-or-later
 
-import dataclasses
+import logging
 import re
 import subprocess
+from dataclasses import dataclass
+from typing import ClassVar
+
+from . import interproc
 from .lock import Mutex
 
+logger = logging.getLogger(__name__)
 mutex = Mutex('email-postconf')
 
 
-@dataclasses.dataclass
+@dataclass
 class ServiceFlags:
     service: str
     type: str
@@ -20,10 +25,31 @@ class ServiceFlags:
     maxproc: str
     command_args: str
 
+    crash_handler: ClassVar[str] = '/dev/null/plinth-crash'
+
+    def _get_flags_ordered(self):
+        return [self.service, self.type, self.private, self.unpriv,
+                self.chroot, self.wakeup, self.maxproc, self.command_args]
+
     def serialize(self) -> str:
-        return ' '.join([self.service, self.type, self.private, self.unpriv,
-                         self.chroot, self.wakeup, self.maxproc,
-                         self.command_args])
+        ordered = self._get_flags_ordered()
+        return ' '.join(ordered)
+
+    def serialize_temp(self) -> str:
+        ordered = self._get_flags_ordered()
+        ordered[-1] = self.crash_handler
+        return ' '.join(ordered)
+
+    def try_remove_crash_handler(self, line) -> str:
+        pattern = re.compile('([^ \\t]+)[ \\t]+([a-z]+)[ \\t]+')
+        match = pattern.match(line)
+        if match is None:
+            return None
+        if match.group(1) != self.service or match.group(2) != self.type:
+            return None
+        if not line.rstrip().endswith(self.crash_handler):
+            return None
+        return line.replace(self.crash_handler, self.command_args)
 
 
 def get_many(key_list):
@@ -52,9 +78,16 @@ def set_many(kv_map):
         set_many_unsafe(kv_map)
 
 
-def set_many_unsafe(kv_map):
+def set_many_unsafe(kv_map, flag=''):
+    args = ['/sbin/postconf']
+
+    if not kv_map:
+        return
+    if flag:
+        args.append(flag)
     for key, value in kv_map.items():
-        set_unsafe(key, value)
+        args.append('{}={}'.format(key, value))
+    _run(args)
 
 
 def set_master_cf_options(service_flags, options={}):
@@ -65,15 +98,19 @@ def set_master_cf_options(service_flags, options={}):
         validate_key(key)
         validate_value(value)
 
-    service_slash_type = service_flags.service + '/' + service_flags.type
-    flag_string = service_flags.serialize()
+    service_key = service_flags.service + '/' + service_flags.type
+    long_opts = {service_key + '/' + k: v for (k, v) in options.items()}
 
+    logger.info('Setting %s service: %r', service_flags.service, options)
+
+    # Crash resistant config setting:
+    # /sbin/postconf -M "service/type=<temp flag string>"
+    # /sbin/postconf -P "service/type/k=v" ...
+    # Delete placeholder string /dev/null/plinth-crash
     with mutex.lock_all():
-        # /sbin/postconf -M "service/type=flag_string"
-        set_unsafe(service_slash_type, flag_string, '-M')
-        for short_key, value in options.items():
-            # /sbin/postconf -P "service/type/short_key=value"
-            set_unsafe(service_slash_type + '/' + short_key, value, '-P')
+        set_unsafe(service_key, service_flags.serialize_temp(), '-M')
+        set_many_unsafe(long_opts, '-P')
+        _master_remove_crash_handler(service_flags)
 
 
 def get_unsafe(key):
@@ -122,6 +159,14 @@ def _run(args):
         raise RuntimeError('Unicode decoding failed') from unicode_error
 
 
+def _master_remove_crash_handler(service_flags):
+    with interproc.atomically_rewrite('/etc/postfix/master.cf') as writer:
+        with open('/etc/postfix/master.cf') as reader:
+            for line in reader:
+                cleaned = service_flags.try_remove_crash_handler(line)
+                writer.write(line if cleaned is None else cleaned)
+
+
 def validate_key(key):
     """Validate postconf key format. Raises ValueError"""
     if not re.match('^[a-zA-Z][a-zA-Z0-9_]*$', key):

plinth/modules/email_server/urls.py

@@ -7,7 +7,7 @@ from . import views
 
 urlpatterns = [
     path('apps/email_server/', views.EmailServerView.as_view(), name='index'),
-    path('apps/email_server/security', views.TLSView.as_view()),
+    path('apps/email_server/email_security', views.TLSView.as_view()),
     path('apps/email_server/domains', views.DomainView.as_view()),
 
     path('apps/email_server/my_mail',

plinth/modules/email_server/views.py

@@ -21,7 +21,7 @@ class TabMixin(View):
         ('', _('Home')),
         ('my_mail', _('My Mail')),
         ('my_aliases', _('My Aliases')),
-        ('security', _('Security')),
+        ('email_security', _('Security')),
         ('domains', _('Domains'))
     ]
 
@@ -218,7 +218,7 @@ class AliasView(TabMixin, TemplateView):
 
 
 class TLSView(TabMixin, TemplateView):
-    template_name = 'security.html'
+    template_name = 'email_security.html'
 
 
 class DomainView(TabMixin, TemplateView):