Skip to content

feat: add opt-in Service Fabric HTTP options - #967

Open
Nilesh Choudhary (4gust) wants to merge 5 commits into
devfrom
4gust-msal-transport-api-design
Open

Nilesh Choudhary (4gust) wants to merge 5 commits into
devfrom
4gust-msal-transport-api-design

Conversation

@4gust

Copy link
Copy Markdown
Contributor

Summary

Add an optional service_fabric_http_options dictionary to ManagedIdentityClient, allowing consumers such as Azure Identity to use MSAL's Service Fabric transport without creating, opening, or exposing a requests.Session.

  • Export msal.ServiceFabricHttpOptions with five optional fields: headers, proxies, trust_env, timeout, and max_retries.
  • Use an isolated MSAL-owned session for each Service Fabric network acquisition, preserving mandatory endpoint certificate pinning and closing owned resources on every exit.
  • Snapshot configuration at construction and validate it only when Service Fabric actually needs network I/O. Invalid explicit configuration cannot be hidden by cached-token fallback.
  • Preserve other managed-identity providers and the legacy Service Fabric path.

Consumption

import msal

client = msal.ManagedIdentityClient(
    msal.SystemAssignedManagedIdentity(),
    http_client=consumer_http_client,  # May remain unopened for Service Fabric
    service_fabric_http_options={},
)

result = client.acquire_token_for_client(
    resource="https://management.azure.com",
)

Every dictionary field is optional. {} uses no custom headers/proxies, trust_env=False, (5, 30) connect/read timeouts, and no retries. Explicit retries cover only connection failures before request transmission.

Compatibility and scope

This is an opt-in API, not an automatic repair for unchanged Azure Identity callers. Omitting the parameter or passing None retains the existing Service Fabric requirement for a Requests session. Azure Identity must pass service_fabric_http_options={} wherever it constructs the MSAL client to adopt this path; an MSAL dependency bump alone does not enable it.

No existing required arguments, dependency declarations, or package versions change. General per-call HTTP configuration and the alternative callback API are intentionally outside this PR.

Related: #952, #964, and Azure/azure-sdk-for-python#48708.

Security behavior

  • Require HTTPS and authenticate the actual endpoint certificate before transmitting Secret, including through proxies and on reconnects.
  • Reject redirects and caller overrides of Secret or Host; validate selected proxies before credential-bearing parser errors can escape.
  • Authenticate HTTPS proxy certificate chains and hostnames before sending CONNECT credentials, independently of Service Fabric endpoint pinning. Unsupported HTTPS-tunneling stacks fail closed.
  • Keep environment configuration disabled by default. Explicit trust_env=True follows Requests proxy/CA selection without bypassing endpoint pinning.
  • Preserve eligible cached-token fallback for transport failures, but never silently fall back to an unpinned network request.

Validation

Final Windows/Python 3.12 revision:

  • Targeted managed-identity, throttled-client, and token-cache suites: 174 passed, 575 subtests passed.
  • Noninteractive unit suite: 447 passed, 596 subtests passed; 17 skipped and 4 deselected. Live E2E and benchmarks were excluded; the four deselections are two interactive broker tests and two cryptography release-version checks.
  • Real local TLS/CONNECT coverage includes endpoint pinning, proxy trust and hostname rejection before CONNECT, strict X.509 verification, redirect handling, retries, privacy, cleanup, cache behavior, and isolation.
  • Sphinx with warnings treated as errors, wheel/sdist builds, Twine checks, wheel-first imports, and source/artifact integrity checks passed.
  • Final independent security review reported no remaining findings after the HTTPS proxy authentication repair.

Remaining release validation: native additional Python versions, Linux/macOS, an installed historical MSAL baseline, and native older-runtime dependency combinations. The explicit strict-verification test on Python 3.12 is not a claim of native Python 3.13/3.14 execution.

MSAL-owned secure transport accepts unopened custom clients via explicit options. None preserves legacy behavior. HTTPS proxies are authenticated separately. Per-call options are out of scope.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 24, 2026 09:27
@4gust
Nilesh Choudhary (4gust) requested a review from a team as a code owner September 24, 2026 09:27
Comment thread tests/test_mi.py Fixed
Comment thread tests/test_mi.py Fixed
Comment thread tests/test_mi.py Fixed

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.

Copilot review overview

🟡 Changes recommended

A critical proxy TLS hostname issue and a moderate proxy-key normalization issue remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds opt-in, isolated Service Fabric HTTP configuration with certificate pinning, proxy support, retries, cleanup, public typing, tests, and documentation.

Changes:

  • Introduces and exports ServiceFabricHttpOptions.
  • Adds owned-session transport with validation and TLS safeguards.
  • Expands managed identity coverage and documents compatibility.

Review findings include one critical proxy TLS hostname issue and one moderate proxy-key normalization issue.

File Summary
tests/​test_mi.py Adds Service Fabric transport, security, cleanup, and compatibility coverage.
msal/​managed_identity.py Implements options, validation, transport, proxy, and TLS behavior; contains the noted findings.
msal/​__init__.py Exports the new public options type.
docs/​index.rst Documents configuration, security, and compatibility.

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

Comment thread msal/managed_identity.py Outdated
Add explicit TLS 1.2 fixture minimums, exact DER proxy-certificate comparison, and a distinct-hostname SNI regression while preserving production pinning.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 10:38

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.

Copilot review overview

🔵 Needs a closer look

Security-sensitive transport changes require final human review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Co-authored-by: 4gust <107404295+4gust@users.noreply.github.com>

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.

Copilot review overview

🔵 Needs a closer look

The extensive TLS, proxy, retry, and credential-handling changes require final human review.

Review effort: Lite
Findings: None

Resolved since last review (1)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 14:36

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.

Copilot review overview

🔵 Needs a closer look

The legacy Service Fabric path must reliably close its owned session.

Review effort: Lite
Findings: None

Delegate endpoint certificate pinning to native urllib3 connections while retaining independent HTTPS proxy authentication. Preserve proxy URL normalization and Requests TLS context isolation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 25, 2026 11:05

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.

Copilot review overview

🔵 Needs a closer look

A moderate legacy Service Fabric session-cleanup issue remains unresolved in security-sensitive transport code.

Review effort: Lite
Findings: None

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants