Fix: Pass Credential to SMB copy functions via New-PSDrive - #624
Fix: Pass Credential to SMB copy functions via New-PSDrive#624Adam Rudell (arudell) with Copilot wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…thentication Fixes #269. Copy-FileToRemoteComputer and Copy-FileFromRemoteComputer now pass the -Credential parameter to their respective SMB helper functions. The SMB functions use New-PSDrive to establish an authenticated SMB session when credentials are provided, since Copy-Item does not natively support -Credential for filesystem operations. The PSDrive is cleaned up in the end block. Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
Address code review feedback: - Derive the admin share UNC root from the actual path instead of hardcoding c$ - Assign $psDriveName only after New-PSDrive succeeds to avoid spurious Remove-PSDrive calls on failure Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/modules/SdnDiag.Utilities.psm1:852
- Issue #269 requires handling an existing SMB mapping, but this always creates a new PSDrive without checking connections to the target. If the process already has an SMB session to this computer under another identity,
New-PSDrivefails with a credential-conflict error and the requested credential still falls back to WinRM. Detect and deliberately reuse or replace a compatible existing connection before creating the drive.
[System.String]$psDriveName = $null
if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
$driveRoot = [System.IO.Path]::GetPathRoot($Destination.FullName).Replace(':','$')
src/modules/SdnDiag.Utilities.psm1:885
- The
endblock is not guaranteed to run whenCopy-Itemraises a terminating error (the caller supplies-ErrorAction Stop), so the temporary drive and authenticated SMB session can leak on the failure path. Put drive removal in cleanup that runs when processing terminates, such as atry/finallyaround the mapped-drive copy lifecycle.
}
}
}
src/modules/SdnDiag.Utilities.psm1:763
- This fixes the credential-forwarding bug for local-to-remote copies, but
tests/offline/Utilities.Tests.ps1has no regression coverage for this path. Add an offline Pester test that supplies a credential, verifies the SMB helper receives it, and covers PSDrive creation and cleanup.
# try SMB Copy first and fallback to WinRM
src/modules/SdnDiag.Utilities.psm1:605
- Issue #269 requires handling an existing SMB mapping, but this always creates a new PSDrive without checking connections to the target. If the process already has an SMB session to this computer under another identity,
New-PSDrivefails with a credential-conflict error and the requested credential still falls back to WinRM. Detect and deliberately reuse or replace a compatible existing connection before creating the drive.
This issue also appears on line 850 of the same file.
$driveRoot = [System.IO.Path]::GetPathRoot($Path[0]).Replace(':','$')
$uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
$tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
src/modules/SdnDiag.Utilities.psm1:642
- The
endblock is not guaranteed to run whenTest-PathorCopy-Itemraises a terminating error (the caller supplies-ErrorAction Stop), so the temporary drive and authenticated SMB session can leak on the failure path. Put drive removal in cleanup that runs when processing terminates, such as atry/finallyaround the mapped-drive copy lifecycle.
This issue also appears on line 882 of the same file.
}
end {
if ($psDriveName) {
src/modules/SdnDiag.Utilities.psm1:518
- This fixes the credential-forwarding bug for remote-to-local copies, but
tests/offline/Utilities.Tests.ps1has no regression coverage for this path. Add an offline Pester test that supplies a credential, verifies the SMB helper receives it, and covers PSDrive creation and cleanup.
This issue also appears on line 763 of the same file.
Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/modules/SdnDiag.Utilities.psm1:855
- The linked issue explicitly requires handling an existing SMB connection, but this always creates another mapping. On Windows, a connection to the same server under a different identity makes
New-PSDrivefail with error 1219, so this path still falls back to WinRM instead of supporting the supplied credential. Detect an existing connection and reuse it when compatible, or explicitly handle the credential conflict before creating the temporary drive.
$null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:765
- This bug fix changes credential-based copy-to behavior, but
tests/offline/Utilities.Tests.ps1has no regression coverage for it. Please add offline Pester tests that verify the credential reaches the SMB helper, the destination drive determines the admin share, and the temporary drive is removed.
Copy-FileToRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:606
- The linked issue explicitly requires handling an existing SMB connection, but this always creates another mapping. On Windows, a connection to the same server under a different identity makes
New-PSDrivefail with error 1219, so this path still falls back to WinRM instead of supporting the supplied credential. Detect an existing connection and reuse it when compatible, or explicitly handle the credential conflict before creating the temporary drive.
This issue also appears on line 855 of the same file.
$null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:518
- This bug fix changes credential-based copy-from behavior, but
tests/offline/Utilities.Tests.ps1has no regression coverage for it. Please add offline Pester tests that verify the credential reaches the SMB helper, the expected admin-share drive is created (including a non-C: path), and the temporary drive is removed.
This issue also appears on line 765 of the same file.
Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
Co-authored-by: arudell <18577812+arudell@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/modules/SdnDiag.Utilities.psm1:606
- The linked issue explicitly requires detecting an existing SMB mapping, but this always creates a new drive. If the process already has a connection to this server under different credentials, Windows rejects the new mapping (typically error 1219), so this still falls back to WinRM. Check existing FileSystem/SMB connections for the server and define whether to reuse or replace them before calling
New-PSDrive; apply the same handling to the upload helper.
$null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:770
- The corresponding upload behavior is also untested:
tests/offline/Utilities.Tests.ps1contains no coverage for the exportedCopy-SdnFileToComputerpath. Add an offline Pester test that verifies credential forwarding, mapping the destination's actual drive root (including a non-C: case), and removing the temporary drive after copying.
try {
src/modules/SdnDiag.Utilities.psm1:518
- This bug fix changes credential forwarding for the exported
Copy-SdnFileFromComputerpath, buttests/offline/Utilities.Tests.ps1has no regression coverage for it. Add an offline Pester test that mocks the SMB dependencies and verifies the credential reachesNew-PSDrive, the source drive root is used, and the temporary drive is removed after copying.
This issue also appears in the following locations of the same file:
- line 606
- line 770
Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/modules/SdnDiag.Utilities.psm1:896
endis skipped when a terminating copy error exitsprocess, so the temporary PSDrive remains mounted precisely when this helper throws and the caller falls back to WinRM. Put the removal on a guaranteed failure path, such as atry/finallycovering the mapping and copy lifecycle.
end {
if ($psDriveName) {
try {
Remove-PSDrive -Name $psDriveName -Force -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:861
- This unconditionally creates another mapping without detecting an existing SMB/PSDrive connection. An existing connection to the same server under different credentials causes Windows error 1219, so this fix still takes the WinRM fallback in that required scenario. Reuse a compatible connection or explicitly handle a credential conflict before calling
New-PSDrive.
if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
$driveRoot = [System.IO.Path]::GetPathRoot($Destination.FullName).Replace(':','$')
$uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
$tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
$null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:771
- The corresponding exported
Copy-SdnFileToComputercredential path is also changed without a regression test. Add offline Pester coverage that verifies credential forwarding, authenticated drive creation for the destination drive, and cleanup on both success and failure.
Copy-FileToRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:645
endis not afinallyblock: ifTest-Path,Copy-Item, or the explicit rethrow terminates theprocessblock, PowerShell skips this cleanup and leaves the temporary drive/credentialed SMB session mounted. Ensure removal runs on the failure path before rethrowing (or restructure the mapping and copy undertry/finally) so the WinRM fallback does not leave stale drives behind.
This issue also appears on line 893 of the same file.
end {
if ($psDriveName) {
try {
Remove-PSDrive -Name $psDriveName -Force -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:606
- This always creates a new mapping without checking existing SMB/PSDrive connections to the host, despite issue #269 requiring detection. If the process already has a connection to this server under another identity, Windows rejects this with error 1219 and the operation still falls back to WinRM. Detect and reuse a compatible existing connection, and handle an incompatible connection explicitly before creating a drive.
This issue also appears on line 857 of the same file.
if ($Credential -ne [System.Management.Automation.PSCredential]::Empty -and $null -ne $Credential) {
$driveRoot = [System.IO.Path]::GetPathRoot($Path[0]).Replace(':','$')
$uncRoot = "\\{0}\{1}" -f $ComputerName, $driveRoot
$tempDriveName = "osPsDrive_{0}" -f [guid]::NewGuid().ToString()
$null = New-PSDrive -Name $tempDriveName -PSProvider FileSystem -Root $uncRoot -Credential $Credential -ErrorAction Stop
src/modules/SdnDiag.Utilities.psm1:518
- This bug fix changes credential handling for the exported
Copy-SdnFileFromComputerpath, buttests/offline/Utilities.Tests.ps1has no regression coverage for forwarding the credential, creating the authenticated drive, and removing it on success/failure. Please add mocked offline Pester tests that would have caught #269.
This issue also appears on line 771 of the same file.
Copy-FileFromRemoteComputerSMB -Path $Path -ComputerName $object -Destination $Destination -Credential $Credential -Force:($Force.IsPresent) -Recurse:($Recurse.IsPresent) -ErrorAction Stop
Description
Copy-FileToRemoteComputerandCopy-FileFromRemoteComputerdid not pass-Credentialto their SMB helper functions, causing "Access Denied" failures and unnecessary WinRM fallback.Since
Copy-Itemdoesn't support-Credentialfor filesystem paths, the fix usesNew-PSDriveto establish an authenticated SMB session before copying:-Credentialfrom parent functions toCopy-FileToRemoteComputerSMB/Copy-FileFromRemoteComputerSMBPSDrivemapped to the remote admin share when credentials are providedPSDrivein theendblockChange type
Checklist: