Skip to content

Add defensive diagnostics to symbol publishing - #4731

Draft
paulmedynski wants to merge 1 commit into
mainfrom
dev/paul/symbols-ppe
Draft

paulmedynski wants to merge 1 commit into
mainfrom
dev/paul/symbols-ppe

Conversation

@paulmedynski

@paulmedynski paulmedynski commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds diagnostics to the symbol publishing path so that the next failure explains itself from the build log, rather than requiring an investigation to reconstruct what happened.

This started as an investigation into symbol publishing failing against the PPE server. That is now resolved and no change in this repository was needed to fix it — see Root cause below. What remains here is the instrumentation, kept deliberately: the failure took far longer to diagnose than it should have, largely because the publish script discarded the information that would have identified it.

Root cause

Symbol publishing to symbolrequestppe.trafficmanager.net began failing on 2026-09-17 with a bare Permission denied, while Production publishing continued to work.

The cause was outside this repository. The 1ES Linux network isolation policy moved from Permissive to DefaultDeny:

- --policies "Permissive,CFSClean,CFSClean2,CFSClean3"     (1ESNetworkIsolation-linux-x64 1.0.172)
+ --policies "DefaultDeny,CFSClean,CFSClean2,CFSClean3"    (1.0.175+)

The change landed between builds 26251.1 (2026-09-08, last Permissive) and 26252.1 (2026-09-09, first DefaultDeny). symbolrequestppe.trafficmanager.net is not in the 1ES endpoint allowlist, so under default-deny the TCP connection is refused with EACCES before any HTTP exchange occurs. symbolrequestprod.trafficmanager.net is allowlisted and continued to work.

Supporting observations:

  • The task is 1ESNetworkIsolation-**linux**-x64, so the Windows container is unaffected. The SNI pipeline published to the same PPE endpoint from windows_build_container 27 seconds before one of our Linux failures (build 176140 vs 175832).
  • The agent image version is identical across passing and failing runs (20260824.1.170484886), and the same pools and regions appear on both sides. Identity, token audience and symbol project were unchanged throughout.
  • The first PPE attempt after the policy change was 2026-09-17, which is why the failure date and the policy date differ.

A request to add the PPE endpoint to the 1ES allowlist is being raised separately with 1ES/CloudBuild.

Why these changes, then

Permission denied is the EACCES errno text. It is indistinguishable, in a log, from an application-level authorization denial — and it was misread as exactly that for some time. The publish script made this worse: Invoke-RestMethod surfaces only a message, and the script discarded the status code, the response body, the correlation identifiers and the exception type. There was no way to tell "the service rejected us" from "we never reached the service".

These changes make that distinction visible, and are intended as defensive instrumentation for the next incident rather than a fix for this one.

What was added

Always emitted:

  • Executed commands — the az token acquisition and each Invoke-RestMethod call, with method, URI, headers and body. Secrets are redacted by default.

  • Token identity — SHA-256 fingerprint plus allow-listed JWT header and payload claims (aud, iss, tid, appid, oid, idtyp, roles, exp, …). These identify the calling principal and audience without exposing the credential. The signature segment is never decoded.

  • HTTP failure detail — status code, response body, and correlation identifiers such as mise-correlation-id, plus the exception and inner-exception types. When no response was received it says so explicitly:

    Exception: System.Net.Http.HttpRequestException
    Inner exception: System.Net.Sockets.SocketException: Permission denied
    HTTP status: <none - no HTTP response was received>
    

    That single line is what finally separated an egress block from a service rejection.

Under the existing debug parameter:

  • Unredacted commands-LogUnredactedCommands, so the real token and Authorization header can be inspected. Emits a warning that the log contains secrets.

  • Reachability probetest-endpoint-reachability.ps1 runs before the publish request and probes both symbol servers from inside the build container, separating DNS resolution from TCP connect and reporting socket error codes verbatim:

    BLOCKED     symbolrequestppe.trafficmanager.net - AccessDenied (errno 13): Permission denied
    REACHABLE   symbolrequestprod.trafficmanager.net - connected
    

Example: these diagnostics in a real run

sqlclient-non-official 26266.6 exercises this branch on the ordinary publish path — publishSymbols=true, debug=true, no special mode. The PDBs upload successfully and only the publish request is refused, so the symbols end up staged but unpublishable.

The probe, from inside the publishing job:

