--- Log opened vr nov 13 00:00:37 2015 00:46 -!- adahms (Andrew Dahms): has joined #vdsm 06:47 -!- adahms: has quit [Read error: Connection reset by peer] 06:57 -!- bala (purple): has joined #vdsm 07:08 -!- bala: has quit [Ping timeout: 244 seconds] 07:50 -!- sbonazzo (purple): has joined #vdsm 07:54 -!- sbonazzo: has quit [Client Quit] 07:55 -!- sbonazzo (purple): has joined #vdsm 08:14 -!- ybronhei (purple): has joined #vdsm 08:20 -!- mskrivanek_away is now known as mskrivanek 08:27 -!- rmohr (Roman Mohr): has joined #vdsm 08:31 -!- sbonazzo is now known as sbonazzo|afk 08:33 -!- pkliczew (Piotr Kliczewski): has joined #vdsm 08:49 -!- mmirecki (Marcin Mirecki): has joined #vdsm 08:50 -!- sbonazzo|afk is now known as sbonazzo 09:06 -!- fromani (Francesco Romani): has joined #vdsm 09:21 -!- fabiand (Fabian Deutsch): has joined #vdsm 09:23 -!- #vdsm ybronhei: has quit [Ping timeout: 240 seconds] 10:05 -!- msivak: has quit [Ping timeout: 240 seconds] 10:12 -!- msivak (Martin Sivak): has joined #vdsm 10:12 -!- rmohr: has quit [Remote host closed the connection] 10:12 -!- rmohr (Roman Mohr): has joined #vdsm 11:09 -!- msivak: has quit [Ping timeout: 240 seconds] 11:15 < mpolednik1> danken1: heya, could you have a look at https://gerrit.ovirt.org/#/c/46912/ ? verification in progress atm 11:18 -!- msivak (Martin Sivak): has joined #vdsm 11:28 -!- msivak: has quit [Ping timeout: 252 seconds] 11:30 -!- nsoffer (Nir Soffer): has joined #vdsm 11:39 -!- msivak (Martin Sivak): has joined #vdsm 11:44 -!- rmohr: has quit [Ping timeout: 250 seconds] 11:46 -!- rmohr (Roman Mohr): has joined #vdsm 11:47 -!- apahim (Amador Pahim): has joined #vdsm 12:08 < nsoffer> fromani, ping 12:09 < fromani> nsoffer: pong 12:09 < nsoffer> fromani, reading your comments o nhttps://gerrit.ovirt.org/#/c/48133 12:09 < nsoffer> fromani, https://gerrit.ovirt.org/#/c/48133/3/vdsm/mkimage.py@135 12:10 < fromani> nsoffer: yep 12:10 -!- hchiramm: has quit [Ping timeout: 240 seconds] 12:10 < nsoffer> fromani, so we create an isofs, that will be used to boot a vm? 12:10 < fromani> nsoffer: yes 12:11 < nsoffer> and you want to create the image file in mode 600? 12:11 < nsoffer> fromani, on the host local file system, right? 12:12 < nsoffer> fromani, ? 12:18 < fromani> yes 12:19 < nsoffer> fromani, so why not: 12:19 < nsoffer> $ touch buildbot.iso 12:19 < nsoffer> $ chmod 0600 buildbot.iso 12:19 < nsoffer> $ mkisofs -R -J -o buildbot.iso buildbot 12:20 < nsoffer> $ ls -lht buildbot.iso 12:20 < nsoffer> -rw-------. 1 nsoffer nsoffer 0 Nov 13 13:17 buildbot.iso 12:20 < nsoffer> fromani, in a way that works :-) 12:22 < fromani> nsoffer: because I didn't thought about this :) 12:22 < fromani> nsoffer: let me play with that 12:23 -!- danken1: has quit [Ping timeout: 240 seconds] 12:23 < nsoffer> fromani, you can create the output file with one call using os.open(path, os.O_CREAT..., 0600) 12:27 < nsoffer> fromani, it actually works, the directory I used before could not be written with -J 12:27 -!- bala (purple): has joined #vdsm 12:27 < nsoffer> $ ls -lht 12:27 < nsoffer> total 1.3M 12:27 < nsoffer> -rw-------. 1 nsoffer nsoffer 370K Nov 13 13:26 image.iso 12:27 < fromani> nsoffer: nice hint! and much simpler 12:28 < nsoffer> fromani, hopefully mkisofs does it on purpose, and not by accident 12:29 < fromani> nsoffer: worth a check, though 12:29 < fromani> nsoffer: will do 12:29 < nsoffer> fromani, anyway it seems useful to use existing file, just truncate it on open, keeps the file permissions 12:29 < nsoffer> fromani, give the user control over the file permissions without adding additional options 12:30 < fromani> nsoffer: I thought so. I tend to believe mkisofs just open()s the file with little check before 12:34 < nsoffer> fromani, maybe it did not work on rhel6? 12:35 < fromani> nsoffer: back in time, I didn't even considered this option :\ 12:35 < fromani> nsoffer: so I don't know 12:35 < nsoffer> fromani, it works on fedora 22, lets test on rhel7 12:35 < fromani> nsoffer: sure 12:35 < nsoffer> it it works, lets use it 12:38 < fromani> nsoffer: agreed 12:38 -!- bala: has quit [Ping timeout: 265 seconds] 12:39 < nsoffer> fromani, I see that in _getFileName, we create a file with embedded md5 digest 12:39 < nsoffer> fromani, is it for preventing name clashes? 12:39 < fromani> nsoffer: AFAIR yes 12:40 < nsoffer> fromani, so we can use tempfile.mkstemp(dir=imagedir) 12:40 < nsoffer> fromani, no need to re-implement it 12:40 < nsoffer> and you have your 0600 file 12:41 < fromani> nsoffer: yep. This is very old code 12:41 < nsoffer> fromani, or maybe they wanted to ensure one file per specific content? 12:42 < fromani> nsoffer: I can't answer without digging git logs 12:42 < nsoffer> fromani, the md5 comes from the names of the files 12:42 < fromani> nsoffer: yep, but I can't tell if this was for uniqueness or for some other reason 12:42 < nsoffer> fromani, do we keep these files after booting? 12:43 < fromani> nsoffer: we kill them when VM shuts down 12:43 < nsoffer> fromani, so there is not need to use that md5 12:44 < nsoffer> but I would not change it in the same patch 12:45 < fromani> nsoffer: indeed. I'm crafting a new patch which just adds the os.open() call and drops the childUmask argument 12:45 < fromani> nsoffer: (much needed) cleanup in later patches 12:45 < nsoffer> fromani, +1 12:51 < fromani> nsoffer: out of curiosity, I peeked at the sources of genisoimage (aka mkisofs), it opens output file with plain fopen(path, "wb") 12:52 < nsoffer> fromani, so it probably truncating the file, which is good, but does not create it if the file exists 12:53 < nsoffer> fromani, so no problem 12:53 < fromani> nsoffer: indeed. 12:54 < nsoffer> fromani, we can add a check for permissions after the operation, to warn if the behavior changes 12:54 < fromani> nsoffer: good idea. Will do in a followup patch 12:55 -!- msivak: has quit [Ping timeout: 260 seconds] 13:39 < nsoffer> fromani, maybe we need a touch utility function? 13:40 < nsoffer> fromani, I think we have something like this 13:40 < fromani> nsoffer: searching in the codebase 13:40 < nsoffer> utils.touchFile 13:41 < nsoffer> fromani, but it is not good for us 13:41 < nsoffer> since it will keep existing file with incorrect permissions 13:41 < nsoffer> I think we need: 13:42 < nsoffer> create file safely with our permissions, fail if file exists 13:42 < nsoffer> fromani - tempfile.mkstemp is what we need 13:44 < nsoffer> when we finish the operation, we can modify the permissions to the final value and close the fd 13:45 < nsoffer> fromani, if we reuse existing file, someone can trick us to put the root password in his readable file 13:46 < fromani> nsoffer: let me see 13:47 < nsoffer> smells like we need to get rid of that content md5 to get this right 13:48 < fromani> nsoffer: do you want to switch to mkstemp in the same patch or do you want to have os.open(...) done right and later switch to mkstemp? 13:49 < nsoffer> fromani, to have os.open done right, we need to reimplement mkstemp 13:49 < fromani> nsoffer: right, so let me address your comments to the existing patch as next step 13:49 < nsoffer> ok 13:51 < nsoffer> fromani, this wil break the tests, assuming predictable file name 13:51 < nsoffer> fromani, which is good, predictable file names are insecure 13:52 < fromani> nsoffer: agreed 13:52 < nsoffer> or not, mkIsoFs returns the path, so the test do not have to predict the name 13:53 < nsoffer> lets also get rid of these annoying names in this module, and move to make_isofs 13:54 < nsoffer> fromani, later of course 13:54 < fromani> nsoffer: yes, this seems much better. 13:54 < fromani> nsoffer: I vaguely remember an issue related to migrations 13:54 < fromani> nsoffer: which required these predictable names 13:55 < fromani> nsoffer: will dig in the archives to check that 13:55 < nsoffer> fromani, good point 13:55 < nsoffer> so we can do this with os.open, changing the permission if they are incorrect 13:56 < nsoffer> like, create file with mode, check permissions, fix if needed, run mkisofs 13:56 < nsoffer> fromani, but how does the isofs move to the dest host? we recreate the file there? 13:57 < fromani> nsoffer: we do recreate, but (IIRC) the problem is the path in the VM XML 13:58 < nsoffer> fromani, where does the contents come from? 13:58 < fromani> nsoffer: VM parameters. We are in the infamous "create image from payload" path. 13:59 < nsoffer> fromani, we pass the contents in a python dict? 13:59 < fromani> nsoffer: it is encoded as base64 string 14:00 < nsoffer> fromani, so engine could also set the file name, using a new uuid? 14:00 < fromani> nsoffer: yep, but the problem IIRC was to make sure the paths on dest side are right, and engine has no control on that 14:03 < nsoffer> fromani, anyway, if we depend on predictable names, we can do 14:03 < nsoffer> 1. remove the file 14:03 < nsoffer> 2. create new file with O_CREAT | O_EXCL 14:04 < nsoffer> if someone is trying to trick us, creation will fail - good 14:04 < nsoffer> if we got a file, it is using the right permissions 14:04 < nsoffer> and add also O_NOFOLLOW 14:07 < nsoffer> fromani, O_NOFOLLOW not needed, O_CREAT | O_EXCL imply this 14:09 < nsoffer> fromani, this is actually a security fix we should backport, previously it was possible to trick us and get iso image readable by an attacker 14:10 < fromani> nsoffer: ok, sounds good 14:10 < nsoffer> fromani, btw, what about selinux label for the image? 14:11 < fromani> nsoffer: AFAIK noone ever cared about that, and honestly I don't know which label it will have 14:11 < nsoffer> fromani, if this is for specific vm, we should use svirt selinux label to allow only specific vm to access this file 14:11 < fromani> nsoffer: yes! definitely we should 14:12 < nsoffer> fromani, but currently we know the label only after the vm starts 14:12 < fromani> nsoffer: right, and furthermore now that I think of, libvirt is in charge to manage them 14:12 < nsoffer> libvirt set it 14:12 < nsoffer> maybe livirt is handling this already 14:26 -!- gpadgett (Greg Padgett): has joined #vdsm 14:58 < nsoffer> fromani, I think we don't need the uid/gid change at all 14:59 < fromani> nsoffer: ok, so I'll drop that patch before the next upload 14:59 < nsoffer> fromani, we already doing this, and creating a new file with O_CREAT|O_EXCL, you know that the file is owned by vdsm 14:59 < nsoffer> fromani, there can be no race 14:59 < fromani> nsoffer: ok, right 15:01 < nsoffer> fromani, maybe add a new test, showing that sneaking in a file with wrong permissions, the file is removed before the operation 15:02 < fromani> nsoffer: good idea, will add 15:02 < nsoffer> fromani, I would also open a security bug for this issue 15:03 < nsoffer> fromani, in the worst case, security guys will explain that there is no issue 15:04 < nsoffer> fromani, using umask does not help if the file exists before, right? 15:07 < fromani> nsoffer: nope, we haven't considered this problem before; the new code is more robust 15:46 < nsoffer> fromani, do we have logger instance per vm? 15:49 < fromani> nsoffer: no, we have one on Vm class shared by all VMs 15:50 < nsoffer> fromani, I see, and we wrap the shared logger with an adapter for each instance 15:50 < fromani> nsoffer: exactly. So the adapter is private, but the real logger is shared. 15:50 < nsoffer> fromani, our SimpleLogAdatper is extremely bad 15:51 < nsoffer> we process the extra arguments for each logging call - instead of once 15:51 < fromani> nsoffer: I suspect we pay a quite high performance price here, but not hard data to back my claim 15:52 < nsoffer> fromani, its probably hidden, but there is no need to pay for this 15:52 < fromani> nsoffer: couldn't agree more 15:57 -!- rmohr: has quit [Ping timeout: 252 seconds] 15:58 -!- fromani: has quit [Remote host closed the connection] 16:02 -!- alitke (Adam Litke): has joined #vdsm 16:19 -!- msivak (Martin Sivak): has joined #vdsm 17:13 -!- pkliczew: has quit [Ping timeout: 244 seconds] 17:24 -!- firemanxbr (Marcelo Barbosa): has joined #vdsm 17:45 -!- sbonazzo: has quit [Quit: Leaving.] 18:02 -!- fabiand: has quit [Quit: Verlassend] 18:43 -!- mskrivanek is now known as mskrivanek_away 19:32 -!- danken (purple): has joined #vdsm 19:32 -!- mode/#vdsm: by ChanServ 19:33 -!- danken: has quit [Client Quit] 20:06 -!- firemanxbr: has quit [Quit: Leaving] 20:17 -!- ybronhei (purple): has joined #vdsm 20:34 -!- #vdsm ybronhei: has quit [Ping timeout: 260 seconds] 20:37 -!- nsoffer: has quit [Quit: Leaving] 20:45 -!- rmohr (Roman Mohr): has joined #vdsm 21:03 -!- rmohr: has quit [Quit: rmohr] 21:10 -!- rmohr (Roman Mohr): has joined #vdsm 21:14 -!- rmohr: has quit [Client Quit] 21:17 -!- gshereme (Greg Sheremeta): has joined #vdsm 21:27 -!- mmirecki: has quit [Ping timeout: 252 seconds] --- Log closed za nov 14 00:00:38 2015