1015187 - add "FILE_UPLOAD_PERMISSIONS" setting to config

Status: RESOLVED FIXED

(Socorro :: Webapp, task)

Description

[Daniel Maher [:phrawzty]]

I propose adding the following setting to the local django config:

FILE_UPLOAD_PERMISSIONS = '2664'

This should, according to the django docs[1], have the desired effect of ensure that the uploaded file is effectively g+rw while preserving the group ownership inherited from system.  This satisfies one part of bug 1010327.

[1] https://docs.djangoproject.com/en/dev/topics/http/file-uploads/#changing-upload-handler-behavior

Comment 1

[Daniel Maher [:phrawzty]]

Upon further inspection of socorro/socorro/cron/jobs/symbolsunpack.py, this change alone might be sufficient to solve bug 1010327: since symbolsunpack.py runs as user socorro on the admin node, its unpacked files are already owned by user socorro, and when it's done it can remove the uploaded archive due to the group perms.

Definitely need to test this.

Comment 2

[Peter Bengtsson [:peterbe]]

A bit out of depth here but if you set this it affects the static files too::

$ ls -l ./static/CACHE/css/screen.854d23c34bcc.css
---sr-x--T+ 1 peterbe  staff  33204 May 23 10:13 ./static/CACHE/css/screen.854d23c34bcc.css

Because I run socorro behind nginx I get a 403 Forbidden when trying to read that file. 

Another thing I struggle with, is that the documentation says to use a leading 0 in the mode. But if you type `FILE_UPLOAD_PERMISSIONS = 0664` (for instance) you get 420.

>>> x=0644
>>> x
420

And you're not allowed to set `FILE_UPLOAD_PERMISSIONS = '0664'` you get a TypeError https://gist.github.com/peterbe/766e1197d3ec2d94a0f3

Comment 3

[Daniel Maher [:phrawzty]]

> ---sr-x--T+ 1 peterbe  staff  33204 May 23 10:13
> ./static/CACHE/css/screen.854d23c34bcc.css
> 
> Because I run socorro behind nginx I get a 403 Forbidden when trying to read
> that file. 

That file is most certainly not x664.  I don't know what it's supposed to be - probably not that though.  From my reading of the documentation, FILE_UPLOAD_PERMISSIONS should not affect the static assets at all.

> Another thing I struggle with, is that the documentation says to use a
> leading 0 in the mode. But if you type `FILE_UPLOAD_PERMISSIONS = 0664` (for
> instance) you get 420.

The leading digit is optional (and often implicit); it represents the setuid/gid bit. https://en.wikipedia.org/wiki/Setuid has a good explanation.

> And you're not allowed to set `FILE_UPLOAD_PERMISSIONS = '0664'` you get a
> TypeError https://gist.github.com/peterbe/766e1197d3ec2d94a0f3

To be fair, I want it to be set to 2664, which is different.  In any case, I'm not a Django expert, and I am working only from their documentation.  Possibilities include:
1. My interpretation of the documentation is faulty. (I concede that this is entirely possible.)
2. The documentation is incorrect.
3. You've exposed an enormous Django bug.
4. You're misinterpreting your test case.

At this time I do not have sufficient information to make any further concrete statements.  I do, however, suggest that we continue to investigate this situation, since it presents intriguing questions in any case.

Comment 4

[Peter Bengtsson [:peterbe]]