=== symbolrequestppe.trafficmanager.net:443 ===
DNS      : 20.119.8.23
TCP      : by name -> FAILED in 117 ms - AccessDenied (errno 13): Permission denied

=== symbolrequestprod.trafficmanager.net:443 ===
DNS      : 40.112.243.108
TCP      : by name -> connected in 66 ms - connected

=== Summary ===
  BLOCKED     symbolrequestppe.trafficmanager.net - AccessDenied (errno 13): Permission denied
  REACHABLE   symbolrequestprod.trafficmanager.net - connected

Then the publish request itself:

Failed to register request name. URI: https://symbolrequestppe.trafficmanager.net/projects/Microsoft.Data.SqlClient/requests
  | Exception: System.Net.Http.HttpRequestException
  | Inner exception: System.Net.Sockets.SocketException: Permission denied
  | HTTP status: <none - no HTTP response was received>
  | Response body: Permission denied
  | Error: Permission denied

Before this change the same failure read, in its entirety, Error: Permission denied.

For contrast, 26266.4 shows the token diagnostics that are emitted on every run, including the JWT claims identifying the calling principal and audience.

Notes for reviewers

  • The publish path is unchanged. No change to how requests are authenticated, constructed or sent. The template diffs are additive; the only behavioural change is the new debug-gated probe step.
  • debug now also exposes the bearer token. Its label is updated to Enable debug output (may expose secrets) with a warning comment. Agent-side scrubbing of the token is best-effort and should not be relied on.
  • This root cause was already in our logs. Every failing run's 🔒 Stop Network Isolation step reported Blocked connections: symbolrequestppe.trafficmanager.net and Policy: Default Deny, from the first failure onward. Reading sibling steps in a failing job, not just the failing step, would have found it immediately. The diagnostics added here are still worthwhile, but the more valuable lesson was cheaper than the instrumentation.

Checklist

  • Tests added or updated (122 Pester tests pass)
  • Public API changes documented (n/a — pipeline-only change)
  • Verified against customer repro (build 177611 on the ordinary publish path; 177314, 177588, 177593 during investigation)
  • Ensure no breaking changes introduced

Copilot AI balanced review requested due to automatic review settings September 22, 2026 19:07
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Sep 22, 2026
@paulmedynski paulmedynski added this to the 8.0.0-preview1 milestone Sep 22, 2026
@paulmedynski paulmedynski moved this from To triage to In progress in SqlClient Board Sep 22, 2026

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

The debug path writes a reusable bearer token into retained pipeline logs, violating the repository’s secret-logging policy.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds command-level diagnostics to OneBranch symbol publishing, with optional unredacted output controlled by the existing debug parameter.

Changes:

  • Logs Azure CLI and REST commands with default redaction.
  • Threads debug configuration through symbol-publishing templates.
  • Adds Pester coverage and pipeline documentation.
File Description
publish-symbols-step.yml Passes debug mode to the script.
publish-symbols-stage.yml Propagates debug mode across package jobs.
sqlclient-official.yml Adds warning and forwards debug mode.
sqlclient-non-official.yml Adds warning and forwards debug mode.
publish-symbols.Tests.ps1 Tests redacted and unredacted logging.
publish-symbols.ps1 Implements command logging and redaction helpers.
publish-symbols-job.yml Forwards debug mode to the publishing step.
onebranch-pipeline-design.instructions.md Documents command-logging behavior.

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

Comment on lines +158 to +159
if ($LogUnredactedCommands) {
return $Value
@paulmedynski paulmedynski added the Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems. label Sep 22, 2026
Copilot AI review requested due to automatic review settings September 22, 2026 20:47

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 debug path writes live Azure bearer tokens to retained pipeline logs, violating repository credential-handling policy.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Copilot AI review requested due to automatic review settings September 22, 2026 21:02

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

Unredacted logging exposes a live bearer token in retained pipeline logs.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Copilot AI review requested due to automatic review settings September 23, 2026 11:32

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

It can persist live bearer tokens in pipeline logs, and one new assertion does not validate the intended header value.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

}

It 'Should log the actual Authorization header value' {
$script:output | Should -BeLike "*Authorization = '******'*"
Copilot AI review requested due to automatic review settings September 23, 2026 13:07

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 debug path writes a reusable bearer token into persistent pipeline logs.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Copilot AI review requested due to automatic review settings September 23, 2026 13:44

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

The implementation exposes bearer credentials and deliberately bypasses pipeline secret masking.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)

