Skip to content

feat: add port security awareness panel to firewall page#12704

Open
zhangt2333 wants to merge 2 commits into1Panel-dev:dev-v2from
zhangt2333:dev-v2
Open

feat: add port security awareness panel to firewall page#12704
zhangt2333 wants to merge 2 commits into1Panel-dev:dev-v2from
zhangt2333:dev-v2

Conversation

@zhangt2333
Copy link
Copy Markdown

@zhangt2333 zhangt2333 commented May 10, 2026

Thanks for building 1Panel — I am a commercial license holder and use it daily.

What this PR does / why we need it?

While using the firewall module extensively, we discovered that Docker container port mappings bypass iptables INPUT chain rules, so ports that appear protected by the firewall may actually be publicly exposed. This issue has existed almost since 1Panel's initial release, spanning 3+ years (2023–2026) with 18 related community reports:

Fully solving the Docker bypass problem (e.g., managing DOCKER-USER chain rules or integrating ufw-docker) would require significant changes to the firewall architecture. This PR takes a deliberate first step — giving users visibility into which ports are effectively protected and which are exposed through Docker. Once awareness is in place, further enforcement mechanisms can be built on top of it incrementally.

Summary of your change

  • New API endpoint (POST /hosts/firewall/port/security, admin-only) — concurrently scans listening ports, Docker container bindings, and firewall rules to classify each port's effective security status
  • New panel component (firewall/port/awareness/index.vue) above the existing rules table
  • Small review surface — only port/index.vue is modified in existing code to import and render the new panel; the rest is additive
  • i18n keys added to all 10 locale files (zh/en translated manually; others use English fallback — native-speaker review welcome)

Screenshots:

image image image image

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
    • No existing test suite for the firewall service module; verified manually. Happy to add unit tests if maintainers prefer.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented May 10, 2026

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented May 10, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wanghe-fit2cloud for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@HynoR
Copy link
Copy Markdown
Contributor

HynoR commented May 11, 2026

对firewalld 和 ufw 做适配测试了么

@HynoR
Copy link
Copy Markdown
Contributor

HynoR commented May 11, 2026

docker network bind 可以做的很复杂,这里也没有做复杂状态下判断

@HynoR
Copy link
Copy Markdown
Contributor

HynoR commented May 11, 2026

想法不错,但是建议多打磨打磨(你目前应该是在用的 ChatGPT codex,可以让他再多看看...)

@wanghe-fit2cloud
Copy link
Copy Markdown
Member

感谢贡献,这个方向是有价值的,Docker 端口映射绕过常规 INPUT 链规则确实容易让用户误判端口暴露状态。

不过当前实现暂不建议合并。主要原因是扫描逻辑还不够准确:Docker 发布端口不一定会表现为宿主机监听 socket,当前以监听端口为主数据源可能漏掉真正需要提示的 Docker 暴露端口;同时对绑定地址、防火墙规则、firewalld / ufw 以及复杂 Docker network bind 场景的判断还比较粗,可能给用户“已保护/安全”的错误结论。

建议继续打磨后再提交,至少需要补充 firewalld、ufw、iptables 以及不同 Docker 端口绑定方式下的验证,并调整判断逻辑和文案,避免安全状态误判。

@zhangt2333
Copy link
Copy Markdown
Author

@HynoR @wanghe-fit2cloud Thank you very much for your valuable feedback.

I'm currently continuing to work on this. I really need this feature and am strongly motivated to implement it well in 1Panel.

Could you please re-open this PR? There's no need to open another PR — I will set the current one to draft status. I'll keep pushing updates and will notify you when it's ready.

@zhangt2333 zhangt2333 marked this pull request as draft May 11, 2026 03:32
@zhangt2333 zhangt2333 marked this pull request as ready for review May 11, 2026 06:52
@zhangt2333
Copy link
Copy Markdown
Author

zhangt2333 commented May 11, 2026

Thank you very much for your valuable feedback — addressing each concern below.


1. Docker-published ports that aren't host LISTEN sockets

The classifier now consults two sources: host LISTEN sockets (gopsutil) and the Docker container port list (c.Ports[]). When the LISTEN-socket source returns nothing — e.g. Docker daemon with {"userland-proxy": false}, where iptables DNAT handles forwarding and no docker-proxy binds the host port — c.Ports[] still records the publish, so the fallback catches it.

Modes where neither source has data (macvlan / ipvlan / overlay / Swarm ingress) fall under the explicit scope limit in §7.

2. Bind-address judgment

