Tomas, There are times when a test need not be written because it is covered elsewhere or some other valid justification can be made. I think in this case there is a valid justification for a test or set of tests being made.
Perhaps one test where a restrictive umask is used and then a wide open umask and checking the permissions against what is expected for each python module with critical file permissions. Thanks, John On Dec 12, 2011, at 8:16 AM, tomas dzik wrote: > Hi John, > is that really a question or you think that I should add these test ? ;-) > Also because we are fixing the issue with root having too restrictive umask - > do you think that these tests should run with restrictive umask ? If yes - > should the restrictive umask be set just in particular test case or as part > of setUpClass ? > > Regards, > > Tomas D. > > On 12.12.2011 15:31, John Fischer wrote: >> 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
