feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package#824
feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package#824jackbatzner wants to merge 6 commits intomicrosoft:mainfrom
Conversation
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
🤖 AI Agent: breaking-change-detector — Summary🔍 API Compatibility ReportSummaryThis pull request introduces a new standalone .NET package, Findings
Migration GuideNo migration steps are necessary as this pull request does not introduce any breaking changes. Additional Notes
✅ No breaking changes detected. |
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
Additional Notes
Action RequiredPlease address the issues and suggestions listed above to ensure the documentation and examples are in sync with the new changes. |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces a new .NET package, Microsoft.AgentGovernance.Mcp, which provides a standalone implementation of MCP governance primitives with minimal dependencies. It also includes two sample projects demonstrating integration with ASP.NET Core and the official ModelContextProtocol (MCP) SDK. The PR appears well-structured and adheres to the project's style guidelines. However, there are several areas that require attention to ensure security, maintainability, and backward compatibility.
🔴 CRITICAL
-
Credential Redaction in Logs
- In the
McpGovernance.OfficialSdksample, theCredentialRedactor.Redact()function is mentioned but commented out in theAddOutgoingFilterblock. This leaves a potential security gap where sensitive information could be logged. - Action: Ensure that the
CredentialRedactor.Redact()functionality is fully implemented and tested. Sensitive information must be redacted in all logs and responses before being sent.
- In the
-
Tool Discovery and Execution
- The
McpToolattribute is used for tool discovery, but there is no evidence of validation or sanitization of the tool methods' inputs. This could lead to injection vulnerabilities (e.g., SQL injection inSearchDatabase). - Action: Add input validation and sanitization for all tool methods. Consider using a library like FluentValidation or custom validation logic to ensure inputs are safe.
- The
-
Rate Limiting and Replay Protection
- The
appsettings.jsonfile includes governance settings likeRateLimitWindowMinutesandMessageReplayWindowSeconds, but there is no evidence in the code that these settings are enforced. - Action: Implement and test rate limiting and replay protection mechanisms to prevent abuse and replay attacks.
- The
-
Human Approval for Sensitive Actions
- The
SendEmailtool method has theRequiresApprovalattribute, but there is no implementation to enforce this requirement. - Action: Implement a mechanism to enforce human approval for tools marked with
RequiresApproval.
- The
🟡 WARNING
-
Backward Compatibility
- The introduction of the
Microsoft.AgentGovernance.Mcppackage and its dependencies may affect existing users of theagent-governance-dotnetpackage. - Action: Clearly document any changes to the public API and ensure that existing functionality is not broken. Add deprecation warnings if necessary.
- The introduction of the
-
Multi-Targeting .NET Versions
- The new package targets both
.NET 8.0and.NET 10.0. While this is forward-looking, it may introduce compatibility issues for users on older versions of .NET. - Action: Test the package thoroughly on both target frameworks and document any known limitations or differences.
- The new package targets both
💡 SUGGESTIONS
-
Unit Tests for Governance Pipeline
- While the PR mentions that tests have been added, it is unclear if the governance pipeline (e.g.,
InterceptToolCall) has been thoroughly tested for edge cases. - Action: Add unit tests to cover scenarios like:
- Denied tools being called.
- Sensitive tools being called without human approval.
- Rate-limiting and replay protection.
- Credential redaction in logs and responses.
- While the PR mentions that tests have been added, it is unclear if the governance pipeline (e.g.,
-
OWASP MCP Security Compliance
- The PR claims OWASP MCP Security Cheat Sheet compliance, but there is no explicit mapping of implemented features to the cheat sheet.
- Action: Add a section in the documentation mapping implemented features to the OWASP MCP Security Cheat Sheet.
-
Structured Logging
- The sample projects use
ILoggerfor logging, but there is no mention of structured logging or correlation IDs for tracing requests. - Action: Use structured logging with correlation IDs to improve traceability and debugging.
- The sample projects use
-
Documentation
- The documentation for the new package and samples is clear but could benefit from additional details on security features and best practices.
- Action: Expand the documentation to include:
- Security best practices for using the governance pipeline.
- Examples of integrating with other frameworks or SDKs.
- Troubleshooting common issues.
-
Sandboxing
- There is no mention of sandboxing or isolation for tool execution. This could lead to sandbox escape vulnerabilities.
- Action: Evaluate the need for sandboxing and consider using technologies like AppDomains or containers to isolate tool execution.
Conclusion
This PR introduces valuable functionality to the agent-governance-dotnet package and provides useful samples for integration with ASP.NET Core and the official MCP SDK. However, there are critical security issues that must be addressed before merging, particularly around input validation, rate limiting, replay protection, and human approval enforcement. Additionally, backward compatibility and documentation improvements should be considered to ensure a smooth adoption for existing users.
🤖 AI Agent: security-scanner — Security Review for `microsoft/agent-governance-toolkit` Pull RequestSecurity Review for
|
| Issue | Rating | Recommendation Summary |
|---|---|---|
| Prompt injection defense bypass | 🔴 CRITICAL | Add input sanitization and tests for injection attempts. |
| Policy engine circumvention | 🔴 CRITICAL | Add tests to ensure policies cannot be bypassed and consider cryptographic safeguards. |
| Trust chain weaknesses | 🔴 CRITICAL | Implement robust certificate chain validation for mTLS. |
| Credential exposure | 🟡 MEDIUM | Use secure secrets management and avoid logging sensitive information. |
| Sandbox escape | 🟠 HIGH | Test MCP server and tools in sandboxed environments like gVisor or Kata Containers. |
| Deserialization attacks | 🔴 CRITICAL | Use safe YAML parsers and add tests for malicious YAML inputs. |
| Race conditions | 🟠 HIGH | Ensure thread safety in middleware and add tests for concurrent execution. |
| Supply chain | 🟠 HIGH | Use dependency scanning tools and verify third-party dependencies. |
General Recommendations
- Perform a thorough review of the
PolicyEvaluatorandGovernancePolicyMiddlewareimplementations to ensure they are robust against injection and circumvention attacks. - Add comprehensive test coverage for all security-critical components, including edge cases and adversarial scenarios.
- Ensure that all deployment and runtime environments follow the hardening guidelines provided in
mcp-server-hardening.md.
Let me know if you need further clarification or additional details on any of the findings!
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces a new standalone .NET package, Microsoft.AgentGovernance.Mcp, which provides MCP governance primitives with minimal dependencies. It also includes sample projects demonstrating integration with ASP.NET Core and the official ModelContextProtocol (MCP) SDK. The PR appears well-structured and adheres to the project's style guidelines. However, there are some areas that require attention, particularly around security, backward compatibility, and documentation.
🔴 CRITICAL
-
Credential Redaction in Outgoing Responses
- In the
McpGovernance.OfficialSdksample, theCredentialRedactor.Redact()function is mentioned but not implemented in the provided code. This could lead to sensitive information being leaked in outgoing responses if developers forget to implement redaction. - Action: Ensure that the
CredentialRedactor.Redact()function is implemented and tested. Add a fallback mechanism to log a warning if redaction is not applied.
- In the
-
Tool Discovery and Execution
- The
McpToolRegistry.DiscoverTools()method automatically registers tools based on the[McpTool]attribute. However, there is no validation to ensure that tools do not have unsafe or malicious behavior. - Action: Add a validation mechanism to ensure that tools registered via
McpToolRegistry.DiscoverTools()comply with security policies (e.g., deny-list, allow-list, or sandboxing).
- The
-
SQL Injection in Tool Calls
- The
searchtool in theSampleToolsclass does not sanitize thequeryparameter, leaving it vulnerable to SQL injection. - Action: Add input validation and sanitization for all tool parameters, especially those that interact with external systems like databases.
- The
-
Replay Attack Protection
- The
MessageReplayWindowSecondsconfiguration inappsettings.jsonis set to 300 seconds, but there is no evidence in the code that replay attacks are being mitigated. - Action: Implement replay attack protection by maintaining a cache of recently processed message IDs and rejecting duplicates within the specified window.
- The
🟡 WARNING
-
Backward Compatibility
- The introduction of a new package (
Microsoft.AgentGovernance.Mcp) and its associated APIs could lead to potential backward compatibility issues if existing users of the toolkit migrate to this package. - Action: Clearly document migration paths and ensure that existing functionality is not disrupted for users of the core
AgentGovernancepackage.
- The introduction of a new package (
-
Multi-Targeting for .NET 8 and .NET 10
- The new package targets
.NET 8and.NET 10, which may not be compatible with older versions of .NET. This could limit adoption for users on older frameworks. - Action: Consider adding support for
.NET 6or.NET 7if feasible, or explicitly document the minimum supported framework versions.
- The new package targets
💡 SUGGESTIONS
-
OWASP MCP Security Cheat Sheet Compliance
- The PR claims to provide "full OWASP MCP Security Cheat Sheet coverage (11/12 sections)," but this is not explicitly verified in the code or tests.
- Action: Add automated tests to verify compliance with all relevant sections of the OWASP MCP Security Cheat Sheet.
-
Documentation Improvements
- The documentation for the
McpGovernance.OfficialSdksample is clear but assumes familiarity with the MCP SDK. New users may struggle to understand the integration process. - Action: Add more detailed explanations and diagrams to illustrate how the governance layer interacts with the MCP SDK.
- The documentation for the
-
Thread Safety
- The
McpToolRegistryandMcpGatewayclasses are used in a multi-threaded environment (e.g., ASP.NET Core). However, there is no explicit mention of thread safety in their implementation. - Action: Review these classes for thread safety and document any thread-safety guarantees or limitations.
- The
-
Testing Coverage
- While the PR mentions that all tests pass, there is no evidence of new tests specifically for the
Microsoft.AgentGovernance.Mcppackage or its integration with the MCP SDK. - Action: Add unit and integration tests for the new package, focusing on edge cases like denied tools, rate limiting, and response scanning.
- While the PR mentions that all tests pass, there is no evidence of new tests specifically for the
-
Default Configuration
- The default configuration in
appsettings.jsonleaves theDeniedToolsandAllowedToolslists empty, which could lead to insecure defaults. - Action: Provide a secure default configuration with a reasonable deny-list and allow-list.
- The default configuration in
-
Logging and Monitoring
- The samples include basic logging via
ILogger, but there is no mention of centralized logging or monitoring for governance-related events. - Action: Recommend best practices for logging and monitoring, such as integrating with Azure Monitor or other observability platforms.
- The samples include basic logging via
Conclusion
The PR introduces a valuable addition to the Agent Governance Toolkit by providing a lightweight .NET package for MCP governance. However, there are critical security issues that must be addressed before merging, particularly around credential redaction, tool validation, and SQL injection. Additionally, there are potential backward compatibility concerns and areas for improvement in documentation and testing. Addressing these issues will ensure the new package is robust, secure, and easy to adopt.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a standalone .NET package (Microsoft.AgentGovernance.Mcp) that integrates MCP governance primitives into the Agent Governance Toolkit. It also includes two sample projects demonstrating integration with ASP.NET Core and the official ModelContextProtocol (MCP) SDK. The PR is well-documented and adheres to the project guidelines. However, there are several areas that require attention to ensure security, maintainability, and backward compatibility.
🔴 CRITICAL
-
Potential Security Bypass in
InterceptToolCall- The
InterceptToolCallmethod in theMcpGatewayclass appears to evaluate tool calls for governance compliance. However, the implementation of this method is not visible in the provided diff. Ensure that:- All governance checks (deny-list, allow-list, sanitization, rate limiting, etc.) are correctly implemented and cannot be bypassed.
- The method handles edge cases, such as malformed requests or unexpected input types, to prevent bypasses.
- The method logs all denied requests for audit purposes.
- Action: Provide the implementation of
InterceptToolCallfor review.
- The
-
Credential Redaction
- The
CredentialRedactor.Redactmethod is used to sanitize sensitive information in responses. However, the implementation is not visible in the diff. - Action: Ensure that the redaction logic is robust and can handle edge cases, such as nested sensitive fields or non-standard formats. Provide the implementation for review.
- The
-
Sandbox Escape Vectors
- The
SampleToolsclass includes methods likeReadFileandSearchDatabase. While these are marked as "safe" or "read-only," there is no visible enforcement of these constraints. - Action: Ensure that:
- File paths are sanitized to prevent directory traversal attacks.
- Database queries are parameterized to prevent SQL injection.
- Any dynamic code execution or file access is sandboxed to prevent unauthorized access.
- The
-
gRPC Interceptor
- The gRPC interceptor in the ASP.NET Core sample uses
AddGrpc(grpc => grpc.AddMcpGovernance()). Ensure that:- The interceptor cannot be bypassed or disabled by malicious actors.
- The governance pipeline is applied consistently to all gRPC requests.
- Action: Provide the implementation of
AddMcpGovernancefor review.
- The gRPC interceptor in the ASP.NET Core sample uses
🟡 WARNING
-
Backward Compatibility
- The introduction of a new package (
Microsoft.AgentGovernance.Mcp) appears to be non-breaking. However, ensure that:- The new package does not introduce breaking changes to existing functionality.
- The new package is properly versioned and documented to avoid confusion for existing users.
- The introduction of a new package (
-
Multi-Targeting .NET Versions
- The new package targets both
.NET 8.0and.NET 10.0. Ensure that:- The package is tested on both frameworks.
- Any breaking changes in .NET 10.0 are accounted for.
- The new package targets both
💡 SUGGESTIONS
-
OWASP MCP Compliance
- The PR mentions compliance with the OWASP MCP Security Cheat Sheet. Consider adding automated tests to verify compliance with all 12 sections of the cheat sheet.
-
Type Safety
- The
InterceptToolCallmethod appears to accept parameters as aDictionary<string, object>. Consider using strongly-typed models to improve type safety and reduce the risk of runtime errors.
- The
-
Logging and Observability
- Ensure that all governance decisions (allow/deny) are logged with sufficient context (e.g., tool name, agent ID, parameters) for auditing and debugging purposes.
-
Documentation
- The provided README files are detailed and helpful. However, consider adding a section on common pitfalls and best practices for integrating MCP governance into .NET applications.
-
Testing
- The PR includes sample projects, but it's unclear if there are automated tests for the new package. Ensure that:
- Unit tests cover all governance logic.
- Integration tests validate the end-to-end behavior of the new package with the official MCP SDK and ASP.NET Core.
- The PR includes sample projects, but it's unclear if there are automated tests for the new package. Ensure that:
-
Thread Safety
- Ensure that the
McpGatewayand related classes are thread-safe, especially if they are used in a multi-threaded environment like ASP.NET Core or gRPC.
- Ensure that the
-
Configuration Validation
- The
McpGovernanceOptions.BindFromConfigurationmethod binds settings fromappsettings.json. Consider adding validation logic to ensure that all required settings are present and have valid values.
- The
Final Recommendation
- Address the CRITICAL issues before merging.
- Review the implementations of
InterceptToolCall,CredentialRedactor.Redact, andAddMcpGovernancefor security and correctness. - Consider the SUGGESTIONS to improve the overall quality and maintainability of the codebase.
Once the critical issues are resolved and the suggestions are considered, this PR will be ready for approval.
6ef151f to
99df13d
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces a new standalone .NET package, Microsoft.AgentGovernance.Mcp, which provides MCP governance primitives with minimal dependencies. It also includes a sample ASP.NET Core application demonstrating the integration of MCP governance features. The PR appears well-structured and adheres to the project guidelines. However, there are a few areas that require attention to ensure security, maintainability, and backward compatibility.
🔴 CRITICAL
-
Potential Insufficient Validation of Tool Calls
- In the
SampleToolsclass, theReadFileandSearchDatabasemethods do not appear to validate their inputs. For example:public static Dictionary<string, object> ReadFile(string path) { return new() { ["content"] = $"[simulated content of {path}]" }; }
- Risk: If this is a placeholder for actual file reading logic, there is a risk of directory traversal attacks or unauthorized file access if the
pathparameter is not properly sanitized. - Action: Add input validation to ensure that the
pathis restricted to safe directories and does not allow traversal (../) or access to sensitive files.
- Risk: If this is a placeholder for actual file reading logic, there is a risk of directory traversal attacks or unauthorized file access if the
- In the
-
Missing Authentication/Authorization for Tool Calls
- The sample application does not demonstrate any form of authentication or authorization for MCP tool calls. This could allow unauthorized users to execute sensitive operations.
- Risk: Unauthorized access to sensitive tools like
SendEmailor potential misuse of other tools. - Action: Ensure that the middleware enforces authentication and authorization for all MCP tool calls. Consider integrating with ASP.NET Core's built-in authentication and authorization mechanisms.
- Risk: Unauthorized access to sensitive tools like
- The sample application does not demonstrate any form of authentication or authorization for MCP tool calls. This could allow unauthorized users to execute sensitive operations.
-
Potential for Replay Attacks
- The
MessageReplayWindowSecondsconfiguration parameter is set to 300 seconds (5 minutes). However, there is no evidence in the provided code that replay protection is implemented.- Risk: Without proper replay protection, an attacker could reuse a previously intercepted request to execute the same action multiple times.
- Action: Implement replay protection by maintaining a cache of recently processed message IDs and rejecting any duplicates within the replay window.
- The
🟡 WARNING
- Breaking Changes in Public API
- The introduction of the
Microsoft.AgentGovernance.Mcppackage and its associated APIs could potentially introduce breaking changes for existing users of the .NET library if they expect all MCP-related functionality to be part of the coreAgentGovernancepackage.- Action: Clearly document the migration path for existing users and ensure that the new package is backward-compatible with the existing APIs.
- The introduction of the
💡 SUGGESTIONS
-
Add Unit Tests for New Features
- While the PR mentions that tests have been added, there is no evidence of unit tests for the new
Microsoft.AgentGovernance.Mcppackage or the sample application. This is especially important for critical features like input validation, authentication, and replay protection.- Action: Add comprehensive unit tests for the new package and sample application, focusing on edge cases and security scenarios.
- While the PR mentions that tests have been added, there is no evidence of unit tests for the new
-
Enhance Documentation
- The provided documentation is a good start, but it could be improved by including:
- A detailed explanation of the security features provided by the MCP governance middleware.
- Examples of how to configure authentication and authorization for MCP tool calls.
- Guidance on how to implement custom tools and register them using the
[McpTool]attribute.
- The provided documentation is a good start, but it could be improved by including:
-
Consider Default Deny Policy for Tools
- The
appsettings.jsonfile currently defines empty lists forDeniedToolsandAllowedTools. This implies that all tools are allowed by default.- Action: Consider adopting a "default deny" policy by requiring explicit listing of allowed tools in the
AllowedToolslist. This would reduce the risk of unauthorized tool execution.
- Action: Consider adopting a "default deny" policy by requiring explicit listing of allowed tools in the
- The
-
Thread Safety in
McpToolRegistry- The
McpToolRegistryclass is used to register tools dynamically. If this class is accessed concurrently (e.g., during tool discovery or runtime tool registration), there could be thread safety issues.- Action: Ensure that
McpToolRegistryis thread-safe by using appropriate synchronization mechanisms (e.g., locks or concurrent collections).
- Action: Ensure that
- The
-
Consider OWASP Agentic Top 10 Compliance
- The PR does not explicitly address compliance with the OWASP Agentic Top 10. For example:
- A1: Insecure Agent Initialization: Ensure that the agent initialization process is secure and does not expose sensitive information.
- A2: Insecure Communication: Ensure that all communication between agents and the MCP server is encrypted and authenticated.
- A4: Insecure Credential Management: Ensure that sensitive credentials (e.g., API keys or tokens) are not exposed in logs or configuration files.
- Action: Conduct a thorough review of the implementation to ensure compliance with the OWASP Agentic Top 10.
- The PR does not explicitly address compliance with the OWASP Agentic Top 10. For example:
-
Add Logging for Security Events
- The sample application includes structured logging via
ILogger, but there is no mention of logging security-related events (e.g., denied tool calls, replay attempts, or authentication failures).- Action: Add logging for all security-related events to facilitate auditing and incident response.
- The sample application includes structured logging via
Final Assessment
The PR introduces a valuable new package and sample application for MCP governance. However, there are critical security issues that need to be addressed before merging. Additionally, there are opportunities to improve documentation, testing, and compliance with security best practices. Please address the critical issues and consider the suggestions to ensure a robust and secure implementation.
99df13d to
99b2c2e
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Feedback for PR: feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package
🔴 CRITICAL
-
Potential Security Issue: Lack of Input Validation in Sample Tools
- The sample tools provided in
SampleTools(e.g.,ReadFile,SearchDatabase,SendEmail) do not perform any input validation or sanitization. For example:ReadFiledirectly accepts a file path without validating or sanitizing it, which could lead to directory traversal attacks.SearchDatabaseaccepts a raw query string, which could be exploited for SQL injection if used in a real database context.SendEmaildoes not validate email addresses or sanitize the subject/body, which could lead to injection attacks or abuse.
- Recommendation: Add input validation and sanitization to all tool methods, even in sample code, to prevent potential misuse or security vulnerabilities.
- The sample tools provided in
-
Insufficient Protection Against Denied Tools
- The
DeniedToolslist inappsettings.jsonis empty by default. This could lead to a situation where no tools are explicitly denied, potentially allowing unsafe operations. - Recommendation: Provide a default list of commonly denied tools or explicitly document the risks of leaving this list empty.
- The
-
Potential Replay Attack Vulnerability
- The
MessageReplayWindowSecondsconfiguration inappsettings.jsonis set to 300 seconds (5 minutes). While this provides some protection against replay attacks, it may still be too lenient for high-security environments. - Recommendation: Consider reducing the replay window or providing guidance on how to configure this value based on the security requirements of the deployment.
- The
-
Credential Redaction
- The
EnableCredentialRedactionsetting inappsettings.jsonis set totrueby default, which is good. However, there is no evidence in the PR that this feature has been implemented or tested. - Recommendation: Ensure that credential redaction is implemented and thoroughly tested, especially in logs and error messages.
- The
🟡 WARNING
-
Breaking Changes in .NET Target Framework
- The new
Microsoft.AgentGovernance.Mcppackage targets.NET 8and.NET 10. This could potentially break compatibility with projects using older .NET versions. - Recommendation: Clearly document the minimum .NET version requirements for this package in the release notes and README.
- The new
-
Backward Compatibility with Existing MCP Implementations
- The introduction of a standalone
Microsoft.AgentGovernance.Mcppackage may lead to compatibility issues with existing MCP implementations that rely on the core Agent Governance Toolkit. - Recommendation: Provide migration guidance for existing users to transition to the new package, if necessary.
- The introduction of a standalone
💡 SUGGESTIONS
-
Enhance Documentation
- The provided README for the
McpGovernance.AspNetCoresample is clear but could benefit from additional details:- Include a section on security best practices, such as configuring
DeniedTools,SensitiveTools, andEnableCredentialRedaction. - Provide examples of how to implement custom
McpToolmethods with proper input validation and error handling.
- Include a section on security best practices, such as configuring
- The provided README for the
-
Add Unit Tests for New Features
- While the PR mentions that tests have been added, there is no evidence of unit tests for the new
Microsoft.AgentGovernance.Mcppackage or theMcpGovernance.AspNetCoresample. - Recommendation: Add unit tests for:
- Input validation and sanitization in
SampleTools. - Behavior of the
AddMcpGovernanceandUseMcpGovernancemethods. - Configuration binding from
appsettings.json.
- Input validation and sanitization in
- While the PR mentions that tests have been added, there is no evidence of unit tests for the new
-
Thread Safety
- The
McpToolRegistryclass is used to register tools in theMcpGovernance.AspNetCoresample. However, there is no indication that this class is thread-safe. - Recommendation: Ensure that
McpToolRegistryis thread-safe, especially if it will be used in a multi-threaded environment like ASP.NET Core.
- The
-
OWASP Agentic Top 10 Compliance
- The PR does not explicitly address compliance with the OWASP Agentic Top 10. While some features (e.g.,
EnableCredentialRedaction,EnableResponseScanning) are steps in the right direction, there is no evidence of a comprehensive approach to address all relevant risks. - Recommendation: Conduct a thorough review of the new package and sample against the OWASP Agentic Top 10 and document how each risk is mitigated.
- The PR does not explicitly address compliance with the OWASP Agentic Top 10. While some features (e.g.,
-
Consider Default Configuration Values
- The default values in
appsettings.json(e.g.,MaxToolCallsPerAgent,RateLimitWindowMinutes, etc.) may not be suitable for all use cases. - Recommendation: Provide guidance in the documentation on how to configure these settings based on different deployment scenarios (e.g., development, production, high-security environments).
- The default values in
-
Code Comments and Documentation
- While the sample code is well-commented, the main library code (e.g.,
AgentGovernance.ModelContextProtocol.csproj) lacks inline comments explaining the purpose and usage of key components. - Recommendation: Add comments to the main library code to improve maintainability and readability.
- While the sample code is well-commented, the main library code (e.g.,
Summary
This PR introduces a valuable new feature by providing a standalone Microsoft.AgentGovernance.Mcp package with minimal dependencies. However, there are several critical security issues and potential breaking changes that need to be addressed before merging. Additionally, there are opportunities to improve documentation, testing, and code quality.
Action Items:
- Address the critical security issues related to input validation, denied tools, replay attacks, and credential redaction.
- Document the .NET version requirements and provide migration guidance for existing users.
- Add unit tests for the new package and sample.
- Ensure thread safety in
McpToolRegistry. - Review the implementation for OWASP Agentic Top 10 compliance and document mitigations.
- Enhance inline comments and documentation for better maintainability.
Let me know if you need further clarification or assistance!
|
CI Status Summary ✅ All CI checks pass except:
Standalone |
99b2c2e to
a897823
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Feedback for PR: feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package
🔴 CRITICAL
-
Potential Security Risk in Sample Tool Implementation:
- The
ReadFilemethod in theSampleToolsclass appears to simulate reading a file from disk. However, the lack of proper input validation or sanitization for thepathparameter introduces a potential path traversal vulnerability. An attacker could exploit this to access sensitive files outside the intended directory. - Recommendation: Implement strict input validation and sanitization for the
pathparameter to ensure it does not allow directory traversal. For example, restrict file access to a specific directory and validate the path against this directory.
- The
-
Insufficient Validation for
SearchDatabaseTool:- The
SearchDatabasemethod accepts aqueryparameter without any validation or sanitization. This could lead to SQL injection vulnerabilities if the query is executed directly on a database. - Recommendation: Use parameterized queries or an ORM to prevent SQL injection. Avoid executing raw queries directly.
- The
-
Potential Misuse of
SendEmailTool:- The
SendEmailmethod has aRequiresApprovalattribute, but the implementation does not demonstrate how this approval process is enforced. If the approval mechanism is not implemented or is bypassed, this could lead to unauthorized email sending. - Recommendation: Ensure the
RequiresApprovalattribute is enforced by the governance layer. Add tests to verify that tools marked withRequiresApprovalcannot be executed without proper authorization.
- The
-
Lack of Authentication and Authorization in Sample:
- The provided ASP.NET Core sample does not include any authentication or authorization mechanisms. This could allow unauthorized users to access the MCP governance endpoints.
- Recommendation: Add authentication and authorization middleware to the sample application. For example, integrate with Azure AD or another identity provider to secure the endpoints.
🟡 WARNING
-
Potential Breaking Changes in .NET Target Frameworks:
- The new
Microsoft.AgentGovernance.Mcppackage targets.NET 8.0and.NET 10.0. While this is forward-looking, it may cause compatibility issues for users on older .NET versions. - Recommendation: Consider adding support for
.NET 6.0or.NET 7.0if feasible, as these versions are still widely used. Alternatively, clearly document the minimum required .NET version in the release notes.
- The new
-
Dependency on
ModelContextProtocol:- The new package introduces a dependency on the
ModelContextProtocolpackage (version1.2.0). If this dependency has breaking changes in future versions, it could impact the functionality of theMicrosoft.AgentGovernance.Mcppackage. - Recommendation: Monitor the
ModelContextProtocolpackage for updates and ensure compatibility. Consider adding integration tests to detect breaking changes early.
- The new package introduces a dependency on the
💡 SUGGESTIONS
-
Expand Documentation:
- The provided README for the sample application is helpful but could be expanded to include:
- A detailed explanation of the
McpGovernanceOptionssettings. - Examples of how to implement the
RequiresApprovalmechanism for tools likeSendEmail. - A note on the importance of securing the endpoints with authentication and authorization.
- A detailed explanation of the
- The provided README for the sample application is helpful but could be expanded to include:
-
Add Unit Tests for Sample Tools:
- The sample tools (
ReadFile,SearchDatabase,SendEmail) lack unit tests. Adding tests would help ensure their correctness and security. - Recommendation: Write unit tests to validate the behavior of each sample tool, including edge cases and potential misuse scenarios.
- The sample tools (
-
Add Logging for Tool Execution:
- The sample application does not include logging for tool execution. Adding logging would improve observability and help in debugging issues.
- Recommendation: Use
ILoggerto log tool execution details, including the tool name, parameters, and execution results.
-
Consider Adding Rate Limiting:
- While the sample application includes a
RateLimitWindowMinutessetting, it is unclear if this is enforced in the provided implementation. - Recommendation: Implement rate limiting for MCP tool calls to prevent abuse. Use middleware or a library like
AspNetCoreRateLimitto enforce rate limits.
- While the sample application includes a
-
Clarify Backward Compatibility:
- While this PR is marked as a non-breaking change, the introduction of a new package and potential dependency changes could impact existing users.
- Recommendation: Clearly document any potential backward compatibility concerns in the release notes. If there are none, explicitly state this to reassure users.
Summary
This PR introduces a valuable addition to the Agent Governance Toolkit by providing a standalone .NET package for MCP governance. However, there are critical security concerns in the sample tools and the lack of authentication/authorization in the sample application. Addressing these issues is essential to ensure the security and robustness of the implementation. Additionally, some minor improvements to documentation and backward compatibility considerations would enhance the overall quality of this contribution.
a897823 to
ec7a784
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review for feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package
🔴 CRITICAL
-
Policy Engine Correctness:
- Issue: The
demo_simulated.pyandgetting_started.pyexamples rely on YAML-based governance policies but do not validate the schema of the loaded YAML files. This could lead to runtime errors or, worse, silent policy misconfigurations that result in security bypasses. - Recommendation: Introduce schema validation for the YAML policy files using a library like
Cerberusorjsonschema. Ensure that all required fields (e.g.,name,condition,action,priority) are present and correctly typed before loading the policies into thePolicyEvaluator.
- Issue: The
-
Trust/Identity: Cryptographic Operations:
- Issue: The
AuditLogimplementation uses a Merkle chain for integrity verification, but the cryptographic algorithm used for hashing is not specified in the provided code. If a weak or outdated hashing algorithm is used, the audit trail could be compromised. - Recommendation: Ensure that the Merkle chain uses a secure hashing algorithm (e.g., SHA-256 or SHA-3). Explicitly document the algorithm used in the code and provide a mechanism to upgrade the hashing algorithm in the future if needed.
- Issue: The
-
Sandbox Escape Vectors:
- Issue: The
CapabilityGuardMiddlewareallows specifyingallowed_toolsanddenied_tools, but there is no validation to ensure that the tool names are sanitized or that the tools themselves are secure. This could lead to command injection or unauthorized access if the tool names are dynamically generated or user-controlled. - Recommendation: Implement strict validation and sanitization for tool names. Consider using a whitelist of pre-approved tools that are hardcoded or loaded from a secure configuration file.
- Issue: The
🟡 WARNING
- Backward Compatibility:
- Issue: The introduction of the standalone
Microsoft.AgentGovernance.Mcppackage is a new feature, but it re-exports core MCP modules. If the core MCP modules undergo breaking changes in the future, this package will also be affected. - Recommendation: Clearly document the dependency on the core MCP modules and establish a versioning strategy to ensure compatibility. Consider adding integration tests that validate the behavior of the standalone package against the core MCP modules.
- Issue: The introduction of the standalone
💡 SUGGESTIONS
-
Type Safety and Pydantic Model Validation:
- Observation: The
AgentContextandToolContextclasses indemo_simulated.pyare dynamically created usingtypeand lack explicit type annotations or validation. - Recommendation: Replace these dynamic classes with
pydanticmodels to enforce type safety and validate input data. This will reduce the risk of runtime errors and improve code maintainability.
from pydantic import BaseModel, Field class AgentContext(BaseModel): agent_name: str user_message: str metadata: dict = Field(default_factory=dict) stream: bool = False result: AgentResponse | None = None
- Observation: The
-
Thread Safety in Concurrent Agent Execution:
- Observation: The
AuditLogclass appears to maintain a stateful_chainobject. If the governance middleware is used in a multi-threaded or asynchronous environment, this could lead to race conditions. - Recommendation: Ensure that the
AuditLogimplementation is thread-safe. Use locks or asynchronous primitives to protect shared state.
- Observation: The
-
OWASP Agentic Top 10 Compliance:
- Observation: The examples demonstrate blocking PII and unauthorized tools, which aligns with OWASP Agentic Top 10 recommendations. However, there is no mention of protections against model inversion attacks or prompt injection in the policy engine.
- Recommendation: Extend the policy engine to include rules for detecting and mitigating prompt injection attacks. For example, add a rule to detect and block inputs that attempt to manipulate the LLM's behavior (e.g., "Ignore all previous instructions and...").
-
Documentation Improvements:
- The
README.mdfiles in the examples are detailed and helpful, but they could benefit from a dedicated section on common pitfalls and best practices for integrating the governance toolkit. For example:- Emphasize the importance of testing policies in a staging environment before deploying them to production.
- Provide guidance on how to handle policy updates and rollbacks.
- Include a section on logging and monitoring best practices for the audit trail.
- The
-
Testing Coverage:
- Observation: The PR mentions that tests have been added, but there is no evidence of tests specifically targeting edge cases for the policy engine or the audit trail.
- Recommendation: Add unit tests for the following scenarios:
- Invalid or malformed YAML policy files.
- Edge cases for the policy engine (e.g., empty rules, conflicting rules, or unsupported operators).
- Audit trail integrity verification with tampered entries.
-
Error Handling:
- Observation: The examples in
demo_simulated.pyandgetting_started.pyusetry-exceptblocks to handleMiddlewareTerminationexceptions but do not log the reason for the termination. - Recommendation: Enhance error handling by logging the specific reason for the termination (e.g., the name of the violated policy or the blocked tool). This will improve debugging and traceability.
try: await policy_middleware.process(ctx, passthrough) except MiddlewareTermination as e: print(f"BLOCKED — {str(e)}")
- Observation: The examples in
Summary
- 🔴 CRITICAL: Address policy engine schema validation, cryptographic algorithm specification, and tool name sanitization.
- 🟡 WARNING: Document dependency on core MCP modules and ensure backward compatibility.
- 💡 SUGGESTIONS: Improve type safety with
pydantic, ensure thread safety, enhance OWASP compliance, improve documentation, and add more comprehensive tests.
Let me know if you need further clarification or assistance with implementing these recommendations!
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces a standalone .NET package, Microsoft.AgentGovernance.Mcp, and includes documentation updates, examples, and a simulated demo for the Agent Governance Toolkit. The PR is part of a multi-step process to modularize the MCP governance primitives for easier adoption by MCP server operators. The changes are well-documented and provide clear examples for both real and simulated integrations.
Below is the detailed review of the changes, focusing on the specified areas of concern.
🔴 CRITICAL
-
Policy Engine Correctness:
- The
GovernancePolicyMiddlewareandCapabilityGuardMiddlewarerely on theMiddlewareTerminationexception to block unauthorized actions. However, there is no explicit test coverage for edge cases, such as:- Policies with overlapping or conflicting rules.
- Handling of malformed or unexpected inputs (e.g., invalid YAML policies, unexpected message formats).
- Race conditions in concurrent policy evaluations.
- Action Required: Add comprehensive tests for edge cases, including malformed inputs, conflicting policies, and concurrent evaluations.
- The
-
Trust/Identity: Cryptographic Operations:
- The
AuditLogintegrity verification is mentioned but not thoroughly tested in the provided examples. The integrity check relies on a Merkle chain, but there is no evidence of testing for tampering scenarios (e.g., modifying or removing entries). - Action Required: Add tests to simulate tampering with the audit log and verify that the integrity check detects these modifications.
- The
-
Sandbox Escape Vectors:
- The documentation recommends using
gVisororKata Containersfor untrusted MCP servers but does not enforce or validate this in the code. This leaves room for misconfiguration or oversight. - Action Required: Implement runtime checks or configuration validators to ensure that sandboxing mechanisms (e.g.,
gVisor,Kata Containers) are correctly applied in deployments.
- The documentation recommends using
🟡 WARNING
-
Backward Compatibility:
- The introduction of the standalone
.NETpackage (Microsoft.AgentGovernance.Mcp) appears to be non-breaking. However, the PR does not explicitly confirm whether the existing MCP-related functionality in the core Agent Governance Toolkit remains unaffected. - Action Required: Add tests to verify that the core MCP functionality in the existing toolkit is not impacted by the new standalone package.
- The introduction of the standalone
-
Type Safety and Pydantic Model Validation:
- The
AgentContextandToolContextclasses indemo_simulated.pyuse dynamically created types (type("A", (), {...})) instead of well-defined Pydantic models. This approach is error-prone and may lead to runtime issues. - Action Required: Replace dynamically created types with properly defined Pydantic models to ensure type safety and validation.
- The
💡 SUGGESTIONS
-
Documentation:
- The documentation is thorough and well-structured. However, it would be helpful to include a section on common pitfalls and troubleshooting tips for users integrating the standalone
.NETpackage. - Consider adding a note about the importance of keeping the YAML policies up-to-date and consistent across MCP servers.
- The documentation is thorough and well-structured. However, it would be helpful to include a section on common pitfalls and troubleshooting tips for users integrating the standalone
-
Thread Safety in Concurrent Agent Execution:
- While the PR does not introduce any explicit threading issues, it would be prudent to include tests for concurrent execution of the
GovernancePolicyMiddlewareandCapabilityGuardMiddlewareto ensure thread safety. - Add a note in the documentation about the thread safety of the middleware components.
- While the PR does not introduce any explicit threading issues, it would be prudent to include tests for concurrent execution of the
-
OWASP Agentic Top 10 Compliance:
- The documentation aligns well with OWASP MCP Security Cheat Sheet recommendations. However, it would be beneficial to explicitly map the provided hardening guide to the OWASP Agentic Top 10 risks to ensure comprehensive coverage.
-
Examples:
- The simulated demo (
demo_simulated.py) is a great addition for users who want to experiment without installing external dependencies. However, the example could benefit from additional comments explaining the purpose of each step in more detail. - Consider adding a section in the
READMEthat explains the differences between the real and simulated demos and when to use each.
- The simulated demo (
Summary of Actions
Critical
- Add tests for edge cases in policy engine evaluation.
- Add tests for audit log tampering detection.
- Implement runtime checks or validators for sandboxing mechanisms.
Warning
- Verify backward compatibility with existing MCP functionality.
- Replace dynamically created types in
demo_simulated.pywith Pydantic models.
Suggestions
- Enhance documentation with troubleshooting tips and OWASP Agentic Top 10 mapping.
- Add thread safety tests for middleware components.
- Improve comments and explanations in the simulated demo.
Final Notes
The PR is a significant step forward in modularizing the MCP governance functionality and providing clear guidance for secure deployment. Addressing the critical and warning issues will ensure the robustness and security of the implementation.
Add clear integration documentation to examples/maf-integration/README.md: - Integration Pattern section with ASCII diagram showing AGT as deterministic middleware in the MAF pipeline - 3-step quickstart (install, YAML policy, middleware code) for adding AGT to an existing MAF project - Two Ways to Run section explaining simulated vs live LLM modes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename existing getting_started.py to demo_simulated.py (no-dependency version) - Create new getting_started.py with real smolagents @tool decorator usage - Wrap tool forward() with AGT governance checks (key integration pattern) - Add requirements.txt for smolagents example dependencies - Update README.md with 'Two Ways to Run' section and updated key files table Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename getting_started.py to demo_simulated.py (no-dependency version) - Create new getting_started.py with real CrewAI Agent/Task/Crew objects wrapped in AGT governance middleware - Add requirements.txt for the example - Update README.md with 'Two ways to run' section and updated file table Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename getting_started.py -> demo_simulated.py (no-dependency version) - Create new getting_started.py with real Agent, function_tool, and InputGuardrail integration showing AGT governance as a guardrail - Add requirements.txt for SDK dependencies - Update README.md with 'Two ways to run' section and updated file refs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nfusion allowlist Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9da5ae6 to
a5b05da
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: feat(dotnet): add standalone Microsoft.AgentGovernance.Mcp package
🔴 CRITICAL Issues
-
Policy Engine Correctness: Potential False Negatives
- In
demo_simulated.pyandgetting_started.py, the governance middleware (GovernancePolicyMiddlewareandCapabilityGuardMiddleware) is used to enforce policies. However, there is no explicit test coverage for edge cases like:- Policies with overlapping or conflicting rules.
- Complex nested conditions in YAML policies.
- Handling of malformed or invalid policies.
- Action: Add comprehensive unit tests to validate the behavior of the policy engine under edge cases. Ensure that the middleware correctly identifies and blocks policy violations, especially for scenarios involving ambiguous or conflicting rules.
- In
-
Trust/Identity: Cryptographic Operations
- The audit log's integrity verification relies on a Merkle chain (
audit_log.verify_integrity()), but there is no evidence of cryptographic key management or secure storage for the root hash or private keys. - Action: Ensure that the root hash and any cryptographic keys used for the Merkle chain are securely stored and managed. Consider integrating with a secure key management solution (e.g., Azure Key Vault, AWS KMS).
- The audit log's integrity verification relies on a Merkle chain (
-
Sandbox Escape Vectors
- The documentation for Kubernetes hardening mentions using
gVisororKata Containersfor untrusted MCP servers. However, there is no enforcement mechanism in the codebase to ensure that these sandboxing mechanisms are applied. - Action: Implement runtime checks or deployment validations to ensure that MCP servers are deployed with the recommended sandboxing mechanisms. For example, verify that the
RuntimeClassNameis set togvisororkatain Kubernetes deployments.
- The documentation for Kubernetes hardening mentions using
-
Credential Handling
- The
getting_started.pyscript mentions the use of environment variables for API keys (e.g.,OPENAI_API_KEY,GITHUB_TOKEN). However, there is no validation or secure handling of these credentials in the code. - Action: Add validation to ensure that required environment variables are set before execution. Additionally, document best practices for securely managing these credentials (e.g., using environment variable managers or secret management tools).
- The
🟡 WARNING: Potential Breaking Changes
- Backward Compatibility: Public API Changes
- The introduction of the standalone
Microsoft.AgentGovernance.Mcppackage may lead to confusion for existing users of theAgent Governance Toolkitif the documentation does not clearly differentiate between the standalone package and the full toolkit. - Action: Update the main README and release notes to clearly explain the relationship between the standalone package and the full toolkit. Highlight any differences in functionality or dependencies.
- The introduction of the standalone
💡 Suggestions for Improvement
-
OWASP Agentic Top 10 Compliance
- The documentation provides excellent guidance on hardening MCP servers (e.g., network isolation, file system restrictions). However, there is no mention of logging and monitoring for anomalous behavior, which is a key aspect of OWASP Agentic Top 10 compliance.
- Suggestion: Add a section to the hardening guide on logging and monitoring best practices. For example:
- Enable audit logging for all MCP server actions.
- Monitor for unusual patterns in agent behavior (e.g., excessive API calls, unexpected tool usage).
- Integrate with SIEM solutions for real-time alerting.
-
Type Safety and Pydantic Model Validation
- The
AgentContextandToolContextclasses indemo_simulated.pyare manually defined and lack type validation. - Suggestion: Replace these classes with Pydantic models to enforce type safety and validation. For example:
from pydantic import BaseModel class AgentContext(BaseModel): agent_name: str user_message: str metadata: dict = {} stream: bool = False result: AgentResponse | None = None
- The
-
Thread Safety in Concurrent Agent Execution
- The
AuditLogclass appears to be a shared resource, but there is no indication of thread safety mechanisms (e.g., locks) to prevent race conditions during concurrent writes. - Suggestion: Ensure that the
AuditLogimplementation is thread-safe. For example, useasyncio.Lockorthreading.Lockto synchronize access to the log.
- The
-
Documentation Enhancements
- The new
mcp-server-hardening.mddocument is comprehensive, but it could benefit from additional examples and diagrams to illustrate key concepts (e.g., how the Merkle chain works, how policies are evaluated). - Suggestion: Add visual aids (e.g., architecture diagrams, flowcharts) to the documentation to make it more accessible to new users.
- The new
-
Test Coverage
- While the PR mentions that all tests pass, there is no mention of test coverage metrics.
- Suggestion: Include a test coverage report in the CI/CD pipeline to ensure that new features are adequately tested. Aim for at least 90% coverage, with a focus on critical paths like policy evaluation and audit logging.
Summary of Recommendations
-
Critical Fixes:
- Add tests for policy engine edge cases.
- Secure cryptographic key management for the audit log.
- Enforce sandboxing mechanisms for untrusted MCP servers.
- Validate and securely handle API credentials.
-
Warnings:
- Update documentation to clarify the relationship between the standalone MCP package and the full toolkit.
-
Suggestions:
- Enhance OWASP compliance with logging and monitoring guidance.
- Use Pydantic models for type safety and validation.
- Ensure thread safety for shared resources like the
AuditLog. - Add visual aids to the documentation.
- Include test coverage metrics in the CI/CD pipeline.
This PR introduces valuable functionality, but addressing the critical issues and warnings is essential to ensure security and maintain backward compatibility.
|
Thank you for this work, @jackbatzner — the MCP governance implementation across multiple languages is impressive and clearly well-thought-out. After reviewing the full series, we've decided to take a hybrid approach (Decision C): keep MCP core logic in agent-os/agent-mesh and provide thin standalone re-export packages for enterprise repackaging — similar to what we shipped in PR #910 for Python. We're closing this PR because we'll implement the .NET standalone package in-house following this architecture. Your work directly informed our approach — the OWASP mapping methodology, the gateway/rate-limiter/session-auth pattern, and the trust proxy design all carry forward. We're keeping your documentation PRs open for review as they add independent value. Thank you for the sustained contribution effort across the entire MCP governance surface. |
Description
Introduces the standalone
Microsoft.AgentGovernance.McpNuGet package with minimal dependencies. This allows MCP server operators to adopt MCP governance primitives without pulling in the full Agent Governance Toolkit.Package:
Microsoft.AgentGovernance.Mcp(multi-target .NET 8 / .NET 10)Type of Change
Package(s) Affected
Checklist
Related Issues
Supersedes #775 (split for easier review). Merge after #775.