Tomas, These changes look good. My only question now is should the tests be updated to check the permissions of these files that get created? The only one that looks a little challenging to do would be the dhcp.py test.
Thanks, John On Dec 12, 2011, at 2:56 AM, Tomas Dzik wrote: > Hi all, > the updated webrev is here: > > https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-3/ > > Could you please let me know whether it is OK now ? > > Best regards, > > Tomas D. > > Dne 9.12.11 01:34, Ethan Quach napsal(a): >> >> >> On 12/08/11 09:42, 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 ? >> >> John's suggestion sounds fine to me. >> >> -ethan >> >>> >>> 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
