Tool endpoints use engine credentials for every caller instead of the per-user OBO branch #387

Activity

1. DCMattyG

   self-assigned this

   on Sep 22, 2026
2. DCMattyG

   added

   bugSomething isn't workingSomething isn't working

   on Sep 22, 2026
3. DCMattyG commented on Sep 22, 2026

   

   DCMattyG

   on Sep 22, 2026

   Contributor

   More actions

   Thank you for opening this issue. The line references, the admin/OBO trace through helper.py, and the contrast against the routed endpoints all made this fast to confirm. As I'm working on the PR for Azure IPAM v4.0.0, I'll make surer these fixes get incorporated.

   I've verified the core claim and want to restate it in terms of how the engine is supposed to behave, since the three endpoints you identified are actually on different sides of an internal distinction that isn't documented anywhere yet — which is itself part of the problem.

   The design rule we hold ourselves to

   IPAM answers two different questions about the network estate:

   1. Occupancy — "is this address space taken?" This must be answered from every network in the tenant. If it isn't, IPAM will happily allocate a CIDR that collides with a vNET the caller can't see, and hand back a plan that breaks at deployment time. Allocation correctness genuinely requires the engine's own credentials here.
   2. Enumeration — "what occupies it?" This is legitimately scoped to the caller's Azure RBAC, and it's what the documentation you quoted is describing.

   The bug is not that the engine consults resources the caller can't see. It's that these endpoints let data from an occupancy query flow into an enumeration response. That's the line, and we agree it's been crossed.

   Endpoint-by-endpoint

   Endpoint	Needs tenant-wide view?	Disposition
   cidrCheck	Yes, for the overlap decision	Confirmed leak. The decision needs the full picture; the response body does not. It returns name/id/resource_group/subscription_id/prefixes for networks the caller has no rights to. I'll keep the tenant-wide overlap evaluation and scope what's returned to the caller's visibility, with non-visible overlaps reported without identifying detail.
   nextAvailableSubnet	No	Confirmed, and simpler than it looks. This endpoint pulls every vNET in the tenant, then discards all but the one the caller named — its free-space math reads only that vNET's own prefixes and subnets. Since Azure has no per-subnet RBAC, a caller who can read the vNET sees the same free space the engine credential would. Resolving the admin flag costs no correctness.
   nextAvailableVNet	Yes, for allocation	Working as intended, poorly documented. The response carries only space/block/cidr — IPAM-managed values the caller supplied. It consumes the tenant-wide view without exposing it, consistent with the reservation path. We'll keep the behavior and document the rationale.

   Two refinements to the report

   On nextAvailableSubnet, the vnet_name / resource_group / subscription_id in the response are echoes of the resource ID the caller supplied, so they aren't themselves a disclosure. Your other observation is the real one: the endpoint behaves as an existence oracle, and repeated size probes allow inference about a vNET's subnet layout. That's what we're closing.

   On the positional-binding issue — you're right, and it's broader than tool.py. The same shape defeated call sites that looked correct: get_network(authorization, is_admin) in space.py also bound the flag to tenant_id and fell through to engine credentials, so the spaces expand/utilization paths were app-only as well. A refactor that makes network query scope an explicit, required argument at every call site is already in flight on our side and removes that entire class of error; it hasn't landed publicly yet, which is why you wouldn't have seen it at 12e41f4. The hardcoded True on the three tool endpoints is a separate defect and, as you note, is still present on ipam-3.7.0 — it will be fixed independently of that release.

   One scoping clarification on the documentation

   The statement you quoted is about Azure resources, and we'll make that explicit. IPAM's own inventory — Space and Block definitions, their CIDRs, the resource IDs of networks an administrator has associated to a Block, and utilization totals — is tenant-visible to any authenticated user by design, and is not filtered by Azure RBAC. Fixing these endpoints won't change that, so the docs need to describe the two planes separately rather than implying a single RBAC-scoped view.

   What's going in

   • Resolve the admin flag on nextAvailableSubnet and pass it through.
   • Reshape the cidrCheck response so tenant-wide occupancy informs the answer without disclosing resources the caller can't read.
   • Keep nextAvailableVNet tenant-wide, with the rationale recorded in code.
   • Add a regression test that asserts every route returning Azure resource identities resolves the admin flag — this class of bug should be caught mechanically, not by review.
   • Update the authorization docs to separate Azure resource visibility from IPAM inventory visibility.

   One heads-up for anyone automating against these: scoping nextAvailableSubnet to the caller means a non-admin service principal using client-credentials can't be exchanged on-behalf-of, so non-admin SPN callers will need IPAM administrator status (this already applies to the /api/azure/* routes). We'll call that out in the automation guidance.

   Thanks again, hopefully you agree with my planned action items and revisions to the specifics from the initial report above.
4. DCMattyG

   added 2 commits that reference this issue on Oct 1, 2026

   docs: add guiding principles for roles and permissions

   ...

   f415586

   fix(engine)!: stop tool endpoints revealing networks outside the call…

   ...

   6f7583e