From 56fb05163fb78016ee8d78448cfe7aad3e32cbcd Mon Sep 17 00:00:00 2001 From: alromeros Date: Thu, 9 Nov 2023 13:37:35 +0100 Subject: [PATCH] Check content-type in importer to warn against unexpected kubevirt imports (#2909) * Check content-type before importing to warn against unexpected content imports This commit adds a check in the http-datasource format reader to trigger a warning when importing unexpected content-types. Signed-off-by: Alvaro Romero * Test import with unexpected content-type This commit adds a test to cover a import with unexpected content-type. It also updates the badserver to allow mocking this behavior. Signed-off-by: Alvaro Romero --------- Signed-off-by: Alvaro Romero --- pkg/importer/http-datasource.go | 26 ++++++++++++++------ pkg/importer/http-datasource_test.go | 18 +++++++------- tests/badserver_test.go | 30 +++++++++++++++++++++++ tools/cdi-func-test-bad-webserver/main.go | 17 +++++++++++++ 4 files changed, 75 insertions(+), 16 deletions(-) diff --git a/pkg/importer/http-datasource.go b/pkg/importer/http-datasource.go index 10316147eb..fd98003d88 100644 --- a/pkg/importer/http-datasource.go +++ b/pkg/importer/http-datasource.go @@ -44,10 +44,12 @@ import ( ) const ( - tempFile = "tmpimage" - nbdkitPid = "/tmp/nbdkit.pid" - nbdkitSocket = "/tmp/nbdkit.sock" - defaultUserAgent = "cdi-golang-importer" + tempFile = "tmpimage" + nbdkitPid = "/tmp/nbdkit.pid" + nbdkitSocket = "/tmp/nbdkit.sock" + defaultUserAgent = "cdi-golang-importer" + httpContentType = "Content-Type" + httpContentLength = "Content-Length" ) // HTTPDataSource is the data provider for http(s) endpoints. @@ -96,7 +98,7 @@ func NewHTTPDataSource(endpoint, accessKey, secKey, certDir string, contentType return nil, errors.Wrap(err, "Error getting extra headers for HTTP client") } - httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders) + httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders, contentType) if err != nil { cancel() return nil, err @@ -294,7 +296,7 @@ func addExtraheaders(req *http.Request, extraHeaders []string) { req.Header.Add("User-Agent", defaultUserAgent) } -func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string) (io.ReadCloser, uint64, bool, error) { +func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string, contentType cdiv1.DataVolumeContentType) (io.ReadCloser, uint64, bool, error) { var brokenForQemuImg bool client, err := createHTTPClient(certDir) if err != nil { @@ -334,6 +336,16 @@ func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certD return nil, uint64(0), true, errors.Errorf("expected status code 200, got %d. Status: %s", resp.StatusCode, resp.Status) } + if contentType == cdiv1.DataVolumeKubeVirt { + // Check the content-type if we are expecting a KubeVirt img. + if val, ok := resp.Header[httpContentType]; ok { + if strings.HasPrefix(val[0], "text/") { + // We will continue with the import nonetheless, but content might be unexpected. + klog.Warningf("Unexpected content type '%s'. Content might not be a KubeVirt image.", val[0]) + } + } + } + acceptRanges, ok := resp.Header["Accept-Ranges"] if !ok || acceptRanges[0] == "none" { klog.V(2).Infof("Accept-Ranges isn't bytes, avoiding qemu-img") @@ -416,7 +428,7 @@ func getContentLength(client *http.Client, ep *url.URL, accessKey, secKey string func parseHTTPHeader(resp *http.Response) uint64 { var err error total := uint64(0) - if val, ok := resp.Header["Content-Length"]; ok { + if val, ok := resp.Header[httpContentLength]; ok { total, err = strconv.ParseUint(val[0], 10, 64) if err != nil { klog.Errorf("could not convert content length, got %v", err) diff --git a/pkg/importer/http-datasource_test.go b/pkg/importer/http-datasource_test.go index 36dbe1fc7a..379711b947 100644 --- a/pkg/importer/http-datasource_test.go +++ b/pkg/importer/http-datasource_test.go @@ -232,7 +232,7 @@ var _ = Describe("Http client", func() { var _ = Describe("Http reader", func() { It("should fail when passed an invalid cert directory", func() { - _, total, _, err := createHTTPReader(context.Background(), nil, "", "", "/invalid", nil, nil) + _, total, _, err := createHTTPReader(context.Background(), nil, "", "", "/invalid", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(err).To(HaveOccurred()) Expect(uint64(0)).To(Equal(total)) }) @@ -249,7 +249,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil) + r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(err).ToNot(HaveOccurred()) Expect(uint64(25)).To(Equal(total)) err = r.Close() @@ -272,7 +272,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil) + r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(err).ToNot(HaveOccurred()) Expect(uint64(25)).To(Equal(total)) err = r.Close() @@ -294,7 +294,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil) + r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(brokenForQemuImg).To(BeFalse()) Expect(err).ToNot(HaveOccurred()) Expect(uint64(25)).To(Equal(total)) @@ -315,7 +315,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil) + r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(err).ToNot(HaveOccurred()) Expect(uint64(0)).To(Equal(total)) err = r.Close() @@ -339,7 +339,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil) + r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(brokenForQemuImg).To(BeTrue()) Expect(err).ToNot(HaveOccurred()) Expect(uint64(25)).To(Equal(total)) @@ -359,7 +359,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil) + r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(brokenForQemuImg).To(BeTrue()) Expect(err).ToNot(HaveOccurred()) Expect(uint64(25)).To(Equal(total)) @@ -374,7 +374,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - _, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil) + _, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt) Expect(err).To(HaveOccurred()) Expect(uint64(0)).To(Equal(total)) Expect("expected status code 200, got 500. Status: 500 Internal Server Error").To(Equal(err.Error())) @@ -391,7 +391,7 @@ var _ = Describe("Http reader", func() { defer ts.Close() ep, err := url.Parse(ts.URL) Expect(err).ToNot(HaveOccurred()) - r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", []string{"Extra-Header: 123"}, nil) + r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", []string{"Extra-Header: 123"}, nil, cdiv1.DataVolumeKubeVirt) Expect(err).ToNot(HaveOccurred()) Expect(uint64(0)).To(Equal(total)) err = r.Close() diff --git a/tests/badserver_test.go b/tests/badserver_test.go index f92f1da460..ad89c7519f 100644 --- a/tests/badserver_test.go +++ b/tests/badserver_test.go @@ -2,9 +2,14 @@ package tests import ( "fmt" + "strings" + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "kubevirt.io/containerized-data-importer/pkg/common" + controller "kubevirt.io/containerized-data-importer/pkg/controller/common" "kubevirt.io/containerized-data-importer/tests/framework" "kubevirt.io/containerized-data-importer/tests/utils" @@ -31,6 +36,31 @@ var _ = Describe("Problematic server responses", func() { Entry("[rfe_id:4326][test_id:5076][crit:low][vendor:cnv-qe@redhat.com][level:component] Should succeed even if Accept-Ranges doesn't exist", "/no-accept-ranges/cirros-qcow2.img"), ) + It("Should warn against unexpected content-types when importing a kubevirt img", func() { + cdiBadServer := fmt.Sprintf("http://cdi-bad-webserver.%s:9090", f.CdiInstallNs) + pathname := "/bad-content-type/cirros-qcow2.img" + dataVolume = utils.NewDataVolumeWithHTTPImport("badserver-dv", "1Gi", cdiBadServer+pathname) + dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true" + By("creating DataVolume") + dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) + Expect(err).ToNot(HaveOccurred()) + f.ForceBindPvcIfDvIsWaitForFirstConsumer(dataVolume) + + err = utils.WaitForDataVolumePhase(f, f.Namespace.Name, cdiv1.Succeeded, dataVolume.Name) + Expect(err).ToNot(HaveOccurred()) + + By("Find importer pod after completion") + importer, err := utils.FindPodByPrefixOnce(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector) + Expect(err).ToNot(HaveOccurred()) + Expect(importer.DeletionTimestamp).To(BeNil()) + + By("Verify HTTP request error in importer log") + Eventually(func() bool { + log, _ := f.RunKubectlCommand("logs", importer.Name, "-n", importer.Namespace) + return strings.Contains(log, "Unexpected content type 'text/html'. Content might not be a KubeVirt image.") + }, time.Minute, pollingInterval).Should(BeTrue()) + }) + AfterEach(func() { By("deleting DataVolume") err := utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, dataVolumeName) diff --git a/tools/cdi-func-test-bad-webserver/main.go b/tools/cdi-func-test-bad-webserver/main.go index 7e5b9ed2e0..556c7084b8 100644 --- a/tools/cdi-func-test-bad-webserver/main.go +++ b/tools/cdi-func-test-bad-webserver/main.go @@ -30,6 +30,22 @@ func flaky(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusServiceUnavailable) } +func badContentType(w http.ResponseWriter, r *http.Request) { + actualFileURL := getEquivalentFileHostURL(r.URL.String()) + + resp, err := http.Get(actualFileURL) + if err != nil { + panic("Couldn't fetch URL") + } + + w.Header().Set("Content-Type", "text/html") + + _, err = io.Copy(w, resp.Body) + if err != nil { + panic(fmt.Errorf("badContentType: failed to write the response; %w", err)) + } +} + func noAcceptRanges(w http.ResponseWriter, r *http.Request) { actualFileURL := getEquivalentFileHostURL(r.URL.String()) @@ -79,6 +95,7 @@ func main() { http.HandleFunc("/forbidden-HEAD/", failHEAD) http.HandleFunc("/flaky/", flaky) http.HandleFunc("/no-accept-ranges/", noAcceptRanges) + http.HandleFunc("/bad-content-type/", badContentType) err := http.ListenAndServe(":9090", nil) if err != nil { log.Fatal("ListenAndServe: ", err)