docs: add policy-as-code tutorial chapter 5 (approval workflows)#911
Conversation
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Feedback on Pull Request: docs: add policy-as-code tutorial chapter 5 (approval workflows)
🔴 CRITICAL: Security Concerns
-
Escalation Timeout Default Action
- The default timeout action is set to
DENY, which is a safe default. However, the tutorial suggests experimenting withDefaultTimeoutAction.ALLOW. This could lead to unintended consequences if users adopt this setting for critical actions. The tutorial should explicitly warn against usingALLOWfor sensitive actions and emphasize the importance of failing safely.
Actionable Recommendation: Add a warning in the tutorial that
DefaultTimeoutAction.ALLOWshould only be used for non-critical actions where escalation is optional. - The default timeout action is set to
-
Audit Trail Integrity
- The tutorial mentions that the audit trail records who approved or denied requests, but it does not discuss mechanisms to ensure the integrity of this audit trail. If the
InMemoryApprovalQueueis used in production, it could be vulnerable to tampering or loss of data.
Actionable Recommendation: Clearly state that
InMemoryApprovalQueueis for demonstration purposes only and recommend using a secure, persistent backend (e.g., a database with access controls or a blockchain-based ledger) for production environments. - The tutorial mentions that the audit trail records who approved or denied requests, but it does not discuss mechanisms to ensure the integrity of this audit trail. If the
-
Escalation Request Spoofing
- The
EscalationHandlerdoes not appear to validate theagent_idoractionfields in the escalation request. This could allow malicious agents to spoof requests or escalate actions they are not authorized to perform.
Actionable Recommendation: Add validation logic to ensure that the
agent_idandactionfields in escalation requests are legitimate and authorized. This could involve checking against a registry of known agents and their allowed actions. - The
🟡 WARNING: Potential Breaking Changes
-
Policy Format Changes
- The introduction of the "requires human approval" keyword in policy messages is a new convention. If users update their policies without including this keyword, actions intended for escalation may be denied outright.
Actionable Recommendation: Provide backward compatibility by allowing users to configure the escalation keyword or fallback behavior for policies missing the keyword.
💡 Suggestions for Improvement
-
Concurrency and Thread Safety
- The
InMemoryApprovalQueueimplementation does not appear to handle concurrent access. In a multi-threaded or distributed environment, this could lead to race conditions or inconsistent states.
Actionable Recommendation: Implement thread-safe mechanisms (e.g., locks) for the
InMemoryApprovalQueueor recommend using a distributed queue system for production use. - The
-
Quorum-Based Approval
- The tutorial briefly mentions quorum-based approval but does not provide an example. This is a critical feature for high-stakes actions requiring multiple sign-offs.
Actionable Recommendation: Add an example demonstrating quorum-based approval using
QuorumConfigto help users understand how to implement it. -
Policy Testing
- The tutorial mentions the importance of automated policy testing but defers it to a future chapter. Given the criticality of policy correctness, users should be encouraged to start testing immediately.
Actionable Recommendation: Provide a basic example of policy testing in this chapter, even if the full details are covered later.
-
Type Safety and Validation
- While the tutorial uses
PolicyDocument.from_yaml()to load policies, it does not explicitly mention whether the YAML schema is validated. This could lead to runtime errors if the YAML is malformed.
Actionable Recommendation: Ensure that the
PolicyDocumentclass uses Pydantic models for schema validation and document this feature in the tutorial. - While the tutorial uses
-
Sandbox Escape Vectors
- The tutorial does not address potential sandbox escape vectors for the agent during escalation. For example, if the agent can modify its own
agent_idoraction, it could bypass restrictions.
Actionable Recommendation: Include a section in the tutorial discussing how to secure the agent's runtime environment and prevent unauthorized modifications to escalation requests.
- The tutorial does not address potential sandbox escape vectors for the agent during escalation. For example, if the agent can modify its own
-
Improved Error Handling
- The tutorial does not cover error handling for cases where the escalation backend is unavailable or fails to process requests.
Actionable Recommendation: Add examples of error handling and fallback mechanisms for escalation workflows.
Summary of Changes Needed
- 🔴 Add warnings and safeguards against insecure configurations (e.g.,
DefaultTimeoutAction.ALLOW). - 🔴 Ensure audit trail integrity and validate escalation requests to prevent spoofing.
- 🟡 Provide backward compatibility for policies missing the escalation keyword.
- 💡 Improve thread safety, add quorum-based approval examples, and introduce basic policy testing.
- 💡 Enhance documentation on type safety, sandbox security, and error handling.
This tutorial is well-written and provides valuable insights into human-in-the-loop escalation workflows. Addressing the above concerns will ensure its robustness and security for production use.
🤖 AI Agent: security-scanner — Security Analysis of the Pull RequestSecurity Analysis of the Pull RequestThis pull request primarily introduces a new tutorial chapter (Chapter 5) on implementing approval workflows in the policy-as-code framework. It includes documentation, a YAML policy file, and a Python script demonstrating the implementation of a three-tier policy system (allow, deny, escalate). Below is a detailed security analysis based on the specified criteria: 1. 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
Summary of Findings
General Recommendations
This pull request introduces a useful feature for human-in-the-loop approval workflows, but it also introduces several potential security risks that need to be addressed before deployment in a production environment. |
There was a problem hiding this comment.
Pull request overview
Adds Chapter 5 (“Approval Workflows”) to the policy-as-code tutorial series, introducing human-in-the-loop escalation (approve/deny/timeout) with accompanying runnable Python and YAML examples, plus tutorial navigation updates.
Changes:
- Add new Chapter 5 markdown tutorial covering escalation requests, human decisions, and timeout-safe defaults.
- Add Chapter 5 runnable Python demo and YAML example policy.
- Update tutorial README and Chapter 4 “Next” link to include Chapter 5.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/tutorials/policy-as-code/README.md | Links Chapter 5 and updates “coming soon” notice. |
| docs/tutorials/policy-as-code/04-conditional-policies.md | Updates Next navigation to point to Chapter 5. |
| docs/tutorials/policy-as-code/05-approval-workflows.md | New Chapter 5 tutorial text, flow, and exercises for approval workflows. |
| docs/tutorials/policy-as-code/examples/05_approval_policy.yaml | New example policy illustrating allow/deny + “escalation-worthy” actions. |
| docs/tutorials/policy-as-code/examples/05_approval_workflows.py | New runnable demo showing escalation queue/handler, approve/deny, and timeout behavior. |
|
Here are my decisions for chapter 5: For escalation I went with a message-field convention. Rules set action: deny with "requires human approval" in the message, and the Python code picks that up to route to the EscalationHandler. The four existing actions are all accounted for and the require_human_approval flag on GovernancePolicy is all-or-nothing, so this felt like the cleanest path. If you'd want to have a proper "action: escalate" in PolicyAction, I kept things flexible enough that only the YAML and classify() would need to change. Regarding quorum, I mentioned it in this tutorial but held off on the demo. Since the approve() method for the queue never populates the votes, so resolve() would result in a timeout. So I didn't want to mention it. The script simulates both sides in one place. |
Introduce human-in-the-loop escalation as a third decision tier between allow and deny. The tutorial covers creating escalation requests, human approve/deny flows, and timeout with safe defaults. New files: - 05-approval-workflows.md: tutorial narrative with bank teller analogy, five steps, ASCII flowchart, and exercises - examples/05_approval_policy.yaml: three-tier policy (allow, deny, escalate) - examples/05_approval_workflows.py: runnable demo with five parts Updated files: - README.md: link chapter 5, update coming-soon notice - 04-conditional-policies.md: update Next navigation link
2b518b6 to
c145de9
Compare
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Feedback on Pull Request: docs: add policy-as-code tutorial chapter 5 (approval workflows)
🔴 CRITICAL: Security Issues
-
Escalation Timeout Default Behavior
- The
DefaultTimeoutActionis set toDENYas a safe default, which is appropriate for most sensitive actions. However, the tutorial suggests experimenting withDefaultTimeoutAction.ALLOW. This could lead to silent approvals in production if misconfigured, creating a security bypass for high-stakes actions. Ensure that the tutorial explicitly warns against usingALLOWfor critical actions and provides clear guidelines on when it might be acceptable.
Actionable Recommendation: Add a prominent warning in the tutorial about the risks of using
DefaultTimeoutAction.ALLOWfor sensitive actions. Highlight scenarios where this might be acceptable and emphasize the importance of careful consideration. - The
-
Lack of Cryptographic Integrity for Escalation Requests
- The
EscalationRequestobject does not appear to include cryptographic integrity checks (e.g., HMAC or digital signatures). This could allow malicious actors to tamper with escalation requests in transit or at rest, potentially altering the decision or bypassing human review.
Actionable Recommendation: Implement cryptographic integrity checks for
EscalationRequestobjects. Use HMAC or digital signatures to ensure that requests cannot be tampered with. Update the tutorial to reflect this change. - The
-
Insecure Default Backend
- The tutorial uses
InMemoryApprovalQueueas the default backend for escalation requests. While this is acceptable for demonstration purposes, it is critical to warn users that this backend is not suitable for production use. It lacks persistence and security features, making it vulnerable to data loss and unauthorized access.
Actionable Recommendation: Add a clear warning in the tutorial that
InMemoryApprovalQueueis for demonstration only. Recommend using a secure, persistent backend (e.g.,WebhookApprovalBackendor a database-backed implementation) for production environments. - The tutorial uses
🟡 WARNING: Potential Breaking Changes
-
Policy YAML Format
- The introduction of the
escalateaction in the policy YAML file changes the structure of policy documents. If users have existing policies, they may need to update them to accommodate this new action. This could lead to compatibility issues with older versions of the library.
Actionable Recommendation: Provide clear migration instructions for users who need to update their existing policies. Consider adding backward compatibility by allowing older policies to function without the
escalateaction. - The introduction of the
💡 Suggestions for Improvement
-
Thread Safety
- The
InMemoryApprovalQueueis used in the tutorial, but there is no mention of thread safety. If multiple agents are running concurrently and interacting with the same queue, race conditions could occur.
Actionable Recommendation: Ensure that
InMemoryApprovalQueueis thread-safe by using synchronization primitives like locks. Alternatively, explicitly state that this implementation is not thread-safe and recommend using a thread-safe backend for concurrent environments. - The
-
Policy Testing
- The tutorial mentions the importance of automated policy testing but does not provide concrete examples or tools for implementing such tests.
Actionable Recommendation: Add a section with sample test cases for policy validation using
pytest. For example, demonstrate how to test thattransfer_fundsalways escalates anddelete_databasealways denies. -
Quorum-Based Approval
- The tutorial briefly mentions quorum-based approval via
QuorumConfigbut does not provide an example.
Actionable Recommendation: Include a code snippet demonstrating how to configure and use
QuorumConfigfor actions requiring multiple sign-offs. This would make the tutorial more comprehensive. - The tutorial briefly mentions quorum-based approval via
-
Type Safety and Validation
- While the tutorial uses Pydantic models for policy documents, it does not explicitly demonstrate how validation errors are handled or how users can debug them.
Actionable Recommendation: Add a section explaining how to handle validation errors when loading policies using
PolicyDocument.from_yaml. Include examples of common validation errors and how to resolve them. -
Sandbox Escape Vectors
- The tutorial does not address potential sandbox escape vectors for agents that may exploit the escalation mechanism to execute unauthorized actions.
Actionable Recommendation: Add a section discussing best practices for securing the escalation mechanism, such as restricting the actions that can be escalated and validating the agent's identity using SPIFFE/SVID.
-
Backward Compatibility
- The tutorial introduces new functionality (
escalateaction) but does not address backward compatibility for existing users.
Actionable Recommendation: Provide clear documentation on how users can upgrade their policies and code to use the new escalation feature without breaking existing functionality.
- The tutorial introduces new functionality (
Summary
This pull request introduces a valuable feature for human-in-the-loop approval workflows, but it raises several critical security concerns and potential compatibility issues. Addressing these concerns will ensure the robustness and safety of the feature in production environments. Additionally, incorporating the suggested improvements will enhance the tutorial's utility and clarity for users.
imran-siddique
left a comment
There was a problem hiding this comment.
Review: CHANGES REQUESTED
Diff reviewed — 5 files: new tutorial chapter 5 (approval workflows) + example YAML + example Python + README/nav updates. 700 additions. Ref #706.
Security checklist:
- No eval(), exec(), pickle, shell=True, innerHTML
- No hardcoded secrets
- Scope matches description — tutorial files only
- Python example has MIT license header
- Additions > 0
Issues to fix:
-
Missing license header on new .md file:
05-approval-workflows.mdneeds the MIT copyright header:
<!-- Copyright (c) Microsoft Corporation. -->
<!-- Licensed under the MIT License. --> -
Missing license header on new .yaml file:
examples/05_approval_policy.yamlneeds:
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.
Content quality is good — the bank teller analogy works well, step progression is clear, and the Python demo covers all 5 parts. Happy to approve once headers are added.
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Review Summary
This pull request introduces Chapter 5 of the policy-as-code tutorial series, focusing on approval workflows and human-in-the-loop escalation mechanisms. The tutorial is well-written, provides clear examples, and introduces a critical concept for security-sensitive systems. However, there are areas that require attention to ensure correctness, security, and compliance with best practices.
🔴 CRITICAL: Security Issues
-
Escalation Timeout Default Behavior
- The tutorial defaults to
DefaultTimeoutAction.DENY, which is a safe choice. However, the tutorial mentions the possibility of usingDefaultTimeoutAction.ALLOWfor less critical actions. This could lead to silent approvals in production environments if misconfigured, creating a security bypass risk. - Recommendation: Explicitly warn users against using
DefaultTimeoutAction.ALLOWunless absolutely necessary. Highlight the risks of silent approvals in production environments.
- The tutorial defaults to
-
Audit Trail Integrity
- The tutorial mentions recording the approver's identity (e.g.,
manager@corp.com) but does not specify how this data is secured or verified. If the identity is spoofed or tampered with, it could lead to unauthorized approvals. - Recommendation: Ensure that the
approve()anddeny()methods enforce cryptographic verification of the approver's identity (e.g., via SPIFFE/SVID or signed tokens).
- The tutorial mentions recording the approver's identity (e.g.,
-
In-Memory Approval Queue
- The
InMemoryApprovalQueueis used for demonstration purposes but is not suitable for production. If accidentally used in production, it could lead to loss of escalation requests on process termination. - Recommendation: Add explicit warnings in the tutorial and code comments that
InMemoryApprovalQueueis for testing only. Provide examples of production-ready backends (e.g., database, message broker).
- The
🟡 WARNING: Potential Breaking Changes
- Policy YAML Format
- The policy YAML introduces a new
messagefield that is critical for escalation decisions. If users accidentally omit this field, the escalation mechanism will silently fail, and sensitive actions could be denied instead of escalated. - Recommendation: Update the
PolicyDocumentschema to enforce validation of themessagefield for escalation rules. Consider making this field mandatory for rules withaction: deny.
- The policy YAML introduces a new
💡 Suggestions for Improvement
-
Thread Safety
- The
InMemoryApprovalQueueandEscalationHandlerdo not appear to be thread-safe. In concurrent agent execution scenarios, race conditions could occur when multiple agents escalate requests simultaneously. - Recommendation: Document the lack of thread safety in the tutorial and suggest using thread-safe or distributed backends for production.
- The
-
Quorum-Based Approval
- The tutorial briefly mentions quorum-based approval but does not provide an example. This is a critical feature for high-stakes actions.
- Recommendation: Add an example demonstrating quorum-based approval using
QuorumConfig.
-
Policy Testing
- The tutorial ends by highlighting the importance of automated policy testing but does not provide actionable steps.
- Recommendation: Add a section on how to write unit tests for policies using
pytestand mock escalation handlers.
-
Type Safety
- The
EscalationRequestandPolicyDocumentclasses should use Pydantic models for strict type validation. This ensures that invalid data (e.g., missing fields) is caught early. - Recommendation: Refactor these classes to use Pydantic models and enforce schema validation.
- The
-
Backward Compatibility
- The addition of the
messagefield in the policy YAML could break existing policies if users upgrade the library without updating their YAML files. - Recommendation: Provide migration guidance for users upgrading from earlier versions of the library.
- The addition of the
Additional Observations
-
Documentation Quality
- The tutorial is well-written and provides clear examples. The use of a bank teller analogy is effective for explaining escalation workflows.
- Suggestion: Consider adding diagrams or screenshots of a hypothetical approval dashboard to make the tutorial more engaging.
-
Sandbox Escape Vectors
- The tutorial does not address potential sandbox escape vectors for agents that are paused during escalation. If an agent can bypass the escalation mechanism, it could execute sensitive actions without approval.
- Recommendation: Ensure that the agent's execution environment is sandboxed and that the escalation mechanism is enforced at the system level.
Action Items
- Address the 🔴 CRITICAL issues related to security bypass risks, audit trail integrity, and production readiness.
- Implement the 🟡 WARNING recommendation to enforce validation of the
messagefield in the policy schema. - Consider the 💡 Suggestions for thread safety, quorum-based approval examples, policy testing, type safety, and backward compatibility.
This tutorial is a valuable addition to the repository, but addressing the highlighted issues will ensure it meets the highest standards for security and reliability.
Thank you! I've now added the license headers on the files. |
Summary
New files
docs/tutorials/policy-as-code/05-approval-workflows.md— tutorial with bank teller analogy, five steps, ASCII flowchart, and exercisesdocs/tutorials/policy-as-code/examples/05_approval_policy.yaml— three-tier policy (allow, deny, escalate)docs/tutorials/policy-as-code/examples/05_approval_workflows.py— runnable demo with five partsUpdated files
docs/tutorials/policy-as-code/README.md— link chapter 5, update coming-soon noticedocs/tutorials/policy-as-code/04-conditional-policies.md— update Next navigation linkTest plan
python docs/tutorials/policy-as-code/examples/05_approval_workflows.pyand verify clean output across all 5 partsRef #706