Copilot commented on code in PR #12708:
URL: https://github.com/apache/cloudstack/pull/12708#discussion_r2853087050
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
- 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);
+ }
- return baos.toString();
+ public String getPrivateKey() {
+ try {
+ return CertUtils.privateKeyToPem(keyPair.getPrivate());
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ return null;
}
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
- 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);
+ }
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
- 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);
+ }
- return baos.toString();
+ public String getPrivateKey() {
+ try {
+ return CertUtils.privateKeyToPem(keyPair.getPrivate());
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ return null;
}
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
- 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);
+ }
- return baos.toString();
+ public String getPrivateKey() {
+ try {
+ return CertUtils.privateKeyToPem(keyPair.getPrivate());
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ return null;
}
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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();
}
}
Review Comment:
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.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
Review Comment:
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).
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
