Hi John,
do you have any comments to this version ?
Best regards,
Tomas D.
Dne 20.12.11 16:52, Tomas Dzik napsal(a):
Hi John,
I added test case for dhcp changes and sorted imports in the
test_create_service.py. New webrev is here:
https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-5/
I still use Popen('setup-sparc') from following reasons:
1) Fix is in setup-sparc.sh script
2) The only method from which this script is called is _init_wanboot in
AIService object. To be able to call this method I would need to create
instance of this object which would require a lot of work and working
infrastructure on test server.
Best regards,
Tomas D.
Dne 16.12.11 23:53, John Fischer napsal(a):
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
