diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index ddc5bf72..9df059a7 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -3,14 +3,19 @@ Change Log All notable changes to the project are documented in this file. -[UNRELEASED][] --------------- +[v26.08.0][UNRELEASED] +------------------------- ### Changes - Add support for firewall address-set (ipset): named sets of IP addresses and networks, usable as zone sources for per-IP access control, issue #1189 +### Fixes + +- Fix annoying "cannot deselect all services" or reset to YANG default in the + web interface's firewall configuration page + [v26.06.0][] - 2026-07-01 ------------------------- @@ -2214,6 +2219,7 @@ Supported YANG models in addition to those used by sysrepo and netopeer: [buildroot]: https://buildroot.org/ [UNRELEASED]: https://github.com/kernelkit/infix/compare/v26.06.0...HEAD +[v26.08.0]: https://github.com/kernelkit/infix/compare/v26.06.0...v26.08.0 [v26.06.0]: https://github.com/kernelkit/infix/compare/v26.05.0...v26.06.0 [v26.05.0]: https://github.com/kernelkit/infix/compare/v26.04.0...v26.05.0 [v26.04.0]: https://github.com/kernelkit/infix/compare/v26.03.0...v26.04.0 diff --git a/src/webui/internal/handlers/configure_firewall.go b/src/webui/internal/handlers/configure_firewall.go index 72cc1f8b..a69e09c8 100644 --- a/src/webui/internal/handlers/configure_firewall.go +++ b/src/webui/internal/handlers/configure_firewall.go @@ -26,11 +26,6 @@ type cfgFwWrapper struct { Firewall *firewallJSON `json:"infix-firewall:firewall,omitempty"` } -// cfgFwZoneWrapper is used when reading a single zone by path. -type cfgFwZoneWrapper struct { - Zone []zoneJSON `json:"infix-firewall:zone"` -} - // ─── Template display rows ──────────────────────────────────────────────────── type cfgZoneRow struct { @@ -63,6 +58,31 @@ type cfgAddrSetRow struct { EntriesTxt string // one entry per line for the textarea } +func zoneConfigBody(cur zoneJSON) map[string]any { + zone := map[string]any{ + "name": cur.Name, + } + if cur.Action != "" { + zone["action"] = cur.Action + } + if cur.Description != "" { + zone["description"] = cur.Description + } + if len(cur.Interface) > 0 { + zone["interface"] = cur.Interface + } + if len(cur.Network) > 0 { + zone["network"] = cur.Network + } + if len(cur.AddressSet) > 0 { + zone["address-set"] = cur.AddressSet + } + if len(cur.Service) > 0 { + zone["service"] = cur.Service + } + return zone +} + // toSet builds a membership map for template "index" lookups. func toSet(ss []string) map[string]bool { set := make(map[string]bool, len(ss)) @@ -72,11 +92,6 @@ func toSet(ss []string) map[string]bool { return set } -// cfgFwSvcWrapper is used when reading a single service by path. -type cfgFwSvcWrapper struct { - Service []fwServiceJSON `json:"infix-firewall:service"` -} - // ─── Template data ──────────────────────────────────────────────────────────── type cfgFirewallPageData struct { @@ -342,15 +357,15 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque } name := r.PathValue("name") - var wrap cfgFwZoneWrapper + var wrap firewallWrapper // keyed GETs nest the zone under its full parent path if err := h.RC.Get(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), &wrap); err != nil { log.Printf("configure firewall zone save %q: GET: %v", name, err) renderSaveError(w, err) return } cur := zoneJSON{Name: name} - if len(wrap.Zone) > 0 { - cur = wrap.Zone[0] + if len(wrap.Firewall.Zone) > 0 { + cur = wrap.Firewall.Zone[0] } cur.Action = r.FormValue("action") @@ -374,20 +389,7 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque } cur.AddressSet = sets - zone := map[string]any{ - "name": cur.Name, - "action": cur.Action, - "interface": cur.Interface, - "service": cur.Service, - "address-set": cur.AddressSet, - } - if cur.Description != "" { - zone["description"] = cur.Description - } - if len(cur.Network) > 0 { - zone["network"] = cur.Network - } - body := map[string]any{"infix-firewall:zone": []map[string]any{zone}} + body := map[string]any{"infix-firewall:zone": []map[string]any{zoneConfigBody(cur)}} if err := h.RC.Put(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), body); err != nil { log.Printf("configure firewall zone save %q: PUT: %v", name, err) renderSaveError(w, err) @@ -396,55 +398,38 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque renderSavedRedirect(w, "Zone saved", "/configure/firewall") } -// ResetZoneLeafList clears a leaf-list (interface or service) on a zone by -// re-PUTting the zone container without that field. RFC 8040 leaf-list -// DELETE requires per-entry key predicates, so a bulk clear has to go -// through the parent. +// ResetZoneLeafList clears a leaf-list (interface or service) on a zone. +// RFC 8040 leaf-list DELETE requires per-entry key predicates, so each +// instance is deleted individually. Deliberately avoids rebuilding the +// zone with PUT: the reset must not be able to touch anything else. func (h *ConfigureFirewallHandler) resetZoneLeafList(w http.ResponseWriter, r *http.Request, leaf string) { name := r.PathValue("name") - var wrap cfgFwZoneWrapper + var wrap firewallWrapper // keyed GETs nest the zone under its full parent path if err := h.RC.Get(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), &wrap); err != nil { log.Printf("configure firewall zone reset %s/%s: GET: %v", name, leaf, err) renderSaveError(w, err) return } - cur := zoneJSON{Name: name} - if len(wrap.Zone) > 0 { - cur = wrap.Zone[0] - } - switch leaf { - case "interface": - cur.Interface = nil - case "service": - cur.Service = nil + var values []string + if len(wrap.Firewall.Zone) > 0 { + switch leaf { + case "interface": + values = wrap.Firewall.Zone[0].Interface + case "service": + values = wrap.Firewall.Zone[0].Service + } } - zone := map[string]any{ - "name": cur.Name, - "action": cur.Action, + for _, val := range values { + path := fmt.Sprintf("%s/zone=%s/%s=%s", fwConfigPath, + restconf.EscapeKey(name), leaf, restconf.EscapeKey(val)) + if err := h.RC.Delete(r.Context(), path); err != nil && !restconf.IsNotFound(err) { + log.Printf("configure firewall zone reset %s/%s=%s: %v", name, leaf, val, err) + renderSaveError(w, err) + return + } } - if cur.Description != "" { - zone["description"] = cur.Description - } - if len(cur.Interface) > 0 { - zone["interface"] = cur.Interface - } - if len(cur.Network) > 0 { - zone["network"] = cur.Network - } - if len(cur.AddressSet) > 0 { - zone["address-set"] = cur.AddressSet - } - if len(cur.Service) > 0 { - zone["service"] = cur.Service - } - body := map[string]any{"infix-firewall:zone": []map[string]any{zone}} - if err := h.RC.Put(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), body); err != nil { - log.Printf("configure firewall zone reset %s/%s: PUT: %v", name, leaf, err) - renderSaveError(w, err) - return - } - renderSaved(w, "Reset to default") + renderSavedRedirect(w, "Reset to default", "/configure/firewall") } // ResetZoneInterfaces clears the zone's interface leaf-list. diff --git a/src/webui/internal/handlers/configure_firewall_test.go b/src/webui/internal/handlers/configure_firewall_test.go index 518fecd2..4d4b44db 100644 --- a/src/webui/internal/handlers/configure_firewall_test.go +++ b/src/webui/internal/handlers/configure_firewall_test.go @@ -82,9 +82,10 @@ func TestConfigureFirewallOverview_AddressSets(t *testing.T) { type recordingFetcher struct { *testutil.MockFetcher - putCalls int - lastPath string - lastBody any + putCalls int + lastPath string + lastBody any + deletePaths []string } func (r *recordingFetcher) Put(_ context.Context, path string, body any) error { @@ -94,15 +95,29 @@ func (r *recordingFetcher) Put(_ context.Context, path string, body any) error { return nil } +func (r *recordingFetcher) Delete(_ context.Context, path string) error { + r.deletePaths = append(r.deletePaths, path) + return nil +} + +// zoneGetResponse mimics the server's response shape for a keyed zone GET: +// the zone is nested under its full parent path, not returned bare. +func zoneGetResponse(zone map[string]any) map[string]any { + return map[string]any{ + "infix-firewall:firewall": map[string]any{ + "zone": []map[string]any{zone}, + }, + } +} + func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T) { mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()} - mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", map[string]any{ - "infix-firewall:zone": []map[string]any{{ - "name": "public", - "action": "drop", - "interface": []string{"eth0"}, - }}, - }) + mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{ + "name": "public", + "action": "drop", + "interface": []string{"eth0"}, + "network": []string{"10.0.0.0/24"}, + })) h := &ConfigureFirewallHandler{ Template: minimalCfgFwTmpl, @@ -153,6 +168,9 @@ func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T) if got, want := zone["address-set"], []string{"allowed"}; !reflect.DeepEqual(got, want) { t.Fatalf("want address-sets %#v got %#v", want, got) } + if got, want := zone["network"], []string{"10.0.0.0/24"}; !reflect.DeepEqual(got, want) { + t.Fatalf("want networks preserved %#v got %#v", want, got) + } var trig map[string]string if err := json.Unmarshal([]byte(w.Header().Get("HX-Trigger")), &trig); err != nil { t.Fatalf("unmarshal HX-Trigger: %v", err) @@ -161,3 +179,99 @@ func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T) t.Fatalf("unexpected success message %q", got) } } + +func TestConfigureFirewallSaveZoneClearsAllServices(t *testing.T) { + mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()} + mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{ + "name": "public", + "action": "drop", + "interface": []string{"eth0"}, + "service": []string{"ssh", "http"}, + })) + + h := &ConfigureFirewallHandler{ + Template: minimalCfgFwTmpl, + RC: mock, + Schema: schema.NewCache(mock, t.TempDir()), + } + + form := url.Values{ + "action": {"drop"}, + "description": {"Public zone"}, + "interfaces": {"eth0"}, + } + req := httptest.NewRequest(http.MethodPost, "/configure/firewall/zones/public", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetPathValue("name", "public") + ctx := restconf.ContextWithCredentials(req.Context(), restconf.Credentials{ + Username: "admin", + Password: "admin", + }) + ctx = security.WithToken(ctx, "test-csrf-token") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + h.SaveZone(w, req) + + if mock.putCalls != 1 { + t.Fatalf("want 1 PUT call got %d", mock.putCalls) + } + body, ok := mock.lastBody.(map[string]any) + if !ok { + t.Fatalf("unexpected PUT body type %T", mock.lastBody) + } + zones, ok := body["infix-firewall:zone"].([]map[string]any) + if !ok || len(zones) != 1 { + t.Fatalf("unexpected PUT zone payload %#v", body["infix-firewall:zone"]) + } + if _, ok := zones[0]["service"]; ok { + t.Fatalf("expected cleared services to be omitted from payload, got %#v", zones[0]["service"]) + } +} + +func TestConfigureFirewallResetZoneServicesOnlyDeletesServices(t *testing.T) { + mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()} + mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{ + "name": "public", + "action": "drop", + "interface": []string{"eth0"}, + "address-set": []string{"allowed"}, + "service": []string{"ssh", "dhcpv6-client"}, + })) + + h := &ConfigureFirewallHandler{ + Template: minimalCfgFwTmpl, + RC: mock, + Schema: schema.NewCache(mock, t.TempDir()), + } + + req := httptest.NewRequest(http.MethodDelete, "/configure/firewall/zones/public/services", nil) + req.SetPathValue("name", "public") + ctx := restconf.ContextWithCredentials(req.Context(), restconf.Credentials{ + Username: "admin", + Password: "admin", + }) + ctx = security.WithToken(ctx, "test-csrf-token") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + h.ResetZoneServices(w, req) + + if w.Code != http.StatusNoContent { + t.Fatalf("want 204 got %d; body: %s", w.Code, w.Body.String()) + } + if got, want := w.Header().Get("HX-Location"), `{"path":"/configure/firewall","target":"#content"}`; got != want { + t.Fatalf("want HX-Location %q got %q", want, got) + } + if mock.putCalls != 0 { + t.Fatalf("reset must not rewrite the zone, got %d PUT call(s) with body %#v", + mock.putCalls, mock.lastBody) + } + want := []string{ + candidatePath + "/infix-firewall:firewall/zone=public/service=ssh", + candidatePath + "/infix-firewall:firewall/zone=public/service=dhcpv6-client", + } + if !reflect.DeepEqual(mock.deletePaths, want) { + t.Fatalf("want DELETE paths %#v got %#v", want, mock.deletePaths) + } +} diff --git a/src/webui/internal/restconf/client.go b/src/webui/internal/restconf/client.go index 0f647090..50cc14aa 100644 --- a/src/webui/internal/restconf/client.go +++ b/src/webui/internal/restconf/client.go @@ -9,8 +9,10 @@ import ( "encoding/json" "fmt" "io" + "log" "net/http" "net/url" + "reflect" "strings" "time" ) @@ -79,6 +81,12 @@ func (c *Client) doRequest(ctx context.Context, method, path string) (*http.Resp // Get fetches a RESTCONF resource, decoding the JSON response into target. // User credentials are taken from the request context (set by auth middleware). +// Get sends a GET request and decodes the JSON response into target. +// +// NOTE: deep-path GETs (keyed list entries, nested containers) return the +// requested node nested under its full parent path, not bare as the RFC +// 8040 examples may suggest. Wrapper structs must model the nesting from +// the top-level container down, or the decode silently matches nothing. func (c *Client) Get(ctx context.Context, path string, target any) error { resp, err := c.doRequest(ctx, http.MethodGet, path) if err != nil { @@ -88,7 +96,20 @@ func (c *Client) Get(ctx context.Context, path string, target any) error { if resp.StatusCode != http.StatusOK { return parseError(resp) } - return json.NewDecoder(resp.Body).Decode(target) + data, err := io.ReadAll(resp.Body) + if err != nil { + return err + } + if err := json.Unmarshal(data, target); err != nil { + return err + } + // Tripwire for the nesting trap above: content arrived but nothing + // in the wrapper matched it. + if v := reflect.ValueOf(target); len(data) > 2 && + v.Kind() == reflect.Pointer && v.Elem().IsZero() { + log.Printf("restconf: GET %s decoded to zero %T, wrapper shape mismatch?", path, target) + } + return nil } // Post sends a POST request to a RESTCONF RPC endpoint.