Improving logging and adding a prompt to regen SSH Keys by DevanshG1 · Pull Request #29413 · Azure/azure-powershell

DevanshG1 · 2026-04-16T06:10:32Z

Description

Mandatory Checklist

Please choose the target release of Azure PowerShell. (⚠️Target release is a different concept from API readiness. Please click below links for details.)
- General release
- Public preview
- Private preview
- Engineering build
- No need for a release
Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

SHOULD update ChangeLog.md file(s) appropriately
- Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
  - A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
- Should not change ChangeLog.md if no new release is required, such as fixing test case only.
SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
SHOULD have proper test coverage for changes in pull request.
SHOULD NOT adjust version of module manually in pull request

azure-client-tools-bot-prd · 2026-04-16T06:10:35Z

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

azure-pipelines · 2026-04-16T06:10:47Z

Azure Pipelines: Successfully started running 2 pipeline(s). 2 pipeline(s) require an authorized user to comment /azp run to run.

VeryEarly · 2026-04-16T06:12:13Z

/azp run

azure-pipelines · 2026-04-16T06:12:27Z

Azure Pipelines: Successfully started running 3 pipeline(s).

Copilot

Pull request overview

This PR enhances the Sftp module’s operational transparency and safety by adding more structured verbose/debug logging and introducing an interactive confirmation flow (with -Force bypass) when SSH key pairs already exist at the target location.

Changes:

Added overwrite confirmation logic for existing SSH key pairs (and a new -Force switch to bypass prompts).
Expanded verbose/debug logging with timing and more contextual details (key prep, cert request, session duration, cleanup).
Updated module changelog to document the new prompt/-Force and logging improvements.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
src/Sftp/Sftp/SftpCommands/NewAzSftpCertificateCommand.cs	Prompts before overwriting default `id_rsa` key pair and adds timing verbose logs around key prep and cert request.
src/Sftp/Sftp/SftpCommands/ConnectAzSftpCommand.cs	Adds pre-generation key overwrite prompt behavior, timing logs, and more detailed session/cleanup verbose logs.
src/Sftp/Sftp/Common/SftpBaseCmdlet.cs	Introduces `-Force` and centralizes key-pair overwrite prompting logic; enhances base verbose logging and SSH client validation logs.
src/Sftp/Sftp/CHANGELOG.md	Documents the new key overwrite prompt/`-Force` and the expanded logging.

Copilot · 2026-04-16T06:17:38Z

