utils: use CertUtils.generateRandomKeyPair to create SSH keypair#12708
utils: use CertUtils.generateRandomKeyPair to create SSH keypair#12708weizhouapache wants to merge 1 commit intoapache:4.22from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the JSch library with the CertUtils utility for SSH key pair generation in the SSHKeysHelper class. The change modernizes the codebase by consolidating cryptographic operations under the already-used CertUtils class (which uses BouncyCastle) and removes a dependency on the JSch library.
Changes:
- Replaced JSch-based key generation with CertUtils.generateRandomKeyPair in the constructor
- Implemented manual SSH public key format encoding (ssh-rsa format with base64-encoded components)
- Replaced JSch's key serialization methods with CertUtils.privateKeyToPem for private key export
- Removed JSch dependency from both utils/pom.xml and the root pom.xml
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| utils/src/main/java/com/cloud/utils/ssh/SSHKeysHelper.java | Replaces JSch key generation and serialization with CertUtils methods; implements SSH public key encoding manually |
| utils/pom.xml | Removes JSch dependency from the utils module |
| pom.xml | Removes JSch version property and dependency management entry from root POM |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String getPrivateKey() { | ||
| try { | ||
| return CertUtils.privateKeyToPem(keyPair.getPrivate()); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this 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.
| 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(); | ||
| } | ||
| } |
There was a problem hiding this 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.
| 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; | ||
| } |
There was a problem hiding this 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.
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this 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.
| 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); | ||
| } |
There was a problem hiding this 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.
| public String getPrivateKey() { | ||
| try { | ||
| return CertUtils.privateKeyToPem(keyPair.getPrivate()); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this 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.
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this 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.
| 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; | ||
| } |
There was a problem hiding this 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).
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16945 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Description
This PR replaces the ssk key generation by jsch with CertUtils.generateRandomKeyPair
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?