Support providing a fixed public host key for SFTP uploads

The SupportBundleCollection and PacketCapture CRDs can upload data to a
user-provided SFTP server. Until now, there was no way for users to
provide a verification mechanism for the host key from the
server. Antrea would accept any host key when uploading the files, which
is considered insecure. As a matter of fact, the usage of
ssh.InsecureIgnoreHostKey as the verification callback was flagged in
the code by security tools. And while there was a comment in the code
explaining that "users can specify their own checks if needed", this is
not accurate, as users have no way to provide a verification callback
without changing the code manually.

We are therefore introducing a new field for both the
SupportBundleCollection and PacketCapture CRDs, so that users can
provide a fixed host public key (as a base64-encoded string), which will
be the only key accepted by Antrea for SFTP upload. This seems like a
good start, and in the future we may expand that support so that users
can provide a "known_hosts" OpenSSH file, either as a ConfigMap or
Secret.

Signed-off-by: Antonin Bas <[email protected]>

build/charts/antrea/crds/packetcapture.yaml

@@ -161,6 +161,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:

build/charts/antrea/crds/supportbundlecollection.yaml

@@ -100,6 +100,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:

docs/packetcapture-guide.md

@@ -57,6 +57,9 @@ metadata:
 spec:
   fileServer:
     url: sftp://127.0.0.1:22/upload # Define your own sftp url here.
+    # Host public key (as a base64-endoded string) that will be accepted when connecting to the file server.
+    # If omitted, any host key will be accepted, which is insecure and not recommended.
+    hostPublicKey: AAAAC3NzaC1lZDI1NTE5AAAAIBCUI6Yi9KbkiPXK2MzqYYtlluw7v_WQz071JZPdZEKr # Replace with your own.
   timeout: 60
   captureConfig:
     firstN:

docs/support-bundle-guide.md

@@ -135,6 +135,9 @@ spec:
   sinceTime: 2h # Collect the logs in the latest 2 hours. Collect all available logs if the time is not set.
   fileServer:
     url: sftp://yourtestdomain.com:22/root/test
+    # Host public key (as a base64-endoded string) that will be accepted when connecting to the file server.
+    # If omitted, any host key will be accepted, which is insecure and not recommended.
+    # hostPublicKey: ...
   authentication:
     authType: "BasicAuthentication"
     authSecret:
@@ -159,6 +162,9 @@ spec:
     namespace: vm-ns # namespace is mandatory when collecting support bundle from external Nodes.
   fileServer:
     url: yourtestdomain.com:22/root/test # Scheme sftp can be omitted. The url of "$controlplane_node_ip:30010/upload" is used if deployed with sftp-deployment.yml.
+    # Host public key (as a base64-endoded string) that will be accepted when connecting to the file server.
+    # If omitted, any host key will be accepted, which is insecure and not recommended.
+    # hostPublicKey: ...
   authentication:
     authType: "BasicAuthentication"
     authSecret:

pkg/agent/packetcapture/packetcapture_controller.go

@@ -585,12 +585,25 @@ func (c *Controller) uploadPackets(ctx context.Context, pc *crdv1alpha1.PacketCa
 	if serverAuth.BasicAuthentication == nil {
 		return fmt.Errorf("failed to get basic authentication info for the file server")
 	}
+	// #nosec G106: users should set hostPublicKey, accepting arbitrary keys is not recommended.
+	hostKeyCallback := ssh.InsecureIgnoreHostKey()
+	var hostKeyAlgorithms []string
+	if pc.Spec.FileServer.HostPublicKey != nil {
+		key, err := ssh.ParsePublicKey(pc.Spec.FileServer.HostPublicKey)
+		if err != nil {
+			return fmt.Errorf("invalid host public key: %w", err)
+		}
+		hostKeyCallback = ssh.FixedHostKey(key)
+		// With a single fixed key, it makes sense to set this in case the server supports
+		// multiple keys (e.g., ed25519 and rsa).
+		hostKeyAlgorithms = sftp.GetAlgorithmsForHostKey(key)
+	}
 	cfg := &ssh.ClientConfig{
-		User: serverAuth.BasicAuthentication.Username,
-		Auth: []ssh.AuthMethod{ssh.Password(serverAuth.BasicAuthentication.Password)},
-		// #nosec G106: skip host key check here and users can specify their own checks if needed
-		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
-		Timeout:         time.Second,
+		User:              serverAuth.BasicAuthentication.Username,
+		Auth:              []ssh.AuthMethod{ssh.Password(serverAuth.BasicAuthentication.Password)},
+		HostKeyCallback:   hostKeyCallback,
+		HostKeyAlgorithms: hostKeyAlgorithms,
+		Timeout:           time.Second,
 	}
 	return uploader.Upload(pc.Spec.FileServer.URL, c.generatePacketsPathForServer(pc.Name), cfg, outputFile)
 }

pkg/agent/packetcapture/packetcapture_controller_test.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"slices"
 	"testing"
 	"time"
 
