Skip to content

Disable external process commands in HTTP + On-Behalf-Of mode #1522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anuchandy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Disable external process commands in HTTP + On-Behalf-Of mode #1522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/Azure.Mcp.Core/src/Areas/Server/Options/ServiceStartOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,14 @@ public class ServiceStartOptions
/// </summary>
[JsonPropertyName("supportLoggingFolder")]
public string? SupportLoggingFolder { get; set; } = null;

/// <summary>
/// Determines whether the server is running in HTTP mode with On-Behalf-Of authentication.
/// </summary>
/// <returns>True if running in HTTP transport with On-Behalf-Of authentication; false otherwise.</returns>
public bool IsHttpOnBehalfOfMode()
Copy link
Member

@vukelich vukelich Jan 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subjective style: this can be a property which looks a subjectively cleaner with c# syntax.

[JsonIgnore]
public bool IsHttpOnBehalfOfMode => Transport == TransportTypes.Http && OutgoingAuthStrategy == OutgoingAuthStrategy.UseOnBehalfOf;

At the end of the day, both are creating parameterless methods.

Copy link
Member Author

@anuchandy anuchandy Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - yes, that’s actually what Copilot suggested. I was thinking it could be a method since we’re "computing" a boolean (coming from a Java mindset), but let’s go with a property given that’s more idiomatic.

Copy link
Member

@vukelich vukelich Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# properties are a hybrid between fields and methods. While writing code, they can be referenced using field-like syntax (i.e., usage infers reading vs writing), but during compilation, they are translated into methods.

Image

{
return Transport == TransportTypes.Http
&& OutgoingAuthStrategy == OutgoingAuthStrategy.UseOnBehalfOf;
}
Comment on lines +89 to +93
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new method IsHttpOnBehalfOfMode() and the conditional registration logic in ExtensionSetup lack test coverage. Consider adding unit tests to verify:

  1. IsHttpOnBehalfOfMode() returns true when Transport is Http and OutgoingAuthStrategy is UseOnBehalfOf
  2. IsHttpOnBehalfOfMode() returns false for other combinations (stdio transport, different auth strategies, etc.)
  3. ShouldExposeExternalProcessCommands() correctly excludes azqr command registration when in HTTP + OBO mode
  4. ShouldExposeExternalProcessCommands() allows azqr command registration when ServiceStartOptions is null or not in HTTP + OBO mode

These tests would help ensure the security fix works as intended and prevent regressions.

Copilot uses AI. Check for mistakes.
Copy link
Member

@vukelich vukelich Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like some good things to practice with having GitHub Copilot CLI or MCP server tools within VS Code follow.

}
3 changes: 3 additions & 0 deletions servers/Azure.Mcp.Server/changelog-entries/1768876346795.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
changes:
- section: "Features Added"
description: "Disable external process commands (azqr) in HTTP + On-Behalf-Of mode for security"
31 changes: 29 additions & 2 deletions tools/Azure.Mcp.Tools.Extension/src/ExtensionSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using Azure.Mcp.Core.Areas.Server.Options;
using Azure.Mcp.Core.Extensions;
using Azure.Mcp.Tools.Extension.Commands;
using Azure.Mcp.Tools.Extension.Services;
Expand Down Expand Up @@ -32,8 +33,12 @@ public CommandGroup RegisterCommands(IServiceProvider serviceProvider)

// Azure CLI and Azure Developer CLI tools are hidden
// extension.AddCommand("az", new AzCommand(loggerFactory.CreateLogger<AzCommand>()));
Copy link
Member

@vukelich vukelich Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend removing this comment OR putting it into the new if block body related to external process commands. If it returns, I suspect we'd want it guarded by the same logic.

var azqr = serviceProvider.GetRequiredService<AzqrCommand>();
extension.AddCommand(azqr.Name, azqr);

if (ShouldExposeExternalProcessCommands(serviceProvider))
{
var azqr = serviceProvider.GetRequiredService<AzqrCommand>();
extension.AddCommand(azqr.Name, azqr);
}

var cli = new CommandGroup("cli", "Commands for helping users to use CLI tools for Azure services operations. Includes operations for generating Azure CLI commands and getting installation instructions for Azure CLI, Azure Developer CLI and Azure Core Function Tools CLI.");
extension.AddSubGroup(cli);
Expand All @@ -44,4 +49,26 @@ public CommandGroup RegisterCommands(IServiceProvider serviceProvider)
cli.AddCommand(cliInstallCommand.Name, cliInstallCommand);
return extension;
}

/// <summary>
/// Determines whether extension commands that use external process execution should be exposed.
/// External process commands (like azqr, azcli, azd) use <see cref="IExternalProcessService"/> to spawn
/// local processes. In HTTP + On-Behalf-Of mode, spawning child processes on a remote server is a security
/// risk: processes run under the server's host identity (not the OBO user's context), and malicious or
/// excessive requests could exhaust resources.
/// </summary>
/// <param name="serviceProvider">The service provider to resolve ServiceStartOptions from.</param>
/// <returns>True if external process commands should be exposed; false otherwise.</returns>
private static bool ShouldExposeExternalProcessCommands(IServiceProvider serviceProvider)
{
ServiceStartOptions? startOptions = serviceProvider.GetService<ServiceStartOptions>();

if (startOptions is null)
Copy link
Member

@vukelich vukelich Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want some modern C# null variable checking syntactic sugar:

if (serviceProvider.GetService<ServiceStartOptions>() is ServiceStartOptions startOptions)
{
	return !startOptions.IsHttpOnBehalfOfMode();
}

return true/whateverDefault;

Totally optional. Current code is fine. Just teaching.

Copy link
Member Author

@anuchandy anuchandy Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will use type pattern matching here

{
// First container (CLI routing) - startOptions not available, allow all commands
Copy link
Member

@vukelich vukelich Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First container

Now, I know what this means, but I doubt anyone else would. Please reference the Program.cs files in the servers folder where the multiple containers are detailed. Ugly for now, but referencing that design complexity in a cleaner way is out of scope for this PR.

Copy link
Member Author

@anuchandy anuchandy Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - yes, the multi-container setup is pretty confusing :). Let me link to the entry point / program.cs where that is happening.

return true;
}

return !startOptions.IsHttpOnBehalfOfMode();
}
}
Loading