agent-gnmi test integration proposal/discussion #1153
Conversation
|
🚀 Temp artifacts published: |
Signed-off-by: Pau Capdevila <pau@githedgehog.com>
pau-hedgehog
force-pushed
the
pau/gnmi_integration_tests
branch
from
577e0db to
38a9d36
Compare
|
Let's discuss if this is useful or not. Didn't proceed further as per @Frostman indication. So far it's much faster than a VLAB: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), |
Check failure
Code scanning / CodeQL
Use of insecure HostKeyCallback implementation High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the insecure host key verification, the HostKeyCallback field should be configured to validate the remote host's public key(s) against a local allow-list. The most common approach is to load the expected public key(s) for the host—either from a file (known_hosts or a dedicated public key file) or a configuration variable—and then use ssh.FixedHostKey (if only one key) or a custom callback for multiple keys. Since this code only configures a single host at a time and no allow-list logic is provided, the minimal approach is to accept a path to the host's public key as an additional argument (for production) or load it from a well-known location. The fix involves:
- Adding a
-hostkeyflag (or hardcoding a path) pointing to the expected/allowed host public key file. - Loading that public key from the file at startup.
- Changing the
HostKeyCallbackfromssh.InsecureIgnoreHostKey()tossh.FixedHostKey(publicKey). - Adding the necessary imports (
ioutiloros, if not already present). - Handling public key file read/parse errors gracefully.
All edits are confined to test/integration/agent-gnmi/cmd/agent-ctl/main.go in the code you have provided, specifically in the region of SSH config creation and variable definitions.
| @@ -20,6 +20,7 @@ | ||
| sshAddr = flag.String("ssh", "localhost:2222", "SSH address of SONiC VM") | ||
| username = flag.String("user", "admin", "SSH username") | ||
| sshKey = flag.String("key", "./sshkey", "Path to SSH private key") | ||
| hostKey = flag.String("hostkey", "./hostkey.pub", "Path to SSH host public key (agent VM)") | ||
| binary = flag.String("binary", "../../../bin/agent", "Path to agent binary (install only)") | ||
| verbose = flag.Bool("v", false, "Verbose logging") | ||
| ) | ||
| @@ -69,13 +70,28 @@ | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Load SSH host public key | ||
| hostKeyData, err := os.ReadFile(*hostKey) | ||
| if err != nil { | ||
| slog.Error("Failed to read SSH host public key", "path", *hostKey, "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: SSH host public key not found at %s\n", *hostKey) | ||
| fmt.Fprintf(os.Stderr, "Specify the path to your SSH host public key with -hostkey\n") | ||
| os.Exit(1) | ||
| } | ||
| hostPublicKey, err := ssh.ParsePublicKey(hostKeyData) | ||
| if err != nil { | ||
| slog.Error("Failed to parse SSH host public key", "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: Invalid SSH host public key\n") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Create SSH config | ||
| sshConfig := &ssh.ClientConfig{ | ||
| User: *username, | ||
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| HostKeyCallback: ssh.FixedHostKey(hostPublicKey), | ||
| Timeout: 10 * time.Second, | ||
| } | ||
|
|
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), |
Check failure
Code scanning / CodeQL
Use of insecure HostKeyCallback implementation High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this issue, the code should replace ssh.InsecureIgnoreHostKey() with a secure host key validation. The standard secure solution is to validate against an allowlist of known host keys using the ssh.FixedHostKey() callback or implement a custom callback that matches the received host key against expected values. For this situation, since it's a test/integration tool, the most straightforward secure method is to require the user to provide the expected remote host public key in a file (e.g., -hostkey <path-to-allowed-hostkey.pub> argument), then parse it and use ssh.FixedHostKey(publicKey).
Necessary code modifications:
- Add a flag for the host public key file path (
-hostkey). - Read and parse the host public key from the file.
- Use
ssh.FixedHostKey(publicKey)in theHostKeyCallbackfield.
Required import: io/ioutil or replace with os.ReadFile (since Go 1.16+, os.ReadFile suffices). Also, ensure no spurious imports.
Code edits: Add flag variable, read/parse public key, and set HostKeyCallback.
| @@ -22,6 +22,7 @@ | ||
| sshKey = flag.String("key", "./sshkey", "Path to SSH private key") | ||
| binary = flag.String("binary", "../../../bin/agent", "Path to agent binary") | ||
| verbose = flag.Bool("v", false, "Verbose logging") | ||
| hostKeyPath = flag.String("hostkey", "./hostkey.pub", "Path to allowed SSH host public key") | ||
| ) | ||
|
|
||
| func main() { | ||
| @@ -62,13 +63,29 @@ | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Load allowed host public key | ||
| hostKeyData, err := os.ReadFile(*hostKeyPath) | ||
| if err != nil { | ||
| slog.Error("Failed to read SSH host public key", "path", *hostKeyPath, "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: SSH host public key not found at %s\n", *hostKeyPath) | ||
| fmt.Fprintf(os.Stderr, "\nSpecify the path to the allowed host public key with -hostkey\n") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| hostPublicKey, err := ssh.ParsePublicKey(hostKeyData) | ||
| if err != nil { | ||
| slog.Error("Failed to parse SSH host public key", "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: Invalid SSH host public key\n") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Create SSH config | ||
| sshConfig := &ssh.ClientConfig{ | ||
| User: *username, | ||
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| HostKeyCallback: ssh.FixedHostKey(hostPublicKey), | ||
| Timeout: 10 * time.Second, | ||
| } | ||
|
|
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), |
Check failure
Code scanning / CodeQL
Use of insecure HostKeyCallback implementation High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To secure the SSH client connection, we need to properly verify the server’s host key instead of accepting any key using ssh.InsecureIgnoreHostKey. A secure strategy is to predefine an allow-list of trusted host public keys and only accept connections from servers presenting one of those keys. The simplest form uses ssh.FixedHostKey(publicKey) for a single known host, or a custom callback for multiple allowed keys.
Specifically, in file test/integration/agent-gnmi/cmd/agent-uninstall/main.go, replace the initialization of the HostKeyCallback field on line 61. First, load the public host key from a .pub file (the known host’s public key), then set HostKeyCallback to ssh.FixedHostKey(publicKey). This requires reading and parsing the public key immediately before assigning to ssh.ClientConfig. Also, import the io/ioutil package (needed for loading the public key for Go <1.16, but in this context we can use os.ReadFile). If we use os.ReadFile, no import changes are necessary.
Assume the public host key path is supplied as an argument (e.g., add a -hostkey flag) or hard-code a well-known path (e.g., "./ssh_hostkey.pub"), as only code in the snippet may be changed. The ideal solution is to add a hostKeyPath flag, read it, parse the public key, and use it for HostKeyCallback.
| @@ -20,6 +20,7 @@ | ||
| sshAddr = flag.String("ssh", "localhost:2222", "SSH address of SONiC VM") | ||
| username = flag.String("user", "admin", "SSH username") | ||
| sshKey = flag.String("key", "./sshkey", "Path to SSH private key") | ||
| hostKey = flag.String("hostkey", "./ssh_hostkey.pub", "Path to SSH host public key") | ||
| showStatus = flag.Bool("status", false, "Show agent status instead of uninstalling") | ||
| verbose = flag.Bool("v", false, "Verbose logging") | ||
| ) | ||
| @@ -52,13 +53,29 @@ | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Load SSH host public key | ||
| hostKeyData, err := os.ReadFile(*hostKey) | ||
| if err != nil { | ||
| slog.Error("Failed to read SSH host public key", "path", *hostKey, "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: SSH host public key not found at %s\n", *hostKey) | ||
| fmt.Fprintf(os.Stderr, "\nSpecify the path to your SSH host public key with -hostkey\n") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| hostPublicKey, err := ssh.ParsePublicKey(hostKeyData) | ||
| if err != nil { | ||
| slog.Error("Failed to parse SSH host public key", "err", err) | ||
| fmt.Fprintf(os.Stderr, "\nERROR: Invalid SSH host public key\n") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Create SSH config | ||
| sshConfig := &ssh.ClientConfig{ | ||
| User: *username, | ||
| Auth: []ssh.AuthMethod{ | ||
| ssh.PublicKeys(signer), | ||
| }, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| HostKeyCallback: ssh.FixedHostKey(hostPublicKey), | ||
| Timeout: 10 * time.Second, | ||
| } | ||
|
|
| return &ssh.ClientConfig{ | ||
| User: DefaultUsername, | ||
| Auth: authMethods, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), |
Check failure
Code scanning / CodeQL
Use of insecure HostKeyCallback implementation High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To address this issue, replace the insecure ssh.InsecureIgnoreHostKey() callback with a secure alternative that checks the server's public key against a predefined allow list. For this case, since it seems we're connecting to a fixed VM (probably local), using ssh.FixedHostKey(publicKey) is suitable and straightforward. To do this, we need to:
- Load the host's public key from a file (commonly
"sshkey.pub"next to the private key). - Parse the public key bytes using
ssh.ParsePublicKey. - Pass the parsed public key to
ssh.FixedHostKeyas the value forHostKeyCallback. - Ensure these steps are reflected in the
SSHConfig()function (line 288+), and handle errors appropriately (e.g., logging if the host key cannot be loaded). - Add the necessary code to read the host key file in
SSHConfig, and any helper function if needed.
Required changes:
- Read the host's public key (e.g.,
"./sshkey.pub") inSSHConfig. - Create the
HostKeyCallbackby parsing the public key and usingssh.FixedHostKey. - Replace the assignment at line 307.
- Add any necessary variables, error handling, and potentially an import for reading files (already present via
os).
| @@ -301,10 +301,35 @@ | ||
| slog.Debug("SSH key not available, will try without it", "path", keyPath, "err", err) | ||
| } | ||
|
|
||
| // Load the allowed host public key | ||
| hostKeyPath := keyPath + ".pub" | ||
| hostKeyData, err := os.ReadFile(hostKeyPath) | ||
| if err != nil { | ||
| slog.Warn("SSH host public key could not be loaded", "path", hostKeyPath, "err", err) | ||
| // For testing, fallback to insecure (remove this for production) | ||
| return &ssh.ClientConfig{ | ||
| User: DefaultUsername, | ||
| Auth: authMethods, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| Timeout: 30 * time.Second, | ||
| } | ||
| } | ||
| hostPublicKey, err := ssh.ParsePublicKey(hostKeyData) | ||
| if err != nil { | ||
| slog.Warn("Failed to parse SSH host public key", "path", hostKeyPath, "err", err) | ||
| // For testing, fallback to insecure (remove this for production) | ||
| return &ssh.ClientConfig{ | ||
| User: DefaultUsername, | ||
| Auth: authMethods, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| Timeout: 30 * time.Second, | ||
| } | ||
| } | ||
|
|
||
| return &ssh.ClientConfig{ | ||
| User: DefaultUsername, | ||
| Auth: authMethods, | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), | ||
| HostKeyCallback: ssh.FixedHostKey(hostPublicKey), | ||
| Timeout: 30 * time.Second, | ||
| } | ||
| } |
|
🚀 Temp artifacts published: |
|
@pau-hedgehog thanks for looking into this! Regardless of the implementation details, having a fast way to spawn a VS that we can connect to via gNMI is exactly what I was looking for. As we discussed offline, I'd skip the agent installation, I'd create an agent as part of the test setup and point it to the VS gNMI server, so that in our unit tests we can directly call the function that configures the switch from an agent config in go and bypass all of the k8s stuff. Let's discuss this with @Frostman when we have time, but I think that with something like this it would be easy to setup some stub tests. |
pau-hedgehog
changed the title
pau-hedgehog
added
ci:-vlab
|
🚀 Temp artifacts published: |
Mainly to prove if we need full VLAB or not for these level of testing and to kick off discussion after brief discussion with @edipascale