+                    else if (targetPrivateKey != null && (File.Exists(targetPrivateKey) || (targetPublicKey != null && File.Exists(targetPublicKey))))
+                    {
+                        WriteVerbose("[Auth] Overwriting existing SSH key pair per user confirmation");
+                        if (File.Exists(targetPrivateKey)) File.Delete(targetPrivateKey);
+                        if (targetPublicKey != null && File.Exists(targetPublicKey)) File.Delete(targetPublicKey);


The overwrite path deletes existing key files via File.Delete(...) without consistent error wrapping. Prefer FileUtils.DeleteFile(...) (already used in cleanup) or handle IO/ACL exceptions and rethrow as AzPSIOException so failures produce actionable, consistent errors.

Copilot · 2026-04-16T06:17:39Z

+            if (Force.IsPresent)
+            {
+                WriteVerbose("[KeyPair] -Force specified, will overwrite existing key pair");
+                return true;
+            }
+
+            bool shouldOverwrite = ShouldContinue(
+                $"An existing SSH key pair was found in '{keysFolder}' ({existingFiles}). " +
+                "Selecting 'Yes' will generate a new key pair and overwrite the existing files. " +
+                "Selecting 'No' will use the existing key pair for certificate generation.",
+                "Existing SSH key pair detected");


New interactive behavior was added (confirmation prompt + -Force to bypass) but there are existing scenario/unit tests in src/Sftp/Sftp.Test that don’t appear to cover the new overwrite/reuse decision paths. Add tests for: (1) existing keys + -Force overwrites without prompting, (2) existing keys + decline prompt reuses keys, and (3) incomplete key pair handling.

Copilot · 2026-04-16T06:17:39Z

+            if (!privateKeyExists && !publicKeyExists)
+            {
+                return true; // No existing keys, safe to generate
+            }
+
+            string keysFolder = Path.GetDirectoryName(privateKeyFile ?? publicKeyFile);
+            string existingFiles = privateKeyExists && publicKeyExists
+                ? "private key and public key"
+                : privateKeyExists ? "private key only" : "public key only";


ShouldRegenerateKeyPair treats a partial key pair (only private or only public key present) as something the user can choose to reuse. If the user selects “No”, downstream flows (e.g., CheckOrCreatePublicPrivateFiles) can still end up generating/overwriting keys or proceeding without a usable private key. Consider treating “only one file exists” as an invalid/incomplete pair: either force regeneration (return true without prompting) or stop with a clear error indicating both files are required to reuse existing keys.

Copilot · 2026-04-16T06:17:39Z

+            string keysFolder = Path.GetDirectoryName(privateKeyFile ?? publicKeyFile);
+            string existingFiles = privateKeyExists && publicKeyExists
+                ? "private key and public key"
+                : privateKeyExists ? "private key only" : "public key only";
+
+            WriteVerbose($"[KeyPair] Existing SSH key pair detected in '{keysFolder}' ({existingFiles})");


keysFolder is computed as Path.GetDirectoryName(privateKeyFile ?? publicKeyFile), but privateKeyFile can be an empty string (PowerShell can bind ""). In that case ?? will pick the empty string and GetDirectoryName("") returns null, leading to confusing prompt/log text. Prefer choosing the first non-empty path (e.g., !string.IsNullOrEmpty(privateKeyFile) ? privateKeyFile : publicKeyFile) before calling GetDirectoryName.

Copilot · 2026-04-16T06:17:40Z

+        /// <summary>
+        /// Forces the command to run without asking for user confirmation when overwriting existing SSH key pairs.
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "Forces the command to run without asking for user confirmation when overwriting existing SSH key pairs.")]
+        public SwitchParameter Force { get; set; }


A new -Force parameter is introduced on the base cmdlet, which will surface on both Connect-AzSftp and New-AzSftpCertificate, but the module’s generated help markdown doesn’t reflect it. Regenerate/update the help files so users can discover and understand -Force.

Copilot · 2026-04-16T06:17:40Z

+                        File.Delete(actualPrivateKeyFile);
+                    }
+                    if (File.Exists(actualPublicKeyFile))
+                    {
+                        WriteVerbose($"[CertGen] Removing existing public key: '{actualPublicKeyFile}'");
+                        File.Delete(actualPublicKeyFile);


The new overwrite flow uses File.Delete(...) directly. If deletion fails (e.g., ACLs, file in use), it will throw raw IOException/UnauthorizedAccessException and bypass the module’s standard error wrapping. Prefer using FileUtils.DeleteFile(...) (used elsewhere in this module) or catching and rethrowing as AzPSIOException with a helpful message.

Suggested change

File.Delete(actualPrivateKeyFile);

}

if (File.Exists(actualPublicKeyFile))

{

WriteVerbose($"[CertGen] Removing existing public key: '{actualPublicKeyFile}'");

File.Delete(actualPublicKeyFile);

FileUtils.DeleteFile(actualPrivateKeyFile);

}

if (File.Exists(actualPublicKeyFile))

