docs(dotnet): add MCP security documentation and examples#826
docs(dotnet): add MCP security documentation and examples#826jackbatzner wants to merge 1 commit 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 primarily introduces documentation updates and examples for .NET MCP security primitives. It does not modify any Python packages or APIs in the Findings
Migration GuideNo migration steps are necessary as there are no breaking changes in this pull request. ✅ No breaking changes found. |
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
If these updates are made, the documentation will be in sync with the changes introduced in this PR. Let me know if you need further assistance! |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review of Pull Request: docs(dotnet): add MCP security documentation and examples
🔴 CRITICAL: Security Issues
-
Missing Details on mTLS Configuration for HTTP Transport
- The documentation recommends using mTLS for HTTP transport but does not provide detailed guidance or examples for its configuration. This omission could lead to misconfigurations, leaving the system vulnerable to man-in-the-middle (MITM) attacks.
- Actionable Recommendation: Include a detailed example of how to configure mTLS for MCP servers, including certificate generation, validation, and renewal processes.
-
Insufficient Details on gVisor/Kata Containers
- While the documentation mentions using gVisor and Kata Containers for untrusted servers, it does not provide sufficient details on how to configure these sandboxes securely. This could lead to improper implementation and potential sandbox escapes.
- Actionable Recommendation: Add detailed configuration examples for gVisor and Kata Containers, including specific security settings, and provide guidance on when to use each option.
-
Lack of Explicit Guidance on Secure Configuration Defaults
- The documentation does not explicitly state whether the provided
.NET SDK McpGatewaysanitization and response scanning features are enabled by default. If they are not, users might deploy the SDK without enabling these critical security features. - Actionable Recommendation: Clearly document whether these features are enabled by default. If not, provide explicit instructions on how to enable them and emphasize their importance.
- The documentation does not explicitly state whether the provided
-
Potential for ReDoS in Regular Expressions
- While the coding conventions mention using
matchTimeout: TimeSpan.FromMilliseconds(200)for regex safety, there is no verification mechanism to ensure this is consistently applied. - Actionable Recommendation: Add automated tests or static analysis checks to verify that all regular expressions in the codebase adhere to this timeout constraint.
- While the coding conventions mention using
🟡 WARNING: Potential Breaking Changes
-
Removal of Rust MCP SDK Documentation
- The removal of references to the Rust MCP SDK (
agentmesh-mcp) from the documentation could confuse users who rely on this package. - Actionable Recommendation: If the Rust MCP SDK is deprecated, explicitly state this in the documentation and provide guidance for migration or alternatives.
- The removal of references to the Rust MCP SDK (
-
Introduction of .NET 10.0 Target
- The addition of .NET 10.0 as a target could potentially introduce compatibility issues for users who are currently using .NET 8.0.
- Actionable Recommendation: Clearly document any breaking changes or additional requirements for users upgrading to .NET 10.0. Ensure backward compatibility with .NET 8.0 is maintained.
💡 Suggestions for Improvement
-
Expand OWASP MCP Coverage
- The documentation states that 11 out of 12 OWASP MCP Security Cheat Sheet sections are covered, with §11 (Consent UI) being out of scope. While this is reasonable, consider providing a brief explanation or a link to resources for implementing §11 to guide users who may need it.
-
Clarify Post-Quantum Cryptography Support
- The documentation mentions "post-quantum cryptography on .NET 10+," but it is unclear what specific algorithms or libraries are being used. This could lead to confusion or misinterpretation.
- Actionable Recommendation: Specify the post-quantum cryptographic algorithms used (e.g., ML-DSA-65) and provide references to their security guarantees and compliance with standards like NIST.
-
Add More Examples for MCP Security Features
- While the documentation includes examples for some features, it would benefit from additional examples, especially for complex features like
McpGatewayandMcpSecurityScanner. - Actionable Recommendation: Include step-by-step examples for setting up and using these features in both ASP.NET Core and gRPC contexts.
- While the documentation includes examples for some features, it would benefit from additional examples, especially for complex features like
-
Internationalization (i18n)
- The Japanese translation has been removed from the README, leaving only English and Simplified Chinese. This might alienate Japanese-speaking users.
- Actionable Recommendation: Either restore the Japanese translation or provide a clear explanation for its removal.
-
Clarify the Role of
AgentGovernance.ModelContextProtocol- The new
AgentGovernance.ModelContextProtocolsub-package is introduced, but its role and use cases are not clearly explained in the documentation. - Actionable Recommendation: Add a dedicated section in the documentation to explain the purpose, features, and usage of this sub-package.
- The new
-
Add Tests for New Features
- While this PR is primarily documentation-focused, the new
.NET MCP Protocol Supportfeatures introduced in #824 should be accompanied by comprehensive tests to ensure correctness and prevent regressions. - Actionable Recommendation: Verify that the features introduced in #824 are thoroughly tested, especially for edge cases and potential security vulnerabilities.
- While this PR is primarily documentation-focused, the new
-
Clarify the Use of
CryptographicOperations.FixedTimeEquals- The coding conventions mention using
CryptographicOperations.FixedTimeEqualsfor secret comparison, but there is no verification mechanism to ensure this is consistently applied. - Actionable Recommendation: Add automated tests or static analysis checks to verify that all secret comparisons use
CryptographicOperations.FixedTimeEquals.
- The coding conventions mention using
-
Improve Documentation Formatting
- There are minor formatting issues in the documentation, such as the missing space in "Runtimegovernance" in the main
README.md. - Actionable Recommendation: Fix these formatting issues to improve readability and professionalism.
- There are minor formatting issues in the documentation, such as the missing space in "Runtimegovernance" in the main
Summary of Recommendations
-
🔴 Address critical security issues:
- Provide detailed mTLS configuration examples.
- Expand guidance on gVisor/Kata Containers.
- Clarify default settings for
.NET SDK McpGatewaysanitization and response scanning. - Add automated checks for regex safety.
-
🟡 Mitigate potential breaking changes:
- Clarify the status of the Rust MCP SDK and provide migration guidance if deprecated.
- Document any breaking changes or additional requirements for .NET 10.0 compatibility.
-
💡 Suggestions for improvement:
- Expand OWASP MCP coverage to include guidance for §11 (Consent UI).
- Clarify post-quantum cryptography support.
- Add more examples for MCP security features.
- Restore or explain the removal of the Japanese translation.
- Add a dedicated section for
AgentGovernance.ModelContextProtocol. - Verify comprehensive testing for new
.NET MCP Protocol Supportfeatures. - Add automated checks for
CryptographicOperations.FixedTimeEquals. - Fix minor formatting issues in the documentation.
This PR provides valuable documentation for the .NET MCP security primitives and aligns well with the project's goals. However, addressing the critical security issues and potential breaking changes is essential before merging.
🤖 AI Agent: security-scanner — 🔵 **LOW: Potential for Misleading Documentation**This pull request primarily updates documentation and configuration files, such as 🔵 LOW: Potential for Misleading DocumentationIssueThe documentation update introduces new terms and examples but does not provide sufficient context or warnings about potential security pitfalls when using the MCP security primitives. For example, terms like "sandboxing," "SPIFFE," and "SVID" are added to the Attack VectorIf the documentation is unclear or incomplete, developers may misuse the MCP security primitives, leading to vulnerabilities such as:
Recommendation
🔵 LOW: Potential for Dependency ConfusionIssueThe Attack VectorAn attacker could publish a malicious package with a similar name to one of these dependencies. If the project inadvertently installs the malicious package, it could lead to unauthorized code execution or data exfiltration. Recommendation
🔵 LOW: Configuration File HygieneIssueThe Attack VectorIf these terms are not adequately explained in the documentation, they could be misused or misunderstood by contributors, potentially leading to security oversights. Recommendation
Overall AssessmentThis PR does not introduce any critical or high-severity security issues, as it primarily focuses on documentation and configuration updates. However, there are opportunities to improve the clarity and security awareness of the documentation to prevent potential misuse of the MCP security primitives.
Let me know if you'd like me to review the related PRs (#775 and #824) for a more comprehensive security assessment. |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces comprehensive documentation for the .NET MCP (Model Context Protocol) security primitives, including API references, compliance mapping with the OWASP MCP Cheat Sheet, quickstart guides, usage examples, and deployment hardening guidelines. While the PR is primarily documentation-focused, it also includes some minor changes to the .NET solution structure and project files.
Below is the detailed review of the PR, focusing on the specified areas of concern.
🔴 CRITICAL
-
Missing Cryptographic Algorithm Justification
- The documentation mentions the use of
HMAC-SHA256for cryptographic signing, with a "migration path for Ed25519 in .NET 9+" and post-quantum cryptography (ML-DSA-65) on .NET 10+. However, there is no clear justification for whyHMAC-SHA256is used instead of Ed25519 or other modern algorithms in the current implementation. This could lead to weaker cryptographic guarantees, especially for scenarios requiring non-repudiation. - Action: Provide a detailed security analysis justifying the choice of
HMAC-SHA256over Ed25519 or other algorithms, especially for scenarios involving digital signatures.
- The documentation mentions the use of
-
Potential Sandbox Escape Vectors
- The
MCP Server Hardening Guideprovides excellent recommendations for securing MCP servers, including the use of gVisor/Kata Containers for untrusted servers. However, the guide does not explicitly address sandbox escape vectors, such as:- Preventing access to sensitive host resources (e.g.,
/proc,/sys, or Docker socket). - Ensuring that the container runtime itself is not misconfigured or vulnerable.
- Preventing access to sensitive host resources (e.g.,
- Action: Expand the hardening guide to include specific recommendations for mitigating sandbox escape risks, such as restricting access to sensitive host resources and ensuring the container runtime is hardened.
- The
-
TOCTOU Race Condition in
McpSessionAuthenticator- The documentation mentions that the
McpSessionAuthenticatoris "TOCTOU-safe," but there is no accompanying code or test case to verify this claim. TOCTOU (Time-of-Check to Time-of-Use) vulnerabilities are critical in security-sensitive components like session authentication. - Action: Provide a detailed explanation of how the
McpSessionAuthenticatormitigates TOCTOU risks. Include unit tests or code snippets that demonstrate and validate this behavior.
- The documentation mentions that the
🟡 WARNING
-
Backward Compatibility
- The
.NET SDKnow targets both.NET 8.0and.NET 10.0, with post-quantum cryptography available only on.NET 10+. This change could potentially break compatibility for users who are not yet on.NET 8.0. - Action: Clearly document the minimum required .NET version for the new features and provide guidance for users on older versions of .NET.
- The
-
Removed Rust MCP SDK Documentation
- The PR removes references to the Rust MCP SDK (
agentmesh-mcp) from theREADME.mdand other documentation files. If this is intentional, it should be explicitly mentioned in the PR description to avoid confusion. - Action: Confirm whether the removal of Rust MCP SDK documentation is intentional and, if so, provide a clear explanation in the PR description.
- The PR removes references to the Rust MCP SDK (
💡 SUGGESTIONS
-
Add Examples for Post-Quantum Cryptography
- The documentation mentions support for ML-DSA-65 post-quantum signing on .NET 10+, but there are no examples or usage details provided.
- Suggestion: Add examples demonstrating how to use the post-quantum cryptography features in the
.NET SDK.
-
Improve Documentation for
McpGateway- The
McpGatewayis described as a "5-stage pipeline," but the documentation does not provide sufficient detail about each stage (e.g., what "deny-list" and "allow-list" entail, how sanitization works, etc.). - Suggestion: Expand the documentation for
McpGatewayto include detailed descriptions and examples for each stage of the pipeline.
- The
-
Add Internationalization (i18n) for Japanese
- The PR removes the Japanese translation link from the main
README.md. While this may be intentional, it would be beneficial to include Japanese translations for the new documentation, given the project's existing support for i18n. - Suggestion: Add Japanese translations for the new
.NET MCPdocumentation and examples.
- The PR removes the Japanese translation link from the main
-
Add Tests for MCP Server Hardening
- While the hardening guide provides excellent recommendations, there are no automated tests or validation scripts to ensure compliance with the guidelines.
- Suggestion: Add scripts or tests to validate that MCP server deployments comply with the hardening guidelines (e.g., checking for non-root user, read-only filesystem, etc.).
-
Clarify OWASP MCP Coverage
- The documentation states that the
.NET SDKcovers 11 of the 12 OWASP MCP Security Cheat Sheet sections, with §11 (Consent UI) being out of scope. However, it does not specify which section is not covered. - Suggestion: Clearly list the covered and uncovered OWASP MCP sections in the documentation.
- The documentation states that the
Additional Notes
- The PR is well-structured and provides comprehensive documentation for the
.NET MCPsecurity primitives. The inclusion of hardening guidelines and OWASP compliance mapping is particularly commendable. - The
.NET SDKcoding conventions and design decisions are well-documented, which will help maintain consistency and security in future contributions.
Summary of Actions
🔴 CRITICAL
- Provide justification for the use of
HMAC-SHA256over Ed25519 or other algorithms. - Expand the hardening guide to address sandbox escape vectors.
- Verify and demonstrate TOCTOU safety in
McpSessionAuthenticator.
🟡 WARNING
- Document the minimum required .NET version for new features.
- Confirm the intentional removal of Rust MCP SDK documentation.
💡 SUGGESTIONS
- Add examples for post-quantum cryptography.
- Expand
McpGatewaydocumentation with detailed descriptions and examples. - Add Japanese translations for the new documentation.
- Add tests or scripts to validate MCP server hardening compliance.
- Clarify which OWASP MCP section is not covered.
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 comprehensive documentation for the .NET MCP (Model Context Protocol) security primitives, including API references, OWASP compliance mapping, quickstart guides, and usage examples. It also includes a Kubernetes hardening guide for MCP servers and updates to the repository's documentation structure. While the PR is primarily documentation-focused, it also includes some minor code changes and project structure updates.
🔴 CRITICAL
-
Potential Sandbox Escape in Kubernetes Hardening Guide:
- The
securityContextconfiguration in the Kubernetes hardening guide does not explicitly setallowPrivilegeEscalation: falsefor the pod-level security context. While it is correctly set for containers, this omission could allow privilege escalation if a container override is misconfigured. - Action: Add
allowPrivilegeEscalation: falseto the pod-levelsecurityContextin the Kubernetes hardening guide.
- The
-
Missing Explicit Denial of Cloud Metadata Access:
- The Kubernetes
NetworkPolicyexample does not explicitly block access to cloud metadata endpoints (e.g.,169.254.169.254). While the policy states that these are blocked by default, relying on implicit behavior is risky. - Action: Add explicit egress rules to deny access to cloud metadata endpoints (e.g., AWS IMDS, Azure IMDS, GCP metadata).
- The Kubernetes
🟡 WARNING
-
Breaking Changes in Documentation:
- The removal of Rust MCP SDK references from the
README.mdand other documentation could be considered a breaking change for users relying on Rust MCP support. - Action: Clearly communicate the deprecation/removal of Rust MCP SDK in the changelog and documentation.
- The removal of Rust MCP SDK references from the
-
Backward Compatibility of
.NET SDKChanges:- The
.NET SDKintroduces new components (e.g.,McpGateway,McpSecurityScanner, etc.) and modifies existing structures. While these changes are additive, ensure that no existing APIs or behaviors are unintentionally altered. - Action: Confirm that all existing
.NET SDKAPIs remain backward compatible and document any changes explicitly.
- The
💡 SUGGESTIONS
-
Regex Timeout for ReDoS Prevention:
- The
.NET SDKcoding conventions mention amatchTimeoutof200msfor regexes to prevent ReDoS attacks. Ensure that this is consistently applied across all regex usage in the.NET SDK. - Action: Audit all regex usage in the
.NET SDKto verify compliance with thematchTimeoutguideline.
- The
-
Expand OWASP MCP Compliance:
- The documentation mentions that 11 out of 12 OWASP MCP Security Cheat Sheet sections are covered. Consider documenting why §11 (Consent UI) is out of scope and if there are plans to address it in the future.
- Action: Add a roadmap or explanation for the missing OWASP MCP section in the documentation.
-
Add Threat Model for MCP Components:
- The
.NET MCP Protocol Supportintroduces several new components (e.g.,McpGateway,McpSecurityScanner, etc.). A high-level threat model for these components would help users understand potential risks and mitigations. - Action: Include a threat model diagram or section in the documentation for the
.NET MCP Protocol Support.
- The
-
Test Coverage for
.NET MCP Protocol Support:- While this PR is documentation-focused, it introduces significant new functionality in the
.NET SDK. Ensure that comprehensive test coverage exists for all new components, especially for security-critical features likeMcpSessionAuthenticatorandMcpMessageSigner. - Action: Verify that unit and integration tests cover edge cases, concurrency scenarios, and cryptographic operations.
- While this PR is documentation-focused, it introduces significant new functionality in the
-
Internationalization (i18n) Consistency:
- The Japanese translation (
README.ja.md) was removed from the i18n section. Ensure that this is intentional and not an oversight. - Action: If the removal is intentional, document the reason in the changelog or PR description.
- The Japanese translation (
-
Clarify
.NET SDKCryptographic Choices:- The
.NET SDKusesHMAC-SHA256instead ofEd25519due to .NET 8 limitations. Consider documenting this decision in the.NET SDKREADME for transparency. - Action: Add a note in the
.NET SDKREADME explaining the cryptographic choices and any future plans for Ed25519 support.
- The
Final Recommendation
- Address the 🔴 CRITICAL issues before merging to ensure sandbox security and explicit metadata blocking.
- Review the 🟡 WARNING items for potential breaking changes and backward compatibility concerns.
- Consider implementing the 💡 SUGGESTIONS to improve documentation clarity, security, and user experience.
Once the critical issues are resolved and the warnings are addressed, this PR can be merged.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request focuses on adding comprehensive documentation for the .NET MCP (Model Context Protocol) security primitives, including API references, compliance mappings, quickstart guides, and usage examples. While this is primarily a documentation update, it introduces critical information about the security model, OWASP compliance, and usage patterns for the .NET MCP package.
Below is a detailed review of the changes, focusing on the specified areas of concern.
🔴 CRITICAL
-
Lack of Explicit Warnings About Cryptographic Misuse:
- The documentation mentions cryptographic operations like HMAC-SHA256 and ML-DSA-65 for message integrity and post-quantum signing. However, there is no explicit warning about the risks of improper key management, nonce reuse, or the importance of using secure random number generators. This could lead to insecure implementations by users.
- Action: Add a dedicated section in the
.NET SDKdocumentation warning users about cryptographic best practices, such as secure key storage, avoiding hardcoding secrets, and ensuring nonces are unique.
-
Concurrency and TOCTOU Risks in
McpSessionAuthenticator:- The documentation mentions "TOCTOU-safe concurrency" for
McpSessionAuthenticator. However, there is no explanation of how this is achieved or what mechanisms are in place to prevent time-of-check-to-time-of-use (TOCTOU) vulnerabilities. - Action: Include a detailed explanation of how TOCTOU safety is ensured in the
McpSessionAuthenticator. If this is not implemented, it should be explicitly stated as a limitation.
- The documentation mentions "TOCTOU-safe concurrency" for
-
Sandbox Escape Risks:
- The
.NET SDKdocumentation does not provide guidance on how to configure the runtime environment to minimize sandbox escape risks. For example, there is no mention of running agents in isolated environments (e.g., containers or VMs). - Action: Add a section in the
.NET SDKdocumentation that explicitly recommends running agents in isolated environments and provides best practices for securing the runtime.
- The
🟡 WARNING
-
Potential Breaking Changes in
.NET MCP:- The introduction of new classes like
McpGateway,McpSecurityScanner, andMcpSessionAuthenticatormay lead to breaking changes if existing users are required to refactor their code to adopt these new features. - Action: Clearly document whether these new classes are optional or mandatory for existing users. If they are mandatory, provide migration guides to help users transition.
- The introduction of new classes like
-
Backward Compatibility of
.NET SDK:- The documentation mentions multi-targeting
.NET 8.0and.NET 10.0. However, it is unclear if this introduces any breaking changes for users on older versions of .NET. - Action: Explicitly state whether the new
.NET MCPfeatures are backward-compatible with earlier versions of the.NET SDK. If not, provide clear guidance on how users can upgrade.
- The documentation mentions multi-targeting
💡 SUGGESTIONS
-
OWASP MCP Cheat Sheet Mapping:
- The documentation claims "11/12 OWASP MCP Security Cheat Sheet coverage." It would be helpful to include a table or matrix explicitly mapping the
.NET MCPfeatures to the OWASP MCP Cheat Sheet controls. - Action: Add a table in the documentation that maps each
.NET MCPfeature to the corresponding OWASP MCP control.
- The documentation claims "11/12 OWASP MCP Security Cheat Sheet coverage." It would be helpful to include a table or matrix explicitly mapping the
-
Examples for Advanced Scenarios:
- While the quickstart examples are helpful, they primarily cover basic use cases. Advanced scenarios like multi-agent coordination, custom policy definitions, and integration with external identity providers are not covered.
- Action: Add advanced usage examples to the
.NET SDKdocumentation, especially for scenarios involving multi-agent coordination and custom policies.
-
Performance Benchmarks for
.NET MCP:- The documentation provides performance benchmarks for the Python SDK but not for the
.NET MCPimplementation. - Action: Include performance benchmarks for the
.NET MCPimplementation, especially for critical components likeMcpGatewayandMcpSessionAuthenticator.
- The documentation provides performance benchmarks for the Python SDK but not for the
-
i18n Translations:
- The PR mentions i18n translations but does not provide details about the languages supported or the process for contributing translations.
- Action: Add a section in the documentation detailing the supported languages and how contributors can add new translations.
-
Structured Logging Examples:
- The documentation mentions
ILogger<T>for structured logging but does not provide examples. - Action: Add examples showing how to configure and use
ILogger<T>for logging MCP-related events.
- The documentation mentions
-
Security Tooling Documentation:
- The security tooling section in the main README is comprehensive but lacks
.NET-specific details. - Action: Update the security tooling section to include
.NET-specific tools or configurations, such as CodeQL for C# or .NET-specific static analysis tools.
- The security tooling section in the main README is comprehensive but lacks
-
Credential Handling in
CredentialRedactor:- The
CredentialRedactoris mentioned as supporting "10 credential pattern redaction." However, there is no information on how users can extend or customize these patterns. - Action: Add documentation on how users can define custom credential patterns for redaction.
- The
-
OpenTelemetry Integration:
- The documentation mentions 4 MCP-specific counters for OpenTelemetry but does not provide examples of how to configure or query these metrics.
- Action: Add a section in the
.NET SDKdocumentation with examples of how to set up and use OpenTelemetry for MCP-specific metrics.
Final Recommendation
- Merge Readiness: This PR is primarily a documentation update and does not introduce code changes. However, the lack of explicit warnings and detailed explanations for critical security features like cryptographic operations, TOCTOU safety, and sandboxing is concerning.
- Blocking Issues: Address the 🔴 CRITICAL issues before merging.
- Post-Merge Enhancements: Consider implementing the 💡 SUGGESTIONS in subsequent updates to improve the usability and security of the
.NET MCPdocumentation.
Addressing AI Reviewer Findings"Missing mTLS configuration details" — The documentation intentionally covers mTLS at an architectural guidance level rather than providing implementation-specific cert generation steps, as these vary significantly by deployment environment (Azure, AWS, on-prem). The docs link to Microsoft Learn for platform-specific mTLS configuration. "gVisor/Kata Containers detail" — Same approach — the docs recommend sandboxing strategies and link to upstream documentation rather than embedding deployment-specific instructions that would go stale. These are enhancement suggestions for future docs iterations, not blockers. |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces comprehensive documentation for the .NET MCP (Model Context Protocol) security primitives, including API references, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, and usage examples. While the PR is documentation-focused, it also includes updates to the .cspell.json file and the CHANGELOG.md.
The documentation is thorough and aligns with the project's goals of providing runtime governance for AI agents. However, there are a few areas that require attention to ensure correctness, clarity, and alignment with the project's focus areas.
🔴 CRITICAL Issues
No critical security issues were identified in this PR, as it primarily involves documentation updates.
🟡 WARNING: Potential Breaking Changes
- Backward Compatibility of
.NET MCPAPI:- The documentation introduces new
.NET MCPprimitives such asMcpGateway,McpSecurityScanner,McpSessionAuthenticator, etc. While these are new additions, their introduction could potentially lead to breaking changes if existing users of the.NETpackage expect a different API surface. - Action: Ensure that these new APIs are introduced in a backward-compatible manner. If breaking changes are unavoidable, clearly document them in the
CHANGELOG.mdand provide migration guides.
- The documentation introduces new
💡 Suggestions for Improvement
-
OWASP MCP Cheat Sheet Compliance Mapping:
- The documentation mentions "11/12 OWASP MCP Security Cheat Sheet coverage." However, it does not specify which item is not covered or why. This could lead to confusion for users.
- Action: Add a section explicitly listing the OWASP MCP Cheat Sheet items, indicating which ones are covered and providing justifications for any gaps.
-
Cryptographic Operations:
- The
.NET MCPprimitives include cryptographic features likeHMAC-SHA256andML-DSA-65 post-quantum signing. While these are mentioned in the documentation, there is no reference to how they are implemented or validated. - Action: Include a brief explanation or link to documentation about the cryptographic libraries used and their compliance with standards (e.g., NIST, FIPS).
- The
-
Concurrency and Thread Safety:
- The documentation mentions
TOCTOU-safe concurrencyinMcpSessionAuthenticator. However, there is no explanation of how thread safety is achieved or tested. - Action: Add details about the thread safety mechanisms in place and any testing performed to validate them.
- The documentation mentions
-
Sandbox Escape Vectors:
- While the documentation emphasizes execution sandboxing, it does not provide details about how sandbox escape vectors are mitigated.
- Action: Include a section in the documentation that outlines the measures taken to prevent sandbox escapes and the limitations of the current implementation.
-
Type Safety and Validation:
- The
.NET MCPprimitives seem to rely on configuration files and runtime inputs (e.g.,IConfigurationbinding). There is no mention of how these inputs are validated or whether type safety is enforced. - Action: Clarify how input validation and type safety are handled, especially for critical components like
McpGatewayandMcpSecurityScanner.
- The
-
Internationalization (i18n):
- The PR mentions i18n translations but does not specify which languages are supported or how users can contribute translations.
- Action: Add a section in the documentation listing supported languages and guidelines for contributing translations.
-
Documentation Structure:
- The
README.mdis very long and covers a wide range of topics. This could overwhelm new users. - Action: Consider splitting the
README.mdinto smaller, more focused documents (e.g., separate files for installation, quickstart, and advanced topics) and linking them from the main README.
- The
-
Security Tooling:
- The documentation lists various security tools (e.g., CodeQL, Gitleaks, ClusterFuzzLite) but does not explain how they are integrated into the CI/CD pipeline.
- Action: Provide a brief overview or link to a document explaining how these tools are used in the CI/CD pipeline.
-
Examples and Tutorials:
- The examples provided are helpful but could benefit from additional context, such as expected outputs or common pitfalls.
- Action: Enhance the examples with more detailed explanations and include links to the corresponding sections in the documentation.
-
Regulatory Alignment:
- The documentation mentions alignment with regulations like the EU AI Act and Colorado AI Act but does not provide detailed mappings.
- Action: Include a detailed mapping of how the toolkit addresses specific requirements of these regulations.
Additional Notes
- The
.cspell.jsonupdate to include "appsettings" is appropriate, given the context of.NETdevelopment. - The
CHANGELOG.mdupdate is detailed and provides a good summary of the new features. However, it could benefit from a clearer distinction between new features and breaking changes. - The documentation does a good job of emphasizing OWASP Agentic Top 10 compliance, but it could be more explicit about how each risk is mitigated.
Final Recommendation
- Merge this PR after addressing the suggestions above, especially those related to backward compatibility and documentation clarity.
- Conduct a thorough review of the
.NET MCPimplementation (introduced in #824) to ensure alignment with the project's security and governance goals.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request focuses on adding comprehensive documentation for .NET MCP (Model Context Protocol) security primitives. While the changes are primarily documentation-related, they touch on critical areas such as OWASP MCP compliance, cryptographic operations, and usage examples. Below is a detailed review of the changes, focusing on the specified focus areas.
🔴 CRITICAL
-
Cryptographic Operations Documentation
- The documentation mentions the use of HMAC-SHA256 and ML-DSA-65 for message integrity and post-quantum signing. However, there is no detailed explanation of how these cryptographic methods are implemented or used. Without this, users might misuse these primitives, leading to potential vulnerabilities.
- Actionable Recommendation: Add a detailed section explaining the cryptographic operations, including key management, rotation, and storage best practices. Provide examples of how to securely use these cryptographic primitives in real-world scenarios.
- The documentation mentions the use of HMAC-SHA256 and ML-DSA-65 for message integrity and post-quantum signing. However, there is no detailed explanation of how these cryptographic methods are implemented or used. Without this, users might misuse these primitives, leading to potential vulnerabilities.
-
Concurrency and TOCTOU (Time-of-Check to Time-of-Use)
- The
McpSessionAuthenticatoris described as providing "TOCTOU-safe concurrency." However, there is no explanation or example of how this is achieved. TOCTOU vulnerabilities are critical and must be explicitly addressed.- Actionable Recommendation: Include a detailed explanation of how the
McpSessionAuthenticatorensures TOCTOU safety. Provide code examples or pseudocode to demonstrate the implementation.
- Actionable Recommendation: Include a detailed explanation of how the
- The
-
Sandbox Escape Vectors
- The documentation mentions "execution sandboxing" and "4-tier privilege rings," but it does not provide sufficient detail on how these mechanisms prevent sandbox escapes.
- Actionable Recommendation: Add a dedicated section in the documentation that explains the sandboxing architecture, including how it prevents escape vectors. Include examples of common attack scenarios and how the toolkit mitigates them.
- The documentation mentions "execution sandboxing" and "4-tier privilege rings," but it does not provide sufficient detail on how these mechanisms prevent sandbox escapes.
-
OWASP Agentic Top 10 Compliance
- While the documentation claims full coverage of the OWASP Agentic Top 10, there is no evidence or test cases provided to substantiate these claims.
- Actionable Recommendation: Link to specific test cases or evidence that demonstrate compliance with each OWASP Agentic Top 10 risk. This will help users trust the claims and understand how to implement the controls.
- While the documentation claims full coverage of the OWASP Agentic Top 10, there is no evidence or test cases provided to substantiate these claims.
🟡 WARNING
- Backward Compatibility
- The documentation introduces new APIs and features for .NET MCP security primitives. While this is a documentation update, it indirectly implies changes to the public API.
- Actionable Recommendation: Clearly document any breaking changes or new requirements introduced by the .NET MCP package. If there are no breaking changes, explicitly state this in the changelog.
- The documentation introduces new APIs and features for .NET MCP security primitives. While this is a documentation update, it indirectly implies changes to the public API.
💡 SUGGESTIONS
-
Type Safety and Pydantic Model Validation
- While this PR is focused on .NET, the Python examples in the README could benefit from explicit type annotations and Pydantic model validation to ensure type safety.
- Actionable Recommendation: Update Python examples to include type annotations and Pydantic validation where applicable.
- While this PR is focused on .NET, the Python examples in the README could benefit from explicit type annotations and Pydantic model validation to ensure type safety.
-
i18n Translations
- The documentation mentions i18n translations but does not provide details on how these are managed or updated.
- Actionable Recommendation: Include a section explaining the i18n process, including how translations are maintained and updated.
- The documentation mentions i18n translations but does not provide details on how these are managed or updated.
-
Quickstart Examples
- The quickstart examples are helpful but could benefit from additional context, such as common pitfalls or best practices.
- Actionable Recommendation: Add a "Common Pitfalls" or "Best Practices" section to the quickstart guides.
- The quickstart examples are helpful but could benefit from additional context, such as common pitfalls or best practices.
-
Regulatory Alignment
- The documentation mentions alignment with regulations like the EU AI Act and Colorado AI Act but does not provide detailed mappings.
- Actionable Recommendation: Include detailed mappings for each regulation, similar to the OWASP compliance mapping.
- The documentation mentions alignment with regulations like the EU AI Act and Colorado AI Act but does not provide detailed mappings.
-
Performance Benchmarks
- The performance benchmarks are impressive but lack details on the testing environment and methodology.
- Actionable Recommendation: Add a section detailing the testing environment, methodology, and any assumptions made during benchmarking.
- The performance benchmarks are impressive but lack details on the testing environment and methodology.
Final Notes
This PR is a significant step forward in documenting .NET MCP security primitives and aligning them with OWASP MCP Cheat Sheet compliance. However, the lack of detailed explanations for cryptographic operations, concurrency safety, and sandboxing mechanisms is a critical gap that must be addressed before merging.
- Priority Actions: Address the 🔴 CRITICAL issues related to cryptographic operations, TOCTOU safety, sandboxing, and OWASP compliance evidence.
- Secondary Actions: Resolve the 🟡 WARNING on backward compatibility and implement the 💡 SUGGESTIONS to improve usability and clarity.
Let me know if you need further clarification or assistance!
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Feedback on Pull Request: docs(dotnet): add MCP security documentation and examples
🔴 CRITICAL: Security Concerns
-
HMAC-SHA256 and ML-DSA-65 Signing:
- The documentation mentions the use of
HMAC-SHA256for message integrity andML-DSA-65for post-quantum signing. Ensure that the cryptographic primitives used are up-to-date with current standards and are implemented securely. Specifically:- Verify that the key management and storage for HMAC keys are secure and follow best practices (e.g., hardware-backed storage or secure vaults).
- Confirm that
ML-DSA-65is a vetted post-quantum algorithm. If it is experimental or proprietary, document its limitations and fallback mechanisms.
- The documentation mentions the use of
-
Concurrency in
McpSessionAuthenticator:- The documentation mentions "TOCTOU-safe concurrency." Ensure that the implementation of session binding and TTL does not introduce race conditions or time-of-check-to-time-of-use vulnerabilities. Specifically:
- Validate that session binding operations are atomic and thread-safe.
- Confirm that the TTL mechanism cannot be bypassed or exploited by malicious actors.
- The documentation mentions "TOCTOU-safe concurrency." Ensure that the implementation of session binding and TTL does not introduce race conditions or time-of-check-to-time-of-use vulnerabilities. Specifically:
-
gRPC Server Interceptor:
- The gRPC server interceptor is mentioned as supporting "all 4 handler types." Ensure that the interceptor properly validates and sanitizes inputs to prevent injection attacks or privilege escalation. Specifically:
- Verify that the interceptor enforces strict input validation and sanitization for all handler types.
- Confirm that the interceptor does not inadvertently expose sensitive internal state or metadata.
- The gRPC server interceptor is mentioned as supporting "all 4 handler types." Ensure that the interceptor properly validates and sanitizes inputs to prevent injection attacks or privilege escalation. Specifically:
-
Credential Redactor:
- The
CredentialRedactorclaims to redact 10 credential patterns. Ensure that the redaction mechanism is robust and cannot be bypassed. Specifically:- Validate that the redactor handles edge cases, such as credentials embedded in non-standard formats or obfuscated strings.
- Confirm that the redactor does not inadvertently redact legitimate data or fail to redact sensitive information.
- The
🟡 WARNING: Potential Breaking Changes
-
Backward Compatibility of
.NET MCP Protocol Support:- The addition of
.NET MCP Protocol Supportintroduces several new APIs and features. Ensure that these changes do not break existing integrations or workflows. Specifically:- Validate that the new APIs are backward-compatible with previous versions of the
.NETpackage. - Confirm that existing users can upgrade without requiring major code changes.
- Validate that the new APIs are backward-compatible with previous versions of the
- The addition of
-
OWASP MCP Cheat Sheet Compliance Mapping:
- The documentation mentions "11/12 OWASP MCP Security Cheat Sheet coverage." Ensure that the compliance mapping does not introduce breaking changes to existing policy configurations or enforcement mechanisms.
💡 Suggestions for Improvement
-
Documentation Clarity:
- The documentation is comprehensive but dense. Consider breaking it into smaller, focused sections with clear headings and subheadings. For example:
- Separate the API reference docs from the quickstart guides and usage examples.
- Provide a summary table for
.NET MCP Protocol Supportfeatures.
- The documentation is comprehensive but dense. Consider breaking it into smaller, focused sections with clear headings and subheadings. For example:
-
Internationalization (i18n):
- The documentation mentions i18n translations. Ensure that the translations are accurate and culturally appropriate. Consider adding a language switcher to the documentation site for easier navigation.
-
OWASP MCP Cheat Sheet Compliance:
- The documentation mentions "11/12 OWASP MCP Security Cheat Sheet coverage." Consider explicitly listing the covered items and providing links to the implementation details for each.
-
Performance Benchmarks:
- The documentation mentions governance latency of
< 0.1 ms per action. Consider providing detailed benchmarks for.NET MCP Protocol Supportto demonstrate its performance under different workloads.
- The documentation mentions governance latency of
-
Sample Apps:
- The documentation mentions two sample apps (ASP.NET Core full-stack and Official MCP SDK integration). Consider providing links to these apps and including screenshots or diagrams to illustrate their functionality.
Summary
This pull request adds valuable documentation for .NET MCP Protocol Support and related security primitives. However, it introduces several critical security concerns and potential breaking changes that must be addressed before merging. Additionally, there are opportunities to improve the documentation's clarity, organization, and usability.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: docs(dotnet): add MCP security documentation and examples
Summary
This pull request adds comprehensive documentation for .NET MCP security primitives, including API reference docs, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, i18n translations, and usage examples. It is part of a multi-step integration process and should be merged after #824.
🔴 CRITICAL: Security Concerns
-
Sensitive Keywords in
.cspell.json
The addition of sensitive keywords likeaccesskey,clientsecret,authtoken,apikey,bearertoken, andsecretpasswordto.cspell.jsonis concerning. While these entries may be intended for spell-checking purposes, they could inadvertently expose sensitive information if not handled properly. Ensure that these keywords are not hardcoded or logged anywhere in the codebase. Consider implementing automated secret scanning tools like GitHub's secret scanning or Gitleaks to detect accidental leaks. -
HMAC-SHA256 and ML-DSA-65
The documentation mentions the use of HMAC-SHA256 for message integrity and ML-DSA-65 for post-quantum signing. While these algorithms are robust, ensure that:- The implementation adheres to the latest cryptographic standards.
- Key management practices are secure (e.g., keys are rotated periodically, stored securely, and not hardcoded).
- The cryptographic library used is up-to-date and free from known vulnerabilities.
-
TOCTOU-Safe Concurrency
TheMcpSessionAuthenticatorclaims to provide TOCTOU-safe concurrency. Ensure that the implementation properly mitigates race conditions, especially when handling cryptographic operations or sensitive data. Consider adding tests to simulate concurrent access and verify the absence of race conditions. -
Sandbox Escape Vectors
The.NET MCP Protocol Supportdocumentation mentions execution sandboxing and privilege rings. Ensure that the sandbox implementation is robust and does not allow for escape vectors such as:- Arbitrary code execution via deserialization vulnerabilities.
- Exploitation of OS-level APIs or system calls.
- Insecure handling of subprocesses (e.g.,
cmdshell,popen).
-
Credential Redaction
TheCredentialRedactoris described as supporting 10 credential pattern redactions. Ensure that this functionality is thoroughly tested against edge cases (e.g., partial matches, encoded credentials) to prevent accidental leakage of sensitive information.
🟡 WARNING: Potential Breaking Changes
-
Backward Compatibility
The addition of new APIs and features (e.g.,McpGateway,McpSecurityScanner,McpSessionAuthenticator) may introduce breaking changes for existing users of the.NETSDK. Ensure that:- The new APIs are backward-compatible with previous versions.
- Deprecation notices are provided for any removed or modified functionality.
- Comprehensive migration guides are included in the documentation.
-
Multi-Targeting .NET Versions
The documentation mentions multi-targeting.NET 8.0and.NET 10.0. Ensure that the SDK is compatible with all supported versions and does not introduce runtime issues for users on older versions.
💡 Suggestions for Improvement
-
OWASP MCP Cheat Sheet Compliance
The documentation highlights 11/12 OWASP MCP Security Cheat Sheet coverage. Consider adding detailed examples or test cases for the remaining category to achieve full compliance. -
Performance Benchmarks
While the documentation mentions sub-millisecond latency for policy enforcement, consider providing detailed benchmarks specific to the.NETimplementation. This will help users understand the performance impact of integrating MCP governance. -
i18n Translations
The pull request includes i18n translations for the documentation. Ensure that these translations are accurate and culturally appropriate. Consider adding a process for community contributions to improve translations over time. -
Sample Applications
The inclusion of two sample apps (ASP.NET Core full-stack and Official MCP SDK integration) is a great addition. Consider expanding these samples to include more complex scenarios, such as multi-agent interactions or integration with third-party frameworks. -
OpenTelemetry Integration
The documentation mentions MCP-specific counters for OpenTelemetry. Consider providing detailed examples or tutorials on how to set up and visualize these metrics in popular observability platforms like Grafana or Azure Monitor.
Action Items
-
Security Audit
Conduct a thorough security audit of the.NET MCP Protocol Supportimplementation, focusing on cryptographic operations, sandboxing, and concurrency. -
Backward Compatibility Testing
Perform regression testing to ensure that the new features do not introduce breaking changes for existing users. -
Documentation Enhancements
Add detailed migration guides, performance benchmarks, and complex sample applications to the documentation. -
Automated Secret Scanning
Integrate tools like Gitleaks or GitHub's secret scanning into the CI/CD pipeline to detect and prevent accidental leaks of sensitive information.
Final Recommendation
This pull request introduces valuable documentation and examples for the .NET MCP Protocol Support. However, the critical security concerns and potential breaking changes must be addressed before merging. Proceed with caution and prioritize the outlined action items.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: docs(dotnet): add MCP security documentation and examples
🔴 CRITICAL: Security Concerns
-
Credential Patterns in
.cspell.json- The addition of sensitive credential patterns like
accesskey,apikey,authtoken,clientsecret,secretkey, etc., in.cspell.jsoncould inadvertently expose these patterns to attackers if the file is misused or leaked. While this is intended for spell-checking, it creates a risk of credential enumeration. - Actionable Recommendation: Remove sensitive credential patterns from
.cspell.json. Instead, use a dedicated secret scanning tool (e.g., Gitleaks or TruffleHog) integrated into CI/CD workflows.
- The addition of sensitive credential patterns like
-
MCP Security Scanner
- The documentation mentions
McpSecurityScannerdetecting threats like tool poisoning, typosquatting, and protocol attacks. However, no details are provided about how these detections are implemented or tested. - Actionable Recommendation: Ensure that the MCP Security Scanner has robust unit tests and integration tests for all listed threat vectors. Provide documentation on its detection mechanisms and limitations.
- The documentation mentions
🟡 WARNING: Potential Breaking Changes
- Backward Compatibility of `.NET MCP Protocol Support
- The addition of
.NET MCP Protocol Supportintroduces new APIs and features, such asMcpGateway,McpSecurityScanner,McpSessionAuthenticator, etc. While these are valuable, they may introduce breaking changes for existing users of the.NETpackage. - Actionable Recommendation: Clearly document all breaking changes in the
CHANGELOG.mdand provide migration guides for users upgrading from previous versions.
- The addition of
💡 Suggestions for Improvement
-
OWASP MCP Cheat Sheet Compliance Mapping
- The documentation claims 11/12 OWASP MCP Security Cheat Sheet coverage but does not specify which item is missing or why. This could lead to confusion or misinterpretation of the toolkit's security capabilities.
- Actionable Recommendation: Add a detailed table or section in the documentation that explicitly maps each OWASP MCP Cheat Sheet item to the corresponding toolkit feature, including the missing item and rationale.
-
Concurrency and Thread Safety
- The
.NET MCP Protocol Supportdocumentation mentions features likeMcpSlidingRateLimiterandMcpSessionAuthenticator. These components likely involve concurrent execution. However, there is no mention of thread safety guarantees or testing. - Actionable Recommendation: Add documentation and tests to verify thread safety for concurrent agent execution, especially for rate limiting and session management.
- The
-
Sandbox Escape Vectors
- The
.NET MCP Protocol Supportintroduces execution sandboxing features. However, the documentation does not address potential sandbox escape vectors or mitigation strategies. - Actionable Recommendation: Include a dedicated section in the documentation on sandbox security, detailing how escape vectors are mitigated (e.g., syscall filtering, seccomp profiles).
- The
-
Type Safety and Validation
- The
.NET MCP Protocol Supportdocumentation does not specify whether the new APIs use strong type validation (e.g., Pydantic models or equivalent in .NET). This is critical for preventing type-related vulnerabilities. - Actionable Recommendation: Ensure all public APIs in the
.NETpackage enforce strict type validation and document the validation rules.
- The
-
Internationalization (i18n)
- The PR mentions i18n translations but does not provide details on the languages supported or how users can contribute translations.
- Actionable Recommendation: Add a section in the documentation explaining the supported languages, how translations are managed, and how users can contribute.
Summary of Actions
🔴 CRITICAL
- Remove sensitive credential patterns from
.cspell.json. - Ensure robust testing and documentation for MCP Security Scanner.
🟡 WARNING
- Document breaking changes in
.NET MCP Protocol Supportand provide migration guides.
💡 SUGGESTIONS
- Add OWASP MCP Cheat Sheet mapping details.
- Verify thread safety in concurrent execution features.
- Document sandbox escape mitigation strategies.
- Enforce and document type validation for
.NETAPIs. - Expand i18n documentation for translations.
Final Recommendation
This PR introduces valuable documentation and examples for .NET MCP security primitives. However, addressing the flagged security concerns and potential breaking changes is critical before merging. Additionally, implementing the suggested improvements will enhance the toolkit's usability and security posture.
0d54c1a to
27db3c9
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Feedback for PR: docs(dotnet): add MCP security documentation and examples
🔴 CRITICAL: Security Issues
-
Replay Protection Window Configuration
- The
ReplayWindowinMcpMessageSigneris set toTimeSpan.FromMinutes(5)in the example. While this is reasonable for many use cases, it may be insufficient for high-security environments where shorter replay windows are required. Ensure that the default configuration or documentation explicitly warns users about the trade-offs of longer replay windows, especially in scenarios involving sensitive data or high-value transactions.
Action: Add a note in the documentation emphasizing the importance of configuring the
ReplayWindowbased on the specific security requirements of the deployment. - The
-
Potential SSRF Vulnerability
- The
McpSecurityScannerdocumentation mentions scanning for "protocol attacks" but does not explicitly address SSRF (Server-Side Request Forgery) vulnerabilities. MCP servers using HTTP transport should explicitly validate and sanitize URLs to prevent SSRF attacks.
Action: Update the documentation to include explicit guidance on mitigating SSRF risks, such as validating URLs and restricting outbound requests to trusted domains.
- The
-
Credential Redaction Scope
- The
CredentialRedactordocumentation mentions redacting sensitive information such as API keys and connection strings. However, it does not specify whether it supports recursive redaction for nested data structures (e.g., JSON objects with embedded secrets).
Action: Ensure the
CredentialRedactorimplementation supports recursive redaction for nested structures and update the documentation accordingly. - The
🟡 WARNING: Potential Breaking Changes
-
Post-Quantum Cryptography Support
- The introduction of ML-DSA-65 for post-quantum cryptography in .NET 10+ is a significant addition. While this is future-proofing, it introduces a potential breaking change for users who upgrade to .NET 10 and expect backward compatibility with older cryptographic algorithms.
Action: Clearly document the implications of using ML-DSA-65, including compatibility considerations with systems that rely on classical cryptographic algorithms (e.g., ECDSA/RSA). Provide migration guidance for users transitioning to .NET 10.
-
Behavior vs Behaviour
- The documentation inconsistently uses "behavior" and "behaviour." While this is not a functional issue, it may confuse users, especially in international contexts.
Action: Standardize the spelling across the documentation. Consider aligning with the primary audience's preferred spelling (e.g., "behavior" for US English).
💡 Suggestions for Improvement
-
Expand OWASP MCP Coverage
- The documentation mentions 11/12 OWASP MCP Cheat Sheet coverage but does not specify which section is missing. This could lead to confusion or concern among users.
Action: Explicitly list the covered sections and clarify which section is not covered, along with a rationale or roadmap for addressing it.
-
Kubernetes Hardening Guide
- The Kubernetes hardening guide is comprehensive but could benefit from additional details on monitoring and auditing. For example, integrating tools like Falco or Kubernetes Audit Logs to detect policy violations or anomalous behavior.
Action: Add a section on monitoring and auditing MCP server pods, including recommended tools and configurations.
-
Credential Redaction Examples
- The examples for
CredentialRedactorare helpful but could be expanded to include real-world scenarios, such as redacting secrets from log files or HTTP request payloads.
Action: Add more detailed examples demonstrating the use of
CredentialRedactorin common scenarios. - The examples for
-
Session Authentication
- The session authentication example is clear but could benefit from additional details on session expiration and renewal strategies.
Action: Include a subsection on best practices for session management, such as token renewal and invalidation.
-
Documentation Structure
- The documentation is dense and may be overwhelming for new users. Consider breaking it into smaller, focused sections or adding a table of contents for easier navigation.
Action: Reorganize the documentation into modular sections with clear headings and a table of contents.
General Observations
- The addition of MCP security documentation and examples significantly enhances the usability and security of the
.NET MCP Protocol Support. The examples are well-structured and provide clear guidance for developers. - The alignment with the OWASP MCP Security Cheat Sheet is commendable and demonstrates a strong commitment to security best practices.
- The inclusion of i18n translations is a thoughtful touch that improves accessibility for non-English-speaking users.
Summary of Actions
🔴 CRITICAL
- Add warnings about replay window configuration.
- Address SSRF mitigation in MCP servers.
- Ensure recursive credential redaction support.
🟡 WARNING
- Document implications of post-quantum cryptography.
- Standardize spelling of "behavior" vs "behaviour."
💡 SUGGESTION
- Expand OWASP MCP coverage details.
- Add monitoring/auditing guidance for Kubernetes.
- Provide more detailed credential redaction examples.
- Include session management best practices.
- Reorganize documentation for better readability.
This PR is well-prepared but requires minor adjustments to address critical security concerns and improve usability.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces comprehensive documentation for .NET MCP security primitives, including API references, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, i18n translations, and usage examples. While the PR is documentation-focused, it also includes critical security-related content and implementation details for the MCP governance layer. Below are the findings and recommendations:
🔴 CRITICAL
-
Replay Protection in
McpMessageSigner:- The
McpMessageSignerimplementation mentions nonce-based replay rejection but does not provide details on how nonces are managed or stored. If nonces are not stored securely or are reused, this could lead to replay attacks. - Action: Ensure that nonces are stored securely and are unique for each message. Consider using a database or in-memory store with TTL for nonce tracking.
- The
-
Session Authentication (
McpSessionAuthenticator):- The
ValidateRequestmethod does not specify how session tokens are validated. If tokens are not cryptographically signed or if the validation mechanism is weak, it could lead to session hijacking or impersonation. - Action: Ensure that session tokens are cryptographically signed and include a mechanism to verify their integrity and authenticity. Use a secure algorithm like HMAC-SHA256 or ML-DSA for signing.
- The
-
Potential Sandbox Escape in gVisor/Kata Containers:
- The documentation suggests using gVisor or Kata Containers for untrusted MCP servers but does not specify how to handle potential sandbox escape vectors (e.g., syscall filtering, file system access).
- Action: Include explicit recommendations for syscall filtering and file system access restrictions to mitigate sandbox escape risks. For example, ensure that sensitive directories like
/procand/sysare inaccessible.
-
Credential Leakage in
McpResponseScanner:- The
McpResponseScannermentions detecting credential leakage but does not specify the patterns or methods used for detection. If the patterns are incomplete or not updated regularly, sensitive data could be leaked. - Action: Provide a detailed list of credential patterns being scanned and ensure regular updates to include new patterns (e.g., cloud provider keys, JWTs).
- The
🟡 WARNING
-
Breaking Changes in .NET Target Versions:
- The documentation mentions multi-targeting .NET 8.0 and .NET 10.0, with post-quantum cryptography support on .NET 10+. This could introduce breaking changes for users on older .NET versions.
- Action: Clearly document the impact of these changes on backward compatibility and provide migration guides for users on .NET versions below 8.0.
-
Behavior vs Behaviour:
- The documentation uses inconsistent spelling for "behavior" (e.g., "Behaviour" in some sections). This could lead to confusion for international users.
- Action: Standardize spelling across all documentation.
💡 SUGGESTIONS
-
Expand on OWASP MCP Compliance:
- The documentation mentions compliance with 11/12 OWASP MCP Security Cheat Sheet sections but does not specify which section is missing or why. This could lead to ambiguity for users aiming for full compliance.
- Action: Clearly identify the missing section and provide guidance on how users can address it.
-
Add Examples for gRPC Interceptor:
- The documentation mentions gRPC server interceptors but does not provide usage examples.
- Action: Include code snippets demonstrating how to use the gRPC interceptors for MCP governance.
-
Clarify Rate Limiting Behavior:
- The
McpSlidingRateLimiterdocumentation mentions "per-agent sliding window rate limiting" but does not specify how the sliding window is implemented or configured. - Action: Add details on the configuration options for the sliding window (e.g., time intervals, max requests).
- The
-
Enhance i18n Documentation:
- The i18n translations for Japanese and Simplified Chinese contain minor formatting inconsistencies (e.g., extra slashes in paths, missing spaces).
- Action: Review and correct formatting issues in the translated documentation.
-
Add Unit Tests for MCP Governance Layer:
- While the documentation is comprehensive, there is no mention of unit tests for the MCP governance layer.
- Action: Ensure that the MCP-related classes (
McpGateway,McpSecurityScanner,McpSessionAuthenticator, etc.) have thorough unit test coverage, especially for edge cases and failure scenarios.
-
Document Cryptographic Algorithms:
- The documentation mentions post-quantum cryptography support (ML-DSA-65) but does not explain how it is implemented or its implications for users.
- Action: Provide a dedicated section explaining the cryptographic algorithms used, their strengths, and any potential limitations.
Final Recommendation
The documentation is well-written and provides valuable insights into the .NET MCP security primitives. However, the critical issues related to replay protection, session authentication, sandbox escape vectors, and credential leakage must be addressed before merging. Additionally, the potential breaking changes should be clearly documented, and the suggested improvements should be considered to enhance the overall quality and usability of the documentation.
|
CI Status Summary ✅ All CI checks pass except:
.NET MCP docs + Official SDK sample + mTLS Kestrel example. Slimmed to 9 files. |
|
Reviewed docs - content looks good. This PR is blocked on its dependency feature PR being merged first. Will merge this once the base PR lands. |
96eca37 to
68c848f
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: docs(dotnet): add MCP security documentation and examples
Summary
This PR introduces comprehensive documentation for .NET MCP security primitives, including API references, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, international translations, and usage examples. It is part of a series of PRs that aim to enhance the .NET MCP security features of the microsoft/agent-governance-toolkit.
🔴 CRITICAL Issues
-
Potential Replay Attack in
McpMessageSigner- The
McpMessageSignerimplementation uses a nonce-based replay protection mechanism. However, the documentation does not specify how nonce uniqueness is enforced across distributed systems. If the nonce is not globally unique or if it is reused, attackers could replay messages within theReplayWindow. - Actionable Recommendation: Ensure nonce generation uses a cryptographically secure random number generator and document the mechanism for ensuring global uniqueness (e.g., UUIDs combined with timestamps). Additionally, clarify how nonce storage and expiration are handled in distributed environments.
- The
-
Sandbox Escape Vectors in Kubernetes Deployment
- The Kubernetes hardening guide mentions using
gVisororKata Containersfor untrusted servers but does not enforce their usage. If these are optional, there is a risk of sandbox escape for MCP servers that execute arbitrary code. - Actionable Recommendation: Update the documentation to mandate the use of sandbox runtimes (e.g.,
gVisororKata Containers) for any MCP server that executes untrusted code. Add explicit warnings about the risks of not using sandboxing.
- The Kubernetes hardening guide mentions using
-
Post-Quantum Cryptography Implementation
- The documentation mentions ML-DSA-65 for post-quantum signing on .NET 10+, but there is no information about how this algorithm is integrated or whether it has been independently audited for security. Without proper validation, this could introduce vulnerabilities.
- Actionable Recommendation: Provide details about the implementation of ML-DSA-65, including references to cryptographic audits or standards compliance. If this algorithm is experimental, explicitly warn users about its limitations.
🟡 WARNING Issues
-
Breaking Changes in
.NET MCP Protocol Support- The addition of post-quantum cryptography (ML-DSA-65) and multi-targeting for .NET 10+ introduces potential breaking changes for users relying on classical cryptographic algorithms (e.g., ECDSA/RSA).
- Actionable Recommendation: Clearly document backward compatibility guarantees and provide migration guides for users transitioning from classical cryptography to post-quantum cryptography.
-
Behavior vs. Behaviour Terminology
- The documentation inconsistently uses "Behaviour" and "Behavior" in tables and examples. This could confuse users, especially in international contexts.
- Actionable Recommendation: Standardize terminology across the documentation to use "Behavior" (American English) for consistency with .NET and Microsoft standards.
💡 Suggestions for Improvement
-
OWASP MCP Cheat Sheet Compliance Mapping
- While the documentation mentions compliance with OWASP MCP Cheat Sheet, it does not provide detailed mapping for each feature. Users may struggle to understand how specific features align with OWASP recommendations.
- Actionable Recommendation: Add a dedicated section that explicitly maps each MCP security feature (e.g.,
McpGateway,McpSecurityScanner) to OWASP MCP Cheat Sheet sections.
-
Internationalization (i18n) Enhancements
- The i18n translations for Japanese and Simplified Chinese contain minor formatting inconsistencies (e.g., broken links, extra slashes in paths).
- Actionable Recommendation: Review and fix all i18n translations for formatting issues. Consider adding automated tests to validate link correctness in translated documentation.
-
Credential Redaction Documentation
- The
CredentialRedactorsection is truncated in the diff, and its functionality is not fully explained. This is a critical feature for audit log security. - Actionable Recommendation: Expand the documentation for
CredentialRedactorto include examples of supported credential patterns and how users can customize redaction rules.
- The
-
Resource Limits in Kubernetes
- The Kubernetes hardening guide provides general recommendations for resource limits but does not specify how to enforce these limits across different environments (e.g., AKS, EKS, GKE).
- Actionable Recommendation: Include platform-specific examples for enforcing resource limits, such as using Kubernetes admission controllers or policies.
-
Testing Recommendations
- The documentation does not mention testing strategies for MCP security features, such as how to validate the effectiveness of
McpSecurityScannerorMcpResponseScanner. - Actionable Recommendation: Add a section on testing MCP security features, including example test cases and integration with CI/CD pipelines.
- The documentation does not mention testing strategies for MCP security features, such as how to validate the effectiveness of
Final Assessment
This PR significantly enhances the documentation for .NET MCP Protocol Support and aligns with OWASP MCP Cheat Sheet recommendations. However, there are critical security concerns related to nonce replay protection, sandboxing, and post-quantum cryptography implementation. Addressing these issues is essential to ensure the robustness and security of the MCP governance layer.
- Approval Status: 🚫 Changes Required
- Priority: High (due to critical security issues)
Please address the flagged issues and suggestions before merging this PR.
68c848f to
a6c0138
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review of Pull Request: docs(dotnet): add MCP security documentation and examples
🔴 CRITICAL
No critical security issues identified in this documentation update. However, ensure that the examples provided in the documentation do not inadvertently introduce insecure practices, especially when dealing with MCP security primitives.
🟡 WARNING
- Breaking Changes in Documentation:
- This PR introduces documentation for
.NET MCP security primitives. While this is not a direct code change, any discrepancies between the documentation and the actual implementation could lead to confusion for users. Ensure that the documentation aligns perfectly with the MCP package's API and behavior.
- This PR introduces documentation for
💡 SUGGESTIONS
-
Expand OWASP MCP Cheat Sheet Compliance Mapping:
- The documentation mentions OWASP MCP Cheat Sheet compliance mapping. Consider adding a detailed section that explicitly maps each MCP security primitive to the relevant OWASP guidelines. This will help users understand how the library adheres to security best practices.
-
Include Threat Modeling Examples:
- Adding threat modeling examples for common use cases would be beneficial. This can help users understand potential attack vectors and how to mitigate them using the MCP primitives.
-
Add Code Snippets for Common Scenarios:
- While the documentation mentions usage examples, ensure that these examples cover common scenarios such as credential handling, cryptographic operations, and SPIFFE/SVID integration. This will make the documentation more practical and user-friendly.
-
Internationalization (i18n):
- The PR mentions i18n translations. Ensure that translations are accurate and culturally appropriate, especially for security-related terminology.
-
Spell Check Updates:
- The
.cspell.jsonfile has been updated with new words. While this is helpful for avoiding false positives during spell checks, ensure that these additions are necessary and relevant to the project. For example:- Words like "asyncio", "deque", "sandboxing", and "SPIFFE" are relevant to the project.
- However, some terms like "Portkey" and "thiserror" may need verification to ensure they are directly related to the project scope.
- The
-
Documentation Style Consistency:
- Ensure that the documentation adheres to the project's style guidelines. Consistent formatting, terminology, and examples will improve readability and usability.
Summary
This PR primarily focuses on documentation updates, which are critical for user adoption and understanding of the .NET MCP security primitives. While no direct security or breaking issues were identified, the suggestions provided aim to enhance the quality and utility of the documentation. Ensure that the documentation is thoroughly reviewed for accuracy and alignment with the actual implementation before merging.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a6c0138 to
eb0dee4
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Feedback on Pull Request: docs(dotnet): add MCP security documentation and examples
General Observations
This PR primarily updates documentation and adds examples for .NET MCP (Microsoft Cloud Policy) security primitives. While it doesn't directly impact the Python codebase, it is important to ensure that the documentation aligns with the security and compliance standards of the project.
🔴 CRITICAL
No critical security issues were identified in this PR.
🟡 WARNING
- Potential Breaking Change in Documentation Dependencies
- The
.cspell.jsonfile has been updated with additional words. While this is not a breaking change in itself, if other parts of the repository rely on this file for spell-checking or CI validation, these changes could inadvertently cause issues in unrelated workflows. - Action: Ensure that all workflows using
.cspell.jsonare tested to confirm compatibility with the updated word list.
- The
💡 SUGGESTIONS
-
Ensure OWASP MCP Cheat Sheet Compliance
- The PR mentions mapping to the OWASP MCP Cheat Sheet. While this is a documentation update, it is critical to ensure that the examples and quickstart guides are fully aligned with OWASP's best practices.
- Action: Perform a thorough review of the examples to confirm they do not inadvertently introduce insecure patterns or anti-patterns.
-
Add Security-Specific Examples
- The documentation could benefit from additional examples that explicitly demonstrate secure usage of MCP primitives, such as:
- Proper handling of sensitive data.
- Secure configuration of cryptographic operations.
- Avoidance of common pitfalls like hardcoded credentials or insecure defaults.
- Action: Add at least one example for each of the above scenarios to strengthen the documentation.
- The documentation could benefit from additional examples that explicitly demonstrate secure usage of MCP primitives, such as:
-
Internationalization (i18n) Validation
- The PR mentions i18n translations. Ensure that the translations are accurate and that they do not inadvertently misrepresent security-critical information.
- Action: If possible, involve native speakers or use automated tools to validate the translations.
-
Spell-Checking Additions
- The
.cspell.jsonfile has been updated with new words. While this is helpful for avoiding false positives in spell-checking, some of the added words (e.g., "backdoors", "cmdshell", "sandboxing") are security-sensitive terms. - Action: Review the added words to ensure they are necessary and appropriate for inclusion.
- The
-
Documentation Testing
- While this PR focuses on documentation, it is important to test the examples provided to ensure they are functional and secure.
- Action: Run all provided examples in a controlled environment to validate their correctness and security.
Summary
This PR is a documentation update and does not introduce direct changes to the Python codebase or the policy engine. However, it is essential to ensure that the documentation aligns with the project's security and compliance standards. The suggestions provided aim to enhance the quality and security of the documentation.
Let me know if you need further assistance!
|
Closing — PR #910 already added the cspell words needed for the OWASP/MCP documentation. This PR also removes the trailing newline from .cspell.json which would cause a ruff W292 equivalent. Thank you for the cleanup effort. |
Description
Adds comprehensive documentation for .NET MCP security primitives, including API reference docs, OWASP MCP Cheat Sheet compliance mapping, quickstart guides, i18n translations, and usage examples.
Type of Change
Package(s) Affected
Checklist
Related Issues
Supersedes #775 (split for easier review). Merge after #824.