feat(dotnet): add MCP protocol support with full OWASP coverage#775
feat(dotnet): add MCP protocol support with full OWASP coverage#775jackbatzner wants to merge 9 commits intomicrosoft:mainfrom
Conversation
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
2 times, most recently
from
607ac16 to
8660bd9
Compare
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Thank you for the .NET MCP work. However, this PR modifies ci.yml (+454/-452) and publish.yml (+264/-262) which are security-critical workflow files protected by CODEOWNERS.
Workflow changes need maintainer review of every line — with 700+ lines of changes, this needs to be split:
- One PR with the .NET SDK code changes only (no .github/ modifications)
- A separate PR with CI/publish workflow changes that we can review line-by-line
Please remove the .github/ changes and resubmit the code portion. We'll handle the CI integration.
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
2 times, most recently
from
4c82db5 to
4d9ca52
Compare
|
@imran-siddique Great catch — the massive diff was a CRLF line-ending artifact from editing on Windows. The actual workflow change is just 3 lines per file (adding 10.0.x to dotnet-version for our .NET 10 multi-targeting). Fixed in 4c82db5: Converted all files back to match their original line endings |
Enterprise Hardening UpdateLatest push adds enterprise-grade MCP seam hardening and 3 self-review fixes: Self-Review Fixes
Enterprise Patterns
Validation
Note: |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Thorough implementation with excellent test coverage (988 tests!). However, several security issues need addressing:
Blocking:
- CredentialRedactor only redacts PEM header, leaves key body intact — the regex pattern matches headers like BEGIN PRIVATE KEY but the body (the actual secret) remains in plaintext. The full PEM block must be redacted.
- 8 ML-DSA tests fail on CI — these need platform guards or skip markers since ML-DSA requires .NET 10 with specific crypto support.
Security concerns (should fix):
- Exception messages leak internal details (stack traces, paths) to callers
- HMAC minimum key size (16 bytes) is below NIST recommendation (32 bytes)
- Regex timeout exceptions are swallowed = fail-open on crafted input
- Rate limiter lock map grows unbounded (no eviction)
- ToolRegistry uses unsynchronized mutable List (thread safety)
Architecture (nice to fix):
- Session rate-limit key uses colon delimiter (ambiguous with IPv6)
- Middleware consumes request body without enabling buffering
- Hardcoded agent ID ignores configuration
Good test-to-code ratio (1:1.2). Consider splitting into 2-3 PRs for easier review if the fixes are substantial.
github-actions
bot
added
documentation
Addressing Review Feedback@imran-siddique — thanks for the thorough reviews! Here's how we've addressed every item: ✅ Review 1: Remove .github/ changes
Fixed: All ✅ Blocking: PEM Body Redaction
Fixed: Regex now matches full PEM blocks from ✅ Blocking: ML-DSA Test Platform Guards
Fixed: Added conditional compilation guards ( ✅ Security: Exception Message Sanitization
Fixed: Security boundary methods now catch internal exceptions, log details via ✅ Security: HMAC Minimum Key Size
Fixed: Increased minimum key size from 16 to 32 bytes per NIST SP 800-107. ✅ Security: Regex Timeout Fail-Closed
Fixed: Regex timeout exceptions now trigger a DENY decision (fail-closed) instead of being silently swallowed. ✅ Security: Rate Limiter Eviction
Fixed: Added TTL-based eviction for inactive client entries in ✅ Security: ToolRegistry Thread Safety
Fixed: Replaced mutable ✅ Architecture: IPv6-Safe Delimiter
Fixed: Changed delimiter from ✅ Architecture: Middleware Body Buffering
Fixed: Added ✅ Architecture: Configurable Agent ID
Fixed: Agent ID now reads from Validation
|
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
from
224a0a0 to
b4a9029
Compare
github-actions
bot
removed
the
documentation
jackbatzner
changed the title
PR Split UpdateThis PR has been updated to contain only the core MCP security primitives and tests for easier review. The original monolithic PR has been split into 3 stacked PRs:
Merge order: #775 → #824 → #826 All previous review feedback from @imran-siddique has been addressed in this updated branch (see earlier comment for details). |
🤖 AI Agent: breaking-change-detector — Summary🔍 API Compatibility ReportSummaryThis pull request primarily introduces new features to the Findings
Migration Guide✅ No breaking changes detected. No migration steps are necessary. Recommendations
Conclusion✅ This pull request introduces new features without any breaking changes. It is safe to merge from an API compatibility perspective. |
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
If everything looks good, say ✅ Documentation is in sync. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces MCP (Model Context Protocol) security primitives to the .NET Agent Governance Toolkit, with a focus on OWASP compliance and robust security features. The implementation includes HMAC-SHA256 and ML-DSA signing, nonce replay protection, rate-limiting, input sanitization, and credential redaction. It also integrates these features into the ModelContextProtocol SDK, providing a seamless way to add governance to the MCP server pipeline.
The PR is well-structured and adheres to the project's style guidelines. However, there are some areas that require attention to ensure security, maintainability, and backward compatibility.
🔴 CRITICAL
-
Replay Protection Implementation
- The PR mentions nonce replay protection but does not provide details about how the
ISignerNonceStoreis implemented or used. Ensure that the nonce storage mechanism is resistant to race conditions and that it properly handles distributed systems (e.g., using a distributed cache like Redis with atomic operations). Without this, the replay protection may be bypassed. - Action: Provide details about the
ISignerNonceStoreimplementation or include it in the PR. Ensure it uses atomic operations and is thread-safe.
- The PR mentions nonce replay protection but does not provide details about how the
-
Fail-Closed Behavior
- The
AddCallToolGovernanceFiltermethod has atry-catchblock that logs errors and denies tool calls if an exception occurs. While this is a good practice, it is critical to ensure that all exceptions are caught and logged. If any exceptions are missed, it could lead to a security bypass. - Action: Review all possible exceptions that could be thrown in the governance pipeline and ensure they are properly handled. Consider adding a generic catch block for unexpected exceptions.
- The
-
Credential Redaction
- The
CredentialRedactor.ContainsCredentialsandCredentialRedactor.Redactmethods are used for redacting sensitive information. However, there is no information about how these methods work or whether they are robust against edge cases (e.g., partial matches, complex patterns). - Action: Provide details about the implementation of these methods and ensure they are thoroughly tested against a wide range of input cases, including edge cases.
- The
-
Input Validation
- The
McpInputSanitizeris mentioned as regex-based, but there is no information about the specific regex patterns used. Improper or overly permissive regex patterns can lead to security vulnerabilities. - Action: Include the regex patterns in the PR or provide documentation on their design and testing. Ensure they are tested against malicious inputs.
- The
🟡 WARNING
-
Breaking Changes
- The
AgentGovernanceproject now targets bothnet8.0andnet10.0instead of justnet8.0. This could potentially introduce compatibility issues for consumers of the library who are not yet ready to migrate to .NET 10. - Action: Clearly document this change in the release notes and communicate it to users. Consider providing guidance on how to handle the transition.
- The
-
Default Configuration Changes
- The
McpGovernanceOptionsclass now includes new properties (e.g.,EnableResponseScanning,EnableCredentialRedaction). If these properties are not set in existing configurations, the defaults may lead to unexpected behavior. - Action: Ensure that the default values for these properties are explicitly documented and align with the principle of least privilege.
- The
💡 SUGGESTIONS
-
Thread Safety
- The PR mentions thread-safe operations in
McpToolRegistry, but there is no information about how this is achieved. - Suggestion: Provide details about the thread-safety mechanisms used in
McpToolRegistry. If locks or other synchronization primitives are used, ensure they are implemented correctly to avoid deadlocks or performance bottlenecks.
- The PR mentions thread-safe operations in
-
Logging
- The logging in
AddCallToolGovernanceFilteris comprehensive, but it may lead to log flooding in high-traffic scenarios. - Suggestion: Consider adding rate-limiting or log sampling to reduce the volume of logs in high-traffic environments.
- The logging in
-
Documentation
- The PR includes some documentation, but additional details about the new features and their usage would be helpful for developers.
- Suggestion: Expand the documentation to include examples of how to implement
ISignerNonceStoreandISessionStore. Also, provide more details about theCredentialRedactorandMcpInputSanitizer.
-
Unit Tests
- While the PR mentions that all tests pass, it is unclear if there are sufficient tests for the new features.
- Suggestion: Ensure that there are comprehensive unit tests for all new features, especially for security-critical components like
McpMessageSigner,McpMessageValidator, andCredentialRedactor.
-
Backward Compatibility
- The PR introduces new methods like
WithGovernanceandUseMcpGovernance. While these are optional, they could lead to confusion for existing users. - Suggestion: Clearly document the new methods and their usage. Consider adding deprecation warnings for any older methods that are being replaced.
- The PR introduces new methods like
-
Performance Testing
- The PR introduces several new features that could impact performance, such as rate-limiting, input sanitization, and response scanning.
- Suggestion: Conduct performance testing to ensure that these features do not introduce significant latency or resource overhead.
Summary of Actions
Critical
- Provide details about the
ISignerNonceStoreimplementation and ensure it is thread-safe and resistant to race conditions. - Review and handle all possible exceptions in the governance pipeline to ensure fail-closed behavior.
- Provide details about the
CredentialRedactorimplementation and test it against edge cases. - Include or document the regex patterns used in
McpInputSanitizerand ensure they are robust.
Warnings
- Document the potential breaking change due to the new target frameworks.
- Ensure default values for new configuration properties are documented and align with security best practices.
Suggestions
- Provide details about thread-safety mechanisms in
McpToolRegistry. - Implement log rate-limiting or sampling to prevent log flooding.
- Expand documentation to include examples and details about new features.
- Ensure comprehensive unit tests for all new features.
- Conduct performance testing to assess the impact of new features.
This PR introduces significant security improvements and aligns well with OWASP guidelines. Addressing the critical issues and implementing the suggested improvements will ensure a robust and secure implementation.
🤖 AI Agent: security-scanner — Security Analysis of the Pull RequestSecurity Analysis of the Pull Request1. Prompt Injection Defense Bypass
2. Policy Engine Circumvention
3. Trust Chain Weaknesses
4. Credential Exposure
5. Sandbox Escape
6. Deserialization Attacks
7. Race Conditions
8. Supply Chain Risks
Summary of Findings
Final RecommendationThe PR introduces critical security features but also presents significant risks, particularly around prompt injection, deserialization, and policy circumvention. These issues must be addressed before merging to ensure the security of downstream users. |
Addressing Review Feedback — Updated@imran-siddique — thanks for the thorough reviews! Here's the current state after the split and latest security fixes: ✅ Review 1: Remove .github/ changes
Fixed — All ✅ Review 2: Security findings
Fixed — Full PEM block redaction (BEGIN + body + END) using Additional security hardening (from internal review):
|
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces MCP (Model Context Protocol) support to the .NET Agent Governance Toolkit with a focus on OWASP compliance and security. The implementation includes a comprehensive set of features such as HMAC-SHA256 and ML-DSA signing, nonce replay protection, rate-limited session management, input sanitization, and credential redaction. The PR also integrates these features into the ModelContextProtocol SDK, providing a seamless way to add governance to the MCP server pipeline.
The implementation is robust and well-documented, with a clear focus on security and extensibility. However, there are a few areas that require attention to ensure compliance with security best practices, maintain backward compatibility, and improve code quality.
🔴 CRITICAL
-
Replay Protection Implementation:
- The PR mentions nonce replay protection, but there is no code provided in the diff that demonstrates how this is implemented. Replay attacks are a critical security concern, and the absence of visible implementation details raises a red flag.
- Action: Ensure that the
McpMessageSignerandMcpMessageValidatorinclude robust nonce management and replay protection mechanisms. Provide unit tests to validate these features.
-
Input Validation:
- The
ExtractParametersmethod extracts parameters from theCallToolRequestParamsobject but does not validate the extracted values. This could lead to injection vulnerabilities. - Action: Add input validation for extracted parameters to ensure they conform to expected formats and constraints. Use regex-based or schema-based validation where applicable.
- The
-
Fail-Closed Behavior:
- While the
AddCallToolGovernanceFiltermethod includes fail-closed behavior for exceptions, it does not log sufficient details about the exception. This could hinder debugging and incident response. - Action: Enhance logging to include more details about the exception, such as the stack trace and additional context.
- While the
-
Credential Redaction:
- The
CredentialRedactor.ContainsCredentialsandCredentialRedactor.Redactmethods are used for redacting sensitive information, but their implementation is not included in the diff. - Action: Review the implementation of these methods to ensure they are comprehensive and do not result in false negatives (missed sensitive data).
- The
🟡 WARNING
-
Backward Compatibility:
- The
TargetFrameworksproperty inAgentGovernance.csprojhas been updated to include.NET 10.0. This could potentially break compatibility for users who are not ready to migrate to .NET 10.0. - Action: Clearly document this change in the release notes and consider providing guidance for users who may need to remain on .NET 8.0.
- The
-
Public API Changes:
- The introduction of new public methods like
WithGovernanceandMapMcpGovernancein extension classes constitutes a public API change. - Action: Ensure these changes are backward-compatible and document them in the release notes.
- The introduction of new public methods like
💡 SUGGESTIONS
-
Unit Tests:
- While the PR mentions that all tests pass, it is not clear if the new features are thoroughly tested.
- Action: Add unit tests for all new features, especially for critical components like
McpMessageSigner,McpMessageValidator, andMcpGateway.
-
Concurrency and Thread Safety:
- The PR claims thread-safe operations in
McpToolRegistry, but there is no evidence in the diff to confirm this. - Action: Review the implementation of
McpToolRegistryand other concurrent components to ensure proper thread safety mechanisms (e.g., locks, thread-safe collections) are in place.
- The PR claims thread-safe operations in
-
OWASP Compliance:
- The PR claims compliance with 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, excluding §11 (Consent UI). While this is acceptable for a server-side SDK, it would be helpful to document this explicitly in the README or documentation.
- Action: Update the documentation to clearly state which OWASP sections are covered and which are out of scope.
-
Logging Best Practices:
- The logging in the
AddCallToolGovernanceFiltermethod could be improved by using structured logging to include more context (e.g., tool name, agent ID, and request parameters). - Action: Use structured logging to provide more actionable insights during debugging and incident response.
- The logging in the
-
Performance Considerations:
- The
ScanAndRedactResponsemethod iterates over all content blocks and performs multiple operations (scanning, sanitization, and redaction). This could become a performance bottleneck for large responses. - Action: Profile the performance of this method with large datasets and optimize as needed. Consider parallelizing the operations if they are computationally expensive.
- The
-
Documentation:
- The PR includes some documentation, but it would benefit from more detailed examples and explanations, especially for the new features.
- Action: Expand the documentation to include detailed usage examples, configuration options, and potential pitfalls.
Conclusion
The PR is a significant step forward in enhancing the security and functionality of the .NET Agent Governance Toolkit. However, the critical issues related to replay protection, input validation, and credential redaction must be addressed before merging. Additionally, attention should be given to backward compatibility, thread safety, and comprehensive testing. Once these issues are resolved, the PR will be ready for approval.
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
from
efbf7e9 to
cce86a0
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces significant new features to the .NET Agent Governance Toolkit, particularly focusing on the Model Context Protocol (MCP) with comprehensive OWASP coverage. The implementation appears robust, with a strong emphasis on security and compliance. However, there are several areas that require attention, including potential breaking changes and security considerations.
Actionable Feedback
🔴 CRITICAL Issues
-
Cryptographic Operations: Ensure that the implementation of HMAC-SHA256 and ML-DSA is thoroughly vetted for security vulnerabilities. Pay particular attention to the nonce replay protection mechanism in
McpMessageSigner. Any weaknesses here could lead to security bypasses. -
Input Validation: The
McpInputSanitizeruses regex-based validation. Ensure that the regex patterns are not susceptible to ReDoS (Regular Expression Denial of Service) attacks. Consider using a timeout for regex operations to mitigate this risk. -
Session Management: The
McpSessionManagershould be reviewed to ensure that it properly handles session expiration and concurrency limits. Any flaws in session management could lead to unauthorized access. -
Logging Sensitive Information: The
CredentialRedactorshould be tested to ensure that it effectively redacts all sensitive information from logs. Any failure to do so could lead to sensitive data exposure.
🟡 WARNING Issues
-
Backward Compatibility: The introduction of new classes and methods (e.g.,
McpMessageSigner,McpSessionManager) may lead to breaking changes if existing consumers of the library rely on previous implementations. Ensure that the changes are well-documented and consider versioning strategies to maintain backward compatibility. -
Thread Safety: While
McpToolRegistryis stated to be thread-safe, it is crucial to verify that all operations within the class are indeed safe for concurrent access. Any race conditions could lead to inconsistent states. -
API Changes: The addition of new public classes and methods may affect existing users. Ensure that the documentation clearly outlines these changes and provides guidance on migration if necessary.
💡 SUGGESTION Improvements
-
Unit Tests: While you mentioned that all tests pass, consider adding more unit tests specifically targeting edge cases for the new features, especially around the cryptographic operations and input validation.
-
Documentation: The documentation for the new features should include examples of usage, particularly for the
McpSessionManagerandMcpMessageSigner. Clear examples can help users understand how to implement these features securely. -
Performance Considerations: Evaluate the performance impact of the new features, particularly the regex validation and session management. Consider adding benchmarks to the documentation to inform users of potential performance trade-offs.
-
Security Hardening: The new
mcp-server-hardening.mdguide is a great addition. Ensure that it is kept up-to-date with best practices as they evolve, and consider integrating it into the CI/CD pipeline to validate configurations against these guidelines. -
Code Review: Have the cryptographic components reviewed by a security expert to ensure that they meet industry standards and best practices.
Conclusion
The proposed changes significantly enhance the security and functionality of the .NET Agent Governance Toolkit. However, careful attention must be paid to the identified critical issues and potential breaking changes to ensure a secure and smooth integration for users.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces significant new features related to the Model Context Protocol (MCP) in the .NET Agent Governance Toolkit. The implementation appears to be well-structured and adheres to OWASP guidelines, but there are several areas that require attention to ensure security, maintainability, and compliance with best practices.
Actionable Feedback
🔴 CRITICAL Issues
-
Cryptographic Operations: Ensure that the HMAC-SHA256 and ML-DSA implementations are using secure libraries and are resistant to timing attacks. Review the implementation of nonce replay protection to confirm it is robust against replay attacks.
-
Input Validation: The
McpInputSanitizeruses regex-based validation. Ensure that the regex patterns are comprehensive enough to prevent injection attacks (e.g., SQL injection, command injection). Consider using a well-established library for input sanitization to avoid common pitfalls. -
Session Management: The
McpSessionManagershould be reviewed for potential race conditions, especially under high concurrency. Ensure that session lifecycle management is thread-safe and that there are no vulnerabilities related to session fixation or hijacking. -
Credential Handling: The
CredentialRedactorshould be thoroughly tested to ensure it effectively redacts all sensitive information. Consider implementing additional logging controls to prevent accidental logging of sensitive data.
🟡 WARNING Issues
-
Backward Compatibility: The introduction of new classes and interfaces (e.g.,
McpMessageSigner,McpSessionManager) may impact existing implementations. Ensure that any public API changes are documented and consider versioning strategies to maintain backward compatibility. -
Concurrency Limits: The
McpSessionManagerhas configurable concurrency limits. Ensure that these limits are well-documented and that users are aware of the implications of setting them too high or too low. -
Documentation: The documentation mentions that the
McpToolRegistryis thread-safe, but it would be beneficial to provide examples or guidelines on how to use it safely in a concurrent environment.
💡 SUGGESTION
-
Testing: While it is noted that all tests pass, consider adding more unit tests that specifically target edge cases and potential failure modes, particularly around concurrency and input validation.
-
Performance Considerations: Evaluate the performance impact of the new features, especially the regex-based input validation and session management. Consider benchmarking these components under load.
-
Code Comments: Adding comments in critical sections of the code, especially where cryptographic operations are performed, can improve maintainability and help future developers understand the rationale behind certain implementations.
-
Security Hardening: In the
mcp-server-hardening.md, consider including more specific examples of how to implement security best practices in various environments (e.g., AWS, Azure, GCP) to assist users in deploying securely. -
Versioning: Given the significant changes introduced, consider using semantic versioning to communicate the nature of the changes clearly to users.
Conclusion
The PR introduces valuable features that enhance the security and functionality of the .NET Agent Governance Toolkit. However, addressing the critical issues related to cryptographic operations, input validation, and session management is essential to ensure the security of the implementation. Additionally, careful consideration of backward compatibility and thorough documentation will help maintain the integrity of the toolkit as it evolves.
|
CI Status Summary — All findings addressed ✅ All CI checks pass except:
Fixes since last review:
This is the Core PR in a 3-PR stack. Merge order: #775 (Core) → #824 (Package) → #826 (Docs). |
|
Approved and ready to merge, but this PR has merge conflicts with main (likely from #774 merging). Please rebase on latest main to resolve conflicts, then we can merge. |
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
from
7e9858b to
92ae400
Compare
github-actions
bot
removed
agent-mesh
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a significant new feature to the microsoft/agent-governance-toolkit repository, adding MCP (Model Context Protocol) support to the .NET Agent Governance Toolkit. The implementation is comprehensive, with a focus on security and compliance with the OWASP MCP Security Cheat Sheet. The PR includes new components for message signing, session management, input sanitization, and credential redaction, among others. It also provides detailed documentation and examples, which is commendable.
Below is the detailed review of the PR:
🔴 CRITICAL
-
Replay Protection in
McpMessageSignerandMcpMessageValidator:- The PR mentions nonce replay protection but does not provide details on how nonces are managed or stored. Ensure that nonces are stored securely and that the storage mechanism is resistant to race conditions and tampering. If the nonce store is not implemented securely, it could lead to replay attacks.
- Action: Provide details on the nonce storage mechanism and ensure it is implemented securely, possibly using a database with atomic operations or a distributed cache with eviction policies.
-
Regex-based Input Validation in
McpInputSanitizer:- While regex-based input validation is mentioned, there is no evidence in the PR that the regex patterns have been reviewed for completeness or that they are resistant to Regular Expression Denial of Service (ReDoS) attacks.
- Action: Ensure that all regex patterns are reviewed for ReDoS vulnerabilities and include tests to validate their performance under edge cases.
-
Thread Safety in
McpSessionManager:- The PR mentions that the session limit is checked under a lock for TOCTOU safety, but there is no evidence of how this is implemented. Improper locking mechanisms can lead to race conditions.
- Action: Provide details on the locking mechanism used in
McpSessionManagerand ensure it is robust against race conditions.
-
Credential Redaction in
CredentialRedactor:- The PR mentions redaction of sensitive information like PEM blocks and secret patterns. However, there is no evidence that the redaction patterns have been tested for edge cases or that the redaction process is fail-safe.
- Action: Include test cases to validate the effectiveness of the redaction patterns and ensure that no sensitive information is leaked in logs.
🟡 WARNING
- Backward Compatibility:
- The addition of new public APIs (
McpMessageSigner,McpMessageValidator, etc.) could potentially introduce breaking changes if existing users rely on the previous API structure. - Action: Clearly document the changes in the
CHANGELOG.mdand ensure that existing functionality is not affected. Consider adding deprecation warnings if any existing APIs are being replaced.
- The addition of new public APIs (
💡 SUGGESTIONS
-
Documentation:
- The documentation is thorough, but it would be helpful to include a section on how to migrate from previous versions of the .NET Agent Governance Toolkit to this new version with MCP support.
- Consider adding a quickstart guide for developers who are new to MCP and want to integrate it into their projects.
-
Test Coverage:
- While the PR mentions that all tests pass (1003/1003), it is unclear if the new components have been thoroughly tested for edge cases, especially for security-related functionality like nonce management, input sanitization, and credential redaction.
- Action: Add test cases for edge scenarios, such as high concurrency in
McpSessionManager, malformed inputs inMcpInputSanitizer, and complex credential patterns inCredentialRedactor.
-
OpenTelemetry Integration:
- The PR mentions MCP-specific counters for OpenTelemetry. It would be beneficial to provide examples of how these metrics can be visualized and used for monitoring and alerting.
- Action: Include a section in the documentation with sample OpenTelemetry dashboards or queries.
-
Post-Quantum Cryptography:
- The use of ML-DSA for post-quantum cryptography is a forward-looking feature. However, it is unclear if there are fallback mechanisms for environments that do not support .NET 10 or ML-DSA.
- Action: Document the fallback mechanisms for environments that do not support ML-DSA and provide guidance on how to configure them.
-
Performance Benchmarks:
- Given the addition of new components like
McpGatewayandMcpSecurityScanner, it would be useful to include performance benchmarks to ensure that the added security features do not introduce significant latency. - Action: Provide performance benchmarks for the new components, especially under high load scenarios.
- Given the addition of new components like
-
Kubernetes Hardening Guide:
- The hardening guide is comprehensive, but it would be helpful to include a reference architecture diagram to visualize the recommended setup.
- Action: Add a diagram to the hardening guide to illustrate the recommended Kubernetes deployment architecture.
Final Notes
This PR is a significant and well-documented addition to the microsoft/agent-governance-toolkit repository. While the implementation appears robust, the critical areas highlighted above must be addressed to ensure the security and reliability of the new MCP support. Additionally, the suggestions provided can further enhance the usability and maintainability of the code and documentation.
Please address the critical issues and consider the warnings and suggestions before merging this PR. Let me know if you need further clarification or assistance!
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces significant enhancements to the .NET Agent Governance Toolkit with the addition of Model Context Protocol (MCP) support. The implementation appears to be well-structured and adheres to many security best practices, particularly in relation to OWASP guidelines. However, there are several areas that require attention to ensure the robustness and security of the new features.
Actionable Feedback
🔴 CRITICAL Issues
-
Cryptographic Operations: Ensure that the implementation of HMAC-SHA256 and ML-DSA post-quantum signing is thoroughly vetted. The security of these cryptographic operations must be validated against known vulnerabilities, especially since they are critical for message integrity and authentication. Consider conducting a formal security audit or code review by cryptographic experts.
-
Replay Protection: The nonce replay protection mechanism needs to be explicitly tested under various scenarios to ensure it effectively prevents replay attacks. Ensure that the nonce is unique per session and properly managed to avoid collisions.
-
Input Sanitization: The regex-based input validation in
McpInputSanitizershould be rigorously tested to ensure it does not allow bypasses. Consider implementing a comprehensive suite of unit tests that cover edge cases and potential attack vectors.
🟡 WARNING Issues
-
Backward Compatibility: The introduction of new classes and methods may lead to breaking changes if existing consumers of the library are not updated accordingly. Ensure that the changes are well-documented and consider providing migration guides for users who may be affected.
-
Concurrency Management: While
McpSessionManagerimplements rate-limiting with configurable concurrency limits, it is crucial to ensure that the locking mechanisms used are free from deadlocks and race conditions. Review the implementation for potential thread safety issues. -
Documentation of Changes: The extensive changes made in this PR require thorough documentation. Ensure that all new features, especially those related to security, are clearly documented in the README and relevant guides to facilitate user understanding and adoption.
💡 SUGGESTION
-
Unit Testing: While you mentioned that all tests pass, consider adding more unit tests specifically targeting edge cases for the new MCP features. This includes testing the behavior of the
McpSecurityScanneragainst known vulnerabilities and attack patterns. -
Performance Testing: Given the potential for increased load due to the new features, it would be beneficial to conduct performance testing to ensure that the new components do not introduce significant latency or resource consumption.
-
Logging and Monitoring: Enhance logging within the new components to provide better visibility into their operation. This is particularly important for security-related features, as it will aid in detecting and diagnosing issues in production environments.
-
Configuration Management: Consider providing default configurations for the new components that align with best practices for security and performance. This will help users get started quickly while ensuring they are following recommended guidelines.
-
Security Hardening Guide: The new
mcp-server-hardening.mdfile is a great addition. Ensure that it is kept up-to-date with the latest security practices and recommendations as the project evolves.
Conclusion
Overall, this PR introduces valuable features to the .NET Agent Governance Toolkit, enhancing its security capabilities. Addressing the critical issues and considering the warnings and suggestions will help ensure that the implementation is robust and secure.
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
from
8c1796f to
7438b84
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: feat(dotnet): add MCP protocol support with full OWASP coverage
This PR introduces a significant feature addition to the .NET Agent Governance Toolkit, implementing the Model Context Protocol (MCP) governance layer with extensive OWASP MCP Security Cheat Sheet coverage. Below is a detailed review of the changes, focusing on the specified focus areas.
🔴 CRITICAL ISSUES
-
Nonce Replay Protection in
McpMessageSigner- The PR mentions that
McpMessageSigneruses HMAC-SHA256 and ML-DSA for signing with nonce replay protection. However, there is no evidence in the provided diff that the nonce replay protection mechanism has been implemented or tested. Without proper nonce management (e.g., ensuring uniqueness and preventing reuse), the system is vulnerable to replay attacks. - Actionable Recommendation: Ensure that the nonce is securely generated (e.g., using a cryptographically secure random number generator) and stored in a tamper-proof manner. Add test cases to verify that reused nonces are detected and rejected.
- The PR mentions that
-
Rate-Limiting Implementation
- The
McpSlidingRateLimiteris mentioned as a feature, but there is no evidence of how it handles distributed environments (e.g., multiple instances of the MCP server). Without a centralized or consistent rate-limiting mechanism, attackers could bypass rate limits by targeting different instances. - Actionable Recommendation: Use a distributed rate-limiting backend (e.g., Redis) or document how the rate-limiting mechanism ensures consistency across multiple instances.
- The
-
Regex-Based Input Validation
- The
McpInputSanitizeruses regex-based input validation. While the PR mentions amatchTimeoutto prevent ReDoS attacks, regex-based validation is inherently brittle and prone to bypasses. - Actionable Recommendation: Consider complementing regex-based validation with a more robust parsing or schema validation mechanism (e.g., JSON Schema validation for structured inputs). Add tests to ensure that common bypass techniques (e.g., encoding, obfuscation) are not effective.
- The
-
Credential Redaction
- The
CredentialRedactoris designed to redact sensitive information (e.g., PEM blocks, API keys). However, there is no evidence of test coverage for edge cases, such as partial matches, overlapping patterns, or malformed inputs. - Actionable Recommendation: Add comprehensive tests for the
CredentialRedactorto ensure it correctly identifies and redacts sensitive information in all edge cases.
- The
-
TOCTOU Vulnerability in
McpSessionAuthenticator- While the PR mentions that the session limit is checked under a lock to prevent TOCTOU (Time-of-Check to Time-of-Use) vulnerabilities, there is no evidence of test coverage for this behavior.
- Actionable Recommendation: Add tests to simulate concurrent session creation and ensure that the locking mechanism prevents race conditions.
🟡 WARNING: Potential Breaking Changes
-
Multi-Targeting .NET 8 and .NET 10
- The PR introduces multi-targeting for
.NET 8and.NET 10. While this is a forward-looking change, it may introduce compatibility issues for users who are not yet on.NET 8. - Actionable Recommendation: Clearly document the minimum required .NET version and provide guidance for users on older versions.
- The PR introduces multi-targeting for
-
Public API Changes
- The addition of new public APIs (
McpMessageSigner,McpMessageValidator,McpSessionManager, etc.) may impact existing users if they inadvertently conflict with existing code or introduce unexpected behavior. - Actionable Recommendation: Highlight these changes in the release notes and ensure that they are well-documented.
- The addition of new public APIs (
💡 SUGGESTIONS
-
Thread Safety
- The PR mentions that
McpToolRegistrysupports thread-safe operations. However, there is no evidence of tests to validate thread safety. - Suggestion: Add concurrency tests to ensure that
McpToolRegistrybehaves correctly under high-concurrency scenarios.
- The PR mentions that
-
Post-Quantum Cryptography
- The PR introduces ML-DSA for post-quantum signing on
.NET 10+. While this is a forward-looking feature, it is important to ensure that the implementation adheres to best practices for post-quantum cryptography. - Suggestion: Provide references to the specific NIST standards or recommendations that the ML-DSA implementation follows. Include tests to validate the correctness of the implementation.
- The PR introduces ML-DSA for post-quantum signing on
-
Backward Compatibility
- The PR mentions that
.NET 8lacks Ed25519 support, which is why HMAC-SHA256 is used. However, this could lead to challenges when migrating to.NET 10or later. - Suggestion: Document a clear migration path for users who may want to transition from HMAC-SHA256 to ML-DSA or other cryptographic algorithms in the future.
- The PR mentions that
-
Documentation
- The PR includes extensive documentation, which is commendable. However, the
README.mdandAGENTS.mdfiles could benefit from additional examples and diagrams to illustrate the architecture and usage of the MCP governance stack. - Suggestion: Add sequence diagrams or flowcharts to visualize the 5-stage pipeline and the interaction between components.
- The PR includes extensive documentation, which is commendable. However, the
-
OWASP MCP Security Coverage
- The PR claims 11/12 coverage of the OWASP MCP Security Cheat Sheet. While this is impressive, it would be helpful to explicitly list the covered sections and provide a rationale for excluding §11 (Consent UI).
- Suggestion: Include a table mapping the OWASP sections to the implemented features in the documentation.
-
Testing
- The PR mentions 100% test coverage for the
.NETimplementation. However, there is no evidence of fuzz testing or property-based testing for critical components likeMcpInputSanitizerandMcpMessageValidator. - Suggestion: Add fuzz testing and property-based testing to ensure robustness against edge cases and malformed inputs.
- The PR mentions 100% test coverage for the
Summary
This PR represents a significant improvement to the .NET Agent Governance Toolkit, adding robust support for the Model Context Protocol with extensive security features. However, there are several critical issues that need to be addressed to ensure the security and reliability of the implementation. Additionally, there are some potential breaking changes and areas for improvement in documentation and testing.
Priority Actions:
- Implement and test nonce replay protection in
McpMessageSigner. - Ensure distributed rate-limiting consistency in
McpSlidingRateLimiter. - Complement regex-based input validation with schema validation.
- Add comprehensive tests for
CredentialRedactoredge cases. - Add concurrency tests for thread safety in
McpToolRegistry.
Additional Suggestions:
- Enhance documentation with diagrams and OWASP mapping.
- Provide a migration path for cryptographic algorithm changes.
- Include fuzz testing and property-based testing for critical components.
Once the critical issues are addressed and additional tests are added, this PR will be a strong addition to the project.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a significant new feature to the .NET Agent Governance Toolkit by adding support for the Model Context Protocol (MCP). The implementation claims to cover 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, with the exception of §11 (Consent UI), which is out of scope for server-side SDKs. The PR includes a comprehensive set of features such as cryptographic signing, nonce replay protection, rate-limiting, input sanitization, and thread-safe operations. It also provides extensive documentation, including a server hardening guide and usage examples.
The PR appears well-structured and adheres to the project's coding conventions. However, there are several areas that require attention to ensure security, correctness, and maintainability.
🔴 CRITICAL
-
Nonce Replay Protection Implementation
- The PR mentions the use of a nonce cache capped at 10,000 entries with eviction of the oldest entries. However, there is no explicit implementation detail provided in the diff for how this is achieved. If the nonce cache is not implemented correctly, it could lead to replay attacks, which would bypass the security guarantees of the protocol.
- Action: Ensure that the nonce cache implementation is included in the PR and is thoroughly tested for edge cases, such as handling high concurrency and ensuring proper eviction of old entries.
-
HMAC-SHA256 and ML-DSA Implementation
- The PR mentions the use of HMAC-SHA256 and ML-DSA for cryptographic signing. However, there is no evidence in the diff of how these cryptographic operations are implemented or tested.
- Action: Verify that the cryptographic operations are implemented using secure libraries (e.g.,
System.Security.Cryptography) and follow best practices, such as using constant-time comparison for signature verification. Add unit tests to validate the correctness of these operations.
-
Input Sanitization
- The PR mentions regex-based input validation with a fail-closed timeout behavior. However, regex-based input validation can be prone to Regular Expression Denial of Service (ReDoS) attacks if not implemented carefully.
- Action: Ensure that all regex patterns are compiled with a timeout (e.g.,
RegexOptions.CompiledandRegex.MatchTimeout). Add tests to verify that the timeout is enforced and that the system behaves as expected under heavy load or malicious input.
-
Concurrency and Thread Safety
- The PR mentions TOCTOU-safe concurrency for
McpSessionAuthenticator, but there is no evidence of how this is implemented or tested. - Action: Include implementation details and tests for concurrency handling, especially for scenarios with high contention. Ensure that locks or other synchronization primitives are used correctly to avoid race conditions.
- The PR mentions TOCTOU-safe concurrency for
-
Sandbox Escape Vectors
- The PR includes a server hardening guide but does not provide evidence of how the
.NETSDK itself prevents sandbox escapes (e.g., path traversal, command injection). - Action: Add tests to validate that the
.NETSDK properly handles sandbox escape scenarios, such as path traversal attacks (../sequences) or command injection attempts.
- The PR includes a server hardening guide but does not provide evidence of how the
🟡 WARNING
- Breaking Changes in Public API
- The PR introduces new public APIs (e.g.,
McpGateway,McpSecurityScanner,McpSessionAuthenticator) but does not provide backward compatibility for existing users of the.NETSDK. - Action: Clearly document any breaking changes in the
CHANGELOG.mdand provide migration guidance for users upgrading to this version.
- The PR introduces new public APIs (e.g.,
💡 SUGGESTIONS
-
OWASP MCP Security Cheat Sheet Compliance
- While the PR claims compliance with 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, it would be helpful to include a detailed mapping of how each section is addressed in the implementation.
- Action: Add a table or section in the documentation that explicitly maps each OWASP MCP Security Cheat Sheet section to the corresponding implementation in the
.NETSDK.
-
OpenTelemetry Metrics
- The PR mentions MCP-specific counters for OpenTelemetry but does not provide details on how these metrics are exposed or consumed.
- Action: Include examples or documentation on how users can integrate these metrics into their observability stack.
-
Test Coverage
- While the PR states that all tests pass (1003/1003), it is unclear whether these tests cover all new features and edge cases.
- Action: Provide a summary of test coverage for the new MCP-related features, including specific tests for cryptographic operations, nonce replay protection, and input sanitization.
-
Documentation Improvements
- The documentation is comprehensive but could benefit from additional clarity in some areas, such as:
- A high-level architecture diagram for the MCP governance stack.
- Examples of how to configure and use the new features in real-world scenarios.
- Action: Enhance the documentation with diagrams and practical examples.
- The documentation is comprehensive but could benefit from additional clarity in some areas, such as:
-
Code Examples
- The provided code examples are helpful but could be expanded to include more complex scenarios, such as handling multiple agents or integrating with third-party MCP servers.
- Action: Add more detailed examples to the documentation and sample apps.
Final Assessment
This PR introduces a robust and comprehensive set of features for MCP protocol support in the .NET Agent Governance Toolkit. However, there are critical areas that need to be addressed to ensure the security, correctness, and maintainability of the implementation. Once the identified issues are resolved, this PR will be a strong addition to the project.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a comprehensive .NET implementation of the Model Context Protocol (MCP) governance layer, targeting .NET 8 and .NET 10. The implementation appears well-structured and adheres to OWASP MCP Security Cheat Sheet guidelines, with a focus on security, thread safety, and extensibility. The PR includes new features such as cryptographic signing, rate-limiting, input sanitization, and a robust governance pipeline. The documentation is thorough and provides clear guidance for integration and deployment.
However, there are some areas that require attention to ensure security, backward compatibility, and maintainability. Below are the detailed observations and recommendations:
🔴 CRITICAL
-
Replay Attack Mitigation in
McpMessageSignerandMcpMessageValidator- Issue: The PR mentions nonce replay protection but does not provide sufficient details about how nonces are generated, stored, and validated. If nonces are not securely stored or if the nonce space is too small, this could lead to replay attacks.
- Recommendation: Ensure that nonces are cryptographically secure (e.g., using a CSPRNG) and are stored in a tamper-proof, persistent store. Add test cases to verify that replayed nonces are correctly rejected.
-
Regex-Based Input Validation
- Issue: The use of regex-based input validation in
McpInputSanitizercould introduce Regular Expression Denial of Service (ReDoS) vulnerabilities if the regex patterns are not carefully crafted. - Recommendation: Ensure all regex patterns are tested for performance and include a timeout (e.g.,
matchTimeout: TimeSpan.FromMilliseconds(200)) to mitigate ReDoS attacks. Add unit tests to verify that the regex patterns handle edge cases and do not exhibit exponential backtracking.
- Issue: The use of regex-based input validation in
-
Credential Redaction
- Issue: The
CredentialRedactorclaims to redact sensitive information, but there is no evidence in the PR that it has been tested against a comprehensive set of patterns (e.g., API keys, tokens, PEM blocks, etc.). - Recommendation: Include a test suite with a wide variety of credential formats to ensure the redaction logic is robust and does not miss any sensitive information.
- Issue: The
-
Concurrency and Thread Safety
- Issue: While the PR mentions TOCTOU-safe concurrency for
McpSessionAuthenticator, there is no explicit mention of how thread safety is ensured inMcpToolRegistryor other shared resources. - Recommendation: Verify that all shared resources, such as
McpToolRegistryandMcpSessionManager, are thread-safe. Use locks or thread-safe collections where necessary. Add concurrency tests to validate thread safety.
- Issue: While the PR mentions TOCTOU-safe concurrency for
-
Post-Quantum Cryptography
- Issue: The PR introduces ML-DSA post-quantum signing for .NET 10+, but there is no mention of the specific library or implementation used. This is critical for ensuring the cryptographic soundness of the solution.
- Recommendation: Provide details about the ML-DSA implementation, including its source, security guarantees, and compliance with standards (e.g., NIST). Add unit tests to validate the correctness of the signing and verification process.
🟡 WARNING
-
Breaking Changes in Public API
- Issue: The addition of
McpGovernanceOptionsand other new APIs could potentially break existing integrations if not properly versioned or documented. - Recommendation: Clearly document all new APIs and ensure backward compatibility. If breaking changes are unavoidable, increment the major version number and provide a migration guide.
- Issue: The addition of
-
Multi-Targeting .NET 8 and .NET 10
- Issue: Multi-targeting introduces complexity and potential compatibility issues. For example, the use of ML-DSA in .NET 10+ may lead to runtime errors if not properly gated.
- Recommendation: Add runtime checks to ensure that features requiring .NET 10 are not invoked in .NET 8 environments. Include tests to verify behavior across both target frameworks.
💡 SUGGESTIONS
-
Enhanced Logging
- Observation: The PR mentions
ILogger<T>for structured logging but does not provide details about the log levels or formats. - Recommendation: Define a consistent logging strategy, including log levels for different events (e.g., INFO for successful operations, WARN for potential issues, ERROR for failures). Add unit tests to verify logging behavior.
- Observation: The PR mentions
-
OpenTelemetry Integration
- Observation: The PR mentions MCP-specific OpenTelemetry counters but does not provide details about their implementation or usage.
- Recommendation: Include examples of how to query and interpret these metrics. Ensure that the metrics are well-documented and tested.
-
Documentation Improvements
- Observation: While the documentation is thorough, it could benefit from additional examples and diagrams to explain complex concepts like the 5-stage governance pipeline and the 6-threat detection model.
- Recommendation: Add sequence diagrams or flowcharts to visually represent the governance pipeline and threat detection process.
-
Testing Coverage
- Observation: The PR mentions 100% test coverage, but it is unclear if this includes edge cases, concurrency scenarios, and failure modes.
- Recommendation: Ensure that the test suite includes:
- Edge cases for input validation and sanitization
- Concurrency tests for shared resources
- Failure mode tests for the governance pipeline (e.g., exceptions, timeouts)
-
Hardening Guide
- Observation: The hardening guide is comprehensive but could include more details about monitoring and alerting.
- Recommendation: Add a section on monitoring MCP server health and security events using tools like Prometheus, Grafana, or Azure Monitor.
Final Assessment
This PR is a significant and well-thought-out addition to the Agent Governance Toolkit. However, the identified critical issues must be addressed before merging to ensure the security and reliability of the MCP governance layer. The warnings and suggestions should also be considered to improve the overall quality and maintainability of the implementation.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review of PR: feat(dotnet): add MCP protocol support with full OWASP coverage
🔴 CRITICAL: Security Issues
-
Nonce Replay Protection Implementation:
- The
McpMessageSignerandMcpMessageValidatorclaim to implement HMAC-SHA256 and ML-DSA post-quantum signing with nonce replay protection. However, the PR does not provide sufficient details or code snippets to verify the correctness of the nonce replay protection mechanism. - Action: Ensure that the nonce is unique and cannot be reused. Add unit tests to verify that the system rejects replayed nonces. Also, document the nonce generation and validation process in detail.
- The
-
Regex-based Input Validation:
- The
McpInputSanitizeruses regex-based input validation with a fail-closed timeout behavior. While this is a good start, regex-based validation can be prone to Regular Expression Denial of Service (ReDoS) attacks. - Action: Confirm that all regex patterns are optimized and include a timeout to mitigate ReDoS risks. Add unit tests to validate the behavior under edge cases and high-load scenarios.
- The
-
Credential Redaction:
- The
CredentialRedactorclaims to redact sensitive information such as PEM blocks and secret patterns. However, the PR does not provide details about the patterns used for redaction. - Action: Ensure that the redaction patterns are comprehensive and tested against a wide range of sensitive data formats. Include test cases to validate that no sensitive information is leaked.
- The
-
TOCTOU Race Conditions:
- The
McpSessionAuthenticatormentions TOCTOU-safe concurrency for session limits, but the implementation details are not provided in the PR. - Action: Verify that the session limit checks are atomic and cannot be bypassed due to race conditions. Add concurrency tests to validate this behavior.
- The
-
Post-Quantum Cryptography:
- The PR introduces ML-DSA post-quantum signing for .NET 10+. While this is a forward-looking feature, it is crucial to ensure that the implementation adheres to the latest standards and has been reviewed by cryptography experts.
- Action: Provide documentation or references to the cryptographic standards followed. Consider an external audit of the cryptographic implementation.
-
gRPC Server Interceptor:
- The gRPC server interceptor is mentioned but not detailed in the PR. gRPC interceptors can be a critical point for enforcing security policies.
- Action: Ensure that the interceptor is fail-closed and does not allow bypassing of security checks. Add unit tests to validate this behavior.
🟡 WARNING: Potential Breaking Changes
-
Multi-Targeting .NET 8 and .NET 10:
- The PR introduces multi-targeting for .NET 8 and .NET 10. While this is a non-breaking change for .NET 8 users, it may introduce compatibility issues for users on earlier .NET versions.
- Action: Clearly document the minimum .NET version requirements and provide guidance for users on older versions.
-
Public API Changes:
- The addition of new classes like
McpMessageSigner,McpMessageValidator, andMcpSessionManagerexpands the public API. While these are new features, any future changes to these APIs could break backward compatibility. - Action: Mark these APIs as experimental or subject to change in the documentation to set the right expectations.
- The addition of new classes like
💡 Suggestions for Improvement
-
Documentation:
- The documentation is comprehensive but could benefit from additional examples, especially for the new MCP-related features. For instance:
- Provide a complete example of how to use the
McpMessageSignerandMcpMessageValidator. - Include a sample configuration file for
McpGovernanceOptions.
- Provide a complete example of how to use the
- The documentation is comprehensive but could benefit from additional examples, especially for the new MCP-related features. For instance:
-
Testing:
- While the PR mentions that all tests pass (1003/1003), it is unclear if the new features are adequately covered by tests.
- Action: Ensure that the new MCP-related features have comprehensive unit and integration tests. Include tests for edge cases and failure scenarios.
-
Performance Testing:
- The introduction of features like rate limiting and nonce replay protection could impact performance.
- Action: Conduct performance testing to ensure that the new features do not introduce significant overhead. Document the results.
-
OpenTelemetry Integration:
- The PR mentions four MCP-specific counters for OpenTelemetry. However, there is no information on how these counters are used or configured.
- Action: Provide documentation and examples for setting up and using the OpenTelemetry counters.
-
Hardening Guide:
- The hardening guide is a great addition, but it could benefit from more specific examples and references.
- Action: Include example configurations for popular cloud providers like AWS and GCP, in addition to Azure.
-
Backward Compatibility Tests:
- Given the addition of new features and multi-targeting, it is essential to ensure backward compatibility.
- Action: Add backward compatibility tests to verify that existing functionality is not broken.
Summary
The PR introduces significant new functionality for MCP protocol support in the .NET Agent Governance Toolkit. While the features align well with OWASP MCP Security Cheat Sheet recommendations, there are critical areas that require further scrutiny, particularly around nonce replay protection, regex safety, and cryptographic implementations. Additionally, the potential for breaking changes due to multi-targeting and API expansion should be carefully managed.
Recommended Actions:
- Address the critical security issues identified.
- Enhance documentation and testing for the new features.
- Conduct performance and backward compatibility testing.
- Consider an external security audit for the cryptographic implementations.
Once these issues are addressed, the PR will be in a strong position for approval.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a significant new feature to the .NET Agent Governance Toolkit by adding support for the Model Context Protocol (MCP). The implementation claims to cover 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, with a focus on security primitives, cryptographic operations, and sandboxing. The PR also includes extensive documentation, a hardening guide for Kubernetes deployments, and sample applications.
The PR is well-structured and adheres to the project's coding conventions. However, there are a few areas that require attention to ensure security, backward compatibility, and maintainability.
🔴 CRITICAL
-
Nonce Replay Protection Implementation
- The PR mentions that
McpMessageSignerandMcpMessageValidatorinclude nonce replay protection. However, the implementation details are not included in the diff. Replay protection is critical for the security of the protocol. Please ensure:- Nonces are unique and time-bound.
- Nonces are stored securely and checked against a reliable storage mechanism (e.g.,
ISignerNonceStore). - Nonce expiration is handled correctly to prevent replay attacks.
- Add unit tests to verify that replayed nonces are correctly rejected.
- The PR mentions that
-
Regex-Based Input Validation
- The
McpInputSanitizeruses regex-based input validation. While regex is a common approach, it is prone to Regular Expression Denial of Service (ReDoS) attacks. The PR mentions amatchTimeoutof 200ms for compiled regexes, but this is not explicitly shown in the diff. Ensure:- All regex patterns are thoroughly tested for performance and edge cases.
- Add tests to verify that the
matchTimeoutis enforced and that the system behaves correctly when the timeout is exceeded.
- The
-
Thread Safety
- The PR mentions that
McpToolRegistryandMcpSessionAuthenticatorare thread-safe. However, there is no evidence in the diff to confirm this. Ensure:- Proper locking mechanisms (e.g.,
lockstatements orConcurrentDictionary) are used to prevent race conditions. - Add unit tests to simulate concurrent access and verify thread safety.
- Proper locking mechanisms (e.g.,
- The PR mentions that
-
Cryptographic Operations
- The PR introduces HMAC-SHA256 and ML-DSA for cryptographic signing. Ensure:
- The cryptographic keys are securely generated, stored, and rotated.
- The implementation uses constant-time comparison for verifying signatures (e.g.,
CryptographicOperations.FixedTimeEquals). - Add unit tests to verify the correctness of the cryptographic operations, including edge cases.
- The PR introduces HMAC-SHA256 and ML-DSA for cryptographic signing. Ensure:
🟡 WARNING
-
Backward Compatibility
- The introduction of the
McpGovernanceOptionsandMcpGovernanceExtensionsclasses may impact existing users of the.NETSDK. Ensure:- These changes are backward-compatible with existing configurations and usage patterns.
- If not, provide clear migration guidance in the documentation.
- The introduction of the
-
Multi-Targeting .NET 8 and .NET 10
- While multi-targeting is a valuable feature, it introduces complexity in maintaining compatibility across versions. Ensure:
- Automated tests are run for both .NET 8 and .NET 10 targets.
- Any version-specific features (e.g., ML-DSA in .NET 10) are properly isolated and do not affect .NET 8 compatibility.
- While multi-targeting is a valuable feature, it introduces complexity in maintaining compatibility across versions. Ensure:
💡 SUGGESTIONS
-
OWASP MCP Security Cheat Sheet Coverage
- The PR claims to cover 11 out of 12 sections of the OWASP MCP Security Cheat Sheet. Consider adding a detailed mapping in the documentation that explicitly links each implemented feature to the corresponding section of the cheat sheet.
-
Documentation Improvements
- The documentation is comprehensive, but it could benefit from additional examples and diagrams to explain the architecture and data flow of the MCP governance stack.
- Include a quickstart guide for developers to integrate the
.NET MCP SDKinto their projects.
-
Testing
- While the PR mentions that all tests pass (1003/1003), consider adding:
- End-to-end tests for the MCP governance stack.
- Fuzz testing for the
McpInputSanitizerto identify potential edge cases and vulnerabilities. - Stress tests for
McpSessionManagerto ensure it handles high concurrency gracefully.
- While the PR mentions that all tests pass (1003/1003), consider adding:
-
Performance Benchmarks
- Given the introduction of new components like
McpSlidingRateLimiterandMcpSecurityScanner, consider adding performance benchmarks to measure the overhead introduced by these features.
- Given the introduction of new components like
-
Hardening Guide
- The hardening guide is a great addition. Consider adding:
- Examples for integrating with other container runtimes (e.g., CRI-O, containerd).
- Guidance on monitoring and alerting for MCP server deployments.
- The hardening guide is a great addition. Consider adding:
Final Assessment
This PR is a significant and well-documented addition to the .NET Agent Governance Toolkit. However, the critical areas mentioned above must be addressed before merging to ensure the security and reliability of the MCP governance stack. Once these issues are resolved, the PR will be ready for approval.
…get .NET 8/10, ML-DSA post-quantum signing Add comprehensive MCP (Model Context Protocol) security governance to the .NET SDK with 11/12 OWASP MCP Security Cheat Sheet sections covered. Multi-targets .NET 8.0 (LTS) and .NET 10.0 with post-quantum ML-DSA-65 (NIST FIPS 204) signing on .NET 10+. Core components: - McpGateway: 5-stage pipeline (deny→allow→sanitize→rate-limit→approve) - McpSecurityScanner: 6-threat detection with SHA-256 fingerprinting - McpMessageHandler: JSON-RPC routing with tool-to-ActionType classification - McpResponseScanner: Output validation (injection, credentials, exfiltration) - McpSessionAuthenticator: Crypto session binding with TOCTOU-safe concurrency - McpMessageSigner: HMAC-SHA256 (.NET 8) + ML-DSA-65 post-quantum (.NET 10+) - CredentialRedactor: 10 credential pattern redaction for audit logs - McpSlidingRateLimiter: Per-agent sliding window rate limiting Integration: - ASP.NET Core: AddMcpGovernance(), UseMcpGovernance(), MapMcpGovernance() - IConfiguration binding, ILogger<T>, IHealthCheck, gRPC interceptor - McpToolRegistry with [McpTool] attribute auto-discovery - AgentGovernance.ModelContextProtocol adapter sub-package (official SDK) - OTel metrics: mcp_decisions, mcp_threats_detected, mcp_rate_limit_hits, mcp_scans Tests: 973 passing (0 failures) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jackbatzner
force-pushed
the
jb/dotnet-mcp-support
branch
from
257869a to
ead70f9
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a significant feature addition to the .NET Agent Governance Toolkit by implementing MCP (Model Context Protocol) support. The implementation claims to cover 11 out of 12 sections of the OWASP MCP Security Cheat Sheet, with a focus on server-side security primitives. The PR includes cryptographic signing, session management, input sanitization, and thread-safe operations, among other features. The changes are well-documented and include tests, examples, and deployment guides.
Below is a detailed review of the PR, categorized into critical issues, warnings, and suggestions.
🔴 CRITICAL ISSUES
-
Nonce Replay Protection Implementation
- The
McpMessageSignerandMcpMessageValidatorinclude nonce replay protection. However, there is no detailed explanation or code provided in the PR to verify the implementation. Ensure that:- Nonces are cryptographically secure (e.g., generated using a CSPRNG).
- Nonces are stored in a secure, tamper-proof, and scalable manner (e.g., using
ISignerNonceStore). - Nonces are properly invalidated after use to prevent replay attacks.
- The nonce cache eviction strategy ensures that old nonces are removed without introducing race conditions or vulnerabilities.
- The
-
Input Validation and ReDoS
- The
McpInputSanitizeruses regex-based input validation. While the PR mentions amatchTimeoutof 200ms to prevent ReDoS attacks, there is no evidence in the code that this is consistently enforced. Ensure that:- All regex patterns are thoroughly tested for catastrophic backtracking.
- The
matchTimeoutis applied to every regex operation. - Consider using precompiled regex patterns to improve performance and reduce runtime risks.
- The
-
Thread Safety in
McpSessionManager- The PR mentions TOCTOU-safe concurrency for session limits, but the implementation is not provided in the diff. Ensure that:
- All shared resources (e.g., session state, nonce cache) are accessed under proper locks or use thread-safe data structures.
- Deadlock scenarios are avoided, especially in high-concurrency environments.
- The PR mentions TOCTOU-safe concurrency for session limits, but the implementation is not provided in the diff. Ensure that:
-
Cryptographic Operations
- The PR introduces ML-DSA post-quantum signing for .NET 10+. Ensure that:
- The implementation adheres to NIST standards for post-quantum cryptography.
- Cryptographic keys are securely generated, stored, and rotated.
CryptographicOperations.FixedTimeEqualsis used for all secret comparisons to prevent timing attacks.
- The PR introduces ML-DSA post-quantum signing for .NET 10+. Ensure that:
🟡 WARNINGS (Potential Breaking Changes)
-
Backward Compatibility
- The introduction of
.NET 10as a target framework could potentially break compatibility for users on older versions of .NET. While.NET 8is still supported, ensure that:- The
.NET 8implementation does not rely on features exclusive to.NET 10. - Clear documentation is provided to guide users on upgrading to
.NET 10for post-quantum cryptography.
- The
- The introduction of
-
Public API Changes
- The addition of new classes (
McpMessageSigner,McpSessionManager, etc.) and methods could affect existing users if they conflict with current APIs. Ensure that:- New APIs are properly namespaced to avoid collisions.
- Existing APIs remain unchanged unless absolutely necessary.
- The addition of new classes (
💡 SUGGESTIONS (Improvements)
-
Test Coverage
- While the PR mentions 100% test coverage, ensure that:
- Tests cover edge cases, such as invalid inputs, high concurrency scenarios, and cryptographic failures.
- Fuzz testing is performed on the
McpInputSanitizerto identify potential vulnerabilities.
- While the PR mentions 100% test coverage, ensure that:
-
Documentation
- The documentation is comprehensive, but consider adding:
- A detailed explanation of the nonce replay protection mechanism.
- Examples of how to configure and use the
McpSessionManagerandMcpToolRegistry. - A section on upgrading from previous versions, especially for users transitioning to
.NET 10.
- The documentation is comprehensive, but consider adding:
-
Performance
- The use of regex-based input validation and session management could introduce performance bottlenecks under high load. Consider:
- Benchmarking the performance of the
McpInputSanitizerandMcpSessionManagerunder realistic workloads. - Providing configuration options to adjust timeouts and limits based on deployment needs.
- Benchmarking the performance of the
- The use of regex-based input validation and session management could introduce performance bottlenecks under high load. Consider:
-
Security Hardening
- The
McpSecurityScannerincludes SHA-256 fingerprinting for detecting schema changes. Consider:- Adding support for stronger hash functions (e.g., SHA-3) for future-proofing.
- Logging all detected threats with sufficient detail for forensic analysis.
- The
-
Kubernetes Hardening Guide
- The provided Kubernetes hardening guide is excellent, but consider:
- Adding examples for integrating with service meshes (e.g., Istio) for mTLS enforcement.
- Including guidance on using PodSecurityPolicies (PSPs) or their replacement (e.g., OPA/Gatekeeper).
- The provided Kubernetes hardening guide is excellent, but consider:
Final Recommendation
- Address the critical issues before merging to ensure the security and correctness of the MCP implementation.
- Review the warnings to avoid breaking changes for existing users.
- Consider the suggestions to improve performance, usability, and security hardening.
Once the critical issues are resolved and additional tests are added, this PR will be a valuable addition to the .NET Agent Governance Toolkit.
2 participants
Description
Adds MCP (Model Context Protocol) security primitives to the .NET Agent Governance Toolkit with full OWASP MCP Security Cheat Sheet coverage (11/12 sections — §11 Consent UI is out of scope for server-side SDKs).
Key additions:
McpMessageSigner/McpMessageValidator— HMAC-SHA256 + ML-DSA post-quantum signing with nonce replay protectionMcpSessionManager— rate-limited session lifecycle with configurable concurrency limitsMcpToolRegistry— schema-validated tool registration with thread-safe operationsMcpInputSanitizer— regex-based input validation with fail-closed timeout behaviorCredentialRedactor— full PEM block + secret pattern redaction for audit-safe loggingISignerNonceStore,ISessionStore), clock injection, fail-closed enforcementMulti-target .NET 8 / .NET 10. All tests pass (1003/1003).
Type of Change
Package(s) Affected
Checklist
Related Issues
Part 1 of 3 (split from original PR for easier review). Followed by #824 and #826.