feat: add standalone agent-mcp-governance package#823
feat: add standalone agent-mcp-governance package#823jackbatzner wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
🤖 AI Agent: breaking-change-detector — Summary🔍 API Compatibility ReportSummaryThis pull request introduces a new standalone Python package, Findings
Migration GuideNo migration is required as no breaking changes were introduced. Downstream users can optionally adopt the new Additional Notes
✅ No breaking changes detected. |
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
Additional Notes
Please address the above issues to ensure documentation is fully synchronized with the codebase. Let me know if you need further clarification! |
🤖 AI Agent: test-generator — `packages/agent-mcp-governance/src/agent_mcp_governance/__init__.py`🧪 Test Coverage Analysis
|
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This PR introduces a new standalone package, agent-mcp-governance, which re-exports MCP governance primitives from agent-os-kernel without introducing dependencies on the broader Agent Governance Toolkit (AGT). The package is designed for independent use cases, such as enterprise workflows, and includes a well-documented README, zero dependencies, and a clean API surface.
The PR also introduces new modules and updates to the agent-os package, including:
- Credential Redactor: A utility to detect and redact sensitive information.
- MCP Metrics: OpenTelemetry-friendly metrics helpers for MCP governance components.
- Updates to the
agent-ospackage to support the new package.
The PR is well-structured and adheres to the project's style guidelines. However, there are several areas that need attention, including potential security issues, type safety concerns, and suggestions for improvement.
🔴 CRITICAL: Security Issues
-
Credential Redactor - Incomplete Redaction
- The
CredentialRedactorclass uses regular expressions to detect and redact sensitive information. However, regex-based detection is inherently prone to false negatives and may fail to catch all sensitive data. For example:- The regex for "OpenAI API key" assumes a specific format (
sk-followed by 18+ alphanumeric characters), but this format could change or be bypassed. - The regex for "Bearer token" assumes a specific length and structure, which may not cover all valid tokens.
- The regex for "OpenAI API key" assumes a specific format (
- Recommendation: Consider using a more robust library or approach for detecting sensitive information, such as a machine learning-based solution or integrating with existing tools like
truffleHogordetect-secrets.
- The
-
Credential Redactor - Logging Redacted Information
- The
CredentialRedactor.redactmethod logs the number of redactions applied. If logging is not properly configured, this could inadvertently expose sensitive information. - Recommendation: Avoid logging redaction counts or ensure that logging is appropriately secured and sanitized.
- The
-
MCP Metrics - OpenTelemetry Integration
- The
MCPMetricsclass initializes OpenTelemetry counters but does not validate the integrity of the metrics data. If an attacker manipulates the metrics input, it could lead to incorrect or misleading telemetry data. - Recommendation: Add validation and sanitization for all inputs to the
MCPMetricsmethods to ensure they conform to expected formats and ranges.
- The
-
MCP Gateway - Policy Enforcement
- The
MCPGatewayclass is responsible for policy enforcement, but the PR does not include sufficient test coverage to ensure that policies are enforced correctly. This could lead to potential security bypasses. - Recommendation: Add comprehensive tests to verify that the
MCPGatewaycorrectly enforces all policy rules, including edge cases.
- The
🟡 WARNING: Potential Breaking Changes
-
Python Version Requirement
- The new package requires Python 3.12 or higher (
requires-python = ">=3.12"). This is a breaking change for users on older Python versions. - Recommendation: Clearly communicate this requirement in the release notes and documentation.
- The new package requires Python 3.12 or higher (
-
Public API Changes in
agent-os- The
agent-ospackage now re-exports several MCP-related classes and functions. This could lead to conflicts or unexpected behavior for existing users. - Recommendation: Document these changes in the release notes and consider versioning the
agent-ospackage to indicate the update.
- The
💡 Suggestions for Improvement
-
Type Annotations
- While the codebase includes type annotations, some methods (e.g.,
CredentialRedactor.redact_data_structure) could benefit from more precise type hints. - Recommendation: Use
typing.Union,typing.Optional, and other type hinting features to improve type safety and clarity.
- While the codebase includes type annotations, some methods (e.g.,
-
Test Coverage
- The PR includes tests, but the coverage for some critical components (e.g.,
MCPGateway,CredentialRedactor) could be improved. - Recommendation: Add tests for edge cases, such as malformed inputs, large payloads, and unusual credential formats.
- The PR includes tests, but the coverage for some critical components (e.g.,
-
Documentation
- The README for the new package is clear and well-written, but it could benefit from additional examples, such as how to use the
MCPMetricsandCredentialRedactorclasses. - Recommendation: Expand the documentation to include more usage examples and best practices.
- The README for the new package is clear and well-written, but it could benefit from additional examples, such as how to use the
-
Thread Safety
- The
MCPMetricsclass uses OpenTelemetry counters, which are generally thread-safe. However, the implementation does not explicitly document its thread safety guarantees. - Recommendation: Add documentation to clarify the thread safety of the
MCPMetricsclass and its methods.
- The
-
Backward Compatibility
- While the new package is designed to be standalone, it re-exports classes from
agent-os-kernel. Any future changes toagent-os-kernelcould inadvertently breakagent-mcp-governance. - Recommendation: Implement integration tests to ensure that
agent-mcp-governanceremains compatible with updates toagent-os-kernel.
- While the new package is designed to be standalone, it re-exports classes from
-
Error Handling
- The
MCPMetricsclass uses a no-op implementation when OpenTelemetry is unavailable. While this is a reasonable fallback, it might lead to silent failures. - Recommendation: Log a warning or provide a mechanism to notify users when OpenTelemetry is not available.
- The
Summary of Actionable Feedback
Critical Issues
- 🔴 Improve the robustness of the
CredentialRedactorto avoid false negatives. - 🔴 Avoid logging sensitive information or redaction counts in
CredentialRedactor. - 🔴 Add input validation to
MCPMetricsmethods to prevent telemetry manipulation. - 🔴 Add comprehensive tests for
MCPGatewayto ensure policy enforcement correctness.
Warnings
- 🟡 Communicate the Python 3.12+ requirement as a breaking change.
- 🟡 Document the new MCP-related exports in
agent-osto avoid potential conflicts.
Suggestions
- 💡 Improve type annotations for better type safety.
- 💡 Expand test coverage for critical components.
- 💡 Add more usage examples to the documentation.
- 💡 Clarify thread safety guarantees for
MCPMetrics. - 💡 Add integration tests to ensure compatibility between
agent-mcp-governanceandagent-os-kernel. - 💡 Notify users when OpenTelemetry is unavailable in
MCPMetrics.
This PR is a significant step forward in modularizing the MCP governance functionality, but addressing the above issues will ensure better security, reliability, and usability.
🤖 AI Agent: security-scanner — Security Review of `feat: add standalone agent-mcp-governance package`Security Review of
|
| Category | Severity | Description |
|---|---|---|
| Prompt Injection Defense | 🔵 LOW | Placeholder matches_pattern method and incomplete credential patterns. |
| Policy Engine Circumvention | 🟠 HIGH | Lack of policy validation and type checking. |
| Trust Chain Weaknesses | 🔵 LOW | No evidence of issues, but underlying implementation is not reviewed in this PR. |
| Credential Exposure | 🔴 CRITICAL | Logging redaction counts could expose sensitive information. |
| Sandbox Escape | 🔵 LOW | No evidence of issues, but external tools should be sandboxed. |
| Deserialization Attacks | 🟠 HIGH | Potential for ReDoS attacks in CredentialRedactor regex patterns. |
| Race Conditions | 🟡 MEDIUM | In-memory rate limiting may be vulnerable to race conditions in distributed environments. |
| Supply Chain | 🔵 LOW | No direct dependencies, but indirect dependency on agent-os-kernel needs regular auditing. |
Overall Risk Assessment: HIGH
The introduction of this standalone package is a significant change that requires careful attention to security. The most critical issue is the potential for credential exposure through logging. Additionally, the lack of policy validation and the potential for ReDoS attacks in the CredentialRedactor class are concerning. Addressing these issues is crucial to ensure the security of the MCP governance layer.
Actionable Recommendations
-
Prompt Injection Defense:
- Implement a robust
matches_patternmethod in theDemoPolicyclass or provide clear documentation for users to implement it securely. - Regularly update the
CredentialRedactor.PATTERNSlist and allow users to define custom patterns.
- Implement a robust
-
Policy Engine Validation:
- Add a validation mechanism to ensure user-defined policies meet minimum security requirements.
- Implement runtime type checks for policy attributes.
-
Logging and Credential Exposure:
- Remove or sanitize logging of redaction counts in
CredentialRedactorto avoid potential credential exposure.
- Remove or sanitize logging of redaction counts in
-
Deserialization Attacks:
- Use a regex library with built-in ReDoS protection or implement timeouts for regex matching.
- Validate input length and format before processing with regex.
-
Rate Limiting:
- Consider using a distributed rate-limiting mechanism (e.g., Redis) for production environments.
- Add thread-safety mechanisms to in-memory stores.
-
Supply Chain Security:
- Regularly audit the
agent-os-kernelpackage and its dependencies for vulnerabilities. - Use a dependency scanning tool to monitor for new vulnerabilities.
- Regularly audit the
-
Documentation:
- Clearly document the limitations and security considerations of the standalone package, especially for user-defined policies and credential redaction.
By addressing these issues, the agent-mcp-governance package can provide a secure and reliable foundation for MCP governance in enterprise environments.
Description
Introduces the standalone
agent-mcp-governancePython package with zero AGT dependency. This allows MCP server operators to adopt MCP governance primitives without pulling in the full Agent Governance Toolkit.Package:
agent-mcp-governance(PyPI name:agent_mcp_governance)dependencies = []— no AGT dependencyType of Change
Package(s) Affected
Checklist
Related Issues
Supersedes #774 (split for easier review). Merge after #822.