484972 - post_upload.py should chmod files/dirs to 644/755 for --release-to-candidates-dir target

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Comment 1

[alice nodelman [:alice] [:anode]]

Ben - is this something that you want to do, or should it be futured for now?

Comment 2

[[e-mail] (:bhearsum)]

I'll do it fairly soon.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: chmod --release-to-candidates-dir things

Nick, you can give this a try when you do your 3.5rc1 test run, if you don't mind. I tested it locally and everything seems in order.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Handle parent dirs

Attachment 370520 [details] [diff] works nicely for incoming files in my mock release runs, but I think we're leaving the permissions on the $version-candidates and buildN dirs up to the local umask. Having write access to a dir allows you to do some sneaky things even if the files are locked down, eg delete files and dirs. This patch is the brute force fix, untested against non-release builds. Lots of other ways this could be done.

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Probably we'll need to tweak the signing stuff, so it chmods the files and then does 
  rsync signed-build2/* ...
instead of the bare dir.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> Created an attachment (id=371218) [details]
> Handle parent dirs
> 
> Attachment 370520 [details] [diff] works nicely for incoming files in my mock release runs, but
> I think we're leaving the permissions on the $version-candidates and buildN
> dirs up to the local umask. Having write access to a dir allows you to do some
> sneaky things even if the files are locked down, eg delete files and dirs. This
> patch is the brute force fix, untested against non-release builds. Lots of
> other ways this could be done.

It looks like we have lots of nightly dirs that are 755 - all of m-c/1.9.1/tm AFAICT:
drwxrwxr-x  2 ffxbld firefox 4096 Apr  6 06:51 2009-04-05-06-mozilla1.9.0
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 04:59 2009-04-06-03-mozilla-1.9.1
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 04:47 2009-04-06-03-mozilla-central
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 04:41 2009-04-06-03-tracemonkey
drwxrwxr-x  4 ffxbld firefox 4096 Apr  6 06:19 2009-04-06-04-mozilla1.8
drwxrwxr-x  2 ffxbld firefox 4096 Apr  6 04:58 2009-04-06-04-mozilla1.9.0
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 06:53 2009-04-06-04-mozilla-central
drwxrwxr-x  2 ffxbld firefox 4096 Apr  6 06:44 2009-04-06-05-mozilla1.9.0
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 06:41 2009-04-06-05-mozilla-1.9.1
drwxr-sr-x  2 ffxbld firefox 4096 Apr  6 06:42 2009-04-06-05-mozilla-central


tinderbox-builds dirs look the same.

I'd bet this change will be fine/go unnoticed.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 371218 [details] [diff] [review]
Handle parent dirs

>diff -r a720158f8e83 stage/post_upload.py
>--- a/stage/post_upload.py	Fri Mar 27 03:33:26 2009 -0700
>+++ b/stage/post_upload.py	Mon Apr 06 02:57:41 2009 -0700
>@@ -26,17 +26,17 @@ def CopyFileToDir(original_file, source_
>     if preserve_dirs:
>         # Add any dirs below source_dir to the final destination
>         filePath = original_file.replace(source_dir, "").lstrip("/")
>         filePath = os.path.dirname(filePath)
>         dest_dir = os.path.join(dest_dir, filePath)
>     new_file = os.path.join(dest_dir, relative_path)
>     full_dest_dir = os.path.dirname(new_file)
>     if not os.path.isdir(full_dest_dir):
>-        os.makedirs(full_dest_dir)
>+        os.makedirs(full_dest_dir, 0755)
>     if os.path.exists(new_file):
>         os.unlink(new_file)
>     shutil.copyfile(original_file, new_file)
> 
> def BuildIDToDict(buildid):
>     """Returns an dict with the year, month, day, hour, minute, and second
>        as keys, as parsed from the buildid"""
>     buildidDict = {}
>@@ -122,16 +122,25 @@ def ReleaseToCandidatesDir(options, uplo
> 
>     for f in files:
>         if f.endswith('crashreporter-symbols.zip'):
>             continue
>         realCandidatesPath = candidatesPath
>         if 'win32' in f:
>             realCandidatesPath = os.path.join(realCandidatesPath, 'unsigned')
>         CopyFileToDir(f, upload_dir, realCandidatesPath, preserve_dirs=True)
>+        # We always want release files chmod'ed this way so other users in
>+        # the group cannot overwrite them.
>+        os.chmod(f, 0644)
>+
>+    # Same thing for directories, but 0755
>+    for root,dirs,files in os.walk(candidatesPath):
>+        for d in dirs:
>+            os.chmod(os.path.join(root, d), 0755)
>+
Doesn't look like this block will chmod the candidatesDir itself - do we need to do that?

Comment 8

[[e-mail] (:bhearsum)]

(In reply to comment #7)

> Doesn't look like this block will chmod the candidatesDir itself - do we need
> to do that?

Of course, now I see the os.makedirs() change that will deal with the candidates dir.

Comment 9

[[e-mail] (:bhearsum)]

(In reply to comment #5)
> Probably we'll need to tweak the signing stuff, so it chmods the files and then
> does 
>   rsync signed-build2/* ...
> instead of the bare dir.

To expand on this, the newly signed MAR files end up with 755 permissions instead of 644 - so we need to chmod them back to 644. Interestingly, the installers ended up 644 to start with.

I'm not sure we need to do signed-build2/* though. In my tests, both of the follow commands ended up giving the build2 dir 755 perms, owned by ffxbld:firefox with no sticky bit:
rsync -e "ssh -i /home/cltsign/.ssh/ffxbld_dsa" -av signed-build2/ [e-mail]:/home/ftp/pub/firefox/nightly/experimental/3.1b3-candidates/build2/
rsync -e "ssh -i /home/cltsign/.ssh/ffxbld_dsa" -av signed-build2/* [e-mail]:/home/ftp/pub/firefox/nightly/experimental/3.1b3-candidates/build2/

Additionally, the perms/ownership on all of the subdirs and files were the same.

Nick, what are we trying to guard against by using signed-build2/* rather than the bare dir?

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 371218 [details] [diff] [review]
Handle parent dirs

Looks to me like this will fix things up. We should already be uploading everything 755/644, since bug 483229 landed, but it doesn't hurt to have this to make sure.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #9)
> Nick, what are we trying to guard against by using signed-build2/* rather than
> the bare dir?

I wanted to make sure we avoided mangling permissions on the dirs when they get sucked down to the signing machine and pushed back up (from mismatched group definitions, umask etc). From your tests it looks like it'll work fine, although it may rely on some implicit config somewhere.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 371218 [details] [diff] [review]
Handle parent dirs

changeset:   270:0f80ac6c75d8

Comment 13

[[e-mail] (:bhearsum)]

I've updated the checkouts on staging-stage.b.m.o and stage.m.o with the new post_upload.py.

I think we're done here too...