Compare commits

...

12 Commits

Author SHA1 Message Date
Alex V.
1d8e44a948 test: add error-path tests for all bug fixes
- test_doveauth: invalid localpart chars rejected, concurrent same-account creation
- test_expire: --mdir filtering uses msg.path correctly
- test_metadata: TURN exception returns N\n, success returns credentials
- test_turnserver: socket timeout, connection refused, happy path
- test_dns: get_dkim_entry returns (None, None) on CalledProcessError
- test_rshell: dovecot_recalc_quota handles empty/malformed output
2026-02-07 21:45:22 +03:00
Alex V.
447c0ee33d fix: handle turn_credentials exceptions in metadata proxy
ConnectionRefusedError/FileNotFoundError/TimeoutError from
turn_credentials() would kill the dict proxy connection.
Return N (not found) response instead and log the error.
2026-02-07 16:51:30 +03:00
Alex V.
47c9586b67 fix: add 5s timeout to TURN credential socket
Hung TURN daemon would block dict proxy handler thread indefinitely.
Per Python docs, settimeout() raises TimeoutError on expiry.
2026-02-07 16:51:19 +03:00
Alex V.
e32816b477 fix(security): validate localpart chars and fix account creation race
- Reject localparts with chars outside [a-z0-9._-] to prevent
  filesystem issues from crafted usernames via IMAP/SMTP auth
- Use filelock to serialize concurrent account creation for same
  address, preventing TOCTOU race where two threads both create
  an account and last writer wins
2026-02-07 16:51:01 +03:00
Alex V.
0d3cde9850 fix(security): remove deprecated TLS 1.0/1.1 from nginx config
TLS 1.0/1.1 deprecated by RFC 8996. Nginx default is TLSv1.2 TLSv1.3.
Aligns with postfix (>=TLSv1.2) and dovecot (TLSv1.3) in the same stack.
2026-02-07 16:50:43 +03:00
Alex V.
c48a7d80dc fix(security): use secrets.choice instead of random.choices for username
Per Python docs, secrets module should be used for security-sensitive
data. random.choices uses Mersenne Twister PRNG which is predictable.
secrets.choice was already used for password generation in the same file.
2026-02-07 16:50:24 +03:00
Alex V.
93eb996a42 fix: guard against IndexError in dovecot_recalc_quota
doveadm output ends with empty line, parts=[] causes parts[2] crash.
2026-02-07 16:30:04 +03:00
Alex V.
0148ecde8e fix: remove dead code and potential NameError in run_cmd
check_call always returns 0 or raises, making retcode!=0 branches
unreachable. Also remote_data was undefined with --skip-dns-check.
2026-02-07 16:29:38 +03:00
Alex V.
3ad8e5a6ee fix: handle build_webpages returning None in WebsiteDeployer
Exception in _build_webpages was silently caught, returning None.
rsync then received "None/" as source path, silently breaking deploy.
2026-02-07 16:29:14 +03:00
Alex V.
a5dc1d886d fix: return tuple from get_dkim_entry on CalledProcessError
Bare return yielded None, causing TypeError on tuple unpacking
in perform_initial_checks on fresh servers without DKIM keys.
2026-02-07 16:28:53 +03:00
Alex V.
0443965f63 fix: use msg.path instead of nonexistent msg.relpath in fsreport
FileEntry namedtuple has (path, mtime, size), not relpath.
Crashes with AttributeError when --mdir flag is used.
2026-02-07 16:28:42 +03:00
Alex V.
ea52fde2d9 chore: fix ruff formatting in acmetool, dovecot, postfix deployers 2026-02-07 16:27:46 +03:00
19 changed files with 343 additions and 26 deletions

View File