(In reply to Daniel Maher [:phrawzty] from comment #3)
> > ---sr-x--T+ 1 peterbe  staff  33204 May 23 10:13
> > ./static/CACHE/css/screen.854d23c34bcc.css
> > 
> > Because I run socorro behind nginx I get a 403 Forbidden when trying to read
> > that file. 
> 
> That file is most certainly not x664.  I don't know what it's supposed to be
> - probably not that though.  From my reading of the documentation,
> FILE_UPLOAD_PERMISSIONS should not affect the static assets at all.
> 
Well, it does. Sad isn't it!

> > Another thing I struggle with, is that the documentation says to use a
> > leading 0 in the mode. But if you type `FILE_UPLOAD_PERMISSIONS = 0664` (for
> > instance) you get 420.
> 
> The leading digit is optional (and often implicit); it represents the
> setuid/gid bit. https://en.wikipedia.org/wiki/Setuid has a good explanation.
> 
> > And you're not allowed to set `FILE_UPLOAD_PERMISSIONS = '0664'` you get a
> > TypeError https://gist.github.com/peterbe/766e1197d3ec2d94a0f3
> 
> To be fair, I want it to be set to 2664, which is different.  In any case,
> I'm not a Django expert, and I am working only from their documentation. 
> Possibilities include:
> 1. My interpretation of the documentation is faulty. (I concede that this is
> entirely possible.)
> 2. The documentation is incorrect.
> 3. You've exposed an enormous Django bug.
> 4. You're misinterpreting your test case.
> 

This is frustrating! So, it affects the staticfiles (not a typo, staticfiles is the package that handles all the .js and .css and stuff) mechanics. And you can't put it to `2644` clearly. And not that we need it but if we tried to put it in with a leading `0` we wouldn't be able to. 

> At this time I do not have sufficient information to make any further
> concrete statements.  I do, however, suggest that we continue to investigate
> this situation, since it presents intriguing questions in any case.

The way files are written to disk is all encapsulated and handled by django. You just set a file (e.g. a InMemoryFile instance from a form upload) to a model and that's all. We have some control over which sub-directory it goes into but not much more. 

What we can do is loop (recursively) over all the files in the root ./media directory and in pure python make sure each file has the right chmod. Shall we look into that?

Comment 5

[Daniel Maher [:phrawzty]]

(In reply to Peter Bengtsson [:peterbe] from comment #4)
> What we can do is loop (recursively) over all the files in the root ./media
> directory and in pure python make sure each file has the right chmod. Shall
> we look into that?

I am deeply concerned by this.  The behaviour that you're describing means not only that the Django documentation is wildly inaccurate, but that there may be very serious flaws in the code base itself - flaws that could have security implications.

It's easy enough to say "fine, we'll just work around it", but I'm not sure that's the best long-term approach.  We'll need to look into the Django bug tracker and see if this is a known issue; if so, hopefully there's a fix, but if we've managed to uncover an as-yet-undiscovered but potentially serious bug, then we need to report on it as soon as possible.

Comment 6

[Laura Thomson :laura]

Did you actually test this in the app? (How do you know it affects the static files?)

Comment 7

[Daniel Maher [:phrawzty]]

(In reply to Laura Thomson :laura from comment #6)
> Did you actually test this in the app? (How do you know it affects the
> static files?)

I just gave it a go in Stage.

I first attempted to set the file mode as a string, as in (literally): FILE_UPLOAD_PERMISSIONS = '2664'
This does, indeed, result in a TypeError, as noted in Errormill[1].

I next set it as a bare value, so as to retain intergerness, as in: FILE_UPLOAD_PERMISSIONS = 2664
This doesn't throw an error, but it does result in bizarre file permissions (see below) as noted previously by peterbe.

[[e-mail]1 26]$ ls -l 6a63cd21e7f32eacd0cdb3a6ad0b6241.gz
---sr-x--- 1 apache socorro 450 May 26 16:15 6a63cd21e7f32eacd0cdb3a6ad0b6241.gz

I have read and re-read the documentation on the topic, and the empirical results just aren't lining up with what's described, which is worrisome.  We are therefore left in the uncomfortable position of having to work around a behaviour that we don't understand (or, worse, may constitute a bug).


[1] https://errormill.mozilla.org/webtools/socorro-stage/group/169054/

Comment 8

[Laura Thomson :laura]

Did you try it as the docs suggest, with a 0 in front? 

In Python, the leading 0 means the number should be interpreted as octal. This is not equivalent to chmodding to 0644 on the command line: the 0 has nothing to do with the gid, it is merely a base signifier. It's not a bug.

Comment 9

[Daniel Maher [:phrawzty]]

(In reply to Laura Thomson :laura from comment #8)
> Did you try it as the docs suggest, with a 0 in front? 
> 
> In Python, the leading 0 means the number should be interpreted as octal.
> This is not equivalent to chmodding to 0644 on the command line: the 0 has
> nothing to do with the gid, it is merely a base signifier. It's not a bug.

Ok, that is a concise explanation, and while I disagree in the strongest terms with that design principle, I do at least now understand the situation.  Thank you.  Moving on, test results as follows.

Setting it as a string (i.e. '0664') results in the expected TypeError[1].  

Setting it as a bare value results in the expected behaviour (yay):
-rw-rw-r-- 1 apache socorro  450 May 26 17:20 b4d4cde8368a8af0745d25fd6a1f0386.gz

Furthermore, I can find no evidence that this setting has any effect on the permissions of the static files, which is the expected behaviour; in other words, at first glance setting "FILE_UPLOAD_PERMISSIONS = 0664" appears to do exactly what we want, which is to ensure that uploaded files gain group read/write permissions.  This, along with :laura's explanation in comment #8 (and the *lack* of the errant behaviour in comment #2), does in fact line up with the documentation.

That said, the reasons for :peterbe's findings regarding static files in comment #2 and comment #4 remain unknown.  I have not (yet) been able to replicate that behaviour in Stage.


[1] https://errormill.mozilla.org/webtools/socorro-stage/group/169054/

Comment 10

[Peter Bengtsson [:peterbe]]

> That said, the reasons for :peterbe's findings regarding static files in
> comment #2 and comment #4 remain unknown.  I have not (yet) been able to
> replicate that behaviour in Stage.
> 

I got this problem when I ran::

 ./manage.py collectstatic --noinput && ./manage.py compress_jingo --force

Comment 11

[Peter Bengtsson [:peterbe]]

I tried, putting 0644 does work with both collectstatic and symbols upload. 
With https://github.com/mozilla/socorro/pull/2063

You get::

 $ ls -l media/symbols_upload/2014/05/27/e614b04600bf457d333bb11218ea3150.zip
 -rw-rw-r--+ 1 peterbe  staff  89602 May 27 07:58 media/symbols_upload/2014/05/27/e614b04600bf457d333bb11218ea3150.zip

And::
 $ ls -ltr static/ | head
 total 8
 drwxr-xr-x+  4 peterbe  staff   136 May 27 08:03 tokens
 drwxr-xr-x+  3 peterbe  staff   102 May 27 08:03 symbols
 drwxr-xr-x+  4 peterbe  staff   136 May 27 08:03 supersearch
 -rw-rw-r--+  1 peterbe  staff  1265 May 27 08:03 stick.js
 drwxr-xr-x+  5 peterbe  staff   170 May 27 08:03 manage
 drwxr-xr-x+ 27 peterbe  staff   918 May 27 08:03 img
 drwxr-xr-x+  5 peterbe  staff   170 May 27 08:03 crashstats
 drwxr-xr-x+  6 peterbe  staff   204 May 27 08:03 browserid
 drwxr-xr-x+  4 peterbe  staff   136 May 27 08:03 auth
 

Now this is on my mac. I'm still not understanding what that 2 means in 2644.

Comment 12

[Daniel Maher [:phrawzty]]

At the system layer 2 in 2664 refers to the setuid/gid bit; however, as per comment #8, the lead digit means something different in Python / Django.

Comment 13

[Laura Thomson :laura]

So...this works as expected in stage? Can we roll it to prod?

Comment 14

[Daniel Maher [:phrawzty]]

This change has been committed to Prod (puppet) and will roll out within the next hour.

Comment 15

[Daniel Maher [:phrawzty]]

There are local.py templates for both stage and prod in Puppet.  Turns out that this template is actually only deployed in stage - prod doesn't use it (despite it being present in the module).  Sigh...

Comment 16

[Daniel Maher [:phrawzty]]

I made the appropriate modification to the actual local.py that is used in prod (for reference, it sits on socorroadm.private.phx1), pushed that change to the webheads, and then restarted Apache on said nodes.  Permissions and ownership were both set properly on the test file that I uploaded through the symbols upload interface.