fix: Modify the terminal connection method#8415
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| } | ||
| let title = res.data.user + '@' + res.data.addr + ':' + res.data.port; | ||
| if (res.data.name.length !== 0) { | ||
| title = res.data.name + '-' + title; |
There was a problem hiding this comment.
The main changes detected are:
-
The
hostInfoobject has been renamed toform. This change affects all subsequent references to this data structure, making it consistent and potentially easier to maintain. -
A new property
isLocalhas been added to theformobject. This seems unused and might be redundant depending on the current logic of handling local hosts separately from other non-local ones. -
The
loadLocalmethod has been introduced to initialize certain fields when opening the dialog withisLocalset to true. -
A conditional check (
if (form.isLocal)) is used in bothsetDefaultandsubmitAddHostmethods to handle different behaviors based on whether a local host is being configured or edited.
Suggestions for Optimization/Improvement
-
Remove Redundant Code: If there's no specific reason to keep
hostInfoaround after moving most of its properties intoform, consider removing it entirely. -
Ensure that any logic related to setting defaults or initializing fields for different scenarios is clearly documented and easy to follow.
-
Verify if updating other parts of the application to use the new
formconsistently would eliminate redundancy elsewhere in the codebase. -
Consider adding some form validation checks at runtime during the initial setup phase to ensure all necessary fields are correctly filled out before proceeding further.
These suggested adjustments aim to improve clarity and possibly streamline future maintenance of the component.
| connAfterEncrypt, _ := encrypt.StringEncrypt(string(localConn)) | ||
| _ = settingRepo.Update("LocalSSHConn", connAfterEncrypt) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The code provided contains several minor improvements and corrections to handle SSH connection encryption, decryption, serialization, and deserialization in addition to setting management:
Minor Changes and Corrections:
- Base64 Encoding and Decoding: Added calls to
base64.StdEncoding.DecodeStringfor password and private key fields when updating them after retrieving from the repository. - Error Handling for Base64 Decoding: Included checks for errors when decoding password and private key base64 strings before using them.
- Connection Testing Logic: The
TestConnByInfofunction now correctly handles authentication methods ("password" and "key") by decrypting passwords before attempting SSH connections.
Additional Features:
- Encrypted Password Management: Ensures passwords are stored encrypted, preventing exposure of sensitive information.
- SSH Client Configuration: The implementation includes creation and testing of an SSH client based on user-provided connection details.
Overall, these changes improve the security and functionality of the SSH settings management service within your application.
| return nil, err | ||
| } | ||
| return ssh.NewClient(connInDB) | ||
| } |
There was a problem hiding this comment.
The code changes seem mostly correct, but there are a few areas where improvements can be made:
-
Error Handling: In
loadLocalConn, ensure that all errors are checked immediately after each operation to avoid cascading errors. -
Database Error Handling: Make sure database operations return appropriate errors and handle them appropriately.
-
User Home Dir Check: Ensure that the directory exists before attempting to read from it, especially if it contains sensitive information like SSH keys.
-
Code DRYness: Consider extracting common logic into utility functions to reduce redundancy and improve readability.
Here's an improved version of the function with some minor corrections:
func loadLocalConn() (*ssh.SSHClient, error) {
currentInfo, err := user.Current()
if err != nil {
return(nil, fmt.Errorf("failed to get current user: %w", err))
}
itemPath := "/"
if len(currentInfo.HomeDir) > 0 {
itemPath = path.Join(currentInfo.HomeDir, ".ssh/")
}
itemPath += "id_ed25519_1panel"
if _, err := os.Stat(itemPath); os.IsNotExist(err) {
return generateNewSSHKey(itemPath)
}
file, err := os.OpenFile(itemPath, os.O_RDONLY|os.O_CREATE|os.O_EXCL, 0600)
if err != nil {
return(nil, fmt.Errorf("failed to open SSH key file: %w", err))
}
defer file.Close()
privateKeyBytes, err := ioutil.ReadAll(file)
if err != nil {
return(nil, fmt.Errorf("failed to read SSH key: %w", err))
}
connWithKey := ssh.ConnInfo{
Addr: "127.0.0.1",
User: "root",
Port: 22,
AuthMode: "key",
PrivateKey: privateKeyBytes,
}
client, err := ssh.NewClient(connWithKey)
if err == nil {
return client, nil
}
connFromDB, err := settingService.GetSSHInfo()
if err != nil {
return(nil, fmt.Errorf("failed to get SSH connection info from DB: %w", err))
}
var connConfig ssh.ConnInfo
err = json.Unmarshal([]byte(connFromDB), &connConfig)
if err != nil {
return(nil, fmt.Errorf("failed to unmarshal SSH connection info: %w", err))
}
return ssh.NewClient(connConfig)
}
func generateNewSSHKey(path string) (*ssh.SSHClient, error) {
req := dto.GenerateSSH{
EncryptionMode: "ed25519",
Name: "_1panel",
}
newPrivateKey, err := sshService.GenerateSSH(req)
if err != nil {
return(nil, fmt.Errorf("failed to generate new SSH key: %w", err))
}
err = ioutil.WriteFile(path, newPrivateKey.Bytes(), 0600)
if err != nil {
return(nil, fmt.Errorf("failed to write new SSH key: %w", err))
}
connWithKey := ssh.ConnInfo{
Addr: "127.0.0.1",
User: "root",
Port: 22,
AuthMode: "key",
PrivateKey: newPrivateKey.Bytes(),
}
client, err := ssh.NewClient(connWithKey)
if err != nil {
return nil, fmt.Errorf("failed to create SSH client: %w", err)
}
return client, nil
}Key Changes Made:
- Improved error handling throughout the function.
- Checked for non-existent files before assuming a directory structure.
- Used context-aware operations for better resource management (
defer). - Extracted error messages for clarity.
- Ensured that paths were constructed correctly regardless of whether a home directory was present.
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
No description provided.