Tomas, My reasoning is fairly simple for removing the chmod. What would happen if someone got in between the mv and chmod? Could they then chmod any file on the system? What I have seen is people stopping a process once they see the move happen and then make a link from the moved file to a file of interest. Then they continue the program or script and it chmod-s the wrong file. A file of interest might include /etc/shadow.
Thanks, John On Dec 8, 2011, at 9:42 AM, Tomas Dzik wrote: > Hi John, > it's easy to change that code to use umask instead of chmod. On the other > hand it's very hard to get consensus on this mailing list how to do such > simple thing like changing ownership of a file in a shell script. > > So I would like to know definitive answer before I will change the code (it > should take 2 minutes) and retest it (it takes me more then 3 hours to build > it on both platforms, run tests, update test ai servers on both platforms and > to create service and install client machines). And this is already 3rd > version of this fix. > > Ethan what do you think ? > > Best regards, > > Tomas D. > > Dne 8.12.11 18:15, John Fischer napsal(a): >> Tomas, >> >> In that case can you please use a umask so that the permissions are correct >> when the file is created. >> >> Thanks, >> >> John >> >> On Dec 8, 2011, at 9:06 AM, Tomas Dzik wrote: >> >>> Dne 8.12.11 17:51, John Fischer napsal(a): >>>> Tomas, >>>> >>>> Why are you using umask for everything except the sparc-setup.sh code? >>>> The chmod still concerns me. Is there a reason why an appropriate umask >>>> can not be used in the sparc-setup.sh script? >>>> >>>> Thanks, >>>> >>>> John >>> >>> Hi John, >>> there is no particular reason for using chmod in sparc-setup.sh code. Both >>> ways would work. I just picked one. >>> >>> Best regards, >>> >>> Tomas D. >>> >>>> On Dec 8, 2011, at 8:45 AM, Ethan Quach wrote: >>>> >>>>> Tomas, >>>>> >>>>> It looks fine to me. >>>>> >>>>> thanks, >>>>> -ethan >>>>> >>>>> >>>>> On 12/08/11 08:26, Tomas Dzik wrote: >>>>>> Hi Ethan, >>>>>> I changed chmod to absolute 0644 and the latest webrev is here: >>>>>> >>>>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-2/ >>>>>> >>>>>> Could you please look at it and let me know whether is it OK now ? >>>>>> >>>>>> Thanks a lot in advance, >>>>>> >>>>>> Tomas D. >>>>>> >>>>>> Dne 7.12.11 09:24, Ethan Quach napsal(a): >>>>>>> Tomas, >>>>>>> >>>>>>> On 12/06/11 04:43, Tomas Dzik wrote: >>>>>>>> Hi Ethan, >>>>>>>> I was considering all three ways and deciding which way to use and I >>>>>>>> choose setting umask to be cleanest and safest option. >>>>>>>> >>>>>>>> Combination of os.fdopen(os.open()) does not work because according to >>>>>>>> python docs: >>>>>>>> http://docs.python.org/library/os.html#os.open >>>>>>>> >>>>>>>> "Open the file file and set various flags according to flags and >>>>>>>> possibly its mode according to mode. The default mode is 0777 (octal), >>>>>>>> and the current umask value is first masked out." >>>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>> >>>>>>> Ok. I didn't realize it masked against the umask for cases where a mode >>>>>>> is passed in ... >>>>>>> >>>>>>>> >>>>>>>> Chmoding these files after creation is possible - however you never >>>>>>>> know what might be security critical. >>>>>>>> >>>>>>>> For the changes in setup-sparc.sh I intentionally just added the 'r' >>>>>>>> permission because I know that these files must be readable for >>>>>>>> everyone and I didn't want to anticipate anything else - it's up to >>>>>>>> root to set his umask according to his needs/security policy. However >>>>>>>> if you feel that application should pretend to be smarter then admin I >>>>>>>> can easily change it. >>>>>>> >>>>>>> But we're effectively doing this for the files being created where umask >>>>>>> is used, so why do it differently with these two files? >>>>>>> >>>>>>>> >>>>>>>> So do you want me to change os.umask to os.chmod(00644) in python code >>>>>>>> and >>>>>>> >>>>>>> Using umask is fine with me here. >>>>>>> >>>>>>>> chmod ugo+r to chmod 0644 in setup-sparc.sh ? >>>>>>> >>>>>>> I would prefer to set them to 0644 explicitly. >>>>>>> >>>>>>> >>>>>>> thanks, >>>>>>> -ethan >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Regards, >>>>>>>> >>>>>>>> Tomas D. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Dne 6.12.11 03:07, Ethan Quach napsal(a): >>>>>>>>> Tomas, >>>>>>>>> >>>>>>>>> Rather than dancing with setting/re-setting the umask, why shouldn't >>>>>>>>> we >>>>>>>>> just explicitly set the perms of the file to exactly what we want at >>>>>>>>> time of creation? >>>>>>>>> >>>>>>>>> I noticed that the built-in open() function does not allow >>>>>>>>> specification >>>>>>>>> of perms for cases where it creates the file, but the os.open() >>>>>>>>> function >>>>>>>>> does, and that can be wrapped around an os.fdopen() call to be used >>>>>>>>> with >>>>>>>>> 'with'. >>>>>>>>> >>>>>>>>> For example, the 'with' line in dhcp.py could be changed to the >>>>>>>>> following to achieve what we need: >>>>>>>>> >>>>>>>>> with os.fdopen(os.open(tmp_cfgfle, os.O_WRONLY | os.O_CREAT, 0644), >>>>>>>>> 'w') >>>>>>>>> as tmp_cfg: >>>>>>>>> .... >>>>>>>>> ... >>>>>>>>> >>>>>>>>> >>>>>>>>> I'm no python expert so I don't know if there are any serious >>>>>>>>> drawbacks >>>>>>>>> to using this versus using the built-in open(), but to me temporarily >>>>>>>>> setting something in the env (the umask in this case) across a chunk >>>>>>>>> of >>>>>>>>> code seems susceptible to future bugs. >>>>>>>>> >>>>>>>>> >>>>>>>>> Alternatively, if the file in question is not a security-critical file >>>>>>>>> (which seem to be true for the files in this fix), what's wrong with >>>>>>>>> simply chmod'ing the file to exactly what we want right after its >>>>>>>>> creation? >>>>>>>>> >>>>>>>>> >>>>>>>>> For the changes in setup-sparc.sh, just adding the 'r' perm would >>>>>>>>> still >>>>>>>>> leave some bits susceptible to the umask; so at a minimum, we should >>>>>>>>> just set it entirely to what we want, 0644 in this case. >>>>>>>>> >>>>>>>>> >>>>>>>>> -ethan >>>>>>>>> >>>>>>>>> >>>>>>>>> On 12/05/11 02:23, Tomas Dzik wrote: >>>>>>>>>> This is just a reminder that I would appreciate if someone could >>>>>>>>>> review these changes. >>>>>>>>>> >>>>>>>>>> Thanks a lot in advance, >>>>>>>>>> >>>>>>>>>> Tomas D. >>>>>>>>>> >>>>>>>>>> Dne 30.11.11 13:14, Tomas Dzik napsal(a): >>>>>>>>>>> Hi all, >>>>>>>>>>> I would like to ask you for a code review for bug: >>>>>>>>>>> >>>>>>>>>>> 7052679 AI Sparc client gets wantboot.conf error when booting due to >>>>>>>>>>> restrictive umask >>>>>>>>>>> Webrev: >>>>>>>>>>> >>>>>>>>>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679/ >>>>>>>>>>> >>>>>>>>>>> Testing: >>>>>>>>>>> >>>>>>>>>>> 1) I built the gate with fix on x86 and on sparc. >>>>>>>>>>> 2) Using distro constructor I created AI iso image on both platforms >>>>>>>>>>> 3) I created virtual machines on x86 and on sparc and updated these >>>>>>>>>>> machines to the built gate >>>>>>>>>>> 4) I set umask for root to 0066 (as reported in bug) >>>>>>>>>>> 5) On x86 I created install service. For sparc I created it from >>>>>>>>>>> publisher and for x86 I created it from built AI iso image >>>>>>>>>>> 6) I verified permissions of wanboot.conf and system.conf >>>>>>>>>>> 7) I modified manifest, added it as a new default manifest and >>>>>>>>>>> installed >>>>>>>>>>> x86 client (also on virtual machine) from this AI server. >>>>>>>>>>> I observed that client installed correctly. >>>>>>>>>>> 8) On sparc I created install service for sparc from built iso, >>>>>>>>>>> added >>>>>>>>>>> new default manifest and also added new profile. (All command were >>>>>>>>>>> run >>>>>>>>>>> with umask set to 0066.) >>>>>>>>>>> 9) I installed sparc client from this AI server and verified that >>>>>>>>>>> client >>>>>>>>>>> installed correctly and used the right manifest and profile. >>>>>>>>>>> 10) Sources are pep8 clean. >>>>>>>>>>> >>>>>>>>>>> Best regards, >>>>>>>>>>> >>>>>>>>>>> Tomas D. >>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>> [email protected] >>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> caiman-discuss mailing list >>>>>>>>>> [email protected] >>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>> >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> caiman-discuss mailing list >>>>>>>>> [email protected] >>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>> >>>>>> >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> [email protected] >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>> >> > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
