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

[alencheung]

Summary

The three user-facing tool endpoints always query Azure with the engine's own credentials regardless of who is calling, while every equivalent routed endpoint correctly branches between engine credentials (administrators) and the caller's on-behalf-of credentials (everyone else). The result contradicts the documented authorization model, in which non-admin users only see resources they already have access to.

Observed

Line references are against current main (12e41f4).

engine/app/routers/tool.py passes a hardcoded True as the admin argument:

• nextAvailableSubnet — arg_query(authorization, True, argquery.VNET) (tool.py:73)
• nextAvailableVNet — get_network(authorization, True) (tool.py:162)
• cidrCheck — arg_query(authorization, True, argquery.NET_BASIC) (tool.py:239)

In engine/app/routers/common/helper.py (lines 277-311), admin=True selects get_client_credentials() — the engine app registration, which the deployment grants Reader at the management group (tenant root by default). admin=False builds an OnBehalfOfCredential from the caller's token, so the caller sees only what their own Azure RBAC permits.

The same defect appears a second time through a positional-binding bug: get_network's second positional parameter is tenant_id, so get_network(authorization, True) binds tenant_id=True and leaves admin at its default — an unresolved Depends(get_admin) marker, which is truthy — again selecting engine credentials. This shape is present on the reservation-create path (~line 2956) and the spaces-utilization path in engine/app/routers/space.py.

For contrast, the routed endpoints do this correctly: get_vnet in engine/app/routers/azure.py declares admin: str = Depends(get_admin) and passes the resolved flag through to arg_query.

Expected vs actual

docs/how-to/README.md states the model:

It uses your existing Azure AD credentials to authenticate you and leverages your existing Azure RBAC permissions to authorize what information is visible from within the IPAM tool.

Once at least one IPAM administrator is set, non-admin users will only see resources in IPAM they already have access to from the Azure Portal.

Actual: a non-admin tenant member holding no Azure RBAC on any subscription can call the three tool endpoints and receive the engine's Reader-level view of the management group.

Repro sketch

1. Deploy with at least one IPAM administrator configured (so the documented default-open bootstrap is closed).
2. Sign in as a second tenant member holding no Azure RBAC on any subscription.
3. POST /api/tools/cidrCheck with {"cidr": "10.0.0.0/8"} — returns 200 with vNets from subscriptions the caller cannot read: name, id, resource_group, subscription_id, address prefixes.
4. Control: GET /api/azure/vnets for the same caller returns the caller's own (empty) view, since that route resolves the admin flag. The differential between the two responses is the issue.

nextAvailableSubnet additionally acts as an existence oracle for any named vNet id (400 "Virtual Network not found" vs 200) and discloses its vnet_name / resource_group / subscription_id; iterating size probes maps a target vNet's subnet layout.

Suggested fix

Resolve the admin flag on the three tool handlers (declare admin: str = Depends(get_admin) and pass it through), mirroring azure.py. At the get_network call sites, pass admin by keyword — the current positional True also corrupts tenant_id. The same hardcoded True is still present on the ipam-3.7.0 branch of #377, so the release PR does not currently cover this.

[DCMattyG]

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.