@@ -1,8 +1,11 @@
import json
import logging
import os
import re
import sys
import filelock
try:
import crypt_r
except ImportError:
@@ -13,6 +16,7 @@ from .dictproxy import DictProxy
from .migrate_db import migrate_from_db_to_maildir
NOCREATE_FILE = "/etc/chatmail-nocreate"
VALID_LOCALPART_RE = re.compile(r"^[a-z0-9._-]+$")
def encrypt_password(password: str):
@@ -52,6 +56,10 @@ def is_allowed_to_create(config: Config, user, cleartext_password) -> bool:
)
return False
if not VALID_LOCALPART_RE.match(localpart):
logging.warning("localpart %r contains invalid characters", localpart)
return False
return True
@@ -140,8 +148,13 @@ class AuthDictProxy(DictProxy):
if not is_allowed_to_create(self.config, addr, cleartext_password):
return
user.set_password(encrypt_password(cleartext_password))
print(f"Created address: {addr}", file=sys.stderr)
lock = filelock.FileLock(str(user.password_path) + ".lock", timeout=5)
with lock:
userdata = user.get_userdb_dict()
if userdata:
return userdata
user.set_password(encrypt_password(cleartext_password))
print(f"Created address: {addr}", file=sys.stderr)
return user.get_userdb_dict()

View File

@@ -68,7 +68,7 @@ class Report:
for size in self.message_buckets:
for msg in mailbox.messages:
if msg.size >= size:
if self.mdir and not msg.relpath.startswith(self.mdir):
if self.mdir and f"/{self.mdir}/" not in msg.path:
continue
self.message_buckets[size] += msg.size

View File

@@ -101,7 +101,11 @@ class MetadataDictProxy(DictProxy):
# Handle `GETMETADATA "" /shared/vendor/deltachat/irohrelay`
return f"O{self.iroh_relay}\n"
elif keyname == "vendor/vendor.dovecot/pvt/server/vendor/deltachat/turn":
res = turn_credentials()
try:
res = turn_credentials()
except Exception:
logging.exception("failed to get TURN credentials")
return "N\n"
port = 3478
return f"O{self.turn_hostname}:{port}:{res}\n"

View File

