Skip to content

[Az.Aks] Enforce RSA as the GenerateSSHKeys default key type #28998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jovieir wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[Az.Aks] Enforce RSA as the GenerateSSHKeys default key type #28998

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed9b1a9
Enforce RSA as the default key type
Dec 19, 2025
d6afc58
Update src/Aks/Aks/Commands/NewAzureRmAks.cs
jovieir Dec 19, 2025
e3a7aca
Apply suggestions from code review
jovieir Dec 19, 2025
699e49c
Apply suggestions from code review
jovieir Dec 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 66 additions & 1 deletion src/Aks/Aks/Commands/NewAzureRmAks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
using Microsoft.Rest;
using Microsoft.WindowsAzure.Commands.Common;
using Microsoft.WindowsAzure.Commands.Utilities.Common;
using System.Runtime.InteropServices;

namespace Microsoft.Azure.Commands.Aks
{
Expand Down Expand Up @@ -311,6 +312,61 @@ private void PreValidate()
}
}

// From OpenSSH 9.4 onwards, the default key type is ed25519, which is not supported in AKS.
// To ensure backward compatibility, we need to check the OpenSSH version installed and adjust the parameters accordingly.
static Version GetOpenSSHVersion()
{
using (Process process = new Process())
{
try
{
// Using runtime information to determine the path to ssh.exe based on OS type.
bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
string defaultWindowsSshPath = @"C:\Windows\System32\OpenSSH\ssh.exe";
process.StartInfo.FileName = isWindows && File.Exists(defaultWindowsSshPath) ? defaultWindowsSshPath : "ssh";
process.StartInfo.Arguments = "-V";
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardInput = true;
process.StartInfo.RedirectStandardError = true;
process.StartInfo.RedirectStandardOutput = true;
process.StartInfo.CreateNoWindow = true;
process.Start();

string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd();

process.WaitForExit();
Comment on lines +335 to +337
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process output reading on line 335 may cause a deadlock if the output buffers fill up. Reading both StandardOutput and StandardError synchronously after process.Start() can block if either buffer becomes full before WaitForExit is called. Consider using asynchronous reading with BeginOutputReadLine and BeginErrorReadLine, or read StandardError and StandardOutput separately to avoid potential deadlock situations.

Copilot uses AI. Check for mistakes.
// OpenSSH version output follows formats like:
// Windows: "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3"
// Windows: "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2"
// Linux/macOS: "OpenSSH_9.5p1, OpenSSL 3.0.13 30 Jan 2024"
// We match the common "OpenSSH_" prefix and make "for_Windows_" optional so this works across platforms.
var regMatch = System.Text.RegularExpressions.Regex.Match(standardOutput, @"OpenSSH_(?:for_Windows_)?(\d+)\.(\d+)");

// We don't really care about the patch version, so only return major and minor version.
return regMatch.Success ? new Version(int.Parse(regMatch.Groups[1].Value), int.Parse(regMatch.Groups[2].Value)) : null;
}
// We're not expecting ssh to be missing, but just in case, we catch any exception and return null.
catch
{
return null;
}
Comment on lines +348 to +352
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broad catch block on line 349 silently swallows all exceptions, which could hide important errors beyond just missing SSH binaries. Consider catching specific exceptions like FileNotFoundException or Win32Exception, and logging the exception details using WriteDebug or WriteWarning before returning null. This would help with troubleshooting when SSH version detection fails.

Suggested change
// We're not expecting ssh to be missing, but just in case, we catch any exception and return null.
catch
{
return null;
}
// We're not expecting ssh to be missing, but just in case, we catch specific exceptions and return null.
catch (FileNotFoundException ex)
{
WriteDebug($"Failed to detect SSH version: SSH executable not found. Detail: {ex.Message}");
return null;
}
catch (Win32Exception ex)
{
WriteDebug($"Failed to detect SSH version due to a Win32 error when starting the SSH process. Detail: {ex.Message}");
return null;
}
catch (InvalidOperationException ex)
{
WriteDebug($"Failed to detect SSH version due to an invalid process configuration. Detail: {ex.Message}");
return null;
}
catch (Exception ex)
{
WriteDebug($"Failed to detect SSH version due to an unexpected error. Detail: {ex.Message}");
return null;
}

Copilot uses AI. Check for mistakes.
finally //Ensure process is properly disposed of and exited
{
if (!process.HasExited)
{
try
{
process.Kill();
}
catch
{
// Ignore exceptions from Kill if process already exited
}
}
}
}
}
Comment on lines +315 to +368
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ChangeLog.md file is missing an entry for this SSH key generation change. According to the repository's contribution guidelines, you SHOULD update the ChangeLog.md file under the "Upcoming Release" section to document this change. Add a bullet point describing the enforcement of RSA as the default key type for OpenSSH 9.4 and above to ensure AKS compatibility.

Copilot uses AI. Check for mistakes.

private string GenerateSshKeyValue()
{
String generateSshKeyFolder = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".ssh");
Expand All @@ -329,8 +385,17 @@ private string GenerateSshKeyValue()
{
try
{
// Validate if OpenSSH version is 9.4 or above to add -t rsa argument
// Which has been defaulted to ed25519 from OpenSSH 9.4 onwards
// https://github.com/openssh/openssh-portable/blob/master/ssh-keygen.c#L70
var openSshVersion = GetOpenSSHVersion();
// If we cannot determine the OpenSSH version, we assume it's below 9.4 to maintain compatibility.
// If openSshVersion isn't null and is >= 9.4, we add the -t rsa argument, otherwise we skip it
var keyTypeArgument = openSshVersion != null && openSshVersion >= new Version(9, 4) ? "-t rsa " : "";
process.StartInfo.FileName = "ssh-keygen";
process.StartInfo.Arguments = String.Format("-f \"{0}\"", generateSshKeyPath);
// if keyTypeArgument is empty, we skip it to maintain compatibility with older OpenSSH versions
// Otherwise, we explicitly set the key type to rsa
process.StartInfo.Arguments = String.Format("{0}-f \"{1}\"", keyTypeArgument, generateSshKeyPath);
Comment on lines +388 to +398
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new OpenSSH version detection logic and conditional SSH key type generation are not covered by automated tests. Given that the Aks.Test project contains comprehensive test scenarios for other module functionality, this new behavior should have test coverage to verify correct version detection, version comparison logic, and proper argument construction for both OpenSSH versions below and above 9.4.

Copilot uses AI. Check for mistakes.
process.StartInfo.UseShellExecute = false;
process.StartInfo.RedirectStandardInput = true;
process.StartInfo.RedirectStandardError = true;
Expand Down
Loading