localOnly is restricted to addresses provably unreachable from outside the host: loopback (127.0.0.0/8 / ::1) and link-local (fe80::/10). Any specific non-loopback IP (public NIC, docker bridge gateway 172.17.0.1, private LAN) goes through the firewall-rule check — e.g. systemd-networkd's public-NIC bind on 68/udp now classifies as "No matching rule" rather than "Local only".

3. Cross-backend verification

Same rule set + container set on each backend; restarted 1panel-agent between switches before screenshotting.

iptables 1.8.11 ufw 0.36.2 firewalld 2.3.1

Implementation note: when a port has both an accept and a drop rule, drop now takes precedence — security-conservative and deterministic across backends (firewalld lists accepts before rich-rule drops, iptables by chain order, ufw by rule index).

4. Docker network bind coverage

Mode Status Result
bridge + wildcard (-p 3306:3306, explicit 0.0.0.0 / [::]) dockerBypass
bridge + 127.0.0.1 localOnly
bridge + specific non-loopback IP falls through to rule check
UDP, port range, multiple ports on one container each classified independently
--net=host treated as a host process
userland-proxy=false resolved via c.Ports[] fallback (see §1)
No -p published not in scan scope
macvlan / ipvlan / overlay / Swarm ingress not in scan scope (see §7) ⚠️

5. Wording — no safety overclaim

Applied to all 10 locales:

Element Copy
Banner (no findings) "No issues found in this scan" / "本次扫描未发现问题"
Summary chips "{N} need fixing / {N} to review / {N} OK" — clickable filters; "to review" routes to whichever of noRule or firewallInactive is active
protected "Allowed by rule" / "规则放行"
blocked "Blocked by rule" / "规则拦截" — neutral info tag, not green
localOnly "Local only" / "仅本机" (actual address is in the "Bind Address" column)
dockerBypassHasRule "A firewall rule exists for this port ({strategy} {port}/{proto}), but it does not apply to Docker-published ports."
Scope disclaimer "Scope: host-side listeners and Docker default bridge networking. macvlan / ipvlan / overlay / Swarm ingress / external NAT paths are not evaluated."

6. Test scenarios

App Store deployed Docker. 1Panel's Redis app installed via App Store → sourceType: "appStore", with a direct "Go to App Settings" jump button in the "How to fix" popover.

Allow external (accept rule). accept tcp/3306 against a wildcard-bound MySQL container → red Docker Bypass tag + ⚠️ tooltip explaining that the rule does not apply to Docker-published ports:

Deny external (drop rule). drop tcp/7777 against a host nc listener → neutral Blocked by rule tag, not green:

Firewall installed but stopped. The panel keeps rendering — that's exactly when a user most needs to see what's exposed. Every wildcard-bound host port reports firewallInactive (red); the per-row "Create Rule" action is suppressed because adding a rule to a stopped firewall has no effect.

7. Scope — what the scanner does not cover

macvlan / ipvlan / overlay containers and Docker Swarm ingress publishes don't produce a host-level LISTEN socket and aren't in c.Ports[]. They're invisible to the scanner by design; the scope disclaimer in the panel banner names them explicitly so users don't read "no issues" as an absolute claim.


If there are specific scenarios you'd still like to see covered or labelled differently, please point them out.

@rowanchen-com
Copy link
Copy Markdown
Contributor

rowanchen-com commented May 11, 2026

Nice direction — 18 linked issues over 3+ years show this is a real pain point. I think the framing in the PR description is the right one: a full fix would mean taking over Docker's default iptables rules (DOCKER / DOCKER-USER chain management, ufw-docker-style integration, etc.), and the compatibility surface there is genuinely large — stopped containers, user-defined networks, --network=host, swarm ingress, userland-proxy on/off, non-default bridges, custom DNAT, IPv6. Starting with visibility before enforcement is a reasonable way to make progress.

One consequence of that scoping: because this PR explicitly doesn't change any iptables rule, the classification output is the product. There's no enforcement backstop that catches a misclassification at runtime — whatever the panel says, the user believes. That raises the bar on accuracy more than it would for a typical feature. A few spots where I think the current classifier could mislead:

1. matchFirewallRule ignores the rule's source Address

FireInfo.Address is populated for firewalld rich rules like accept from 1.2.3.4 to port 80, but the matcher only looks at port + protocol:

for _, r := range rules {
    if r.protocol != "" && r.protocol != "tcp/udp" && r.protocol != proto {
        continue
    }
    if !portMatchesRule(portStr, port, r.portStr) {
        continue
    }
    ...
}