@@ -46,6 +47,7 @@ import (
 	fakeversioned "antrea.io/antrea/pkg/client/clientset/versioned/fake"
 	crdinformers "antrea.io/antrea/pkg/client/informers/externalversions"
 	"antrea.io/antrea/pkg/util/k8s"
+	sftptesting "antrea.io/antrea/pkg/util/sftp/testing"
 )
 
 var (
@@ -145,17 +147,22 @@ func genTestCR(name string, num int32) *crdv1alpha1.PacketCapture {
 type testUploader struct {
 	url      string
 	fileName string
-	// for concurrent cases, no need to check
-	checkFileName bool
+	hostKey  ssh.PublicKey
 }
 
 func (uploader *testUploader) Upload(url string, fileName string, config *ssh.ClientConfig, outputFile io.Reader) error {
 	if url != uploader.url {
 		return fmt.Errorf("expected url: %s for uploader, got: %s", uploader.url, url)
 	}
-	if uploader.checkFileName {
-		if fileName != uploader.fileName {
-			return fmt.Errorf("expected filename: %s, got: %s ", uploader.fileName, fileName)
+	if fileName != "" && fileName != uploader.fileName {
+		return fmt.Errorf("expected filename: %s, got: %s ", uploader.fileName, fileName)
+	}
+	if uploader.hostKey != nil {
+		if config.HostKeyAlgorithms != nil && !slices.Equal(config.HostKeyAlgorithms, []string{uploader.hostKey.Type()}) {
+			return fmt.Errorf("unsupported host key algorithm")
+		}
+		if err := config.HostKeyCallback("", nil, uploader.hostKey); err != nil {
+			return fmt.Errorf("invalid host key: %w", err)
 		}
 	}
 	return nil
@@ -594,3 +601,69 @@ func TestMergeConditions(t *testing.T) {
 		})
 	}
 }
+
+func TestUploadPackets(t *testing.T) {
+	ctx := context.Background()
+
+	generateHostKey := func(t *testing.T) ssh.PublicKey {
+		publicKey, _, err := sftptesting.GenerateEd25519Key()
+		require.NoError(t, err)
+		return publicKey
+	}
+	hostKey1 := generateHostKey(t)
+	hostKey2 := generateHostKey(t)
+
+	fs := afero.NewMemMapFs()
+
+	testCases := []struct {
+		name            string
+		serverHostKey   ssh.PublicKey
+		expectedHostKey []byte
+		expectedErr     string
+	}{
+		{
+			name:            "matching key",
+			serverHostKey:   hostKey1,
+			expectedHostKey: hostKey1.Marshal(),
+		},
+		{
+			name:            "non matching key",
+			serverHostKey:   hostKey2,
+			expectedHostKey: hostKey1.Marshal(),
+			expectedErr:     "host key mismatch",
+		},
+		{
+			name:            "ignore host key",
+			serverHostKey:   hostKey1,
+			expectedHostKey: nil,
+		},
+		{
+			name:            "invalid key format",
+			serverHostKey:   hostKey1,
+			expectedHostKey: []byte("abc"),
+			expectedErr:     "invalid host public key",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			pc := genTestCR("foo", testCaptureNum)
+			pcc := newFakePacketCaptureController(t, nil, nil)
+			pcc.sftpUploader = &testUploader{
+				url:      testFTPUrl,
+				fileName: pcc.generatePacketsPathForServer(pc.Name),
+				hostKey:  tc.serverHostKey,
+			}
+			pc.Spec.FileServer.HostPublicKey = tc.expectedHostKey
+			f, err := afero.TempFile(fs, "", "upload-test")
+			require.NoError(t, err)
+			defer f.Close()
+			err = pcc.uploadPackets(ctx, pc, f)
+			if tc.expectedErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.ErrorContains(t, err, tc.expectedErr)
+			}
+		})
+	}
+}

pkg/agent/supportbundlecollection/support_bundle_controller.go

@@ -299,13 +299,26 @@ func (c *SupportBundleController) uploadSupportBundle(supportBundle *cpv1b2.Supp
 	if _, err := outputFile.Seek(0, 0); err != nil {
 		return fmt.Errorf("failed to upload to the file server while setting offset: %v", err)
 	}
+	// #nosec G106: users should set hostPublicKey, accepting arbitrary keys is not recommended.
+	hostKeyCallback := ssh.InsecureIgnoreHostKey()
+	var hostKeyAlgorithms []string
+	if supportBundle.FileServer.HostPublicKey != nil {
+		key, err := ssh.ParsePublicKey(supportBundle.FileServer.HostPublicKey)
+		if err != nil {
+			return fmt.Errorf("invalid host public key: %w", err)
+		}
+		hostKeyCallback = ssh.FixedHostKey(key)
+		// With a single fixed key, it makes sense to set this in case the server supports
+		// multiple keys (e.g., ed25519 and rsa).
+		hostKeyAlgorithms = sftp.GetAlgorithmsForHostKey(key)
+	}
 	fileName := c.nodeName + "_" + supportBundle.Name + ".tar.gz"
 	cfg := &ssh.ClientConfig{
-		User: supportBundle.Authentication.BasicAuthentication.Username,
-		Auth: []ssh.AuthMethod{ssh.Password(supportBundle.Authentication.BasicAuthentication.Password)},
-		// #nosec G106: skip host key check here and users can specify their own checks if needed
-		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
-		Timeout:         time.Second,
+		User:              supportBundle.Authentication.BasicAuthentication.Username,
+		Auth:              []ssh.AuthMethod{ssh.Password(supportBundle.Authentication.BasicAuthentication.Password)},
+		HostKeyCallback:   hostKeyCallback,
+		HostKeyAlgorithms: hostKeyAlgorithms,
+		Timeout:           time.Second,
 	}
 
 	return uploader.Upload(supportBundle.FileServer.URL, fileName, cfg, outputFile)