feat(openshell): add governance skill package and runnable example

[Imran Siddique (imran-siddique)]

Adds code + working demo for the OpenShell integration.

Skill package (packages/agentmesh-integrations/openshell-skill/): GovernanceSkill class, 9 tests.
Example (examples/openshell-governed/): 6-scenario demo, trust decay.
Docs: Updated links in docs/integrations/openshell.md.

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces a new governance skill package for integrating with NVIDIA's OpenShell sandbox. It includes the GovernanceSkill class, a runnable example, and associated documentation. The implementation is a significant addition to the repository, but there are several areas that require attention to ensure correctness, security, and maintainability.

---

🔴 CRITICAL

1. Policy Bypass via Regex Matching

   • The _match method in GovernanceSkill uses re.search for the matches operator. This can lead to unintended matches if the regex is not properly anchored. For example, the regex shell:(rm|dd|mkfs) could match shell:echo rm or shell:ls | grep dd.
   • Action: Ensure that all regex patterns are properly anchored (e.g., ^ and $ for start and end of string) to avoid partial matches. Alternatively, consider using a safer matching mechanism.

2. Trust Score Manipulation

   • The adjust_trust method allows arbitrary adjustments to the trust score without any validation or constraints. This could be exploited to bypass governance checks by artificially inflating the trust score.
   • Action: Add constraints to ensure that trust scores remain within a valid range (e.g., 0.0 to 1.0). Consider logging or alerting when trust scores are adjusted significantly.

3. Lack of Input Validation

   • The check_policy method does not validate the action or context inputs. This could lead to unexpected behavior or errors if malformed inputs are provided.
   • Action: Use Pydantic models to validate the structure and types of action and context inputs.

4. Potential Sandbox Escape

   • The check_policy method relies on the action string to determine the policy decision. If the action string is manipulated (e.g., by encoding malicious commands), it could bypass the policy checks.
   • Action: Implement stricter parsing and validation of the action string. Consider splitting the action into structured fields (e.g., type, command, path) and validating each field separately.

---

🟡 WARNING

1. Breaking Changes in Public API

   • The GovernanceSkill class has been modified, but the changes are not documented. For example, the load_policies method now returns the number of rules loaded, which is a change in behavior.
   • Action: Document all changes to the public API in the changelog and ensure backward compatibility where possible.

2. Policy Format Changes

   • The sandbox-policy.yaml file has been modified to use a more compact YAML format. While this is not inherently a breaking change, it could cause issues for users who rely on the previous format.
   • Action: Provide a migration guide or backward compatibility for the old policy format.

---

💡 Suggestions

1. Improve Code Readability

   • The _match method and other parts of the code use overly compact syntax, which reduces readability.
   • Action: Use more descriptive variable names and expand inline conditionals for better readability.

2. Enhance Test Coverage

   • The test suite is missing edge cases, such as:
     • Invalid action strings (e.g., empty strings, malformed actions).
     • Invalid context structures.
     • Policies with missing or malformed fields.
   • Action: Add tests for these edge cases to ensure robustness.

3. Thread Safety

   • The GovernanceSkill class uses a dictionary (_trust_scores) to store trust scores, but there is no synchronization mechanism. This could lead to race conditions in a multi-threaded environment.
   • Action: Use thread-safe data structures or locks to ensure thread safety.

4. Audit Log Security

   • The audit log is stored in memory and does not appear to have any size limits or persistence mechanism. This could lead to memory exhaustion or loss of audit data.
   • Action: Implement a mechanism to limit the size of the audit log and consider adding an option to persist logs to disk or a secure logging service.

5. Documentation Improvements

   • The documentation for the GovernanceSkill class and its methods is minimal.
   • Action: Add detailed docstrings for all public methods, including parameter descriptions, return types, and examples.

6. Code Style

   • The code contains several instances of overly compact or inconsistent formatting (e.g., inline conditionals, missing spaces, inconsistent naming).
   • Action: Use black or another code formatter to ensure consistent formatting. Address issues flagged by ruff.

7. Error Handling

   • The load_policies method silently skips invalid YAML files, which could lead to unexpected behavior.
   • Action: Log a warning or raise an exception when an invalid YAML file is encountered.

