Skip to content

fix(sonicwall): recover addresses, firewall actions and event outcomes (+ committed contract test) - #2756

Merged
osmontero merged 3 commits into
v11from
sonicwall-admin-auth-contract
Sep 25, 2026
Merged

osmontero merged 3 commits into
v11from
sonicwall-admin-auth-contract

Conversation

@osmontero

Copy link
Copy Markdown
Member

Carries #2743 SonicWall filter+rule fix (CEF header non-empty pattern; per-shape src/dst grok with inCIDR guards; fw_action fallback; eventCode outcome mapping) PLUS a committed raw-replay contract test that #2743 was missing.

sonicwall_contract_test.go models the engine grok/kv/rename/trim/cast/add/delete steps (mirrors the merged fortigate contract test), replays 6 admin-auth logins, and asserts origin.ip/origin.port/log.eventCode/actionResult plus that the shipped admin_auth_failures where clause fires on bad-credential events (32/33/200) and not on allowed (29) / policy-denied (986).

Verified discriminating: PASSES on this filter, FAILS on the base v11 filter (origin.ip and actionResult absent). Merging this subsumes #2743; #2743 can be closed.

kryonsx and others added 3 commits September 24, 2026 19:18
The address step splits src and dst only as ip:port:interface. The grok
step writes nothing unless every pattern matches and the filter then
deletes the value, so ip:port (VPN negotiation events) and
ip::interface (user logins) lose their address. When fw_action is the
last key, the rescue grok cannot bound it and the fallback rename reads
log.fwaction, while go-sdk v1.1.35 and later keep the underscore, so
those events get no action and no outcome. Events whose firewall action
is NA get no outcome at all, and the CEF header step captures its fields
with a lone lazy pattern, which the grok step rejects.

Each address shape now has its own step, and the original value is
deleted only after an address was read. A fallback reads fw_action
under both spellings. Events without a firewall decision get an outcome
from the meaning of their message: allowed logins and completed IKEv2
negotiations give success, as does Connection Closed when bytes came
back; bad credentials, RADIUS, LDAP, XAUTH and SSO failures and failed
IKE negotiations give failure; policy and zone refusals give denied.
The CEF header fields use a non-empty pattern.

The administrator authentication-failure rule counted "Administrator
login allowed" (29) as a failure and, through its message branch, SSO
probe failures and policy denials. It now counts logins denied due to
bad credentials with a failure outcome, its history counts failures from
the same address, and alerts group by address, so a password spray is
one alert instead of one per user name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grouping by address only nests repeats: every attempt that passes the
history check still creates a child alert. On the busiest day in the
last week one deployment had 41,227 bad-credential denials from 41
addresses, which would create 40,756 alerts. Deduplicating by address
creates 15 and keeps one alert per source for seven days.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#2743 shipped no in-repo test (all evidence was out-of-band playground).
Adds sonicwall_contract_test.go: an engine-faithful grok model (mirrors the
merged fortigate contract test) that replays 6 admin-auth logins through the
ordered filter and asserts origin.ip/origin.port/log.eventCode/actionResult
plus that the shipped admin_auth_failures where: clause fires on the
bad-credential cases (32/33/200) and not on allowed (29) / policy-denied (986).
Verified discriminating: PASSES on this filter, FAILS on the base v11 filter
(origin.ip and actionResult absent). Built at #2743's head.
@osmontero
osmontero requested a review from a team September 25, 2026 16:08
@osmontero
osmontero merged commit b7ba297 into v11 Sep 25, 2026
5 checks passed
@osmontero
osmontero deleted the sonicwall-admin-auth-contract branch September 25, 2026 16:09
@github-actions

Copy link
Copy Markdown

❌ Go dependencies check failed

There are outdated Go dependencies, or modules that could not be inspected.
Run bash .github/scripts/go-deps.sh --update --discover locally and
commit the updated go.mod / go.sum files.

Script output
🔍 Discovered 25 Go projects

📦 Dependencies with updates available:

  📁 ./utmstack-collector:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/gcp:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/aws:
     - github.com/aws/aws-sdk-go-v2: v1.47.0 → v1.47.1
     - github.com/aws/aws-sdk-go-v2/config: v1.33.5 → v1.33.6
     - github.com/aws/aws-sdk-go-v2/credentials: v1.20.5 → v1.20.6
     - github.com/aws/aws-sdk-go-v2/service/cloudwatchlogs: v1.88.0 → v1.88.1
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/events:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/inputs:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/stats:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/rule-flood-guard:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/o365:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/modules-config:
     - github.com/aws/aws-sdk-go-v2/config: v1.33.5 → v1.33.6
     - github.com/aws/aws-sdk-go-v2/credentials: v1.20.5 → v1.20.6
     - github.com/aws/aws-sdk-go-v2/service/cloudwatchlogs: v1.88.0 → v1.88.1
     - github.com/aws/aws-sdk-go-v2/service/sts: v1.51.0 → v1.51.1
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/config:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/soc-ai:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/sophos:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/azure:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/crowdstrike:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/bitdefender:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./plugins/geolocation:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./agent-manager:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./agent:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./as400:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

  📁 ./as400/updater:
     - github.com/threatwinds/go-sdk: v1.1.34 → v1.1.36

�[0;31m❌ Please update dependencies before merging.�[0m

@github-actions

Copy link
Copy Markdown

✅ AI review — Approved with warnings

Only minor (medium/low) issues were found. They won't block the merge, but consider addressing them.

✅ architecture (silas-1.7-pro) — clean

Summary: Adds an offline contract test and fixtures only; no production, contract, migration, agent, CI, or shared-module structural changes detected.

No findings.

⚠️ bugs (silas-1.7-pro) — non-blocking warnings

Summary: New SonicWall contract tests are test-only but include weak assertions, unchecked type assertions, and a grok helper that does not enforce documented non-empty anonymous groups.

  • medium plugins/alerts/sonicwall_contract_test.go:130 — sonicGrokMatches skips anonymous groups (continue on empty FieldName) even though the comment says every group must be non-empty; a grok step with an anonymous group that matches empty will write fields when the modeled EventProcessor behavior should reject. Add checks for all capture indices.
  • medium plugins/alerts/sonicwall_contract_test.go:243 — kv handler uses v.(string) after only checking existence; if s.Kv.Source resolves to nil or a non-string (e.g. numeric after cast), the test panics. Use the comma-ok form or t.Fatalf.
  • medium plugins/alerts/sonicwall_contract_test.go:306 — Expected values are compared with fmt.Sprint, so type mismatches pass (string "4455" equals number 4455). This hides cast bugs for fields such as origin.port. Compare Go types or require exact JSON types.
  • low plugins/alerts/sonicwall_contract_test.go:233 — add handler assumes Params contains key and value; if missing, GetStringValue returns an empty path and AsInterface returns nil, writing an unexpected top-level key. Validate Params before sonicPut.
  • low plugins/alerts/sonicwall_contract_test.go:43 — sonicPut overwrites an existing non-map value when writing a nested path, silently losing data: m{a: scalar}; sonicPut(m, a.b, x, false) replaces a with a map. Consider returning an error or preserving the scalar.

✅ security (silas-1.7-pro) — clean

Summary: Test-only changes; no new vulnerabilities or customer-facing information disclosure identified.

No findings.

@utmstackprapprover utmstackprapprover Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested — Go dependencies check failed (see above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants