Skip to content

Commit 84e9c28

Browse files
itcmsgrclaude
itcmsgr
and
claude
committed
installer: address PR26.4 auditor conditions A–E
A. Explicit port-22 negative regression guard - TestRequiredPorts_ConfDDoesNotIncludeSSHPort22: dedicated test proving DirectAdmin RequiredPorts (TCP_IN + UDP_IN) does NOT include port 22. Independent of full-surface identity test so a future conf.d edit re-introducing 22 trips a clearly-named failure. Comment cites the four-truth rule (conf.d wins, SSH managed by /etc/nftban/ports.d/00-ssh.conf, shell-library port 22 inclusion is stale). B. Fail-closed branches — no fallback to [2222] under any error - assertNoControlPlaneFallback test helper added. - TestRequiredPorts_MissingConfD_FailsClosed: now also asserts returned tcp/udp slices are nil/empty (no [2222] fallback). - TestRequiredPorts_EmptyTCPIn_FailsClosed: same. - TestRequiredPorts_NilPanelConfig_FailsClosed: same. - TestRequiredPorts_RealLoader_MissingConfD_FailsClosed (was already present; the structural shape of fail-closed is now covered uniformly). C. Range-form (35000-35999) regression guard - TestRequiredPorts_RealLoader_RangeExpansion_LengthAndEndpoints: fixture conf.d with the canonical TCP_IN; asserts EXACT length 14 + 1000 = 1014, both endpoints (35000 and 35999) present, a mid-range port (35500) present, every discrete declared port present, and SSH 22 still excluded. Catches a future loader change that drops range expansion or shifts the boundary. D. Removed stale "PR26.4 follow-up" doc-comment from ValidateReachability — replaced with PR26.4-current text noting that RequiredPorts now loads the full conf.d surface but ValidateReachability still probes only the control plane. The ValidateReachability error message no longer says "validated in PR26.4"; new wording: "loaded from conf.d via RequiredPorts but not probed here". E. Renamed misleading test TestFrameworkIntegration_DA_Reason_DoesNotImplyFullPortSurvival → TestFrameworkIntegration_DA_ControlPlaneError_DoesNotClaimFullSurfaceReachability The semantic is unchanged (control-plane error must not claim full-surface probing), but the name now reflects post-PR26.4 reality where RequiredPorts does load the full surface declaratively. No production code paths changed beyond the doc-comment + error wording. All test additions/changes are test-file only. Lab4 proof (post A–E, base 5366caf): go vet ... clean go test -v panelfw/ports/validate 100 sub-tests PASS, 0 FAIL go test ./... 66 packages PASS, 0 FAIL Hard exclusions preserved: no cPanel/Plesk/other adapters; no shell decommission; no parser rewrite; no restore/firewall/authority changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8244c7b commit 84e9c28

2 files changed

Lines changed: 154 additions & 21 deletions

File tree

