Skip to content

Python: Tighten security label enforcement - #8238

Merged
Eduard van Valkenburg (eavanvalkenburg) merged 5 commits into
microsoft:mainfrom
eavanvalkenburg:musical-memory
Sep 15, 2026
Merged

Python: Tighten security label enforcement#8238
Eduard van Valkenburg (eavanvalkenburg) merged 5 commits into
microsoft:mainfrom
eavanvalkenburg:musical-memory

Conversation

@eavanvalkenburg

@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) commented Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Motivation & Context

Security labels and identity-scoped data must preserve locally established trust and ownership boundaries across tool results, approvals, sessions, and remote content conversion.

Description & Review Guide

  • What are the major changes? Make embedded labels restriction-only unless stamped by a framework-owned producer, bind USER_IDENTITY flows and approvals to canonical tenant/user principals, bound transitive variable expansion with cycle detection, and add a runnable user_identity_security_example.py showing authenticated principal setup with allowed and blocked destinations.
  • What is the impact of these changes? Malformed or mismatched identity metadata fails closed while existing PUBLIC and PRIVATE ranking remains unchanged. The security README and developer guide now document canonical principal metadata and restriction-only mixed-trust labeling. Full core, hosting-A2A, sample lint/type, and markdown code checks pass.
  • What do you want reviewers to focus on? Label authority boundaries, principal propagation and approval binding, bounded variable expansion, and whether the new sample clearly demonstrates host-authenticated principal configuration.

Related Issue

No related open issue.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and the title prefix in sync automatically.

Copilot AI balanced review requested due to automatic review settings September 10, 2026 12:28
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Sep 10, 2026
Copilot AI reviewed Sep 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Public API validation, sensitive metadata logging, and approval-resume integration coverage need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Tightens Python security-label authority, identity-principal enforcement, approval binding, and variable expansion limits.

Changes:

  • Restricts embedded labels unless framework-authoritative.
  • Enforces canonical tenant/user principals across flows and approvals.
  • Adds bounded transitive variable expansion and security regressions.
File summaries
File Description
python/packages/core/agent_framework/security.py Implements security enforcement changes.
python/packages/core/tests/test_security.py Adds principal, approval, label, expansion, and isolation tests.
python/packages/hosting-a2a/tests/hosting_a2a/test_conversion.py Verifies remote A2A metadata cannot upgrade integrity.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
github-actions[bot]
github-actions Bot reviewed Sep 10, 2026
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): f183929bf5e1
Model: gpt-5.6-sol-fast

Overview

The change establishes strong authority boundaries through process-local label stamps, canonical tenant/user principal sets, principal-bound approval records, and bounded transitive variable expansion. Tests cover forged labels, principal drift, cross-session isolation, malformed principal state, cycles, and traversal limits. Two Medium compatibility gaps remain: the new public helper does not honor its advertised input types, and existing documented USER_IDENTITY configurations are blocked without corresponding migration guidance.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/security.py

Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
@agent-framework-automation agent-framework-automation Bot added the documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs label Sep 10, 2026
@jpalvarezl

Jose Alvarez (jpalvarezl) commented Sep 14, 2026

Copy link
Copy Markdown
Member

In a USER_IDENTITY context, _check_policy now requires every destination to declare max_allowed_confidentiality="user_identity" and a matching authorized principal set. The automatically injected quarantined_llm and inspect_variable tools do not declare such a destination policy, so the default blocking configuration appears to reject both with principal_mismatch after identity-scoped content enters the session.

Could these built-ins receive explicit host/session-bound authorization, with an end-to-end SecureAgentConfig policy-enforcement regression? An unconditional exemption would weaken the identity boundary.

jpalvarezl
Jose Alvarez (jpalvarezl) reviewed Sep 14, 2026
View reviewed changes
Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
Comment thread python/packages/core/agent_framework/security.py Resolved
@eavanvalkenburg

Eduard van Valkenburg (eavanvalkenburg) commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

Jose Alvarez (@jpalvarezl) Addressed the built-in security-tool issue in 9036e44. quarantined_llm and inspect_variable now carry a framework-owned in-process identity marker, and policy dynamically authorizes only those exact marked tools for the active valid USER_IDENTITY source principal set. The marker cannot be reproduced by remote/custom metadata. Added SecureAgentConfig regressions for both built-ins plus same-named boolean/string marker forgeries, which remain blocked.

Merged via the queue into microsoft:main with commit 36053be Sep 15, 2026
41 checks passed
@antsok Anton Sokolovskyi (antsok) mentioned this pull request Sep 25, 2026
Closed

This branch was previously deployed

1 inactive deployment
github-app-auth — 25a1c758 Deployed Sep 15, 2026 by moonbox3 via add_label #22831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpalvarezl Jose Alvarez (jpalvarezl) jpalvarezl left review comments
@github-actions github-actions[bot] github-actions[bot] left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@moonbox3 Evan Mattson (moonbox3) moonbox3 approved these changes
@chetantoshniwal chetantoshniwal Awaiting requested review from chetantoshniwal
@TaoChenOSU Tao Chen (TaoChenOSU) Awaiting requested review from TaoChenOSU
@giles17 Giles Odigwe (giles17) Awaiting requested review from giles17
@SergeyMenshykh SergeyMenshykh Awaiting requested review from SergeyMenshykh SergeyMenshykh is a code owner
@rogerbarreto Roger Barreto (rogerbarreto) Awaiting requested review from rogerbarreto rogerbarreto is a code owner
@westey-m westey (westey-m) Awaiting requested review from westey-m westey-m is a code owner

Assignees

No one assigned

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eavanvalkenburg @jpalvarezl @moonbox3