Skip to content

{Storage} Secure AzCopy executable resolution - #34171

Draft
YangAn-microsoft wants to merge 3 commits into
Azure:devfrom
YangAn-microsoft:fix/storage-azcopy-executable-resolution
Draft

YangAn-microsoft wants to merge 3 commits into
Azure:devfrom
YangAn-microsoft:fix/storage-azcopy-executable-resolution

Conversation

@YangAn-microsoft

@YangAn-microsoft YangAn-microsoft commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Related command

az storage copy, az storage remove, az storage blob sync, and az storage fs directory copy

Description

Resolve PATH-discovered AzCopy executables to a validated absolute path before version probing or command execution. Executables resolved from the current working directory are rejected, while trusted system installations remain supported and managed installation remains the fallback.

The managed AzCopy location is also normalized to an absolute path. Expected probe, decoding, malformed-output, and invalid-version failures fall back safely to managed installation.

Focused unit tests cover current-directory rejection, absolute system-path reuse, managed-path normalization, operation execution with the selected absolute path, and probe-failure fallback.

Testing Guide

  • Focused unit tests: 6 passed, 3 subtests passed
  • Flake8 on changed files: passed
  • Pylint on changed files: 10.00/10
  • Pylance syntax validation: passed
  • git diff --check: passed

History Notes

None. This is internal security hardening with no command-surface change.


Resolve AzCopy to a validated absolute path before probing or running it, reject executables discovered in the current working directory, and add focused regression coverage.

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

Path-validation errors can still abort instead of falling back to managed installation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Hardens AzCopy executable discovery and execution.

Changes:

  • Resolves and validates absolute AzCopy paths.
  • Adds focused executable-resolution tests.
File Description
storage/​azcopy/​util.py Secures resolution and version fallback.
tests/​latest/​test_storage_azcopy_util.py Tests resolution and execution paths.

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

Comment thread src/azure-cli/azure/cli/command_modules/storage/azcopy/util.py Outdated
Treat working-directory and path-canonicalization failures as unsafe so AzCopy resolution falls back to the managed installation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yonzhan

Copy link
Copy Markdown
Collaborator

Storage

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

Distinct malformed-output, decoding, and invalid-version fallback paths lack regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add tests for decode, malformed output, and invalid version fallbacks

src/​azure-cli/​azure/​cli/​command_modules/​storage/​tests/​latest/​test_storage_azcopy_util.py:80

This test exercises only the OSError probe path. The newly added fallback behavior for decode errors, malformed/no-match output, and invalid version strings is untested, even though these are distinct paths in check_version() and _is_supported_version(). Please add cases confirming each result triggers managed installation and is never reused; this is important regression coverage for the security hardening.

Add regression coverage for decoding failures, malformed output, and invalid system AzCopy versions falling back to managed installation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@YangAn-microsoft

Copy link
Copy Markdown
Contributor Author

Addressed the latest Copilot review feedback in 5d35d3a. Added distinct regression cases for UTF-8 decode failure, malformed/no-match output, and invalid version text, each confirming fallback to the managed AzCopy executable. Focused validation now passes 6 tests and 3 subtests; Flake8, Pylint, and diff checks also pass.

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 security-critical symlink-to-CWD rejection path lacks regression coverage.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Add regression test for realpath symlink bypass outside CWD

src/​azure-cli/​azure/​cli/​command_modules/​storage/​tests/​latest/​test_storage_azcopy_util.py:30

The CWD test only exercises the direct resolved path. The new validation also relies on realpath to reject an executable found outside CWD whose symlink target is inside CWD; without coverage, removing that security-critical check would still leave this suite green. Please add a regression case for that bypass scenario, as the analogous ACR validation does in acr/tests/latest/test_acr_docker_path_validation.py:152-165.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants