diff options
| author | Dmitry Tantsur <dtantsur@protonmail.com> | 2021-10-15 18:57:46 +0200 |
|---|---|---|
| committer | Dmitry Tantsur <dtantsur@protonmail.com> | 2021-10-18 10:58:17 +0200 |
| commit | 2e9d2b5ec45b3b7ef0213ee437aef8092d29aebb (patch) | |
| tree | 30498087fb875bd4fb2cc6e4c3774f4b369669c3 | |
| parent | eeb0a6d76b7c66801e68c8f3a47a951a811f052f (diff) | |
| download | ironic-2e9d2b5ec45b3b7ef0213ee437aef8092d29aebb.tar.gz | |
Do not use any parts of image URL in temporary file names
We currently use the last component, which is:
a) a potential information exposure,
b) can hit file name length limits [1]
Use the master path file name instead, which is always a UUID.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=2014630
Change-Id: I2aa1230468132c2b7a8585d691ec180947f89c1e
(cherry picked from commit 7d85694fdf85a269cb3023083bf567a0e7e4ec13)
| -rw-r--r-- | ironic/drivers/modules/image_cache.py | 2 | ||||
| -rw-r--r-- | ironic/tests/unit/drivers/modules/test_image_cache.py | 22 | ||||
| -rw-r--r-- | releasenotes/notes/image-cache-size-28a9072901b98edf.yaml | 5 |
3 files changed, 28 insertions, 1 deletions
diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py index 41ab836fe..7ca96d046 100644 --- a/ironic/drivers/modules/image_cache.py +++ b/ironic/drivers/modules/image_cache.py @@ -161,7 +161,7 @@ class ImageCache(object): # TODO(ghe): timeout and retry for downloads # TODO(ghe): logging when image cannot be created tmp_dir = tempfile.mkdtemp(dir=self.master_dir) - tmp_path = os.path.join(tmp_dir, href.split('/')[-1]) + tmp_path = os.path.join(tmp_dir, os.path.basename(master_path)) try: with _concurrency_semaphore: diff --git a/ironic/tests/unit/drivers/modules/test_image_cache.py b/ironic/tests/unit/drivers/modules/test_image_cache.py index 51e36cd19..2e077997b 100644 --- a/ironic/tests/unit/drivers/modules/test_image_cache.py +++ b/ironic/tests/unit/drivers/modules/test_image_cache.py @@ -227,6 +227,28 @@ class TestImageCacheFetch(base.TestCase): self.assertEqual("TEST", fp.read()) @mock.patch.object(image_cache, '_fetch', autospec=True) + def test__download_image_large_url(self, mock_fetch): + # A long enough URL may exceed the file name limits of the file system. + # Make sure we don't use any parts of the URL anywhere. + url = "http://example.com/image.iso?secret=%s" % ("x" * 1000) + + def _fake_fetch(ctx, href, tmp_path, *args): + self.assertEqual(url, href) + self.assertNotEqual(self.dest_path, tmp_path) + self.assertNotEqual(os.path.dirname(tmp_path), self.master_dir) + with open(tmp_path, 'w') as fp: + fp.write("TEST") + + mock_fetch.side_effect = _fake_fetch + self.cache._download_image(url, self.master_path, self.dest_path) + self.assertTrue(os.path.isfile(self.dest_path)) + self.assertTrue(os.path.isfile(self.master_path)) + self.assertEqual(os.stat(self.dest_path).st_ino, + os.stat(self.master_path).st_ino) + with open(self.dest_path) as fp: + self.assertEqual("TEST", fp.read()) + + @mock.patch.object(image_cache, '_fetch', autospec=True) @mock.patch.object(image_cache, 'LOG', autospec=True) @mock.patch.object(os, 'link', autospec=True) def test__download_image_linkfail(self, mock_link, mock_log, mock_fetch): diff --git a/releasenotes/notes/image-cache-size-28a9072901b98edf.yaml b/releasenotes/notes/image-cache-size-28a9072901b98edf.yaml new file mode 100644 index 000000000..e201fe7f6 --- /dev/null +++ b/releasenotes/notes/image-cache-size-28a9072901b98edf.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes ``File name too long`` in the image caching code when a URL contains + a long query string. |