{

WriteVerbose($"[CertGen] Removing existing public key: '{actualPublicKeyFile}'");

FileUtils.DeleteFile(actualPublicKeyFile);

azure-pipelines · 2026-04-16T06:51:43Z

Azure Pipelines: Successfully started running 2 pipeline(s). 2 pipeline(s) require an authorized user to comment /azp run to run.

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

azure-pipelines · 2026-04-16T07:04:00Z

Azure Pipelines: Successfully started running 2 pipeline(s). 2 pipeline(s) require an authorized user to comment /azp run to run.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Copilot · 2026-04-16T07:11:49Z

+                        if (File.Exists(targetPrivateKey)) File.Delete(targetPrivateKey);
+                        if (targetPublicKey != null && File.Exists(targetPublicKey)) File.Delete(targetPublicKey);


This overwrite path deletes key files using File.Delete(...) directly. Elsewhere in this module (and later in this cmdlet) deletion uses FileUtils.DeleteFile(...) for consistent exception handling and AzPS-friendly errors. Consider switching these deletes to FileUtils.DeleteFile(...) (and keeping the log message) so failures don’t surface as unhandled IO/UnauthorizedAccess exceptions.

Suggested change

if (File.Exists(targetPrivateKey)) File.Delete(targetPrivateKey);

if (targetPublicKey != null && File.Exists(targetPublicKey)) File.Delete(targetPublicKey);

if (File.Exists(targetPrivateKey)) FileUtils.DeleteFile(targetPrivateKey);

if (targetPublicKey != null && File.Exists(targetPublicKey)) FileUtils.DeleteFile(targetPublicKey);

Copilot · 2026-04-16T07:11:49Z

+    - Both 'New-AzSftpCertificate' and 'Connect-AzSftp' now detect existing key pairs before generating new ones
+    - Users are prompted to choose whether to overwrite existing keys or reuse them


Changelog guidance in this repo asks for entries under “Upcoming Release” to be written in the past tense. The sub-bullets here use present tense phrasing like “now detect” / “Users are prompted…”. Consider adjusting these to past tense (e.g., “detected”, “prompted”) for consistency.

Suggested change

- Both 'New-AzSftpCertificate' and 'Connect-AzSftp' now detect existing key pairs before generating new ones

- Users are prompted to choose whether to overwrite existing keys or reuse them

- Both 'New-AzSftpCertificate' and 'Connect-AzSftp' detected existing key pairs before generating new ones

- Users were prompted to choose whether to overwrite existing keys or reuse them

Copilot · 2026-04-16T07:11:50Z

+    /// <summary>
+    /// Tests for the SSH key pair overwrite prompt behavior.
+    /// Validates that both New-AzSftpCertificate and Connect-AzSftp
+    /// have a Force parameter and support ShouldContinue confirmation
+    /// when existing SSH key pairs are detected.
+    /// </summary>


These tests mostly verify reflection/constant values and filesystem setup, but they don’t actually exercise the new overwrite decision logic (ShouldRegenerateKeyPair) or validate that selecting “No” prevents any key regeneration/overwrite. Consider adding behavior-focused tests (e.g., via a small testable subclass that exposes the protected method and a mocked ShouldContinue decision, or via running the cmdlets in a PowerShell runspace with $ConfirmPreference) to cover overwrite vs reuse outcomes and partial-key scenarios.

Copilot · 2026-04-16T07:11:50Z

+                    // Check for existing key pair at target location before generation
+                    string targetPrivateKey = !string.IsNullOrEmpty(PrivateKeyFile) ? PrivateKeyFile
+                        : (!string.IsNullOrEmpty(credentialsFolder) ? Path.Combine(credentialsFolder, SftpConstants.SshPrivateKeyName) : null);
+                    string targetPublicKey = !string.IsNullOrEmpty(PublicKeyFile) ? PublicKeyFile
+                        : (targetPrivateKey != null ? targetPrivateKey + ".pub" : null);
+
+                    if (targetPrivateKey != null && !ShouldRegenerateKeyPair(targetPrivateKey, targetPublicKey))
+                    {
+                        WriteVerbose("[Auth] Using existing SSH key pair for certificate generation");
+                    }
+                    else if (targetPrivateKey != null && (File.Exists(targetPrivateKey) || (targetPublicKey != null && File.Exists(targetPublicKey))))
+                    {
+                        WriteVerbose("[Auth] Overwriting existing SSH key pair per user confirmation");
+                        if (File.Exists(targetPrivateKey)) File.Delete(targetPrivateKey);
+                        if (targetPublicKey != null && File.Exists(targetPublicKey)) File.Delete(targetPublicKey);
+                    }
+
+                    var keyGenStartTime = DateTime.UtcNow;
                    var (pubKey, privKey, delKeys) = FileUtils.CheckOrCreatePublicPrivateFiles(
                        PublicKeyFile, PrivateKeyFile, credentialsFolder, SshClientFolder);
                    PublicKeyFile = pubKey;


ShouldRegenerateKeyPair(...) can return false (reuse) even when only one of the key files exists. But the subsequent CheckOrCreatePublicPrivateFiles(...) call may still trigger key regeneration when the private key is missing, which can overwrite/delete the existing .pub file (via CreateSshKeyfile). Consider detecting the “partial key pair” case here and either (a) fail fast with guidance, or (b) force regeneration explicitly, so the user’s choice can’t result in unexpected overwrites.

Copilot · 2026-04-16T07:11:50Z

@@ -206,9 +226,13 @@ protected override void ProcessRecord()
                // Check for cancellation before starting
                CmdletCancellationToken.ThrowIfCancellationRequested();

+                WriteVerbose("[CertGen] Preparing SSH key pair...");
+                var keyPrepStartTime = DateTime.UtcNow;
                var (pubKeyFile, _, _) = FileUtils.CheckOrCreatePublicPrivateFiles(
                    actualPublicKeyFile, actualPrivateKeyFile, keysFolder, SshClientFolder);
                actualPublicKeyFile = pubKeyFile;


If ShouldRegenerateKeyPair(...) returns false (user chose to reuse existing keys) but the key pair is incomplete (e.g., only id_rsa.pub exists), CheckOrCreatePublicPrivateFiles(...) can still regenerate keys because the private key is missing—overwriting/deleting the existing public key. To ensure the prompt decision is respected, handle the “reuse” branch by (a) validating both files exist before proceeding, or (b) skipping any regeneration path and failing with a clear message when the pair is incomplete.

VeryEarly · 2026-04-16T07:12:12Z

/azp run

azure-pipelines · 2026-04-16T07:12:28Z

Azure Pipelines: Successfully started running 3 pipeline(s).

VeryEarly

please regenerate help markdowns:
https://github.com/Azure/azure-powershell/blob/main/documentation/development-docs/help-generation.md

Improving logging and adding a prompt to regen SSH Keys

edad9b1

Copilot AI review requested due to automatic review settings April 16, 2026 06:10

Copilot started reviewing on behalf of DevanshG1 April 16, 2026 06:12 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Adding tests

31950f7

Update src/Sftp/Sftp/SftpCommands/NewAzSftpCertificateCommand.cs

5a4e51e

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

Copilot AI review requested due to automatic review settings April 16, 2026 07:03

Copilot started reviewing on behalf of DevanshG1 April 16, 2026 07:05 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

VeryEarly self-assigned this Apr 17, 2026

VeryEarly reviewed Apr 17, 2026

View reviewed changes

		if (File.Exists(targetPrivateKey)) File.Delete(targetPrivateKey);
		if (targetPublicKey != null && File.Exists(targetPublicKey)) File.Delete(targetPublicKey);

		- Both 'New-AzSftpCertificate' and 'Connect-AzSftp' now detect existing key pairs before generating new ones
		- Users are prompted to choose whether to overwrite existing keys or reuse them

Conversation

DevanshG1 commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Mandatory Checklist

Uh oh!

azure-client-tools-bot-prd bot commented Apr 16, 2026

Uh oh!

azure-pipelines bot commented Apr 16, 2026

Uh oh!

VeryEarly commented Apr 16, 2026

Uh oh!

azure-pipelines bot commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azure-pipelines bot commented Apr 16, 2026

Uh oh!

azure-pipelines bot commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

VeryEarly commented Apr 16, 2026

Uh oh!

azure-pipelines bot commented Apr 16, 2026

Uh oh!

VeryEarly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DevanshG1 commented Apr 16, 2026 •

edited

Loading