Support providing a fixed public host key for SFTP uploads (#6848)

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]>
antrea-io · Dec 12, 2024 · 521cd4b · 521cd4b
1 parent e404ccc
commit 521cd4b
Show file tree

Hide file tree

Showing 34 changed files with 966 additions and 365 deletions.
diff --git a/build/charts/antrea/crds/packetcapture.yaml b/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:

diff --git a/build/charts/antrea/crds/supportbundlecollection.yaml b/build/charts/antrea/crds/supportbundlecollection.yaml
@@ -100,6 +100,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:

diff --git a/build/charts/antrea/templates/webhooks/validating/crdvalidator.yaml b/build/charts/antrea/templates/webhooks/validating/crdvalidator.yaml
@@ -161,7 +161,7 @@ webhooks:
         namespace: {{ .Release.Namespace }}
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/build/yamls/antrea-aks.yml b/build/yamls/antrea-aks.yml
@@ -3061,6 +3061,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3197,6 +3200,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:
@@ -5999,7 +6005,7 @@ webhooks:
         namespace: kube-system
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/build/yamls/antrea-crds.yml b/build/yamls/antrea-crds.yml
@@ -3034,6 +3034,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3168,6 +3171,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:

diff --git a/build/yamls/antrea-eks.yml b/build/yamls/antrea-eks.yml
@@ -3061,6 +3061,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3197,6 +3200,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:
@@ -6000,7 +6006,7 @@ webhooks:
         namespace: kube-system
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/build/yamls/antrea-gke.yml b/build/yamls/antrea-gke.yml
@@ -3061,6 +3061,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3197,6 +3200,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:
@@ -5997,7 +6003,7 @@ webhooks:
         namespace: kube-system
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/build/yamls/antrea-ipsec.yml b/build/yamls/antrea-ipsec.yml
@@ -3061,6 +3061,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3197,6 +3200,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:
@@ -6056,7 +6062,7 @@ webhooks:
         namespace: kube-system
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/build/yamls/antrea.yml b/build/yamls/antrea.yml
@@ -3061,6 +3061,9 @@ spec:
                     url:
                       type: string
                       pattern: 'sftp:\/\/[\w-_./]+:\d+'
+                    hostPublicKey:
+                      type: string
+                      format: byte
             status:
               type: object
               properties:
@@ -3197,6 +3200,9 @@ spec:
                   properties:
                     url:
                       type: string
+                    hostPublicKey:
+                      type: string
+                      format: byte
                 authentication:
                   type: object
                   properties:
@@ -5997,7 +6003,7 @@ webhooks:
         namespace: kube-system
         path: "/validate/supportbundlecollection"
     rules:
-      - operations: ["UPDATE", "DELETE"]
+      - operations: ["CREATE", "UPDATE", "DELETE"]
         apiGroups: ["crd.antrea.io"]
         apiVersions: ["v1alpha1"]
         resources: ["supportbundlecollections"]

diff --git a/docs/packetcapture-guide.md b/docs/packetcapture-guide.md
@@ -57,6 +57,10 @@ metadata:
 spec:
   fileServer:
     url: sftp://127.0.0.1:22/upload # Define your own sftp url here.
+    # Host public key (as a base64-encoded string) that will be accepted when connecting to the file server.
+    # Get this key from your SSH server configuration, or from a known_hosts file.
+    # 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:

diff --git a/docs/support-bundle-guide.md b/docs/support-bundle-guide.md
@@ -135,6 +135,10 @@ 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-encoded string) that will be accepted when connecting to the file server.
+    # Get this key from your SSH server configuration, or from a known_hosts file.
+    # If omitted, any host key will be accepted, which is insecure and not recommended.
+    # hostPublicKey: ...
   authentication:
     authType: "BasicAuthentication"
     authSecret:
@@ -159,6 +163,10 @@ 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-encoded string) that will be accepted when connecting to the file server.
+    # Get this key from your SSH server configuration, or from a known_hosts file.
+    # If omitted, any host key will be accepted, which is insecure and not recommended.
+    # hostPublicKey: ...
   authentication:
     authType: "BasicAuthentication"
     authSecret:

diff --git a/pkg/agent/packetcapture/packetcapture_controller.go b/pkg/agent/packetcapture/packetcapture_controller.go
@@ -31,7 +31,6 @@ import (
 	"github.com/gopacket/gopacket/layers"
 	"github.com/gopacket/gopacket/pcapgo"
 	"github.com/spf13/afero"
-	"golang.org/x/crypto/ssh"
 	"golang.org/x/time/rate"
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -585,12 +584,13 @@ 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")
 	}
-	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,
+	cfg, err := sftp.GetSSHClientConfig(
+		serverAuth.BasicAuthentication.Username,
+		serverAuth.BasicAuthentication.Password,
+		pc.Spec.FileServer.HostPublicKey,
+	)
+	if err != nil {
+		return fmt.Errorf("failed to generate SSH client config: %w", err)
 	}
 	return uploader.Upload(pc.Spec.FileServer.URL, c.generatePacketsPathForServer(pc.Name), cfg, outputFile)
 }

diff --git a/pkg/agent/packetcapture/packetcapture_controller_test.go b/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 uploader.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)
+			}
+		})
+	}
+}
diff --git a/pkg/agent/supportbundlecollection/support_bundle_controller.go b/pkg/agent/supportbundlecollection/support_bundle_controller.go
@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"github.com/spf13/afero"
-	"golang.org/x/crypto/ssh"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -300,14 +299,14 @@ func (c *SupportBundleController) uploadSupportBundle(supportBundle *cpv1b2.Supp
 		return fmt.Errorf("failed to upload to the file server while setting offset: %v", err)
 	}
 	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,
+	cfg, err := sftp.GetSSHClientConfig(
+		supportBundle.Authentication.BasicAuthentication.Username,
+		supportBundle.Authentication.BasicAuthentication.Password,
+		supportBundle.FileServer.HostPublicKey,
+	)
+	if err != nil {
+		return fmt.Errorf("failed to generate SSH client config: %w", err)
 	}
-
 	return uploader.Upload(supportBundle.FileServer.URL, fileName, cfg, outputFile)
 }