Copilot commented on code in PR #12708:
URL: https://github.com/apache/cloudstack/pull/12708#discussion_r2889388286
##########
utils/src/test/java/com/cloud/utils/ssh/SSHKeysHelperTest.java:
##########
@@ -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"));
Review Comment:
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.
```suggestion
assertTrue(privateKey.contains("BEGIN PRIVATE KEY"));
assertTrue(privateKey.contains("END PRIVATE KEY"));
```
##########
utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java:
##########
@@ -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;
}
- 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();
Review Comment:
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.
```suggestion
return CertUtils.privateKeyToPem(keyPair.getPrivate());
```
--
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]
