Copilot commented on code in PR #12708:
URL: https://github.com/apache/cloudstack/pull/12708#discussion_r2910821426
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -105,17 +110,53 @@ 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");
Review Comment:
Minor formatting: missing space after the comma between `buffer` and
`"ssh-rsa"`. This is inconsistent with the style on the next two lines
(122-123) which have a space after the comma.
```suggestion
writeString(buffer, "ssh-rsa");
```
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -105,17 +110,53 @@ 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();
Review Comment:
Using `e.printStackTrace()` is inappropriate for production code — it writes
to stderr and is easy to miss. This pattern appears in both `getPublicKey()`
(line 129) and `getPrivateKey()` (line 157). These should use a proper logger
(the project likely has a logging framework available). The same issue exists
in the constructor at line 55, though that was pre-existing.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -105,17 +110,53 @@ 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) {
Review Comment:
Catching `Exception` is overly broad. Since the helper methods
(`writeString`, `writeBigInt`) don't actually throw checked exceptions and the
only real operation is byte array manipulation, there shouldn't be a need for
this catch at all. If you do keep it, catch a more specific exception type like
`IOException`.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -105,17 +110,53 @@ 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;
}
- 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 {
Review Comment:
The `writeString` and `writeBigInt` methods declare `throws Exception`,
which is overly broad. `ByteArrayOutputStream.write` does not throw checked
exceptions, so neither method can actually throw a checked exception. The
`throws Exception` clause should be removed from both methods.
```suggestion
private static void writeString(ByteArrayOutputStream out, String str) {
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) {
```
##########
test/integration/smoke/test_network.py:
##########
@@ -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):
Review Comment:
This test method rename from `test_01_deployVMInSharedNetwork` to
`test_01_deployVMInSharedNetworkWithConfigDrive` appears unrelated to the PR's
purpose of replacing JSch with CertUtils for SSH key generation. If this is
intentional, it should be called out in the PR description; otherwise, it may
be an accidental inclusion.
--
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]
