Tomas,
Thanks for creating the test cases. I would have expected _write_config_file()
to be called or some other function instead of Popen('sparc_setup.sh').
Also the imports for the test_create_service.py should be alphabetized.
Finally, does this test the dhcp changes?
Thanks,
John
On Dec 16, 2011, at 12:09 PM, Tomas Dzik wrote:
> Hi John,
> I added unit tests for file permissions and fixed typo in bug synopsis.
> New webrev is here:
>
> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-4/
>
> Could you please look at it ?
>
> Best regards,
>
> Tomas D.
>
> Dne 12.12.11 17:50, John Fischer napsal(a):
>> 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