@@ -3,7 +3,6 @@
"""CGI script for creating new accounts."""
import json
import random
import secrets
import string
@@ -15,7 +14,9 @@ ALPHANUMERIC_PUNCT = string.ascii_letters + string.digits + string.punctuation
def create_newemail_dict(config: Config):
user = "".join(random.choices(ALPHANUMERIC, k=config.username_max_length))
user = "".join(
secrets.choice(ALPHANUMERIC) for _ in range(config.username_max_length)
)
password = "".join(
secrets.choice(ALPHANUMERIC_PUNCT)
for _ in range(config.password_min_length + 3)

View File

@@ -120,6 +120,60 @@ def test_handle_dovecot_protocol_iterate(gencreds, example_config):
assert not lines[2]
def test_invalid_localpart_characters(make_config):
"""Test that is_allowed_to_create rejects localparts with invalid characters."""
config = make_config("chat.example.org", {"username_min_length": "3"})
password = "zequ0Aimuchoodaechik"
domain = config.mail_domain
# valid localparts
assert is_allowed_to_create(config, f"abc123@{domain}", password)
assert is_allowed_to_create(config, f"a.b-c_d@{domain}", password)
# uppercase rejected
assert not is_allowed_to_create(config, f"Abc123@{domain}", password)
assert not is_allowed_to_create(config, f"ABCDEFG@{domain}", password)
# spaces and special chars rejected
assert not is_allowed_to_create(config, f"a b cde@{domain}", password)
assert not is_allowed_to_create(config, f"abc+def@{domain}", password)
assert not is_allowed_to_create(config, f"abc!def@{domain}", password)
assert not is_allowed_to_create(config, f"ab@cdef@{domain}", password)
assert not is_allowed_to_create(config, f"abc/def@{domain}", password)
assert not is_allowed_to_create(config, f"abc\\def@{domain}", password)
def test_concurrent_creation_same_account(dictproxy):
"""Test that concurrent creation of the same account doesn't corrupt password."""
addr = "racetest1@chat.example.org"
password = "zequ0Aimuchoodaechik"
num_threads = 10
results = queue.Queue()
def create():
try:
res = dictproxy.lookup_passdb(addr, password)
results.put(("ok", res))
except Exception:
results.put(("err", traceback.format_exc()))
threads = [threading.Thread(target=create, daemon=True) for _ in range(num_threads)]
for t in threads:
t.start()
for t in threads:
t.join(timeout=10)
passwords_seen = set()
for _ in range(num_threads):
status, res = results.get()
if status == "err":
pytest.fail(f"concurrent creation failed\n{res}")
passwords_seen.add(res["password"])
# all threads must see the same password hash
assert len(passwords_seen) == 1
def test_50_concurrent_lookups_different_accounts(gencreds, dictproxy):
num_threads = 50
req_per_thread = 5

View File

@@ -112,6 +112,43 @@ def test_report(mbox1, example_config):
report_main(args)
def test_report_mdir_filters_by_path(mbox1, example_config):
"""Test that Report with mdir='cur' only counts messages in cur/ subdirectory."""
from chatmaild.fsreport import Report
now = datetime.utcnow().timestamp()
# Set password mtime to old enough so min_login_age check passes
password = Path(mbox1.basedir).joinpath("password")
old_time = now - 86400 * 10 # 10 days ago
os.utime(password, (old_time, old_time))
# Reload mailbox with updated mtime
from chatmaild.expire import MailboxStat
mbox = MailboxStat(mbox1.basedir)
# Report without mdir — should count all messages
rep_all = Report(now=now, min_login_age=1, mdir=None)
rep_all.process_mailbox_stat(mbox)
total_all = rep_all.message_buckets[0]
# Report with mdir='cur' — should only count cur/ messages
rep_cur = Report(now=now, min_login_age=1, mdir="cur")
rep_cur.process_mailbox_stat(mbox)
total_cur = rep_cur.message_buckets[0]
# Report with mdir='new' — should only count new/ messages
rep_new = Report(now=now, min_login_age=1, mdir="new")
rep_new.process_mailbox_stat(mbox)
total_new = rep_new.message_buckets[0]
# cur has 500-byte msg, new has 600-byte msg (from fill_mbox)
assert total_cur == 500
assert total_new == 600
assert total_all == 500 + 600
def test_expiry_cli_basic(example_config, mbox1):
args = (str(example_config._inipath),)
expiry_main(args)

View File

@@ -314,6 +314,51 @@ def test_persistent_queue_items(tmp_path, testaddr, token):
assert not queue_item < item2 and not item2 < queue_item
def test_turn_credentials_exception_returns_N(notifier, metadata, monkeypatch):
"""Test that turn_credentials() failure returns N\\n instead of crashing."""
import chatmaild.metadata
dictproxy = MetadataDictProxy(
notifier=notifier,
metadata=metadata,
turn_hostname="turn.example.org",
)
def mock_turn_credentials():
raise ConnectionRefusedError("socket not available")
monkeypatch.setattr(chatmaild.metadata, "turn_credentials", mock_turn_credentials)
transactions = {}
res = dictproxy.handle_dovecot_request(
"Lshared/0123/vendor/vendor.dovecot/pvt/server/vendor/deltachat/turn"
"\tuser@example.org",
transactions,
)
assert res == "N\n"
def test_turn_credentials_success(notifier, metadata, monkeypatch):
"""Test that valid turn_credentials() returns TURN URI."""
import chatmaild.metadata
dictproxy = MetadataDictProxy(
notifier=notifier,
metadata=metadata,
turn_hostname="turn.example.org",
)
monkeypatch.setattr(chatmaild.metadata, "turn_credentials", lambda: "user:pass")
transactions = {}
res = dictproxy.handle_dovecot_request(
"Lshared/0123/vendor/vendor.dovecot/pvt/server/vendor/deltachat/turn"
"\tuser@example.org",
transactions,
)
assert res == "Oturn.example.org:3478:user:pass\n"
def test_iroh_relay(dictproxy):
rfile = io.BytesIO(
b"\n".join(

View File

@@ -0,0 +1,73 @@
import socket
import threading
import time
from unittest.mock import patch
import pytest
from chatmaild.turnserver import turn_credentials
SOCKET_PATH = "/run/chatmail-turn/turn.socket"
@pytest.fixture
def turn_socket(tmp_path):
"""Create a real Unix socket server at a temp path."""
sock_path = str(tmp_path / "turn.socket")
server = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
server.bind(sock_path)
server.listen(1)
yield sock_path, server
server.close()
def _call_turn_credentials(sock_path):
"""Call turn_credentials but connect to sock_path instead of hardcoded path."""
original_connect = socket.socket.connect
def patched_connect(self, address):
if address == SOCKET_PATH:
address = sock_path
return original_connect(self, address)
with patch.object(socket.socket, "connect", patched_connect):
return turn_credentials()
def test_turn_credentials_timeout(turn_socket):
"""Server accepts but never responds — must raise socket.timeout."""
sock_path, server = turn_socket
def accept_and_hang():
conn, _ = server.accept()
time.sleep(30)
conn.close()
t = threading.Thread(target=accept_and_hang, daemon=True)
t.start()
with pytest.raises(socket.timeout):
_call_turn_credentials(sock_path)
def test_turn_credentials_connection_refused(tmp_path):
"""Socket file doesn't exist — must raise ConnectionRefusedError or FileNotFoundError."""
missing = str(tmp_path / "nonexistent.socket")
with pytest.raises((ConnectionRefusedError, FileNotFoundError)):
_call_turn_credentials(missing)
def test_turn_credentials_success(turn_socket):
"""Server responds with credentials — must return stripped string."""
sock_path, server = turn_socket
def respond():
conn, _ = server.accept()
conn.sendall(b"testuser:testpass\n")
conn.close()
t = threading.Thread(target=respond, daemon=True)
t.start()
result = _call_turn_credentials(sock_path)
assert result == "testuser:testpass"

View File

@@ -4,6 +4,7 @@ import socket
def turn_credentials() -> str:
with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as client_socket:
client_socket.settimeout(5)
client_socket.connect("/run/chatmail-turn/turn.socket")
with client_socket.makefile("rb") as file:
return file.readline().decode("utf-8").strip()

View File

@@ -67,7 +67,7 @@ class AcmetoolDeployer(Deployer):
)
files.template(
src=importlib.resources.files(__package__).joinpath("desired.yaml.j2"),
dest=f"/var/lib/acme/desired/{self.domains[0]}", # 0 is mailhost TLD
dest=f"/var/lib/acme/desired/{self.domains[0]}", # 0 is mailhost TLD
user="root",
group="root",
mode="644",

View File

@@ -113,24 +113,15 @@ def run_cmd(args, out):
return 1
try:
retcode = out.check_call(cmd, env=env)
out.check_call(cmd, env=env)
if args.website_only:
if retcode == 0:
out.green("Website deployment completed.")
else:
out.red("Website deployment failed.")
elif retcode == 0:
out.green("Deploy completed, call `cmdeploy dns` next.")
elif not remote_data["acme_account_url"]:
out.red("Deploy completed but letsencrypt not configured")
out.red("Run 'cmdeploy run' again")
retcode = 0
out.green("Website deployment completed.")
else:
out.red("Deploy failed")
out.green("Deploy completed, call `cmdeploy dns` next.")
return 0
except subprocess.CalledProcessError:
out.red("Deploy failed")
retcode = 1
return retcode
return 1
def dns_cmd_options(parser):

View File

@@ -264,6 +264,9 @@ class WebsiteDeployer(Deployer):
# if www_folder is a hugo page, build it
if build_dir:
www_path = build_webpages(src_dir, build_dir, self.config)
if www_path is None:
logger.warning("Web page build failed, skipping website deployment")
return
# if it is not a hugo page, upload it as is
files.rsync(
f"{www_path}/", "/var/www/html", flags=["-avz", "--chown=www-data"]

View File

@@ -37,7 +37,9 @@ class DovecotDeployer(Deployer):
restart = False if self.disable_mail else self.need_restart
systemd.service(
name="Disable dovecot for now" if self.disable_mail else "Start and enable Dovecot",
name="Disable dovecot for now"
if self.disable_mail
else "Start and enable Dovecot",
service="dovecot.service",
running=False if self.disable_mail else True,
enabled=False if self.disable_mail else True,

View File

@@ -51,7 +51,7 @@ http {
include /etc/nginx/mime.types;
default_type application/octet-stream;
ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;
ssl_protocols TLSv1.2 TLSv1.3;
ssl_prefer_server_ciphers on;
ssl_certificate /var/lib/acme/live/{{ config.domain_name }}/fullchain;
ssl_certificate_key /var/lib/acme/live/{{ config.domain_name }}/privkey;

View File

@@ -83,7 +83,9 @@ class PostfixDeployer(Deployer):
server.shell(
name="Validate postfix configuration",
# Extract stderr and quit with error if non-zero
commands=["""bash -c 'w=$(postconf 2>&1 >/dev/null); [[ -z "$w" ]] || { echo "$w"; false; }'"""],
commands=[
"""bash -c 'w=$(postconf 2>&1 >/dev/null); [[ -z "$w" ]] || { echo "$w"; false; }'"""
],
)
self.need_restart = need_restart

View File

@@ -53,7 +53,7 @@ def get_dkim_entry(mail_domain, pre_command, dkim_selector):
print=log_progress,
)
except CalledProcessError:
return
return None, None
dkim_value_raw = f"v=DKIM1;k=rsa;p={dkim_pubkey};s=email;t=s"
dkim_value = '" "'.join(re.findall(".{1,255}", dkim_value_raw))
web_dkim_value = "".join(re.findall(".{1,255}", dkim_value_raw))

View File

@@ -40,5 +40,5 @@ def dovecot_recalc_quota(user):
#
for line in output.split("\n"):
parts = line.split()
if parts[2] == "STORAGE":
if len(parts) >= 6 and parts[2] == "STORAGE":
return dict(value=int(parts[3]), limit=int(parts[4]), percent=int(parts[5]))

View File

@@ -60,6 +60,29 @@ def mockdns(request, mockdns_base, mockdns_expected):
return mockdns_base
class TestGetDkimEntry:
def test_dkim_entry_returns_tuple_on_success(self, mockdns):
entry, web_entry = remote.rdns.get_dkim_entry(
"some.domain", "", dkim_selector="opendkim"
)
# May return None,None if openssl not available, but should never crash
if entry is not None:
assert "opendkim._domainkey.some.domain" in entry
assert "opendkim._domainkey.some.domain" in web_entry
def test_dkim_entry_returns_none_tuple_on_error(self, monkeypatch):
"""CalledProcessError must return (None, None), not bare None."""
from subprocess import CalledProcessError
def failing_shell(command, fail_ok=False, print=print):
raise CalledProcessError(1, command)
monkeypatch.setattr(remote.rdns, "shell", failing_shell)
result = remote.rdns.get_dkim_entry("some.domain", "", dkim_selector="opendkim")
assert result == (None, None)
assert result[0] is None and result[1] is None
class TestPerformInitialChecks:
def test_perform_initial_checks_ok1(self, mockdns, mockdns_expected):
remote_data = remote.rdns.perform_initial_checks("some.domain")

View File

@@ -0,0 +1,68 @@
from unittest.mock import patch
from cmdeploy.remote.rshell import dovecot_recalc_quota
def test_dovecot_recalc_quota_normal_output():
"""Normal doveadm output returns parsed dict."""
normal_output = (
"Quota name Type Value Limit %\n"
"User quota STORAGE 5 102400 0\n"
"User quota MESSAGE 2 - 0\n"
)
with patch("cmdeploy.remote.rshell.shell", return_value=normal_output):
result = dovecot_recalc_quota("user@example.org")
# shell is called twice (recalc + get), patch returns same for both
assert result == {"value": 5, "limit": 102400, "percent": 0}
def test_dovecot_recalc_quota_empty_output():
"""Empty doveadm output (trailing newline) must not IndexError."""
call_count = [0]
def mock_shell(cmd):
call_count[0] += 1
if "recalc" in cmd:
return ""
# quota get returns only empty lines
return "\n\n"
with patch("cmdeploy.remote.rshell.shell", side_effect=mock_shell):
result = dovecot_recalc_quota("user@example.org")
assert result is None
def test_dovecot_recalc_quota_malformed_output():
"""Malformed output with too few columns must not crash."""
call_count = [0]
def mock_shell(cmd):
call_count[0] += 1
if "recalc" in cmd:
return ""
# partial line, fewer than 6 parts
return "Quota name\nUser quota STORAGE\n"
with patch("cmdeploy.remote.rshell.shell", side_effect=mock_shell):
result = dovecot_recalc_quota("user@example.org")
assert result is None
def test_dovecot_recalc_quota_header_only():
"""Only header line, no data rows."""
call_count = [0]
def mock_shell(cmd):
call_count[0] += 1
if "recalc" in cmd:
return ""
return "Quota name Type Value Limit %\n"
with patch("cmdeploy.remote.rshell.shell", side_effect=mock_shell):
result = dovecot_recalc_quota("user@example.org")
assert result is None