feat: add MCP server allowlist/blocklist and plugin trust tiers (#425, #426)#434
Conversation
ADO requires service connection names at compile time for task authorization. Runtime variables cannot be used for connectedservicename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…osoft#425, microsoft#426) MCP Server Allowlist/Blocklist (microsoft#425): - MarketplacePolicy model with MCPServerPolicy (allowlist/blocklist mode) - evaluate_plugin_compliance() checks manifests against policy - PluginRegistry enforces policy on register() — rejects non-compliant plugins - CLI: agent-marketplace evaluate --marketplace-policy <path> --manifest <path> - load_marketplace_policy() for YAML policy files - 35 new tests Plugin Trust Tiers (microsoft#426): - 5-tier trust model (revoked/probationary/standard/trusted/verified, 0-1000) - PluginTrustConfig with per-tier capability limits (token budget, tool calls, access level) - PluginTrustStore — file-backed JSON persistence with event recording - compute_initial_score() based on manifest quality (signature, capabilities) - filter_capabilities() gates capabilities by trust tier - CLI: agent-marketplace trust <plugin-name> - 43 new tests Total: 135 tests passing. Closes microsoft#425 Closes microsoft#426 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Agent: breaking-change-detector — Summary🔍 API Compatibility ReportSummaryThis pull request introduces new features to the Findings
Migration GuideNo migration steps are required as no breaking changes were introduced. Notes
✅ No breaking changes detected. |
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
Type HintsAll new public APIs appear to have complete type annotations. ✅ Example CodeNo updates to Please address the above issues and suggestions to ensure documentation remains in sync with the codebase. Let me know if you need further clarification! |
🤖 AI Agent: test-generator — `packages/agent-marketplace/src/agent_marketplace/__init__.py`🧪 Test Coverage Analysis
|
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces two major features to the agent-marketplace package: an MCP Server Allowlist/Blocklist mechanism and Plugin Trust Tiers. These features are critical for enhancing the security and governance of the marketplace. The implementation appears to be well-structured, with clear documentation, comprehensive test coverage, and adherence to the existing codebase's style and conventions. However, there are a few areas that require attention to ensure robustness, security, and maintainability.
🔴 CRITICAL
-
Insufficient Validation for MCP Server Policy:
- The
MCPServerPolicyclass does not validate whether theallowedandblockedlists contain valid MCP server names (e.g., valid domain names or IP addresses). This could lead to misconfigurations or potential security issues. - Action: Add validation logic to ensure that the
allowedandblockedlists contain valid MCP server names. Consider using a regex or a domain validation library.
- The
-
Potential Sandbox Escape via Plugin Capabilities:
- The
filter_capabilitiesfunction uses a static_TIER_CAPABILITY_REQUIREMENTSdictionary to determine which capabilities are allowed for a given tier. However, this mechanism does not account for dynamic or user-defined capabilities, which could lead to a sandbox escape if a malicious plugin declares an unexpected capability. - Action: Implement stricter validation for capabilities, ensuring that only predefined, explicitly allowed capabilities are processed. Consider raising an error for any unrecognized capabilities.
- The
-
Lack of Cryptographic Signature Verification:
- While the
evaluate_plugin_compliancefunction checks for the presence of a signature ifrequire_signatureis enabled, it does not verify the validity of the signature. This could allow plugins with invalid or tampered signatures to bypass the policy. - Action: Integrate cryptographic signature verification into the compliance evaluation process. Ensure that the signature is validated against a trusted public key.
- While the
-
Trust Store Integrity:
- The
PluginTrustStoreclass relies on a JSON file for persistence but does not implement any integrity checks (e.g., cryptographic hashing or signing) to detect tampering with the file. - Action: Add integrity checks to the trust store file. For example, compute a hash of the file's contents and store it separately, or use a digital signature to verify the file's authenticity.
- The
🟡 WARNING
-
Breaking Change in
PluginRegistry.register:- The
registermethod inPluginRegistrynow requires an additionalmcp_serversargument. This is a breaking change for any existing code that calls this method without the new argument. - Action: Make the
mcp_serversargument optional by providing a default value ofNone. Update the method's docstring to reflect this change.
- The
-
Backward Compatibility for Trust Store:
- The
PluginTrustStoreclass introduces a new JSON-based storage format. If this replaces an existing storage mechanism, it could break compatibility with older versions of the library. - Action: Provide a migration path or a compatibility layer for users upgrading from older versions of the library.
- The
💡 SUGGESTIONS
-
Improved Error Messages:
- The error messages in
evaluate_plugin_complianceand other functions are clear but could include more actionable details. For example, when a plugin violates the MCP server policy, the error message could suggest how to fix the issue (e.g., "Add the following MCP servers to the allowlist: ..."). - Action: Enhance error messages to provide actionable guidance for resolving policy violations.
- The error messages in
-
Thread Safety in
PluginTrustStore:- The
PluginTrustStoreclass is not thread-safe, which could lead to race conditions if multiple threads attempt to read/write to the trust store simultaneously. - Action: Add thread safety mechanisms, such as file locks or a threading lock, to ensure safe concurrent access to the trust store.
- The
-
Unit Test Coverage for Edge Cases:
- While the PR mentions 135 tests, it is unclear if edge cases (e.g., malformed MCP server names, invalid YAML policies, etc.) are adequately covered.
- Action: Review the test suite to ensure that edge cases are tested, particularly for the
load_marketplace_policy,evaluate_plugin_compliance, andPluginTrustStoremethods.
-
Dynamic Configuration for Trust Tiers:
- The trust tier configurations are currently hardcoded in
DEFAULT_TIER_CONFIGS. This could limit flexibility for marketplace operators who may want to customize these settings. - Action: Allow the trust tier configurations to be loaded from an external configuration file or database, with the hardcoded values serving as defaults.
- The trust tier configurations are currently hardcoded in
-
Logging Improvements:
- The logging in
PluginTrustStore.record_eventis useful but could be extended to include more context, such as the plugin's current tier and the reason for the score adjustment. - Action: Enhance logging to include additional context, such as the plugin's current tier and the reason for the score adjustment.
- The logging in
-
Documentation:
- While the docstrings are generally well-written, there is no high-level documentation explaining how to use the new features.
- Action: Update the project's documentation to include a detailed guide on configuring and using the MCP Server Allowlist/Blocklist and Plugin Trust Tiers features.
-
Performance Considerations:
- The
evaluate_plugin_compliancefunction performs multiple list operations (e.g.,not in,append, etc.) that could become a bottleneck for large policies or manifests with many MCP servers. - Action: Optimize the function by using sets for membership checks and other operations that can benefit from constant-time lookups.
- The
Summary of Actionable Feedback
Critical Issues
- Add validation for MCP server names in
MCPServerPolicy. - Implement stricter validation for plugin capabilities to prevent sandbox escapes.
- Add cryptographic signature verification in
evaluate_plugin_compliance. - Implement integrity checks for the
PluginTrustStorefile.
Warnings
- Make
mcp_serversargument inPluginRegistry.registeroptional to avoid breaking changes. - Provide a migration path for the new trust store format if it replaces an existing mechanism.
Suggestions
- Improve error messages to provide actionable guidance.
- Add thread safety mechanisms to
PluginTrustStore. - Ensure edge cases are covered in the test suite.
- Allow dynamic configuration of trust tier settings.
- Enhance logging for better debugging and auditability.
- Optimize
evaluate_plugin_compliancefor better performance with large datasets.
Overall Assessment
The PR introduces valuable features that enhance the security and governance capabilities of the agent-marketplace package. However, addressing the critical issues and warnings is essential to ensure the robustness and backward compatibility of the implementation. The suggestions, while not mandatory, would further improve the quality and usability of the code.
🤖 AI Agent: security-scanner — Security Review for PR #425 and #426Security Review for PR #425 and #426This pull request introduces two new features to the Findings1. 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
Overall Risk Assessment: 🟠 HIGHThe proposed changes introduce critical governance features, but there are several areas where security can be improved to ensure robust protection against potential attacks. Addressing the identified issues will significantly enhance the security posture of the Let me know if you'd like me to review specific sections in more detail or provide additional recommendations. |
Description
Two marketplace governance features:
MCP Server Allowlist/Blocklist (#425)
Marketplace operators can restrict which MCP servers plugins are allowed to use.
Plugin Trust Tiers (#426)
Progressive trust levels for plugins based on agentmesh trust scoring.
Total: 135 tests passing
Closes #425, Closes #426