Fix regression with CHUNKED_UPLOAD_DIR outside MEDIA_ROOT #1074

ekohl · 2021-01-18T16:46:31Z

In Django, any file that's uploaded outside a FileSystemStorage (which MEDIA_ROOT is), is a SuspicuousOperation`. It generally indicates an attempt to write outside of a designated area which is a common attack vector.

In the default Pulp deployment, MEDIA_ROOT is /var/lib/pulp and CHUNKED_UPLOAD_DIR is /var/lib/pulp/tmp.

As part of #4498 (1b6c736), uploads are done to an absolute path rather than use a FileSystemStorage for this. In deployments where MEDIA_ROOT is different (such as /var/lib/pulp/media, it suddenly becomes a problem.

Fixes #8099

ekohl · 2021-01-18T16:47:23Z

pulpcore/app/models/upload.py


-    file = fields.FileField(null=False, upload_to=storage_path, max_length=255)
+    file = fields.FileField(
+        null=False, storage=storage.CHUNKED_UPLOAD_STORAGE, upload_to=storage_path, max_length=255


Note that this is essentially the same as it was: 1b6c736#diff-03bb26bef2f063c5e13c435d8c58f624a9649bad78b53df8cc2463204238f674L22-L24

I remember that we previously had problems when using settings in the models, because they imply new migrations are needed once you changed them no the client side.
Can anyone make sure that this change does not reintroduce that problem?
cc @ipanova @lubosmj

I wonder why FILE_UPLOAD_TEMP_DIR couldn't be used for this.

You can probably also trick Django by using a callable but that requires Django 3.1.

It does indeed want a migration and fails the tests on it. I can fix it up, but first I'd like to figure out if that's even desired.

Yes, I believe it needs fixing, as once an admin runs into it an realizes he "needs" to create a migration, he is in deep trouble.

I sadly don't know how to.

So it seems to me that we should not have introduced CHUNKED_UPLOAD_DIR as a setting at all. This setting was introduced in 3.9.0 and in 3.9.1 we should just get rid of it and have it be a constant that can't be changed. Otherwise users that modify it will always be told that a migration is needed.

As I said, Django 3.1 allows a callable which probably means Django migrations can't detect a difference depending on settings. Another may be implement a subclass of FileSystemStorage that doesn't take a parameter but always uses a setting. Then Django probably can't detect a difference either.

So it seems to me that we should not have introduced CHUNKED_UPLOAD_DIR as a setting at all. This setting was introduced in 3.9.0 and in 3.9.1 we should just get rid of it and have it be a constant that can't be changed. Otherwise users that modify it will always be told that a migration is needed.

I don't think this is true. 6a3efe7 put this in settings and was part of 3.3.0. In 3.8.0 and earlier changing this setting worked well for us.

Edit: if you mean that we should remove it altogether and ensure FILE_UPLOAD_TEMP_DIR is used to store chunks then I think that would be a good thing, but it feels risky for a cherry pick.

It is risky for a cherry-pick if it needs a migration. 🤯

ianballou

I tried to upload a file through Katello after applying this PR to Pulpcore 3.9 and got the following error:

Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: pulp [58d5c866d80147f6bcac99869ba4618f]: django.request:ERROR: Internal Server Error: /pulp/api/v3/uploads/2f5f71bd-3344-4acb-a5aa-76ca0b32ce33/
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: Traceback (most recent call last):
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/core/handlers/exception.py", line 34, in inner
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: response = get_response(request)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/core/handlers/base.py", line 115, in _get_response
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: response = self.process_exception_by_middleware(e, request)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/core/handlers/base.py", line 113, in _get_response
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: response = wrapped_callback(request, *callback_args, **callback_kwargs)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: return view_func(*args, **kwargs)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/rest_framework/viewsets.py", line 125, in view
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: return self.dispatch(request, *args, **kwargs)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 509, in dispatch
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: response = self.handle_exception(exc)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 469, in handle_exception
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: self.raise_uncaught_exception(exc)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: raise exc
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 506, in dispatch
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: response = handler(request, *args, **kwargs)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/pulpcore/app/viewsets/upload.py", line 77, in update
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: upload.append(chunk, start, sha256)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/local/lib/python3.6/site-packages/pulpcore/app/models/upload.py", line 36, in append
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: upload_chunk.file.save("", ContentFile(chunk_read))
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/db/models/fields/files.py", line 86, in save
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: name = self.field.generate_filename(self.instance, name)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/db/models/fields/files.py", line 307, in generate_filename
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: return self.storage.generate_filename(filename)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib/python3.6/site-packages/django/core/files/storage.py", line 100, in generate_filename
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: dirname, filename = os.path.split(filename)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: File "/usr/lib64/python3.6/posixpath.py", line 107, in split
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: p = os.fspath(p)
Jan 18 18:05:27 centos7-katello-devel-2 pulpcore-api: TypeError: expected str, bytes or os.PathLike object, not UUID

ekohl · 2021-01-18T18:18:22Z

Ah, that was the merge conflict I got. I forgot the str() part.

In Django, any file that's uploaded outside a `FileSystemStorage` (which `MEDIA_ROOT` is)`, is a `SuspicuousOperation`. It generally indicates an attempt to write outside of a designated area which is a common attack vector. In the default Pulp deployment, `MEDIA_ROOT` is `/var/lib/pulp` and `CHUNKED_UPLOAD_DIR` is `/var/lib/pulp/tmp`. As part of pulp#4498 (1b6c736), uploads are done to an absolute path rather than use a FileSystemStorage for this. In deployments where `MEDIA_ROOT` is different (such as `/var/lib/pulp/media`, it suddenly becomes a problem. Fixes #8099

ianballou

Working for me with Pulpcore 3.9! I tested CHUNKED_UPLOAD_DIR = "/var/lib/pulp/upload". I was able to upload to file and rpm repositories.

pulpbot · 2021-01-18T18:41:18Z

Attached issue: https://pulp.plan.io/issues/8099

dkliban · 2021-01-19T18:33:19Z

pulpcore/app/models/storage.py

@@ -7,6 +7,9 @@
 from django.core.files.storage import FileSystemStorage


+CHUNKED_UPLOAD_STORAGE = FileSystemStorage(location=settings.CHUNKED_UPLOAD_DIR)


This is not always going to be FileSystemStorage. We need to use default storage.

I think default storage should work. I'd also consider just removing CHUNKED_UPLOAD_DIR setting. It's effectively broken since changing it generates a new migration which we can't have.

daviddavis · 2021-01-19T19:31:49Z

CHANGES/8099.bugfix

+
+As part of #4498, uploads are done to an absolute path rather than use a FileSystemStorage for this.
+In deployments where `MEDIA_ROOT` is different (such as `/var/lib/pulp/media`, it suddenly becomes a
+problem.


The changelog doesn't need this level of detail.

ekohl · 2021-01-20T14:30:43Z

After some offline discussion it turns out #1078 is the correct implementation.

ekohl commented Jan 18, 2021

View reviewed changes

This was referenced Jan 18, 2021

Pulpcore 3.9 requires upload to be under media theforeman/puppet-pulpcore#162

Closed

Change the default deployment layout #799

Merged

ianballou reviewed Jan 18, 2021

View reviewed changes

ekohl force-pushed the 8099-chunked-upload-location branch from cec9c80 to 120c19b Compare January 18, 2021 18:19

ianballou approved these changes Jan 18, 2021

View reviewed changes

dkliban reviewed Jan 19, 2021

View reviewed changes

daviddavis reviewed Jan 19, 2021

View reviewed changes

ekohl closed this Jan 20, 2021

ekohl deleted the 8099-chunked-upload-location branch January 20, 2021 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression with CHUNKED_UPLOAD_DIR outside MEDIA_ROOT #1074

Fix regression with CHUNKED_UPLOAD_DIR outside MEDIA_ROOT #1074

ekohl commented Jan 18, 2021

ekohl Jan 18, 2021

mdellweg Jan 19, 2021

ekohl Jan 19, 2021

ekohl Jan 19, 2021

mdellweg Jan 19, 2021

mdellweg Jan 19, 2021

dkliban Jan 19, 2021

ekohl Jan 19, 2021

ekohl Jan 19, 2021 •

edited

Loading

mdellweg Jan 20, 2021

ianballou left a comment •

edited

Loading

ekohl commented Jan 18, 2021

ianballou left a comment

pulpbot commented Jan 18, 2021

dkliban Jan 19, 2021

daviddavis Jan 19, 2021

daviddavis Jan 19, 2021

ekohl commented Jan 20, 2021

		@@ -7,6 +7,9 @@
		from django.core.files.storage import FileSystemStorage


		CHUNKED_UPLOAD_STORAGE = FileSystemStorage(location=settings.CHUNKED_UPLOAD_DIR)

Fix regression with CHUNKED_UPLOAD_DIR outside MEDIA_ROOT #1074

Fix regression with CHUNKED_UPLOAD_DIR outside MEDIA_ROOT #1074

Conversation

ekohl commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment • edited Loading

Choose a reason for hiding this comment

ekohl commented Jan 18, 2021

ianballou left a comment

Choose a reason for hiding this comment

pulpbot commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Jan 20, 2021

ekohl Jan 19, 2021 •

edited

Loading

ianballou left a comment •

edited

Loading