From 85d265187da6d01a295a2772215bb77dd036fe8c Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Mon, 8 Jun 2026 10:45:15 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20Caddy=20TLS=20cert=20acquisition=20?= =?UTF-8?q?=E2=80=94=20two=20DNS-01=20blockers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. caddy_manager: embed ddns.token (registration bearer token) in Caddyfile, not DDNS_TOTP_SECRET. The pic_ngo plugin sends the token to POST /api/v1/dns-challenge; using the TOTP secret caused 401 on every attempt. 2. firewall_manager: add _acme-challenge. forwarding block before each split-horizon zone in the Corefile. Without this, CoreDNS was authoritative for the challenge name and returned NODATA for TXT queries (wildcard A record matches but wrong type), blocking Caddy's internal DNS pre-verification step. Co-Authored-By: Claude Sonnet 4.6 --- api/caddy_manager.py | 7 +++++-- api/firewall_manager.py | 12 ++++++++++++ tests/test_caddy_manager.py | 14 ++++++-------- tests/test_firewall_manager.py | 18 +++++++++++++----- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/api/caddy_manager.py b/api/caddy_manager.py index bfcec58..c687021 100644 --- a/api/caddy_manager.py +++ b/api/caddy_manager.py @@ -286,8 +286,11 @@ class CaddyManager(BaseServiceManager): # Resolve credentials at write time — Caddy runs in its own container # and does not inherit the API's environment variables, so we embed the # actual values instead of {$VAR} placeholders. - ddns_token = (os.environ.get('DDNS_TOTP_SECRET') or '').strip() - ddns_api = (os.environ.get('DDNS_URL') or 'https://ddns.pic.ngo/api/v1').strip() + # Use the registration bearer token (ddns.token), NOT the TOTP secret — + # the pic_ngo plugin authenticates to POST /api/v1/dns-challenge with this token. + ddns_cfg = self.config_manager.configs.get('ddns', {}) + ddns_token = (ddns_cfg.get('token') or os.environ.get('DDNS_TOKEN') or '').strip() + ddns_api = (os.environ.get('DDNS_URL') or ddns_cfg.get('url') or 'https://ddns.pic.ngo/api/v1').strip() return ( f"{self._global_acme_block(email)}\n" diff --git a/api/firewall_manager.py b/api/firewall_manager.py index 4ead462..1650714 100644 --- a/api/firewall_manager.py +++ b/api/firewall_manager.py @@ -758,6 +758,18 @@ def generate_corefile(peers: List[Dict[str, Any]], corefile_path: str = COREFILE # *.pic1.pic.ngo to the internal Caddy IP without hairpin NAT. if split_horizon_zones: for sz in split_horizon_zones: + # More-specific block for ACME DNS-01 challenge records: forward + # to public DNS so Caddy can verify TXT records it creates on the + # DDNS server. Without this, the wildcard A record in the zone + # file causes CoreDNS to return NODATA for TXT queries, blocking + # Caddy's internal pre-verification step. + corefile += ( + f'\n_acme-challenge.{sz} {{\n' + f' forward . 8.8.8.8 1.1.1.1\n' + f' cache\n' + f' log\n' + f'}}\n' + ) corefile += ( f'\n{sz} {{\n' f' file /data/{sz}.zone\n' diff --git a/tests/test_caddy_manager.py b/tests/test_caddy_manager.py index 74be7c9..20b8ac7 100644 --- a/tests/test_caddy_manager.py +++ b/tests/test_caddy_manager.py @@ -59,17 +59,16 @@ class TestGenerateCaddyfileLan(unittest.TestCase): class TestGenerateCaddyfilePicNgo(unittest.TestCase): def test_pic_ngo_has_dns_plugin_and_wildcard(self): mgr = _mgr() + mgr.config_manager.configs = { + 'ddns': {'token': 'TESTSECRET123', 'url': 'https://ddns.pic.ngo/api/v1'}, + } identity = {'cell_name': 'alpha', 'domain_mode': 'pic_ngo'} - import os - with unittest.mock.patch.dict(os.environ, { - 'DDNS_TOTP_SECRET': 'TESTSECRET123', - 'DDNS_URL': 'https://ddns.pic.ngo/api/v1', - }): + with unittest.mock.patch.dict(os.environ, {'DDNS_URL': 'https://ddns.pic.ngo/api/v1'}): out = mgr.generate_caddyfile(identity, []) self.assertIn('dns pic_ngo', out) self.assertIn('*.alpha.pic.ngo', out) self.assertIn('alpha.pic.ngo', out) - # Credentials are resolved at write time and embedded — no {$VAR} placeholders + # Registration token (not TOTP secret) is embedded — no {$VAR} placeholders self.assertIn('token TESTSECRET123', out) self.assertIn('api_base_url https://ddns.pic.ngo/api/v1', out) self.assertNotIn('{$PIC_NGO_DDNS_TOKEN}', out) @@ -80,10 +79,9 @@ class TestGenerateCaddyfilePicNgo(unittest.TestCase): def test_pic_ngo_acme_ca_included_when_env_set(self): mgr = _mgr() + mgr.config_manager.configs = {'ddns': {'token': 'TESTSECRET123'}} identity = {'cell_name': 'alpha', 'domain_mode': 'pic_ngo'} - import os with unittest.mock.patch.dict(os.environ, { - 'DDNS_TOTP_SECRET': 'TESTSECRET123', 'DDNS_URL': 'https://ddns.pic.ngo/api/v1', 'ACME_CA_URL': 'https://acme-staging-v02.api.letsencrypt.org/directory', }): diff --git a/tests/test_firewall_manager.py b/tests/test_firewall_manager.py index 70f199b..7e29ea9 100644 --- a/tests/test_firewall_manager.py +++ b/tests/test_firewall_manager.py @@ -241,13 +241,21 @@ class TestGenerateCorefileSplitHorizon(unittest.TestCase): self.assertIn('pic1.pic.ngo {', content) self.assertIn('file /data/pic1.pic.ngo.zone', content) - def test_split_horizon_zone_does_not_add_forward(self): - """Split-horizon blocks must use 'file', not 'forward'.""" + def test_split_horizon_zone_has_acme_challenge_forward(self): + """Each split-horizon zone gets a more-specific _acme-challenge forwarding block + so Caddy's DNS-01 pre-verification bypasses the authoritative local zone.""" firewall_manager.generate_corefile([], self.path, split_horizon_zones=['pic1.pic.ngo']) content = self._content() - # Only the default internet forwarder; no extra forward for the horizon zone - forward_lines = [l for l in content.splitlines() if 'forward' in l and 'pic1' in l] - self.assertEqual(len(forward_lines), 0) + self.assertIn('_acme-challenge.pic1.pic.ngo {', content) + # ACME block appears before the zone file block so CoreDNS matches it first + acme_pos = content.index('_acme-challenge.pic1.pic.ngo {') + zone_pos = content.index('\npic1.pic.ngo {') + self.assertLess(acme_pos, zone_pos) + # ACME block contains a forward directive, not a local file + acme_block_end = content.index('}', acme_pos) + acme_block = content[acme_pos:acme_block_end] + self.assertIn('forward .', acme_block) + self.assertNotIn('file /data/', acme_block) def test_multiple_split_horizon_zones(self): """Multiple zones all get their own file block."""