‎internal/installer/panelfw/adapters/directadmin/directadmin.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -224,17 +224,21 @@ func (a *adapter) RequiredPorts(ctx context.Context, exec executor.Executor) ([]
224224
// state. Returns nil on success; a structured error otherwise. Does
225225
// NOT mutate ports, services, or rules.
226226
//
227-
// Scope is the control plane (default 2222) only. The full DirectAdmin
228-
// service-port surface (mail/web/SSH/etc.) is NOT validated here —
229-
// see the file-level "PR26.4 follow-up" comment.
227+
// Scope is the control plane (default 2222 or per-`directadmin.conf`
228+
// override) only. RequiredPorts (PR26.4) loads the full DirectAdmin
229+
// service-port surface from the canonical conf.d via panel_loader, but
230+
// this method deliberately does NOT probe each conf.d-declared port —
231+
// full-surface reachability probing is intentionally out of scope here
232+
// and remains the broader rebuild/validate path's responsibility.
230233
func (a *adapter) ValidateReachability(ctx context.Context, exec executor.Executor) error {
231234
port := readConfiguredPort(exec)
232235
if portInListenState(exec, port) {
233236
return nil
234237
}
235238
return fmt.Errorf(
236239
"DirectAdmin control-plane port %d not in LISTEN state — control-plane unreachable "+
237-
"(note: this assertion validates the control plane only; full DirectAdmin port surface validated in PR26.4)",
240+
"(this assertion probes the control plane only; the full DirectAdmin port surface is loaded "+
241+
"from conf.d via RequiredPorts but not probed here)",
238242
port)
239243
}
240244

‎internal/installer/panelfw/adapters/directadmin/directadmin_test.go‎

Lines changed: 146 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,8 @@ func TestRequiredPorts_ConfDLoaded_NotJust2222(t *testing.T) {
281281
}
282282

283283
// PR26.4 R4: missing conf.d main.conf must fail closed.
284+
// Loader returns (nil, err) — adapter must propagate error AND must
285+
// NOT silently fall back to [2222].
284286
func TestRequiredPorts_MissingConfD_FailsClosed(t *testing.T) {
285287
withStubLoader(t, func(configDir, panelName string) (*ports.PanelConfig, error) {
286288
return nil, fmt.Errorf("panel config not found: %s/conf.d/panels/%s/main.conf",
@@ -294,9 +296,12 @@ func TestRequiredPorts_MissingConfD_FailsClosed(t *testing.T) {
294296
if !strings.Contains(err.Error(), "DirectAdmin conf.d") {
295297
t.Errorf("error must reference DirectAdmin conf.d; got %v", err)
296298
}
299+
assertNoControlPlaneFallback(t, tcp, udp)
297300
}
298301

299302
// PR26.4 R5: malformed conf.d (loaded but empty TCP_IN) must fail closed.
303+
// Loader returns a PanelConfig with empty TCPIn — adapter must error
304+
// AND must NOT fall back to [2222].
300305
func TestRequiredPorts_EmptyTCPIn_FailsClosed(t *testing.T) {
301306
withStubLoader(t, func(configDir, panelName string) (*ports.PanelConfig, error) {
302307
return &ports.PanelConfig{
@@ -307,23 +312,139 @@ func TestRequiredPorts_EmptyTCPIn_FailsClosed(t *testing.T) {
307312
}, nil
308313
})
309314

310-
_, _, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor())
315+
tcp, udp, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor())
311316
if err == nil {
312317
t.Fatalf("expected error on empty TCP_IN")
313318
}
314319
if !strings.Contains(err.Error(), "no TCP_IN") {
315320
t.Errorf("error must explain malformed TCP_IN; got %v", err)
316321
}
322+
assertNoControlPlaneFallback(t, tcp, udp)
317323
}
318324

319-
// Loader returning nil PanelConfig (defensive).
325+
// Loader returning nil PanelConfig (defensive). Loader returns
326+
// (nil, nil) — adapter must error AND must NOT fall back to [2222].
320327
func TestRequiredPorts_NilPanelConfig_FailsClosed(t *testing.T) {
321328
withStubLoader(t, func(configDir, panelName string) (*ports.PanelConfig, error) {
322329
return nil, nil
323330
})
324-
if _, _, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor()); err == nil {
331+
tcp, udp, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor())
332+
if err == nil {
325333
t.Fatalf("expected error when loader returns nil cfg")
326334
}
335+
assertNoControlPlaneFallback(t, tcp, udp)
336+
}
337+
338+
// PR26.4 condition A: explicit regression guard — port 22 (SSH) must
339+
// NOT appear in DirectAdmin RequiredPorts output. The canonical
340+
// conf.d intentionally excludes 22 (managed separately by
341+
// /etc/nftban/ports.d/00-ssh.conf); the legacy shell library
342+
// historically included 22 (four-truth drift). Conf.d wins.
343+
//
344+
// This test is independent of the full-surface identity test so a
345+
// future conf.d edit that re-introduces 22 trips a clearly-named
346+
// failure even if the surface-identity test has been amended.
347+
func TestRequiredPorts_ConfDDoesNotIncludeSSHPort22(t *testing.T) {
348+
withStubLoader(t, func(configDir, panelName string) (*ports.PanelConfig, error) {
349+
return &ports.PanelConfig{
350+
Name: "directadmin",
351+
Enabled: true,
352+
TCPIn: canonicalDA.tcpIn,
353+
UDPIn: canonicalDA.udpIn,
354+
}, nil
355+
})
356+
tcp, udp, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor())
357+
if err != nil {
358+
t.Fatalf("unexpected error from canonical DA stub: %v", err)
359+
}
360+
if containsInt(tcp, 22) {
361+
t.Errorf("DirectAdmin RequiredPorts TCP_IN must NOT include port 22 — "+
362+
"SSH is managed by /etc/nftban/ports.d/00-ssh.conf, conf.d wins over shell library; got %v", tcp)
363+
}
364+
if containsInt(udp, 22) {
365+
t.Errorf("DirectAdmin RequiredPorts UDP_IN must NOT include port 22; got %v", udp)
366+
}
367+
}
368+
369+
// PR26.4 condition C: range-form (35000-35999) regression guard.
370+
// internal/ports/panel_loader.parsePortList expands ranges into
371+
// individual ints (35000..35999 = 1000 values). Verify the loader
372+
// integration produces the expected expanded length and both endpoints
373+
// so a future loader change that drops range expansion or shifts the
374+
// boundary surfaces here.
375+
//
376+
// Canonical conf.d declares:
377+
//
378+
// TCP_IN: 14 discrete + 35000-35999 range = 14 + 1000 = 1014 ports
379+
func TestRequiredPorts_RealLoader_RangeExpansion_LengthAndEndpoints(t *testing.T) {
380+
if _, err := os.Stat("/bin/bash"); err != nil {
381+
t.Skipf("/bin/bash unavailable on this host: %v", err)
382+
}
383+
const fixtureMain = `
384+
NFTBAN_DIRECTADMIN_PATH="/usr/local/directadmin"
385+
NFTBAN_DIRECTADMIN_PANEL_PORT="2222"
386+
NFTBAN_DIRECTADMIN_TCP_IN="20,21,25,53,853,80,110,143,443,465,587,993,995,2222,35000-35999"
387+
NFTBAN_DIRECTADMIN_UDP_IN="20,21,53,853,80,443"
388+
`
389+
withFixtureConfD(t, fixtureMain)
390+
391+
tcp, udp, err := New().RequiredPorts(context.Background(), executor.NewMockExecutor())
392+
if err != nil {
393+
t.Fatalf("real-loader RequiredPorts: %v", err)
394+
}
395+
396+
// Exact length: 14 discrete + 1000 expanded range = 1014.
397+
const expectedTCP = 14 + 1000
398+
if len(tcp) != expectedTCP {
399+
t.Errorf("TCP_IN length = %d; want %d (14 discrete + 1000-port range expansion)",
400+
len(tcp), expectedTCP)
401+
}
402+
// Both range endpoints must be present.
403+
if !containsInt(tcp, 35000) {
404+
t.Errorf("TCP_IN must include range start 35000; got %v ports total", len(tcp))
405+
}
406+
if !containsInt(tcp, 35999) {
407+
t.Errorf("TCP_IN must include range end 35999; got %v ports total", len(tcp))
408+
}
409+
// Spot-check one mid-range port to confirm the loader didn't only
410+
// keep endpoints.
411+
if !containsInt(tcp, 35500) {
412+
t.Errorf("TCP_IN must include mid-range port 35500 (loader range expansion broken?)")
413+
}
414+
// Every discrete declared port must be present.
415+
for _, p := range []int{20, 21, 25, 53, 853, 80, 110, 143, 443, 465, 587, 993, 995, 2222} {
416+
if !containsInt(tcp, p) {
417+
t.Errorf("TCP_IN missing discrete declared port %d", p)
418+
}
419+
}
420+
// SSH port 22 still excluded even with the real loader.
421+
if containsInt(tcp, 22) {
422+
t.Errorf("real loader produced TCP_IN containing port 22 — conf.d four-truth violation; got %v", tcp)
423+
}
424+
// UDP_IN exact length and contents.
425+
wantUDP := []int{20, 21, 53, 853, 80, 443}
426+
if len(udp) != len(wantUDP) {
427+
t.Errorf("UDP_IN length = %d; want %d", len(udp), len(wantUDP))
428+
}
429+
for _, p := range wantUDP {
430+
if !containsInt(udp, p) {
431+
t.Errorf("UDP_IN missing %d", p)
432+
}
433+
}
434+
}
435+
436+
// assertNoControlPlaneFallback is a fail-closed assertion helper: when
437+
// RequiredPorts errors, the returned slices must be nil/empty — never
438+
// the legacy [2222] fallback. This catches a regression where a future
439+
// edit re-introduces the pre-PR26.4 default-port behavior.
440+
func assertNoControlPlaneFallback(t *testing.T, tcp, udp []int) {
441+
t.Helper()
442+
if len(tcp) != 0 {
443+
t.Errorf("fail-closed: tcp must be nil/empty on error — must NOT fall back to [2222]; got %v", tcp)
444+
}
445+
if len(udp) != 0 {
446+
t.Errorf("fail-closed: udp must be nil/empty on error; got %v", udp)
447+
}
327448
}
328449

329450
// PR26.4 defensive: returned slices must not alias internal loader state.
@@ -500,10 +621,12 @@ func TestValidateReachability_NotListening_ErrorMentionsControlPlane(t *testing.
500621
t.Errorf("error must explicitly say 'control-plane'; got %q", msg)
501622
}
502623
// Negative: the message must NOT make affirmative claims of full
503-
// survival or full surface validation. We allow the explanatory
504-
// negation form ("full DirectAdmin port surface validated in
505-
// PR26.4") because it reads as scope clarification, not a claim
506-
// the assertion has validated those ports.
624+
// surface probing. The PR26.4-shape error mentions "full
625+
// DirectAdmin port surface" as part of an explanatory negation
626+
// ("loaded from conf.d via RequiredPorts but not probed here") —
627+
// that's scope clarification, not a claim the method has probed
628+
// those ports. The forbidden list below catches affirmative-claim
629+
// verbs only.
507630
for _, forbidden := range []string{
508631
"full panel survival validated",
509632
"full panel survived",
@@ -637,11 +760,16 @@ func TestFrameworkIntegration_DA_Detected_NotReachable_Blocks(t *testing.T) {
637760
}
638761
}
639762

640-
// PR26.3 Path A: the surfaced Reason on a failing DA host must NOT
641-
// claim that the full panel survival was checked — the assertion
642-
// covers only the control plane in PR26.3. Full port-surface
643-
// validation lands in PR26.4.
644-
func TestFrameworkIntegration_DA_Reason_DoesNotImplyFullPortSurvival(t *testing.T) {
763+
// PR26.4: the surfaced Reason on a control-plane-unreachable host
764+
// must NOT claim full-surface reachability has been probed. After
765+
// PR26.4, RequiredPorts loads the full DirectAdmin port surface from
766+
// conf.d (panel_loader), but ValidateReachability still probes the
767+
// control plane only — so the Reason on failure must read as a
768+
// control-plane miss, not as "all 1014 panel ports unreachable".
769+
//
770+
// Renamed from TestFrameworkIntegration_DA_Reason_DoesNotImplyFullPortSurvival
771+
// so the name matches the post-PR26.4 semantics.
772+
func TestFrameworkIntegration_DA_ControlPlaneError_DoesNotClaimFullSurfaceReachability(t *testing.T) {
645773
stubCanonicalDA(t)
646774

647775
a := New()
@@ -657,10 +785,11 @@ func TestFrameworkIntegration_DA_Reason_DoesNotImplyFullPortSurvival(t *testing.
657785
if !res.Fatal {
658786
t.Fatalf("expected Fatal=true; got %#v", res)
659787
}
660-
// These phrases would imply the assertion validated more than the
661-
// control plane. The error MAY mention "full ... port surface" in
662-
// a NEGATION (e.g., "...full DirectAdmin port surface validated in
663-
// PR26.4"), so we look for affirmative-claim verbs instead.
788+
// Affirmative-claim phrases that would overstate the assertion's
789+
// scope. The error MAY mention "full ... port surface" inside a
790+
// negation (the PR26.4 wording: "loaded from conf.d via
791+
// RequiredPorts but not probed here"); that is intentional scope
792+
// clarification, not a claim the method has probed those ports.
664793
for _, forbidden := range []string{
665794
"full panel survival validated",
666795
"full panel survived",
@@ -670,7 +799,7 @@ func TestFrameworkIntegration_DA_Reason_DoesNotImplyFullPortSurvival(t *testing.
670799
"all panel ports validated",
671800
} {
672801
if strings.Contains(res.Reason, forbidden) {
673-
t.Errorf("Reason must NOT claim %q (overstates PR26.3 scope); got %q", forbidden, res.Reason)
802+
t.Errorf("Reason must NOT claim %q (control-plane probe only); got %q", forbidden, res.Reason)
674803
}
675804
}
676805
}

0 commit comments

Comments
 (0)