Comment thread eng/pipelines/onebranch/scripts/publish-symbols.ps1 Outdated
Copilot AI review requested due to automatic review settings September 23, 2026 14:56

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 debug path exposes a live bearer token and Authorization header in persistent pipeline logs.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)

Copilot AI review requested due to automatic review settings September 23, 2026 16:11
@paulmedynski paulmedynski changed the title Log symbol publishing commands, unredacted under debug Add defensive diagnostics to symbol publishing Sep 23, 2026
Symbol publishing to the PPE server began failing with a bare "Permission
denied". That text is the EACCES errno, and in a log it is indistinguishable
from an application-level authorization denial. The publish script made the
ambiguity worse: Invoke-RestMethod surfaces only a message, and the script
discarded the status code, response body, correlation identifiers and
exception type, so there was no way to tell "the service rejected us" from
"we never reached the service".

The cause turned out to be outside this repository. The 1ES Linux network
isolation policy moved from Permissive to DefaultDeny between builds 26251.1
and 26252.1, and symbolrequestppe.trafficmanager.net is not in the endpoint
allowlist, so the connection is refused before any HTTP exchange occurs. The
Production endpoint is allowlisted and was unaffected, as was the Windows
container, since the task is linux-only. That is being raised with 1ES
separately; nothing here caused it and nothing here can fix it.

These diagnostics are kept anyway, for the next time symbol publishing
misbehaves.

Always emitted:
  - the commands executed, covering the token acquisition and each REST
    call, with secrets redacted;
  - the token's SHA-256 fingerprint and its allow-listed JWT header and
    payload claims, which identify the calling principal and audience
    without exposing the credential. The signature segment is never decoded;
  - the HTTP status, response body, correlation identifiers and exception
    types on failure, stating explicitly when no response was received. That
    distinction is what separates an egress block from a service rejection.

Under the existing debug parameter:
  - the same commands with the bearer token and Authorization header
    unredacted, preceded by a warning that the log contains secrets;
  - a reachability probe that tests both symbol servers from inside the
    build container, separating DNS resolution from TCP connect and
    reporting socket error codes verbatim.

The publish path itself is unchanged: how requests are authenticated,
constructed and sent is the same, and the template changes are additive
aside from the new debug-gated probe step.

Worth recording: every failing run already reported this in its Stop Network
Isolation step, naming the blocked domain and the Default Deny policy, from
the first failure onward. Reading the sibling steps of a failing job, rather
than only the failing step, would have identified it immediately.

Co-authored-by: Copilot <223556219+Copilot@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

🟡 Changes recommended

Debug mode exposes live bearer credentials in retained logs, and some reachability conclusions are inaccurate.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Low severity Add test coverage for response-body extraction in failure diagnostics

eng/​pipelines/​onebranch/​scripts/​publish-symbols.ps1:363

The new response-body extraction is not covered by the added Pester cases: the HTTP mock verifies status and a correlation header, but no test supplies ErrorDetails.Message and asserts the Response body: diagnostic. Because this exception shape varies by PowerShell version and the response body is a stated goal of the change, add a focused failure test for it.

Low severity Clarify never-throws contract for endpoint failures versus invalid input

eng/​pipelines/​onebranch/​scripts/​test-endpoint-reachability.ps1:19

The “never throws” contract contradicts the explicit throw for an empty parsed host list at line 64 (and parameter binding can also fail). Clarify that endpoint failures are non-terminating while invalid invocation still fails.

Comment on lines +141 to +143
Write-Host "Verdict : DNS resolves but the TCP connection did not succeed, so the request never"
Write-Host " reached the service. This is a network path or egress policy problem"
Write-Host " rather than anything the destination service decided."
Copilot AI review requested due to automatic review settings September 23, 2026 16:15

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

Debug mode exposes live credentials, while the probe also contains misleading attribution and a nondeterministic fixed-port test.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 3 Medium severity

Open (5)

-ArtifactName "${{ parameters.artifactName }}"
-PublishToInternal $${{ parameters.publishToInternal }}
-PublishToPublic $${{ parameters.publishToPublic }}
-LogUnredactedCommands:$${{ parameters.debug }}
$scriptPath = Join-Path $PSScriptRoot '..' 'test-endpoint-reachability.ps1'

# A port on loopback that nothing is listening on.
$script:closedPort = 9

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

Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems.

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants