Skip to content

Remediate Set-ToolsRepo external download security risk - #422

Merged
Kavyareddyguntaka11 merged 7 commits into
mainfrom
kguntaka/39455329-Remediate-set-toolsrepo
Sep 21, 2026
Merged

Kavyareddyguntaka11 merged 7 commits into
mainfrom
kguntaka/39455329-Remediate-set-toolsrepo

Conversation

@Kavyareddyguntaka11

@Kavyareddyguntaka11 Kavyareddyguntaka11 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

This PR partially addresses IcM#850732573.

This PR contains the Set-ToolsRepo changes and incorporates the legacy Set-CustomDRS cleanup from #419. The storage module changes will be submitted in a separate PR.

The changes in this PR are as follows:

  • Removed the ToolsURL download flow. The VMware Tools ZIP must now be uploaded to a staging folder on a vSAN datastore before running the command.
  • Added SourceDatastoreName, ToolsZipPath, and ExpectedHash inputs for upload mode.
  • Added SHA-256 verification before extracting or distributing the staged ZIP.
  • Added safe path validation to reject absolute paths, directory traversal, invalid characters, and non-ZIP files.
  • Refactored Set-ToolsRepo into smaller helper functions for input validation, archive handling, version comparison, datastore discovery, and ESXi host configuration.
  • Added validation of the staged ZIP structure, including the windows64 directory, version folder, and both required metadata.json files.
  • Added JSON parsing and version checks to confirm that the top-level and version-folder metadata match the extracted VMware Tools version before any destination datastore is changed.
  • Added version-aware behavior: older versions can be stored without replacing the top-level metadata, while newer versions update the top-level metadata.
  • Added isolated temporary folders and uniquely named PSDrives, with cleanup on both success and failure.
  • Added clear reporting for complete and partial datastore failures.
  • Added Pester tests for input validation, hash mismatch, archive errors, malformed metadata, datastore selection, version handling, cleanup, host configuration failure, and partial datastore failure.
  • Removed the legacy TryBalanceVmsPerHost and IsClusterManaged settings from Set-CustomDRS, including the unused option-array allocation.

Validation performed

  • All Pester tests passed: 46 passed, 0 failed, 1 environment-dependent test skipped.
  • End-to-end testing was completed against a lab SDDC containing two vSAN datastores.
  • VMware Tools versions 13.0.5 and 13.0.10 were uploaded successfully.
  • The staged ZIP hashes and metadata versions were verified successfully.
  • All six associated ESXi hosts were configured successfully.
  • Set-ToolsRepo -Validate reported synchronized metadata on both datastores after each upload.
  • The top-level metadata correctly advanced from version 13.0.5 to 13.0.10.
  • Verified that Set-CustomDRS no longer sends legacy or empty advanced-option entries to vCenter.

I have read the contributor guidelines and have completed the following:

  • Formatted the code using VSCode default formatter for PowerShell.
  • Tested the code end-to-end against an SDDC.
  • Documented the functions using standard PowerShell markup and applied AVSAttribute to newly exported functions.

…d require source datastore, ZIP path, and SHA-256 hash in upload mode

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

Seven unresolved review findings remain in the implementation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR replaces external VMware Tools downloads with staged vSAN uploads, integrity validation, and expanded failure handling.

Changes:

  • Adds datastore, ZIP path, and SHA-256 hash inputs.
  • Validates paths, archives, metadata, and versions.
  • Expands cleanup, datastore handling, host configuration, and Pester coverage.
File summaries
File Summary
tests/Microsoft.AVS.Management.Tests.ps1 Adds coverage for validation, hash/archive errors, metadata, cleanup, version handling, and partial failures.
Microsoft.AVS.Management/Microsoft.AVS.Management.psm1 Implements the staged-upload workflow. Findings remain at lines 181, 430, 526, 651, 793, 815, and 833: six moderate findings (votes: 2, 2, 2, 1, 1, 1) and one nit (1 vote) concerning metadata consistency, rollback, sanitization, PSDrive cleanup, missing or invalid metadata, and version-aware artifact copying.
Review details

Suppressed comments (4)

Microsoft.AVS.Management/Microsoft.AVS.Management.psm1:837

  • This condition now gates the copy of every non-metadata file from windows64 on the incoming version being newer. The previous flow copied those top-level GuestStore artifacts for every upload and only gated metadata.json; uploading an older version will therefore leave its top-level artifacts absent, even though its version folder is stored. Keep the artifact-copy block unconditional and apply $shouldUpdateTopLevelMetadata only to the top-level metadata file.
                    # Update top-level files only when the uploaded version is newer.
                    if ($shouldUpdateTopLevelMetadata) {
                        # Copy any additional top-level files from windows64, if present.
                        # Handle metadata.json separately below.
                        $topLevelSourceDir = Split-Path -Path $sourceDir -Parent

Microsoft.AVS.Management/Microsoft.AVS.Management.psm1:795

  • When the destination already has a higher version but its top-level metadata.json is missing, $highestExistingVersion is non-null and this condition preserves the missing file. The upload then reports success while leaving a repository that -Validate will reject at the required top-level metadata check; treat a missing top-level metadata file as an update condition or fail before copying.
                if ($null -eq $highestExistingVersion -or
                    (Compare-ToolsRepoVersion -Left $tools_short_version -Right $highestExistingVersion) -gt 0) {
                    $shouldUpdateTopLevelMetadata = $true

Microsoft.AVS.Management/Microsoft.AVS.Management.psm1:821

  • An existing version folder is treated as valid solely because a metadata.json file exists. If that file is malformed or describes a different version, upload mode skips the verified archive and still proceeds to host configuration, so the command can report success while the same repository fails -Validate. Parse and verify the existing metadata before skipping the copy, or fail this datastore when it is invalid.
                    if (Test-Path -Path $versionDestPath) {
                        $versionMetadataPath = Join-Path -Path $versionDestPath -ChildPath 'metadata.json'
                        if (-not (Test-Path -Path $versionMetadataPath -PathType Leaf)) {
                            throw "Version folder '$tools_version' already exists on datastore '$ds_name', but its required metadata.json is missing. Inspect the folder and, if it is incomplete, remove it and rerun Set-ToolsRepo."
                        }

                        Write-Information "Version $tools_version already exists on $ds_name. Skipping copy." -InformationAction Continue

Microsoft.AVS.Management/Microsoft.AVS.Management.psm1:529

  • This new upload path bypasses the module's established string-input sanitization: Get-EsxtopData sanitizes each string parameter before use (Microsoft.AVS.Management.psm1:1101-1104), but SourceDatastoreName, ToolsZipPath, and ExpectedHash are passed directly here. Apply the same sanitizer or an equivalent reject-before-use check to all three inputs before invoking datastore/provider commands.
        # Source datastore details and a trusted hash are required for upload mode.
        if (-not $Validate) {
            Test-ToolsRepoUploadInput -SourceDatastoreName $SourceDatastoreName -ToolsZipPath $ToolsZipPath -ExpectedHash $ExpectedHash
        }
  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Lite

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

Comment thread Microsoft.AVS.Management/Microsoft.AVS.Management.psm1 Outdated
Comment thread Microsoft.AVS.Management/Microsoft.AVS.Management.psm1
Comment thread Microsoft.AVS.Management/Microsoft.AVS.Management.psm1 Outdated
@gungazoo

Copy link
Copy Markdown
Contributor

I'd put a number of comments into the code to help someone who is trying to troubleshoot it.

@Kavyareddyguntaka11
Kavyareddyguntaka11 merged commit db70ce9 into main Sep 21, 2026
9 checks passed
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.

5 participants