Harden plugin assignment override narrowing #3105

Merged

mfreeman451 merged 1 commit from refs/pull/3105/head into staging

2026-03-31 03:33:03 +00:00

mfreeman451 commented

2026-03-31 03:32:27 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #3107
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/3107
Original created: 2026-03-31T03:32:27Z
Original updated: 2026-03-31T03:39:20Z
Original head: carverauto/serviceradar:claude/security-scan
Original base: staging
Original merged: 2026-03-31T03:33:03Z by @mfreeman451

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

Imported from GitHub pull request. Original GitHub pull request: #3107 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/3107 Original created: 2026-03-31T03:32:27Z Original updated: 2026-03-31T03:39:20Z Original head: carverauto/serviceradar:claude/security-scan Original base: staging Original merged: 2026-03-31T03:33:03Z by @mfreeman451 --- ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test?

Copilot commented

2026-03-31 03:39:20 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3107#discussion_r3013311121
Original created: 2026-03-31T03:39:20Z
Original path: elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex
Original line: 481

narrow_string_scope/2 currently narrows by exact string intersection (Enum.filter(base, member?(override))). This breaks legitimate narrowing when the manifest uses wildcard domain patterns (e.g. "*", "*.svc.local", which the Go agent treats specially) or when narrowing CIDR networks to more-specific prefixes (e.g. base 10.0.0.0/16 -> override 10.0.0.0/24). In those cases the intersection can incorrectly return [] (deny-all) even though the override is a valid subset of what the manifest allows.