8. Backward Compatibility

   • The pyproject.toml file has been modified to remove some metadata (e.g., classifiers, keywords). While this is not a breaking change for the library, it reduces the discoverability of the package.
   • Action: Restore the removed metadata to maintain the package's visibility and compliance with Python packaging standards.

9. Policy Rule Priority

   • The priority field in _PolicyRule is used to sort rules, but there is no validation to ensure that priorities are unique or within a specific range.
   • Action: Add validation to ensure that rule priorities are unique or provide guidance in the documentation on how to manage priorities.

---

Summary of Actions

Critical

• Fix regex matching to avoid partial matches.
• Add constraints to the adjust_trust method to prevent arbitrary trust score manipulation.
• Validate action and context inputs using Pydantic models.
• Implement stricter parsing and validation of the action string to prevent sandbox escape.

Warning

• Document all changes to the public API in the changelog.
• Provide backward compatibility or a migration guide for the updated policy format.

Suggestions

• Improve code readability by using descriptive variable names and expanding inline conditionals.
• Add tests for edge cases (e.g., invalid inputs, malformed policies).
• Ensure thread safety for _trust_scores using thread-safe data structures or locks.
• Implement size limits and persistence for the audit log.
• Add detailed docstrings for all public methods.
• Use a code formatter like black and address issues flagged by ruff.
• Add error handling for invalid YAML files in load_policies.
• Restore removed metadata in pyproject.toml.
• Add validation for rule priorities or provide guidance in the documentation.

Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: security-scanner — Security Review of Pull Request: feat(openshell): add governance skill package and runnable example

Security Review of Pull Request: feat(openshell): add governance skill package and runnable example

---

🔴 CRITICAL: Prompt Injection Defense Bypass

Issue

The sandbox-policy.yaml file in the runnable example changes the condition operator for file read and write rules from starts_with to matches. The matches operator uses regular expressions, which introduces the risk of prompt injection attacks if the regex patterns are not properly validated or sanitized. For example, a malicious input like file:read../../etc/passwd could bypass the policy if the regex is not strict enough to prevent directory traversal.

Attack Vector

An attacker could craft input that matches the regex pattern but bypasses the intended restrictions. For example:

• The regex ^file:read would allow file:read../../etc/passwd to pass, potentially exposing sensitive system files.
• Similarly, ^file:write:/workspace could allow file:write:/workspace/../../etc/shadow to overwrite critical system files.

Fix

1. Replace the matches operator with a more restrictive operator like starts_with or equals wherever possible.
2. If regex is absolutely necessary, ensure the patterns are strictly defined to prevent bypasses. For example:

   condition: {field: action, operator: matches, value: "^file:read:/workspace/.*$"}

3. Add unit tests to validate that malicious inputs (e.g., directory traversal attempts) are blocked.

---

🔴 CRITICAL: Policy Engine Circumvention

Issue

The provided sandbox-policy.yaml file does not include a deny-all fallback rule. This omission creates a significant risk of policy circumvention. If no rule explicitly matches an action, the action may be implicitly allowed, depending on the policy engine's default behavior.

Attack Vector

An attacker could exploit this by crafting an action that doesn't match any of the defined rules. For example:

• If an action like file:delete:/workspace is not explicitly denied, it might be allowed by default, enabling destructive operations.

Fix

Add a deny-all fallback rule with the lowest priority to ensure that any action not explicitly allowed is denied:

- name: deny-all
  condition: {field: action, operator: matches, value: ".*"}
  action: deny
  priority: 0

---

🟠 HIGH: Trust Chain Weaknesses

Issue

The PR does not include any mention of SPIFFE/SVID validation or certificate pinning in the context of the OpenShell integration. Since OpenShell is described as a "runtime sandbox for AI agents," it is critical to ensure that the trust chain between the governance toolkit and OpenShell is robust.

Attack Vector

If the trust chain is not properly validated, an attacker could impersonate the OpenShell runtime or perform a man-in-the-middle (MITM) attack to inject malicious policies or actions.

Fix

1. Ensure that SPIFFE/SVID validation is implemented and tested in the GovernanceSkill class.
2. Add certificate pinning to prevent MITM attacks.
3. Include unit tests to verify that invalid certificates or SVIDs are rejected.

