If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Ben - is this something that you want to do, or should it be futured for now?
(Assignee)

Comment 2

9 years ago
I'll do it fairly soon.
Assignee: nobody → bhearsum
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Priority: -- → P3
(Assignee)

Updated

9 years ago
Priority: P3 → P2
(Assignee)

Comment 3

9 years ago
Created attachment 370520 [details] [diff] [review]
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.
Created attachment 371218 [details] [diff] [review]
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.

Updated

9 years ago
Attachment #371218 - Attachment is patch: true
Attachment #371218 - Attachment mime type: application/octet-stream → text/plain
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.
(Assignee)

Comment 6

9 years ago
(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.
(Assignee)

Comment 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
(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.
(Assignee)

Comment 9

9 years ago
(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/ ffxbld@stage.mozilla.org:/home/ftp/pub/firefox/nightly/experimental/3.1b3-candidates/build2/
rsync -e "ssh -i /home/cltsign/.ssh/ffxbld_dsa" -av signed-build2/* ffxbld@stage.mozilla.org:/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?
(Assignee)

Comment 10

9 years ago
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.
Attachment #371218 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #370520 - Attachment is obsolete: true
(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.
(Assignee)

Comment 12

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

changeset:   270:0f80ac6c75d8
Attachment #371218 - Flags: checked‑in+ checked‑in+
(Assignee)

Comment 13

9 years ago
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...
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 722202
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.