diff --git a/build/charts/antrea/crds/packetcapture.yaml b/build/charts/antrea/crds/packetcapture.yaml index 013a878b241..62aea5bbaea 100644 --- 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 index 0487cfd4925..62795a5ec6c 100644 --- 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/docs/packetcapture-guide.md b/docs/packetcapture-guide.md index 044f791ff1e..19ce5448b91 100644 --- a/docs/packetcapture-guide.md +++ b/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: diff --git a/docs/support-bundle-guide.md b/docs/support-bundle-guide.md index 88930e8fea9..82a9852b415 100644 --- a/docs/support-bundle-guide.md +++ b/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: diff --git a/pkg/agent/packetcapture/packetcapture_controller.go b/pkg/agent/packetcapture/packetcapture_controller.go index 5282ee904af..02542a712cc 100644 --- a/pkg/agent/packetcapture/packetcapture_controller.go +++ b/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) } diff --git a/pkg/agent/packetcapture/packetcapture_controller_test.go b/pkg/agent/packetcapture/packetcapture_controller_test.go index 29f0f6fe62b..22c8e788959 100644 --- 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 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 index 9d12536d377..926ef85bb24 100644 --- a/pkg/agent/supportbundlecollection/support_bundle_controller.go +++ b/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) diff --git a/pkg/agent/supportbundlecollection/support_bundle_controller_test.go b/pkg/agent/supportbundlecollection/support_bundle_controller_test.go index 5589d189fbc..4566755759f 100644 --- a/pkg/agent/supportbundlecollection/support_bundle_controller_test.go +++ b/pkg/agent/supportbundlecollection/support_bundle_controller_test.go @@ -17,10 +17,12 @@ package supportbundlecollection import ( "fmt" "io" + "slices" "testing" "github.com/spf13/afero" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "golang.org/x/crypto/ssh" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,6 +40,7 @@ import ( "antrea.io/antrea/pkg/querier" "antrea.io/antrea/pkg/support" "antrea.io/antrea/pkg/util/sftp" + sftptesting "antrea.io/antrea/pkg/util/sftp/testing" ) type fakeController struct { @@ -65,103 +68,140 @@ func newFakeController(t *testing.T) (*fakeController, *fakeversioned.Clientset) } func TestSupportBundleCollectionAdd(t *testing.T) { + uploadErr := fmt.Errorf("upload failed") + generateHostKey := func(t *testing.T) ssh.PublicKey { + publicKey, _, err := sftptesting.GenerateEd25519Key() + require.NoError(t, err) + return publicKey + } + hostKey1 := generateHostKey(t) + hostKey2 := generateHostKey(t) + testcases := []struct { name string supportBundleCollection *cpv1b2.SupportBundleCollection - expectedCompleted bool agentDumper *mockAgentDumper uploader sftp.Uploader + expectedSyncErr string }{ { name: "Add SupportBundleCollection", - supportBundleCollection: generateSupportbundleCollection("supportBundle1", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: true, + supportBundleCollection: generateSupportbundleCollection("supportBundle1", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{}, uploader: &testUploader{}, }, { name: "Add SupportBundleCollection without url prefix", - supportBundleCollection: generateSupportbundleCollection("supportBundle2", "10.220.175.92:22/root/supportbundle"), - expectedCompleted: true, + supportBundleCollection: generateSupportbundleCollection("supportBundle2", "10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{}, uploader: &testUploader{}, }, { name: "Add SupportBundleCollection with unsupported url prefix", - supportBundleCollection: generateSupportbundleCollection("supportBundle3", "https://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle3", "https://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{}, - uploader: &testFailedUploader{}, + uploader: &testUploader{ + err: uploadErr, + }, + expectedSyncErr: uploadErr.Error(), }, { name: "Add SupportBundleCollection with retry logics", - supportBundleCollection: generateSupportbundleCollection("supportBundle4", "10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle4", "10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{}, - uploader: &testFailedUploader{}, + uploader: &testUploader{ + err: uploadErr, + }, + expectedSyncErr: uploadErr.Error(), }, { name: "SupportBundleCollection failed to dump log", - supportBundleCollection: generateSupportbundleCollection("supportBundle5", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle5", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpLogErr: fmt.Errorf("failed to dump log")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump log", }, { name: "SupportBundleCollection failed to dump flows", - supportBundleCollection: generateSupportbundleCollection("supportBundle6", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle6", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpFlowsErr: fmt.Errorf("failed to dump flows")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump flows", }, { name: "SupportBundleCollection failed to dump host network info", - supportBundleCollection: generateSupportbundleCollection("supportBundle7", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle7", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpHostNetworkInfoErr: fmt.Errorf("failed to dump host network info")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump host network info", }, { name: "SupportBundleCollection failed to dump agent info", - supportBundleCollection: generateSupportbundleCollection("supportBundle8", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle8", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpAgentInfoErr: fmt.Errorf("failed to dump agent info")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump agent info", }, { name: "SupportBundleCollection failed to dump network policy resources", - supportBundleCollection: generateSupportbundleCollection("supportBundle9", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle9", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpNetworkPolicyResourcesErr: fmt.Errorf("failed to dump network policy resources")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump network policy resources", }, { name: "SupportBundleCollection failed to dump heap Pprof", - supportBundleCollection: generateSupportbundleCollection("supportBundle10", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle10", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpHeapPprofErr: fmt.Errorf("failed to dump heap Pprof")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump heap Pprof", }, { name: "SupportBundleCollection failed to dump OVS ports", - supportBundleCollection: generateSupportbundleCollection("supportBundle11", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle11", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpOVSPortsErr: fmt.Errorf("failed to dump OVS ports")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump OVS ports", }, { name: "SupportBundleCollection failed to dump goroutine Pprof", - supportBundleCollection: generateSupportbundleCollection("supportBundle12", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle12", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpGoroutinePprofErr: fmt.Errorf("failed to dump goroutine Pprof")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump goroutine Pprof", }, { name: "SupportBundleCollection failed to dump groups", - supportBundleCollection: generateSupportbundleCollection("supportBundle13", "sftp://10.220.175.92:22/root/supportbundle"), - expectedCompleted: false, + supportBundleCollection: generateSupportbundleCollection("supportBundle13", "sftp://10.220.175.92:22/root/supportbundle", nil), agentDumper: &mockAgentDumper{dumpGroupsErr: fmt.Errorf("failed to dump groups")}, uploader: &testUploader{}, + expectedSyncErr: "failed to generate support bundle: failed to dump groups", + }, + { + name: "Add SupportBundleCollection with host key", + supportBundleCollection: generateSupportbundleCollection("supportBundle13", "sftp://10.220.175.92:22/root/supportbundle", hostKey1.Marshal()), + agentDumper: &mockAgentDumper{}, + uploader: &testUploader{ + hostKey: hostKey1, + }, + }, + { + name: "Add SupportBundleCollection with host key mismatch", + supportBundleCollection: generateSupportbundleCollection("supportBundle14", "sftp://10.220.175.92:22/root/supportbundle", hostKey1.Marshal()), + agentDumper: &mockAgentDumper{}, + uploader: &testUploader{ + hostKey: hostKey2, + }, + expectedSyncErr: "failed to generate support bundle: invalid host key: ssh: host key mismatch", + }, + { + name: "Add SupportBundleCollection with invalid host key", + supportBundleCollection: generateSupportbundleCollection("supportBundle14", "sftp://10.220.175.92:22/root/supportbundle", []byte("abc")), + agentDumper: &mockAgentDumper{}, + uploader: &testUploader{ + hostKey: hostKey1, + }, + expectedSyncErr: "failed to generate support bundle: invalid host public key", }, } @@ -182,47 +222,59 @@ func TestSupportBundleCollectionAdd(t *testing.T) { return false, bundleStatus, nil })) controller.addSupportBundleCollection(tt.supportBundleCollection) - controller.syncSupportBundleCollection(tt.supportBundleCollection.Name) - assert.Equal(t, tt.expectedCompleted, bundleStatus.Nodes[0].Completed) + err := controller.syncSupportBundleCollection(tt.supportBundleCollection.Name) + if tt.expectedSyncErr == "" { + assert.NoError(t, err) + assert.True(t, bundleStatus.Nodes[0].Completed) + } else { + assert.ErrorContains(t, err, tt.expectedSyncErr) + assert.False(t, bundleStatus.Nodes[0].Completed) + } }) } } func TestSupportBundleCollectionDelete(t *testing.T) { controller, _ := newFakeController(t) - deletedBundle := generateSupportbundleCollection("deletedBundle", "sftp://10.220.175.92/root/supportbundle") + deletedBundle := generateSupportbundleCollection("deletedBundle", "sftp://10.220.175.92/root/supportbundle", nil) controller.addSupportBundleCollection(deletedBundle) controller.deleteSupportBundleCollection(deletedBundle) assert.NoError(t, controller.syncSupportBundleCollection("deletedBundle")) } type testUploader struct { + err error + hostKey ssh.PublicKey } func (uploader *testUploader) Upload(address string, path string, config *ssh.ClientConfig, tarGzFile io.Reader) error { - klog.Info("Called test uploader") + klog.InfoS("Called test uploader", "err", uploader.err) + if uploader.err != nil { + return uploader.err + } if _, err := sftp.ParseSFTPUploadUrl(address); err != nil { return err } + 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 } -type testFailedUploader struct { -} - -func (uploader *testFailedUploader) Upload(address string, path string, config *ssh.ClientConfig, tarGzFile io.Reader) error { - klog.Info("Called test uploader for failed case") - return fmt.Errorf("uploader failed") -} - -func generateSupportbundleCollection(name string, url string) *cpv1b2.SupportBundleCollection { +func generateSupportbundleCollection(name string, url string, hostPublicKey []byte) *cpv1b2.SupportBundleCollection { return &cpv1b2.SupportBundleCollection{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, FileServer: cpv1b2.BundleFileServer{ - URL: url, + URL: url, + HostPublicKey: hostPublicKey, }, Authentication: cpv1b2.BundleServerAuthConfiguration{ BasicAuthentication: &cpv1b2.BasicAuthentication{ diff --git a/pkg/apis/controlplane/types.go b/pkg/apis/controlplane/types.go index 24120dcb987..acd46ca0d50 100644 --- a/pkg/apis/controlplane/types.go +++ b/pkg/apis/controlplane/types.go @@ -560,6 +560,9 @@ type BundleFileServer struct { // The URL of the bundle file server. It is set with format: scheme://host[:port][/path], // e.g, https://api.example.com:8443/v1/supportbundles/. If scheme is not set, https is used by default. URL string + // HostPublicKey specifies the only host public key that will be accepted when connecting to + // the file server. If omitted, any host key will be accepted, which is not recommended. + HostPublicKey []byte } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/controlplane/v1beta2/types.go b/pkg/apis/controlplane/v1beta2/types.go index 13647845abf..96ee7ccbf28 100644 --- a/pkg/apis/controlplane/v1beta2/types.go +++ b/pkg/apis/controlplane/v1beta2/types.go @@ -587,7 +587,8 @@ type SupportBundleCollectionNodeStatus struct { } type BundleFileServer struct { - URL string `json:"url" protobuf:"bytes,1,opt,name=url"` + URL string `json:"url" protobuf:"bytes,1,opt,name=url"` + HostPublicKey []byte `json:"hostPublicKey,omitempty" protobuf:"bytes,2,opt,name=hostPublicKey"` } type BasicAuthentication struct { diff --git a/pkg/apis/crd/v1alpha1/types.go b/pkg/apis/crd/v1alpha1/types.go index 839b227101f..4ea30d1c3fc 100644 --- a/pkg/apis/crd/v1alpha1/types.go +++ b/pkg/apis/crd/v1alpha1/types.go @@ -176,6 +176,10 @@ type BundleFileServer struct { // The URL of the bundle file server. It is set with format: scheme://host[:port][/path], // e.g, https://api.example.com:8443/v1/supportbundles/. If scheme is not set, https is used by default. URL string `json:"url"` + // HostPublicKey specifies the only host public key that will be accepted when connecting to + // the file server. If omitted, any host key will be accepted, which is not recommended. + // For SFTP, the key must be formatted for use in the SSH wire protocol according to RFC 4253, section 6.6. + HostPublicKey []byte `json:"hostPublicKey,omitempty"` } // BundleServerAuthType defines the authentication type to access the BundleFileServer. @@ -446,6 +450,10 @@ type PacketCaptureFileServer struct { // The URL of the file server. It is set with format: scheme://host[:port][/path], // e.g., https://api.example.com:8443/v1/packets/. Currently only `sftp` protocol is supported. URL string `json:"url"` + // HostPublicKey specifies the only host public key that will be accepted when connecting to + // the file server. If omitted, any host key will be accepted, which is not recommended. + // For SFTP, the key must be formatted for use in the SSH wire protocol according to RFC 4253, section 6.6. + HostPublicKey []byte `json:"hostPublicKey,omitempty"` } type PacketCaptureSpec struct { diff --git a/pkg/controller/supportbundlecollection/controller.go b/pkg/controller/supportbundlecollection/controller.go index 138524718ef..c23b7f83b66 100644 --- a/pkg/controller/supportbundlecollection/controller.go +++ b/pkg/controller/supportbundlecollection/controller.go @@ -23,6 +23,7 @@ import ( "sync" "time" + "golang.org/x/crypto/ssh" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/conversion" @@ -401,6 +402,12 @@ func (c *Controller) createInternalSupportBundleCollection(bundle *v1alpha1.Supp Password: authentication.BasicAuthentication.Password, } } + // Validate the format of the public key here, to catch invalid keys early. + if bundle.Spec.FileServer.HostPublicKey != nil { + if _, err := ssh.ParsePublicKey(bundle.Spec.FileServer.HostPublicKey); err != nil { + return nil, fmt.Errorf("invalid host public key: %w", err) + } + } internalBundleCollection := c.addInternalSupportBundleCollection(bundle, nodeSpan, bundleAuthConfig, metav1.NewTime(expiredAt)) // Process the support bundle collection when time is up, this will create a CollectionFailure condition if the // bundle collection is not completed in time because any Agent fails to upload the files and does not report diff --git a/pkg/controller/supportbundlecollection/controller_test.go b/pkg/controller/supportbundlecollection/controller_test.go index c15048eb0a6..2813026a650 100644 --- a/pkg/controller/supportbundlecollection/controller_test.go +++ b/pkg/controller/supportbundlecollection/controller_test.go @@ -45,6 +45,7 @@ import ( "antrea.io/antrea/pkg/controller/types" "antrea.io/antrea/pkg/util/auth" "antrea.io/antrea/pkg/util/k8s" + sftptesting "antrea.io/antrea/pkg/util/sftp/testing" ) const ( @@ -84,6 +85,7 @@ type bundleConfig struct { authType v1alpha1.BundleServerAuthType secretName string secretNamespace string + hostPublicKey []byte conditions []v1alpha1.SupportBundleCollectionCondition phase bundlePhase createTime *time.Time @@ -674,6 +676,9 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { testClient.waitForSync(stopCh) + hostPublicKey, _, err := sftptesting.GenerateEd25519Key() + require.NoError(t, err) + expiredDuration, _ := time.ParseDuration("-61m") expiredCreationTime := time.Now().Add(expiredDuration) testCases := []struct { @@ -690,7 +695,8 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { names: []string{"n1", "n2"}, labels: map[string]string{"test": "selected"}, }, - authType: v1alpha1.APIKey, + authType: v1alpha1.APIKey, + hostPublicKey: hostPublicKey.Marshal(), }, expectedNodes: sets.New[string]("n1", "n2", "n3", "n4"), expectedAuth: controlplane.BundleServerAuthConfiguration{ @@ -751,6 +757,18 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { }, expectFailure: true, }, + { + bundleConfig: bundleConfig{ + name: "b6", + nodes: &bundleNodes{ + names: []string{"n1", "n2"}, + }, + authType: v1alpha1.APIKey, + // invalid key + hostPublicKey: []byte("abc"), + }, + expectedError: "invalid host public key", + }, } for _, tc := range testCases { @@ -763,8 +781,9 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { bundleConfig.secretName = secretName bundleConfig.secretNamespace = secretNamespace } - bundle, err := testClient.crdClient.CrdV1alpha1().SupportBundleCollections().Create(context.TODO(), generateSupportBundleResource(bundleConfig), metav1.CreateOptions{}) - require.Nil(t, err) + bundle := generateSupportBundleResource(bundleConfig) + _, err := testClient.crdClient.CrdV1alpha1().SupportBundleCollections().Create(context.TODO(), bundle, metav1.CreateOptions{}) + require.NoError(t, err) err = wait.PollUntilContextTimeout(context.Background(), time.Millisecond*50, time.Second, true, func(ctx context.Context) (done bool, err error) { _, getErr := controller.supportBundleCollectionLister.Get(tc.bundleConfig.name) if getErr == nil { @@ -792,6 +811,7 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { internalBundle, _ := obj.(*types.SupportBundleCollection) assert.Equal(t, tc.expectedNodes, internalBundle.NodeNames) assert.Equal(t, tc.expectedAuth, internalBundle.Authentication) + assert.Equal(t, bundle.Spec.FileServer, internalBundle.FileServer) } else { updatedBundle, err := testClient.crdClient.CrdV1alpha1().SupportBundleCollections().Get(context.TODO(), bundle.Name, metav1.GetOptions{}) require.NoError(t, err) @@ -810,8 +830,7 @@ func TestCreateAndDeleteInternalSupportBundleCollection(t *testing.T) { } // Test update span - err := testClient.client.CoreV1().Nodes().Delete(context.TODO(), "n3", metav1.DeleteOptions{}) - require.NoError(t, err) + require.NoError(t, testClient.client.CoreV1().Nodes().Delete(context.TODO(), "n3", metav1.DeleteOptions{})) updatedBundleCollection := generateSupportBundleResource( bundleConfig{ name: "b1", @@ -1864,7 +1883,8 @@ func generateSupportBundleResource(b bundleConfig) *v1alpha1.SupportBundleCollec }, Spec: v1alpha1.SupportBundleCollectionSpec{ FileServer: v1alpha1.BundleFileServer{ - URL: "https://1.1.1.1:443/supportbundles/upload", + URL: "https://1.1.1.1:443/supportbundles/upload", + HostPublicKey: b.hostPublicKey, }, ExpirationMinutes: 60, SinceTime: "2h", diff --git a/pkg/util/sftp/ssh.go b/pkg/util/sftp/ssh.go new file mode 100644 index 00000000000..8947b0a1402 --- /dev/null +++ b/pkg/util/sftp/ssh.go @@ -0,0 +1,34 @@ +// Copyright 2024 Antrea Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package sftp + +import ( + "golang.org/x/crypto/ssh" +) + +// GetAlgorithmsForHostKey returns the list of supported key algorithms for a given public key. In +// most cases, there is a single algorithms, which matches the key type. This is useful when setting +// the HostKeyCallback in ssh.ClientConfig to accept a fixed host key. The server may support +// multiple key types / key algorithms, and if we use the default value for HostKeyAlgorithms, the +// server may present a key that does not match our HostKeyCallback. When using a fixed host key, it +// makes sense to set HostKeyAlgorithms to the list of algorithms matching that specific key type. +func GetAlgorithmsForHostKey(key ssh.PublicKey) []string { + switch t := key.Type(); t { + case ssh.KeyAlgoRSA: + return []string{ssh.KeyAlgoRSA, ssh.KeyAlgoRSASHA256, ssh.KeyAlgoRSASHA512} + default: + return []string{t} + } +} diff --git a/pkg/util/sftp/testing/ssh.go b/pkg/util/sftp/testing/ssh.go new file mode 100644 index 00000000000..ed00cb937dc --- /dev/null +++ b/pkg/util/sftp/testing/ssh.go @@ -0,0 +1,61 @@ +// Copyright 2024 Antrea Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package testing + +import ( + "crypto" + "crypto/ed25519" + "crypto/rand" + "crypto/rsa" + "encoding/pem" + + "golang.org/x/crypto/ssh" +) + +// GenerateRSAKey generates a RSA key pair. The public key is returned as a ssh.PublicKey. The +// private key is returned as PEM data, serialized in the OpenSSH format. +func GenerateRSAKey(bits int) (ssh.PublicKey, []byte, error) { + privateKey, err := rsa.GenerateKey(rand.Reader, bits) + if err != nil { + return nil, nil, err + } + publicKeySSH, err := ssh.NewPublicKey(&privateKey.PublicKey) + if err != nil { + return nil, nil, err + } + privateKeyPEM, err := ssh.MarshalPrivateKey(crypto.PrivateKey(privateKey), "") + if err != nil { + return nil, nil, err + } + return publicKeySSH, pem.EncodeToMemory(privateKeyPEM), nil +} + +// GenerateRSAKey generates a ed25519 key pair. The public key is returned as a ssh.PublicKey. The +// private key is returned as PEM data, serialized in the OpenSSH format. +func GenerateEd25519Key() (ssh.PublicKey, []byte, error) { + publicKey, privateKey, err := ed25519.GenerateKey(nil) + if err != nil { + return nil, nil, err + } + publicKeySSH, err := ssh.NewPublicKey(publicKey) + if err != nil { + return nil, nil, err + } + privateKeyPEM, err := ssh.MarshalPrivateKey(crypto.PrivateKey(privateKey), "") + if err != nil { + return nil, nil, err + } + return publicKeySSH, pem.EncodeToMemory(privateKeyPEM), nil +} diff --git a/test/e2e/framework.go b/test/e2e/framework.go index 38901711ff6..387445e5adc 100644 --- a/test/e2e/framework.go +++ b/test/e2e/framework.go @@ -2840,7 +2840,7 @@ func (data *TestData) collectCovFiles(podName string, containerName string, nsNa continue } if err := wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, 5*time.Second, true, func(ctx context.Context) (bool, error) { - if err = data.copyPodFiles(podName, containerName, nsName, file, covDir); err != nil { + if err = data.copyPodFile(podName, containerName, nsName, file, covDir); err != nil { log.Infof("Coverage file not available yet for copy: %v", err) return false, nil } @@ -2880,11 +2880,19 @@ func (data *TestData) collectAntctlCovFilesFromControlPlaneNode(covDir string) e } -// copyPodFiles copies file from a Pod and save it to specified directory -func (data *TestData) copyPodFiles(podName string, containerName string, nsName string, fileName string, destDir string) error { - // getPodWriter creates the file with name podName-fileName-suffix. It returns nil if the - // file cannot be created. File must be closed by the caller. - getPodWriter := func(fileName string) *os.File { +// readPodFile reads a file from a Pod and returns the file contents as a string. +func (data *TestData) readPodFile(podName string, containerName string, nsName string, fileName string) (string, error) { + cmd := []string{"cat", fileName} + stdout, stderr, err := data.RunCommandFromPod(nsName, podName, containerName, cmd) + if err != nil { + return "", fmt.Errorf("cannot retrieve content of file '%s' from Pod '%s', stderr: <%v>, err: <%v>", fileName, podName, stderr, err) + } + return stdout, nil +} + +// copyPodFile copies a file from a Pod and save it to specified directory. +func (data *TestData) copyPodFile(podName string, containerName string, nsName string, fileName string, destDir string) error { + getWriter := func(fileName string) *os.File { destFile := filepath.Join(destDir, fileName) f, err := os.Create(destFile) if err != nil { @@ -2895,20 +2903,16 @@ func (data *TestData) copyPodFiles(podName string, containerName string, nsName } // dump the file from Antrea Pods to disk. basename := path.Base(fileName) - w := getPodWriter(basename) + w := getWriter(basename) if w == nil { return nil } defer w.Close() - cmd := []string{"cat", fileName} - log.Infof("Copying file: %s", basename) - stdout, stderr, err := data.RunCommandFromPod(nsName, podName, containerName, cmd) + stdout, err := data.readPodFile(podName, containerName, nsName, fileName) if err != nil { - return fmt.Errorf("cannot retrieve content of file '%s' from Pod '%s', stderr: <%v>, err: <%v>", fileName, podName, stderr, err) - } - if stdout == "" { - return nil + return err } + log.Infof("Copying file %q from Pod %s/%s", fileName, nsName, podName) w.WriteString(stdout) return nil } diff --git a/test/e2e/packetcapture_test.go b/test/e2e/packetcapture_test.go index aa728e1c5f2..28e1f41f1a1 100644 --- a/test/e2e/packetcapture_test.go +++ b/test/e2e/packetcapture_test.go @@ -42,6 +42,7 @@ import ( crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1" "antrea.io/antrea/pkg/features" + sftptesting "antrea.io/antrea/pkg/util/sftp/testing" ) var ( @@ -80,6 +81,19 @@ func genSFTPService() *v1.Service { } } +func genSSHKeysSecret(ed25519Key, rsaKey []byte) *v1.Secret { + return &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ssh-keys", + }, + Immutable: ptr.To(true), + Data: map[string][]byte{ + "ed25519": ed25519Key, + "rsa": rsaKey, + }, + } +} + func genSFTPDeployment() *appsv1.Deployment { replicas := int32(1) selector := map[string]string{"app": "sftp"} @@ -113,6 +127,31 @@ func genSFTPDeployment() *appsv1.Deployment { }, PeriodSeconds: 3, }, + VolumeMounts: []v1.VolumeMount{ + { + Name: "ssh-keys", + ReadOnly: true, + MountPath: "/etc/ssh/ssh_host_ed25519_key", + SubPath: "ed25519", + }, + { + Name: "ssh-keys", + ReadOnly: true, + MountPath: "/etc/ssh/ssh_host_rsa_key", + SubPath: "rsa", + }, + }, + }, + }, + Volumes: []v1.Volume{ + { + Name: "ssh-keys", + VolumeSource: v1.VolumeSource{ + Secret: &v1.SecretVolumeSource{ + SecretName: "ssh-keys", + DefaultMode: ptr.To[int32](0400), + }, + }, }, }, }, @@ -142,13 +181,18 @@ func TestPacketCapture(t *testing.T) { } defer teardownTest(t, data) + ed25519PubKey, ed25519PrivateKey, err := sftptesting.GenerateEd25519Key() + require.NoError(t, err) + rsaPubKey, rsaPrivateKey, err := sftptesting.GenerateRSAKey(4096) + require.NoError(t, err) + + _, err = data.clientset.CoreV1().Secrets(data.testNamespace).Create(context.TODO(), genSSHKeysSecret(ed25519PrivateKey, rsaPrivateKey), metav1.CreateOptions{}) + require.NoError(t, err) deployment, err := data.clientset.AppsV1().Deployments(data.testNamespace).Create(context.TODO(), genSFTPDeployment(), metav1.CreateOptions{}) require.NoError(t, err) - defer data.clientset.AppsV1().Deployments(data.testNamespace).Delete(context.TODO(), deployment.Name, metav1.DeleteOptions{}) - svc, err := data.clientset.CoreV1().Services(data.testNamespace).Create(context.TODO(), genSFTPService(), metav1.CreateOptions{}) + _, err = data.clientset.CoreV1().Services(data.testNamespace).Create(context.TODO(), genSFTPService(), metav1.CreateOptions{}) require.NoError(t, err) - defer data.clientset.CoreV1().Services(data.testNamespace).Delete(context.TODO(), svc.Name, metav1.DeleteOptions{}) - failOnError(data.waitForDeploymentReady(t, data.testNamespace, "sftp", defaultTimeout), t) + failOnError(data.waitForDeploymentReady(t, deployment.Namespace, deployment.Name, defaultTimeout), t) sec := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -166,14 +210,14 @@ func TestPacketCapture(t *testing.T) { defer data.clientset.CoreV1().Secrets(sec.Namespace).Delete(context.TODO(), sec.Name, metav1.DeleteOptions{}) t.Run("testPacketCaptureBasic", func(t *testing.T) { - testPacketCaptureBasic(t, data) + testPacketCaptureBasic(t, data, ed25519PubKey.Marshal(), rsaPubKey.Marshal()) }) } // testPacketCaptureTCP verifies if PacketCapture can capture tcp packets. this function only contains basic // cases with pod-to-pod. -func testPacketCaptureBasic(t *testing.T, data *TestData) { +func testPacketCaptureBasic(t *testing.T, data *TestData, ed25519PubKey, rsaPubKey []byte) { node1 := nodeName(0) clientPodName := "client" tcpServerPodName := "tcp-server" @@ -329,7 +373,8 @@ func testPacketCaptureBasic(t *testing.T, data *TestData) { }, }, FileServer: &crdv1alpha1.PacketCaptureFileServer{ - URL: fmt.Sprintf("sftp://%s:30010/upload", controlPlaneNodeIPv4()), + URL: fmt.Sprintf("sftp://%s:30010/upload", controlPlaneNodeIPv4()), + HostPublicKey: ed25519PubKey, }, Packet: &crdv1alpha1.Packet{ Protocol: &tcpProto, @@ -393,7 +438,8 @@ func testPacketCaptureBasic(t *testing.T, data *TestData) { }, }, FileServer: &crdv1alpha1.PacketCaptureFileServer{ - URL: fmt.Sprintf("sftp://%s:30010/upload", controlPlaneNodeIPv4()), + URL: fmt.Sprintf("sftp://%s:30010/upload", controlPlaneNodeIPv4()), + HostPublicKey: rsaPubKey, }, Packet: &crdv1alpha1.Packet{ Protocol: &udpProto, @@ -617,7 +663,7 @@ func runPacketCaptureTest(t *testing.T, data *TestData, tc pcTestCase) { tmpDir := t.TempDir() dstFileName := filepath.Join(tmpDir, fileName) packetFile := filepath.Join("/tmp", "antrea", "packetcapture", "packets", fileName) - require.NoError(t, data.copyPodFiles(antreaPodName, "antrea-agent", "kube-system", packetFile, tmpDir)) + require.NoError(t, data.copyPodFile(antreaPodName, "antrea-agent", "kube-system", packetFile, tmpDir)) defer os.Remove(dstFileName) file, err := os.Open(dstFileName) require.NoError(t, err)