---

🟠 HIGH: Credential Exposure

Issue

The PR does not explicitly mention how sensitive information (e.g., API keys, tokens) is handled in the OpenShell integration. If credentials are logged or exposed in error messages, it could lead to unauthorized access.

Attack Vector

An attacker could exploit exposed credentials to gain unauthorized access to the OpenShell runtime or other integrated systems.

Fix

1. Ensure that all logging and error messages redact sensitive information.
2. Add automated tests to verify that no secrets are exposed in logs or error outputs.

---

🟡 MEDIUM: Sandbox Escape

Issue

The sandbox-policy.yaml file includes a rule to allow "safe shell" actions but does not define what constitutes a "safe shell." This ambiguity could lead to sandbox escape if unsafe shell commands are inadvertently allowed.

Attack Vector

An attacker could exploit this by crafting a shell command that appears safe but executes malicious actions. For example:

• A command like ls; rm -rf / could bypass a naive "safe shell" check.

Fix

1. Explicitly define what "safe shell" means in the policy.
2. Use a whitelist approach to allow only specific, vetted shell commands.
3. Add tests to ensure that unsafe commands are blocked.

---

🟡 MEDIUM: Supply Chain Risks

Issue

The PR introduces a new dependency on OpenShell but does not include any verification of the dependency's integrity (e.g., checksum validation, signature verification).

Attack Vector

An attacker could compromise the OpenShell repository or publish a malicious version to a package registry, leading to a supply chain attack.

Fix

1. Use checksum validation or signature verification for the OpenShell dependency.
2. Monitor the OpenShell repository for security advisories or suspicious activity.

---

Summary of Findings

Finding	Severity	Fix Summary
Prompt injection defense bypass	🔴 CRITICAL	Use strict regex patterns or replace matches with starts_with/equals.
Policy engine circumvention	🔴 CRITICAL	Add a deny-all fallback rule to the policy file.
Trust chain weaknesses	🟠 HIGH	Implement SPIFFE/SVID validation and certificate pinning.
Credential exposure	🟠 HIGH	Redact sensitive information in logs and error messages.
Sandbox escape	🟡 MEDIUM	Define "safe shell" explicitly and use a whitelist approach.
Supply chain risks	🟡 MEDIUM	Verify the integrity of the OpenShell dependency.

---

Recommendations for Approval

• Address the CRITICAL and HIGH severity issues before merging.
• Consider fixing the MEDIUM severity issues to improve overall security posture.
• Add unit tests to validate fixes and prevent regressions.

[github-actions]

🤖 AI Agent: code-reviewer

Pull Request Review: feat(openshell): add governance skill package and runnable example

---

🔴 CRITICAL

1. Policy Engine Correctness:

   • The _match function in GovernanceSkill is critical for policy enforcement. However, the implementation of the matches operator uses re.search, which could lead to unintended matches if the regex is too permissive or improperly constructed. This could result in false negatives, allowing potentially harmful actions to bypass the policy.
     • Recommendation: Validate and sanitize the value field in the policy rules to ensure it is a safe and intended regex pattern. Consider using re.fullmatch instead of re.search to ensure the entire string matches the pattern.

2. Trust/Identity:

   • The agent_did is used as a key for trust scores, but there is no validation to ensure its integrity or authenticity. This could allow an attacker to spoof an agent's identity and manipulate trust scores.
     • Recommendation: Implement a mechanism to validate the authenticity of agent_did, such as using cryptographic signatures or integrating with a trusted identity provider (e.g., SPIFFE/SVID).

3. Sandbox Escape Vectors:

   • The os.system call in delete_all (in github-reviewer/main.py) is commented as a placeholder, but it is still a potential security risk if uncommented or misused.
     • Recommendation: Replace os.system with a safer alternative, such as subprocess.run, and ensure proper input sanitization and validation.

4. SQL Injection:

   • The NL_TO_SQL_TEMPLATES in sql-agent/main.py contains SQL queries that use string interpolation. This is a critical security risk as it could lead to SQL injection attacks.
     • Recommendation: Replace all string-interpolated SQL queries with parameterized queries. For example, use cursor.execute("SELECT * FROM table WHERE column = ?", (value,)).

