Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ repos:
^services/console-proxy/rdpconsole/src/test/doc/rdp-key\.pem$|
^systemvm/agent/certs/localhost\.key$|
^systemvm/agent/certs/realhostip\.key$|
^test/integration/smoke/test_ssl_offloading\.py$
^test/integration/smoke/test_ssl_offloading\.py$|
^utils/src/test/java/com/cloud/utils/ssh/SSHKeysHelperTest\.java$
- id: end-of-file-fixer
exclude: \.vhd$
- id: file-contents-sorter
Expand Down
6 changes: 0 additions & 6 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@
<cs.jna.version>5.5.0</cs.jna.version>
<cs.joda-time.version>2.12.5</cs.joda-time.version>
<cs.jpa.version>2.2.1</cs.jpa.version>
<cs.jsch.version>0.1.55</cs.jsch.version>
<cs.json.version>20231013</cs.json.version>
<cs.jstl.version>1.2</cs.jstl.version>
<cs.kafka-clients.version>2.7.0</cs.kafka-clients.version>
Expand Down Expand Up @@ -335,11 +334,6 @@
<artifactId>java-ipv6</artifactId>
<version>${cs.java-ipv6.version}</version>
</dependency>
<dependency>
<groupId>com.jcraft</groupId>
<artifactId>jsch</artifactId>
<version>${cs.jsch.version}</version>
</dependency>
<dependency>
<groupId>com.rabbitmq</groupId>
<artifactId>amqp-client</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion test/integration/smoke/test_network.py
Original file line number Diff line number Diff line change
Expand Up @@ -2349,7 +2349,7 @@ def _get_ip_address_output(self, ssh):
return '\n'.join(res)

