Skip to content

utils: use CertUtils.generateRandomKeyPair to create SSH keypair #12708

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

Draft
weizhouapache wants to merge 1 commit into
base: 4.22
Choose a base branch
from
+41 −23
Draft

utils: use CertUtils.generateRandomKeyPair to create SSH keypair #12708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 0 additions & 6 deletions pom.xml
View file
Open in desktop
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
4 changes: 0 additions & 4 deletions utils/pom.xml
View file
Open in desktop
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
54 changes: 41 additions & 13 deletions utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,17 @@
package com.cloud.utils.ssh;

import java.io.ByteArrayOutputStream;
import java.math.BigInteger;
import java.nio.ByteBuffer;
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;

public class SSHKeysHelper {

private KeyPair keyPair;
Expand All @@ -45,8 +47,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();
}
}
Comment on lines 48 to 54
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 constructor catches exceptions and uses printStackTrace without proper logging. Following the codebase convention seen in other utility classes (like CertUtils, RSAHelper, SSHCmdHelper), this class should have a Logger field and use LOGGER.error to log exceptions instead of printStackTrace. Additionally, if the keyPair generation fails, it remains null which could cause NullPointerException in getPublicKey and getPrivateKey methods.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -105,17 +107,43 @@ public static String getPublicKeyFromKeyMaterial(String keyMaterial) {
}

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

ByteArrayOutputStream buffer = new ByteArrayOutputStream();

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

return baos.toString();
String base64 = Base64.encodeBase64String(buffer.toByteArray());

return "ssh-rsa " + base64;
} catch (Exception e) {
e.printStackTrace();
}
return null;
}
Comment on lines 109 to 126
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.

This method should check if keyPair is null before attempting to access it. If the constructor's key generation failed (caught exception at line 51), keyPair will be null and this will throw a NullPointerException. Consider adding a null check and either throwing a more descriptive exception or returning a meaningful error.

Copilot uses AI. Check for mistakes.
Comment on lines +122 to 126
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.

This method catches a generic Exception and uses printStackTrace instead of proper logging. Following the codebase convention seen in other utility classes, a Logger should be added to the class and used here. Also, the broad Exception catch masks specific error conditions that could be helpful for debugging.

Copilot uses AI. Check for mistakes.
Comment on lines 109 to 126
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("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);
}
Comment on lines +128 to +138
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.

These helper methods declare throwing a generic Exception which is not a best practice. Consider declaring more specific exceptions (e.g., IOException, UnsupportedEncodingException) or using StandardCharsets.UTF_8 instead of the hardcoded "UTF-8" string to avoid UnsupportedEncodingException entirely.

Copilot uses AI. Check for mistakes.

return baos.toString();
public String getPrivateKey() {
try {
return CertUtils.privateKeyToPem(keyPair.getPrivate());
} catch (Exception e) {
e.printStackTrace();
}
return null;
}
Comment on lines +140 to 147
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 getPrivateKey that uses CertUtils.privateKeyToPem is not covered by tests. Consider adding a test that creates an SSHKeysHelper instance, generates a key pair, retrieves both public and private keys, and validates that the private key in PEM format can be correctly parsed and used to decrypt data encrypted with the corresponding public key.

Copilot uses AI. Check for mistakes.
Comment on lines +140 to 147
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.

This method should check if keyPair is null before attempting to access it. If the constructor's key generation failed, keyPair will be null and this will throw a NullPointerException at line 142. Consider adding a null check and proper error handling.

Copilot uses AI. Check for mistakes.
Comment on lines +143 to 147
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.

This method catches a generic Exception and uses printStackTrace instead of proper logging. Following the codebase convention, a Logger field should be added to the class and used here with LOGGER.error.

Copilot uses AI. Check for mistakes.

}
Loading