---

🟡 WARNING

1. Backward Compatibility:
   • The GovernanceSkill class has been introduced as a new feature. While this is not a breaking change, any future modifications to this class or its methods could potentially break existing integrations.
     • Recommendation: Clearly document the public API of GovernanceSkill and its methods. Use semantic versioning to indicate breaking changes in future releases.

---

💡 SUGGESTIONS

1. Code Readability:

   • The _match function in GovernanceSkill could be refactored for better readability. The current implementation uses a series of if statements, which could be replaced with a dictionary mapping operators to functions.

     @staticmethod
     def _match(operator: str, target: str, value: Any) -> bool:
         operators = {
             "equals": lambda t, v: t == v,
             "starts_with": lambda t, v: t.startswith(str(v)),
             "contains": lambda t, v: str(v) in t,
             "matches": lambda t, v: bool(re.search(str(v), t)),
             "in": lambda t, v: t in (v if isinstance(v, list) else [v]),
         }
         return operators.get(operator, lambda t, v: False)(target, value)

2. Policy Rule Validation:

   • The load_policies method does not validate the structure or completeness of the policy rules. This could lead to runtime errors or unexpected behavior.
     • Recommendation: Add schema validation for the policy YAML files using a library like pydantic or jsonschema.

3. Logging and Audit Trail:

   • The log_action method in GovernanceSkill appends logs to an in-memory list. This could lead to memory issues if the list grows too large.
     • Recommendation: Implement a mechanism to persist logs to a file or database. Additionally, consider adding a maximum size for the in-memory log to prevent memory exhaustion.

4. Thread Safety:

   • The GovernanceSkill class uses a dictionary (_trust_scores) to store trust scores, but there is no synchronization mechanism to ensure thread safety.
     • Recommendation: Use a thread-safe data structure or implement locks to ensure thread safety when accessing or modifying _trust_scores.

5. Documentation:

   • The documentation for the openshell-skill package is minimal and does not provide detailed examples or explanations of the policy rules and their usage.
     • Recommendation: Expand the documentation to include detailed examples of policy rules, how to use the GovernanceSkill class, and best practices for writing secure policies.

6. Test Coverage:

   • The test coverage for the openshell-skill package is not explicitly mentioned. Given the critical nature of the policy engine, comprehensive test coverage is essential.
     • Recommendation: Ensure that all edge cases for the _match function and check_policy method are covered in the tests. Include tests for invalid policy rules and edge cases for each operator.

7. Code Style:

   • The code contains several instances of overly compact or hard-to-read lines, such as the one-liner in the load_policies method. This can make the code harder to maintain.
     • Recommendation: Follow PEP 8 guidelines for line length and code readability. Break down complex lines into multiple lines for clarity.

8. Error Handling:

   • The load_policies method does not handle exceptions that may occur during file reading or YAML parsing.
     • Recommendation: Add error handling to gracefully handle cases where a policy file is malformed or cannot be read.

9. Policy Rule Priority:

   • The priority field in _PolicyRule is used to sort rules, but there is no validation to ensure that priorities are unique or within a specific range.
     • Recommendation: Add validation to ensure that rule priorities are unique and within a reasonable range.

10. Default Trust Score:

    • The default trust score for new agents is hardcoded as 1.0. This might not be suitable for all use cases.
      • Recommendation: Allow the default trust score to be configurable via the GovernanceSkill constructor.

---

Summary

This PR introduces a significant new feature with the openshell-skill package and a runnable example. While the implementation is functional, there are several critical security issues and areas for improvement:

• 🔴 Critical: Address potential regex vulnerabilities in the _match function, validate agent_did, and replace insecure practices like os.system and string-interpolated SQL queries.
• 🟡 Warning: Document the public API of GovernanceSkill to ensure backward compatibility in future updates.
• 💡 Suggestions: Improve code readability, add schema validation for policy rules, enhance logging and audit mechanisms, ensure thread safety, expand documentation, improve test coverage, and address minor code style issues.

Addressing these issues will significantly enhance the security, maintainability, and usability of the new feature. Let me know if you need further clarification or assistance!

[github-actions]

🤖 AI Agent: code-reviewer