@attr(tags=["advanced", "shared"], required_hardware="true")
def test_01_deployVMInSharedNetwork(self):
def test_01_deployVMInSharedNetworkWithConfigDrive(self):
try:
self.virtual_machine = VirtualMachine.create(self.apiclient, self.services["virtual_machine"],
networkids=[self.shared_network.id, self.isolated_network.id],
Expand Down
4 changes: 0 additions & 4 deletions utils/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,6 @@
<groupId>org.bouncycastle</groupId>
<artifactId>bctls-jdk15on</artifactId>
</dependency>
<dependency>
<groupId>com.jcraft</groupId>
<artifactId>jsch</artifactId>
</dependency>
<dependency>
<groupId>org.jasypt</groupId>
<artifactId>jasypt</artifactId>
Expand Down
69 changes: 56 additions & 13 deletions utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,20 @@
package com.cloud.utils.ssh;

import java.io.ByteArrayOutputStream;
import java.io.StringWriter;
import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.security.KeyPair;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
import java.security.interfaces.RSAPublicKey;

import org.apache.cloudstack.utils.security.CertUtils;
import org.apache.commons.codec.binary.Base64;

import com.jcraft.jsch.JSch;
import com.jcraft.jsch.JSchException;
import com.jcraft.jsch.KeyPair;
import org.bouncycastle.util.io.pem.PemObject;
import org.bouncycastle.util.io.pem.PemWriter;

public class SSHKeysHelper {

Expand All @@ -45,8 +51,8 @@ private static String toHexString(byte[] b) {

public SSHKeysHelper(Integer keyLength) {
try {
keyPair = KeyPair.genKeyPair(new JSch(), KeyPair.RSA, keyLength);
} catch (JSchException e) {
keyPair = CertUtils.generateRandomKeyPair(keyLength);
} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
e.printStackTrace();
}
}
Expand Down Expand Up @@ -105,17 +111,54 @@ public static String getPublicKeyFromKeyMaterial(String keyMaterial) {
}

public String getPublicKey() {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
keyPair.writePublicKey(baos, "");
if (keyPair == null || keyPair.getPublic() == null) {
return null;
}
try {
RSAPublicKey rsaPublicKey = (RSAPublicKey) keyPair.getPublic();

ByteArrayOutputStream buffer = new ByteArrayOutputStream();

return baos.toString();
writeString(buffer,"ssh-rsa");
writeBigInt(buffer, rsaPublicKey.getPublicExponent());
writeBigInt(buffer, rsaPublicKey.getModulus());

String base64 = Base64.encodeBase64String(buffer.toByteArray());

return "ssh-rsa " + base64;
} catch (Exception e) {
e.printStackTrace();
}
return null;
}
Comment on lines 113 to 133
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The new implementation of getPublicKey that uses CertUtils.generateRandomKeyPair and manually constructs the SSH public key format is not covered by tests. The existing tests only verify static methods (getPublicKeyFromKeyMaterial and getPublicKeyFingerprint). Consider adding a test that creates an SSHKeysHelper instance, generates a key pair, retrieves the public key, and validates that it can be correctly parsed back and used for encryption (similar to what RSAHelper.encryptWithSSHPublicKey does).

Copilot uses AI. Check for mistakes.

public String getPrivateKey() {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
keyPair.writePrivateKey(baos);
private static void writeString(ByteArrayOutputStream out, String str) throws Exception {
byte[] data = str.getBytes(StandardCharsets.UTF_8);
out.write(ByteBuffer.allocate(4).putInt(data.length).array());
out.write(data);
}

private static void writeBigInt(ByteArrayOutputStream out, BigInteger value) throws Exception {
byte[] data = value.toByteArray();
out.write(ByteBuffer.allocate(4).putInt(data.length).array());
out.write(data);
}

return baos.toString();
public String getPrivateKey() {
if (keyPair == null || keyPair.getPrivate() == null) {
return null;
}
try {
final PemObject pemObject = new PemObject("RSA PRIVATE KEY", keyPair.getPrivate().getEncoded());
final StringWriter sw = new StringWriter();
try (final PemWriter pw = new PemWriter(sw)) {
pw.writeObject(pemObject);
}
return sw.toString();
Comment on lines +152 to +157
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The PEM type "RSA PRIVATE KEY" denotes PKCS#1 format, but keyPair.getPrivate().getEncoded() returns PKCS#8 (DER-encoded PrivateKeyInfo) bytes. This mismatch means the generated PEM file will have a PKCS#1 header wrapping PKCS#8 content, which will cause failures in tools (like OpenSSH) that try to parse it.

The codebase already handles this correctly in CertUtils.privateKeyToPem() (at utils/src/main/java/org/apache/cloudstack/utils/security/CertUtils.java:134) which uses "PRIVATE KEY" (PKCS#8 header) with key.getEncoded().

There are two correct approaches:

  1. Use "PRIVATE KEY" as the PEM type (PKCS#8 format), matching what CertUtils.privateKeyToPem() does, or better yet, just call CertUtils.privateKeyToPem(keyPair.getPrivate()) directly.
  2. Convert the key to PKCS#1 format using BouncyCastle's PrivateKeyInfo and RSAPrivateKey before wrapping it with the "RSA PRIVATE KEY" header.

Note: Option 1 would change the output format from PKCS#1 (which JSch previously produced) to PKCS#8. If downstream consumers specifically expect PKCS#1, option 2 would be needed for backward compatibility. However, most modern SSH tools accept both formats.

Suggested change
final PemObject pemObject = new PemObject("RSA PRIVATE KEY", keyPair.getPrivate().getEncoded());
final StringWriter sw = new StringWriter();
try (final PemWriter pw = new PemWriter(sw)) {
pw.writeObject(pemObject);
}
return sw.toString();
return CertUtils.privateKeyToPem(keyPair.getPrivate());

Copilot uses AI. Check for mistakes.
} catch (Exception e) {
e.printStackTrace();
}
return null;
}

}
58 changes: 58 additions & 0 deletions utils/src/test/java/com/cloud/utils/ssh/SSHKeysHelperTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@

package com.cloud.utils.ssh;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import java.nio.charset.StandardCharsets;
import java.util.Base64;

import org.junit.Test;

public class SSHKeysHelperTest {
Expand Down Expand Up @@ -70,4 +76,56 @@ public void dsaKeyTest() {
assertTrue("fc:6e:ef:31:93:f8:92:2b:a9:03:c7:06:90:f5:ec:bb".equals(fingerprint));

}

@Test
public void getPublicKeyFromKeyMaterialShouldHandleSupportedPrefixes() {
assertEquals("ecdsa-sha2-nistp256 AAAA", SSHKeysHelper.getPublicKeyFromKeyMaterial("ecdsa-sha2-nistp256 AAAA comment"));
assertEquals("ecdsa-sha2-nistp384 AAAA", SSHKeysHelper.getPublicKeyFromKeyMaterial("ecdsa-sha2-nistp384 AAAA comment"));
assertEquals("ecdsa-sha2-nistp521 AAAA", SSHKeysHelper.getPublicKeyFromKeyMaterial("ecdsa-sha2-nistp521 AAAA comment"));
assertEquals("ssh-ed25519 AAAA", SSHKeysHelper.getPublicKeyFromKeyMaterial("ssh-ed25519 AAAA comment"));
}

@Test
public void getPublicKeyFromKeyMaterialShouldParseBase64EncodedMaterial() {
String keyMaterial = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITestKeyData comment";
String encoded = Base64.getEncoder().encodeToString(keyMaterial.getBytes(StandardCharsets.UTF_8));

assertEquals("ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITestKeyData", SSHKeysHelper.getPublicKeyFromKeyMaterial(encoded));
}

@Test
public void getPublicKeyFromKeyMaterialShouldReturnNullForInvalidFormats() {
assertNull(SSHKeysHelper.getPublicKeyFromKeyMaterial("not-a-valid-key"));
assertNull(SSHKeysHelper.getPublicKeyFromKeyMaterial("ssh-unknown AAAA"));
assertNull(SSHKeysHelper.getPublicKeyFromKeyMaterial("ssh-rsa"));
}

@Test(expected = RuntimeException.class)
public void getPublicKeyFingerprintShouldThrowForInvalidPublicKey() {
SSHKeysHelper.getPublicKeyFingerprint("invalid-key-format");
}

@Test
public void generatedKeysShouldBeWellFormedAndFingerprintConsistent() {
SSHKeysHelper helper = new SSHKeysHelper(2048);

String publicKey = helper.getPublicKey();
String privateKey = helper.getPrivateKey();
String fingerprint = helper.getPublicKeyFingerPrint();

assertNotNull(publicKey);
assertTrue(publicKey.startsWith("ssh-rsa "));

String[] keyParts = publicKey.split(" ");
assertEquals(2, keyParts.length);

assertNotNull(privateKey);
assertTrue(privateKey.contains("BEGIN RSA PRIVATE KEY"));
assertTrue(privateKey.contains("END RSA PRIVATE KEY"));
Comment on lines +123 to +124
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The test asserts "BEGIN RSA PRIVATE KEY" and "END RSA PRIVATE KEY", but keyPair.getPrivate().getEncoded() returns PKCS#8 format bytes. Using the PEM type "RSA PRIVATE KEY" (PKCS#1 header) with PKCS#8 content is incorrect. If the production code is fixed to use "PRIVATE KEY" (the correct PKCS#8 header), these assertions will need to be updated to check for "BEGIN PRIVATE KEY" and "END PRIVATE KEY" instead. Alternatively, if PKCS#1 is chosen, the production code needs to convert the key format accordingly.

Suggested change
assertTrue(privateKey.contains("BEGIN RSA PRIVATE KEY"));
assertTrue(privateKey.contains("END RSA PRIVATE KEY"));
assertTrue(privateKey.contains("BEGIN PRIVATE KEY"));
assertTrue(privateKey.contains("END PRIVATE KEY"));

Copilot uses AI. Check for mistakes.

assertNotNull(fingerprint);
assertEquals(SSHKeysHelper.getPublicKeyFingerprint(publicKey), fingerprint);

assertTrue("Legacy MD5 fingerprint should be colon-separated hex", fingerprint.matches("^([0-9a-f]{2}:){15}[0-9a-f]{2}$"));
}
}
Loading