Skip to content

Fix #1748: make cloud-metadata/private-range denylist non-overridable#2055

Open
S0412-2007 wants to merge 1 commit into
mainfrom
fix/1748-mandatory-denylist-metadata-ssrf
Open

Fix #1748: make cloud-metadata/private-range denylist non-overridable#2055
S0412-2007 wants to merge 1 commit into
mainfrom
fix/1748-mandatory-denylist-metadata-ssrf

Conversation

@S0412-2007

Copy link
Copy Markdown
Collaborator

Description

_init_default_policies() built the entire network denylist from a single Pydantic field, settings.network_denylist. Pydantic replaces (rather than merges) a list field's default when SECUSCAN_NETWORK_DENYLIST is set via env var, so any operator adding even one custom denylist entry silently dropped the built-in protection for cloud metadata (169.254.169.254), loopback, RFC1918/CGNAT ranges, and IPv6 link-local/ULA space — reopening SSRF to the metadata endpoint despite the code comment claiming the denylist was "always enforced."

Note: the issue's literal repro (default denylist missing the metadata IP entirely) doesn't reproduce as-is on current main — that part looks to have been partially addressed already. This PR fixes the related, still-live variant: the protection exists by default but isn't durable against operator customization.

Fix: moved those ranges into a new MANDATORY_DENYLIST module constant that is not read from settings/env and is applied unconditionally in _init_default_policies() before any operator-configured entries. The operator-facing network_denylist setting is now purely additive.

Related Issues

Closes #1748

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Updated the existing default-denylist test, which had been asserting on the old (overridable) field.
  • Added a regression test reproducing the exact scenario from [#2] Network policy default-allow exposes all public egress (incl. cloud metadata IP) #1748: an operator setting SECUSCAN_NETWORK_DENYLIST to a custom, unrelated value must not lose metadata/private-range protection.
  • Ran testing/backend/unit/test_network_policy.py, test_admin_network_policy.py, test_validate_command_egress_policy.py, and test_network_scanner_plugin.py — all 63 tests pass.

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

_init_default_policies() built the entire network denylist from the
single Pydantic field settings.network_denylist. Pydantic replaces
(rather than merges) a list field's default when SECUSCAN_NETWORK_DENYLIST
is set via env var, so any operator adding even one custom denylist
entry silently dropped the built-in protection for cloud metadata
(169.254.169.254), loopback, RFC1918/CGNAT ranges, and IPv6
link-local/ULA space -- reopening SSRF to the metadata endpoint despite
the code comment claiming the denylist was 'always enforced'.

Fix: move those ranges into a new MANDATORY_DENYLIST module constant
that is not read from settings and is applied unconditionally in
_init_default_policies before any operator-configured entries. The
operator-facing network_denylist setting is now purely additive.

Also updates the existing default-denylist test and adds a regression
test reproducing the exact scenario from #1748.
@S0412-2007

Copy link
Copy Markdown
Collaborator Author

@utksh1 ready for review.

One thing worth calling out: the original issue's exact repro (default denylist missing 169.254.169.254 entirely) doesn't reproduce on current main — that part looks like it was already fixed at some point. What I found instead is a related, still-live version of the same bug: settings.network_denylist is a single Pydantic field, and setting SECUSCAN_NETWORK_DENYLIST via env var replaces rather than merges with its default. So an operator who customizes the denylist at all silently loses the metadata/private-range protection, even though the code comment claims it's "always enforced."

This PR splits those ranges into a MANDATORY_DENYLIST constant that's not read from config at all, so they can't be dropped via env var — network_denylist is now purely additive. Added a regression test (test_operator_denylist_override_does_not_drop_mandatory_ranges) that reproduces this scenario directly.

Happy to adjust the approach if you'd rather keep denylist config fully in Settings and handle the merge differently (e.g. a validator that appends instead of replaces) — went with the module-constant approach since it guarantees the protection can't be disabled even by a misconfigured env var, not just a careless one.

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] Network policy default-allow exposes all public egress (incl. cloud metadata IP)

1 participant