Review Summary

This pull request introduces a new governance skill package for OpenShell integration and a runnable example demonstrating its functionality. The changes include updates to the documentation, a new policy YAML file, and the addition of a skill package with tests. Below is a detailed review of the changes, with a focus on the specified areas.

---

🔴 CRITICAL: Security Issues

1. Regex Injection in Policy Matching

   • The change in sandbox-policy.yaml modifies the condition operator from starts_with to matches with regex patterns (^file:read and ^file:write:/workspace). While regex provides more flexibility, it introduces the risk of regex injection if the value field is dynamically generated or user-controlled.
   • Impact: If an attacker can influence the value field, they could craft malicious regex patterns that bypass or disrupt policy enforcement.
   • Recommendation: Ensure that the value field is sanitized and strictly controlled. If regex flexibility is not essential, revert to starts_with for simplicity and security.

2. Sandbox Policy Enforcement

   • The allow-safe-shell rule is mentioned but not shown in the diff. If this rule allows unrestricted shell access, it could lead to sandbox escape vulnerabilities.
   • Impact: Unrestricted shell access could allow malicious agents to execute arbitrary commands, bypassing governance.
   • Recommendation: Review the allow-safe-shell rule to ensure it is tightly scoped. Consider using syscall filtering (e.g., seccomp, Landlock) to enforce stricter sandboxing.

---

🟡 WARNING: Potential Breaking Changes

1. Policy Engine Behavior Change
   • Changing the operator from starts_with to matches alters the behavior of the policy engine. Existing policies relying on starts_with may no longer work as expected.
   • Impact: This could break backward compatibility for users relying on the old behavior.
   • Recommendation: Document this change prominently in the release notes and provide a migration guide for users to update their policies.

---

💡 Suggestions for Improvement

1. Thread Safety in GovernanceSkill

   • The GovernanceSkill class is not shown in the diff, but given its role in enforcing policies, it is likely to be invoked concurrently in multi-agent scenarios.
   • Suggestion: Ensure that the GovernanceSkill implementation is thread-safe. Use thread-safe data structures or synchronization primitives where necessary.

2. OWASP Agentic Top 10 Compliance

   • The OpenShell integration should be reviewed for compliance with the OWASP Agentic Top 10, particularly:
     • A1: Policy Bypass: Ensure policies cannot be bypassed through edge cases or malformed inputs.
     • A4: Trust Decay: The demo mentions "trust decay," but it is unclear how this is implemented. Ensure trust decay mechanisms are robust and cannot be manipulated by malicious agents.
   • Suggestion: Add automated tests to validate compliance with these security principles.

3. Type Safety and Validation

   • The sandbox-policy.yaml file uses structured data that could benefit from validation using Pydantic models.
   • Suggestion: Define a Pydantic model for policy rules and validate the YAML file against this model during loading. This ensures type safety and catches misconfigurations early.

4. Documentation

   • The documentation update is helpful but could include more details about the security implications of using regex in policies.
   • Suggestion: Add a section in openshell.md explaining the trade-offs of using matches vs. starts_with and provide examples of secure regex patterns.

5. Test Coverage

   • The PR mentions 9 tests for the GovernanceSkill class, but it is unclear if they cover edge cases like malformed policies, invalid regex patterns, or concurrent execution.
   • Suggestion: Add tests for:
     • Invalid or malicious regex patterns in policies.
     • Concurrent execution of GovernanceSkill.
     • Edge cases for trust decay and policy enforcement.

---

Summary of Actionable Feedback

🔴 CRITICAL

1. Sanitize and validate the value field in policy rules to prevent regex injection.
2. Review the allow-safe-shell rule to ensure it does not enable sandbox escape.

🟡 WARNING

1. Document the behavior change from starts_with to matches in the policy engine and provide a migration guide.

💡 SUGGESTIONS

1. Ensure thread safety in the GovernanceSkill class.
2. Validate sandbox-policy.yaml using Pydantic models.
3. Add tests for edge cases, including regex injection, malformed policies, and concurrent execution.
4. Expand documentation to include security considerations for regex-based policies.

---

Let me know if you need a deeper review of the GovernanceSkill implementation or additional guidance on any of the points above!