Consider implementing subset checking consistent with the agent semantics: for domains, treat "*"/"*.suffix" in the base as patterns and keep override entries that match those patterns; for networks, parse prefixes and keep override prefixes contained within any base prefix. That preserves the “cannot widen” guarantee without requiring exact string equality.

    cond do
      base == [] ->
        # No permissions in base; cannot widen, so remain deny-all.
        []

      override == [] ->
        # Explicitly narrowed to an empty list.
        []

      true ->
        {base_domains, base_networks} =
          Enum.split_with(base, fn entry -> not String.contains?(entry, "/") end)

        {override_domains, override_networks} =
          Enum.split_with(override, fn entry -> not String.contains?(entry, "/") end)

        allowed_domains =
          if base_domains == [] do
            []
          else
            Enum.filter(override_domains, fn domain ->
              domain_allowed?(domain, base_domains)
            end)
          end

        allowed_networks =
          if base_networks == [] do
            []
          else
            Enum.filter(override_networks, fn cidr ->
              Enum.any?(base_networks, fn base_cidr ->
                cidr_contains?(base_cidr, cidr)
              end)
            end)
          end

        allowed_domains ++ allowed_networks
    end
  end

  defp domain_allowed?(override, base_domains) do
    Enum.any?(base_domains, fn base_entry ->
      domain_pattern_allows?(base_entry, override)
    end)
  end

  defp domain_pattern_allows?("*", _override), do: true

  defp domain_pattern_allows?(<<"*.", suffix::binary>>, override) do
    # Match domains that end with ".suffix" and have at least one label before it.
    min_length = String.length(suffix) + 2

    String.length(override) >= min_length and
      String.ends_with?(override, "." <> suffix)
  end

  defp domain_pattern_allows?(base_entry, override), do: base_entry == override

  defp cidr_contains?(base_cidr, candidate_cidr) do
    with {:ok, {base_ip, base_bits}} <- parse_cidr(base_cidr),
         {:ok, {cand_ip, cand_bits}} <- parse_cidr(candidate_cidr),
         true <- tuple_size(base_ip) == tuple_size(cand_ip),
         true <- cand_bits >= base_bits do
      base_bin = ip_to_binary(base_ip)
      cand_bin = ip_to_binary(cand_ip)

      <<base_prefix::bitstring-size(base_bits), _::bitstring>> = base_bin
      <<cand_prefix::bitstring-size(base_bits), _::bitstring>> = cand_bin

      base_prefix == cand_prefix
    else
      _ -> false
    end
  end

  defp parse_cidr(cidr) when is_binary(cidr) do
    case String.split(cidr, "/", parts: 2) do
      [addr_str, bits_str] ->
        with {:ok, addr} <- :inet.parse_address(String.to_charlist(addr_str)),
             {bits, ""} <- Integer.parse(bits_str),
             total_bits when is_integer(total_bits) <- ip_bits(addr),
             true <- bits >= 0 and bits <= total_bits do
          {:ok, {addr, bits}}
        else
          _ -> :error
        end

      _ ->
        :error
    end
  end

  defp ip_bits({_, _, _, _}), do: 32
  defp ip_bits({_, _, _, _, _, _, _, _}), do: 128

  defp ip_to_binary({a, b, c, d}), do: <<a, b, c, d>>

  defp ip_to_binary({a, b, c, d, e, f, g, h}) do
    <<a::16, b::16, c::16, d::16, e::16, f::16, g::16, h::16>>
  end

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3107#discussion_r3013311121 Original created: 2026-03-31T03:39:20Z Original path: elixir/serviceradar_core/lib/serviceradar/edge/agent_config_generator.ex Original line: 481 --- `narrow_string_scope/2` currently narrows by exact string intersection (`Enum.filter(base, member?(override))`). This breaks legitimate narrowing when the manifest uses wildcard domain patterns (e.g. `"*"`, `"*.svc.local"`, which the Go agent treats specially) or when narrowing CIDR networks to more-specific prefixes (e.g. base `10.0.0.0/16` -> override `10.0.0.0/24`). In those cases the intersection can incorrectly return `[]` (deny-all) even though the override is a valid subset of what the manifest allows. Consider implementing subset checking consistent with the agent semantics: for domains, treat `"*"`/`"*.suffix"` in the base as patterns and keep override entries that match those patterns; for networks, parse prefixes and keep override prefixes contained within any base prefix. That preserves the “cannot widen” guarantee without requiring exact string equality. ```suggestion cond do base == [] -> # No permissions in base; cannot widen, so remain deny-all. [] override == [] -> # Explicitly narrowed to an empty list. [] true -> {base_domains, base_networks} = Enum.split_with(base, fn entry -> not String.contains?(entry, "/") end) {override_domains, override_networks} = Enum.split_with(override, fn entry -> not String.contains?(entry, "/") end) allowed_domains = if base_domains == [] do [] else Enum.filter(override_domains, fn domain -> domain_allowed?(domain, base_domains) end) end allowed_networks = if base_networks == [] do [] else Enum.filter(override_networks, fn cidr -> Enum.any?(base_networks, fn base_cidr -> cidr_contains?(base_cidr, cidr) end) end) end allowed_domains ++ allowed_networks end end defp domain_allowed?(override, base_domains) do Enum.any?(base_domains, fn base_entry -> domain_pattern_allows?(base_entry, override) end) end defp domain_pattern_allows?("*", _override), do: true defp domain_pattern_allows?(<<"*.", suffix::binary>>, override) do # Match domains that end with ".suffix" and have at least one label before it. min_length = String.length(suffix) + 2 String.length(override) >= min_length and String.ends_with?(override, "." <> suffix) end defp domain_pattern_allows?(base_entry, override), do: base_entry == override defp cidr_contains?(base_cidr, candidate_cidr) do with {:ok, {base_ip, base_bits}} <- parse_cidr(base_cidr), {:ok, {cand_ip, cand_bits}} <- parse_cidr(candidate_cidr), true <- tuple_size(base_ip) == tuple_size(cand_ip), true <- cand_bits >= base_bits do base_bin = ip_to_binary(base_ip) cand_bin = ip_to_binary(cand_ip) <<base_prefix::bitstring-size(base_bits), _::bitstring>> = base_bin <<cand_prefix::bitstring-size(base_bits), _::bitstring>> = cand_bin base_prefix == cand_prefix else _ -> false end end defp parse_cidr(cidr) when is_binary(cidr) do case String.split(cidr, "/", parts: 2) do [addr_str, bits_str] -> with {:ok, addr} <- :inet.parse_address(String.to_charlist(addr_str)), {bits, ""} <- Integer.parse(bits_str), total_bits when is_integer(total_bits) <- ip_bits(addr), true <- bits >= 0 and bits <= total_bits do {:ok, {addr, bits}} else _ -> :error end _ -> :error end end defp ip_bits({_, _, _, _}), do: 32 defp ip_bits({_, _, _, _, _, _, _, _}), do: 128 defp ip_to_binary({a, b, c, d}), do: <<a, b, c, d>> defp ip_to_binary({a, b, c, d, e, f, g, h}) do <<a::16, b::16, c::16, d::16, e::16, f::16, g::16, h::16>> end ```