A rule that only allows a single source IP ends up counted as a global accept, and an IP-scoped drop ends up counted as a global blocked. In an awareness-only design this is exactly the kind of false positive/negative the panel can't afford — it's the "protected / safe" misjudgement the maintainer flagged.

Suggestion: when r.address is non-empty and not Anywhere, skip it for the global verdict, or surface it in the UI as "rule scoped to X.X.X.X" so the user sees the caveat.

2. §4 of your reply doesn't match determineStatus

Your table says:

bridge + specific non-loopback IP → falls through to rule check ✅

But the code is:

if isLoopbackOrLinkLocal(bindAddr) {
    return "localOnly"
}
if sourceType == "docker" || sourceType == "appStore" {
    return "dockerBypass"   // regardless of bindAddr
}

A container bound to 172.17.0.1 (reachable only on the bridge) or a specific public NIC IP both get labelled dockerBypass unconditionally. Either the code needs a branch for "docker + non-wildcard bind → rule check", or the table row should be reworded to "falls back to dockerBypass (conservative)". With code and docs disagreeing, a reviewer can't tell which is intentional.

3. portMatchesRule doesn't cover all rule syntaxes

Range handling currently accepts 3000-3100 and 3000:3100. Two gaps:

  • iptables multiport rules use comma-separated lists (80,443,8080) — not matched today.
  • A rule with portStr == "" silently returns false, which happens to be correct but is worth an explicit guard + comment.

These won't show up in the §3 cross-backend screenshots because those use a single port per rule; a *_test.go with table-driven cases would cover them and directly answer the maintainer's ask for cross-backend verification.

4. Unit tests are probably non-optional for this merge

Given that classification accuracy is the deliverable, matchFirewallRule, determineStatus, portMatchesRule, and buildDockerPortMap all being pure functions makes them an easy target for table-driven tests. Minimum set I'd cover:

  • accept + drop on the same port (drop precedence)
  • range rule, multiport comma list, single port, empty port
  • rich rule with a from clause (§1 above)
  • loopback / link-local / wildcard / specific public IP / bridge-gateway bind
  • userland-proxy=false synthetic-entry path
  • UDP same port as a registered App Store HttpPort

15–20 cases, no external dependencies.

5. Scope the UI strictly to "visibility"

Since the iptables-modification half is deliberately out of scope, the panel should lean into being a read-only advisor and avoid anything that looks like enforcement:

  • The "Create Rule" action on a noRule row is fine for non-Docker sources, but on a dockerBypass row it would create an ineffective rule. Consider disabling it there (or at least a tooltip warning) — a rule that "exists but doesn't apply" is worse than no rule.
  • dockerBypassHasRule tooltip is already the right pattern: state the fact, don't imply a fix.
  • goToApp for App Store sources is the correct escape hatch — it hands the user off to the one place where the fix can actually be made. Deep-linking to the specific app's edit page (instead of the list page) would be a nice follow-up.

6. Locale scope

HynoR's comment about the translations is a fair point. Since most of the nuance lives in the wording ("rule exists but doesn't apply to Docker-published ports" is legally load-bearing), keeping this PR to zh.ts + zh-Hant.ts + en.ts and letting native speakers follow up on the other eight would tighten the review surface without losing value.

7. Smaller things

  • appPortMap is keyed only by port number — a host UDP listener on the same port as an App Store app's HttpPort gets mislabelled appStore. Key it by portKey{port, protocol}.
  • ContainerList(All: false) excludes stopped containers — a container defined with -p 80:80 but currently stopped is invisible to the scan, even though its port mapping will be reinstated on restart. Depending on intent, All: true + a container-state column might be more honest.
  • Scan runs on every panel mount. On a busy host net.ConnectionsMax(..., 32768) isn't cheap. A short TTL cache or a manual "Scan" button would be kinder in production.
  • Error stylefmt.Errorf(...) works, but the rest of the service layer uses the project's buserr helper and gets i18n'd error messages for free.
  • IFirewallService widening — the new method aggregates Docker + process table + firewall, which is a different concern than the rule CRUD this interface was originally about. Splitting into a separate IPortSecurityService would be a small change with cleaner boundaries.

Nothing above challenges the core design — awareness without iptables changes is the right first step given how many edge cases a full DOCKER-USER / ufw-docker-style takeover would bring. The suggestions are about tightening the accuracy and test story of that first step so the panel stays trustworthy. Happy to see this land.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants