| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-04-17 | |||
| 14:28:59 | dansmith | cinder, glance, etc.. so perhaps we should not replicate the same thing here | |
| 14:29:21 | sean-k-mooney | bauzas: the probelm is leakign the ip/subnet of the nova comptue host | |
| 14:29:25 | bauzas | since anyone from the same project can read anything about shares, then a service token needs to be related to a specific different project | |
| 14:29:30 | sean-k-mooney | the fix for that is to use a differnt grant type | |
| 14:29:30 | dansmith | I know that even if nova uses admin or service, manila probably needs extra work to not allow a user to delete/see that thing | |
| 14:29:43 | dansmith | but I'm saying.. using the user's token puts us at a disadvantage for actually filtering those things | |
| 14:30:15 | dansmith | sean-k-mooney: that may be better anyway, but I'm saying it might be a good idea to not go down this same road again | |
| 14:30:16 | gibi | so when nova adds the compute IP to a request sent to manila with the user token, nova basically leaked the IP of the compute to that user (and project) | |
| 14:30:17 | bauzas | sean-k-mooney: that's not only an IP leak problem, this is also a security problem since a malicious user can block the mount by deleting the ACL | |
| 14:30:39 | Uggla | sean-k-mooney, we need to take care even with certs, I'm not sure that manila will not expose the IP in the export location as well. | |
| 14:30:40 | dansmith | even if we use cert auth, we're leaking something about the compute node | |
| 14:30:48 | sean-k-mooney | bauzas: im aware but if we have locked the share you shoudl not be able to do that | |
| 14:30:56 | sean-k-mooney | so we can include that in the spec for the new lock api | |
| 14:31:02 | dansmith | two users can conspire to compare certs about their hosts if they can see them and determine if they're on the same compute node, for example | |
| 14:31:04 | bauzas | yup, + again, we allow anyone to block a mount | |
| 14:31:10 | dansmith | the cert probably has the hostname in it too right? | |
| 14:31:28 | sean-k-mooney | dansmith not if the cert is per instance | |
| 14:31:31 | bauzas | I think sean was proposing an instance-based cert | |
| 14:31:32 | sean-k-mooney | but otherwise yes | |
| 14:31:42 | bauzas | but that doesn't solve the ACL delete case | |
| 14:31:48 | dansmith | ack | |
| 14:31:51 | dansmith | right | |
| 14:31:56 | bauzas | and it adds a lot of complexity for little benefits | |
| 14:32:16 | bauzas | + I'm unsure that the export location doesn't show the IP address, even with certr | |
| 14:32:27 | dansmith | I think manila is going to need finer granularity for this sort of thing, and nova is going to have to not use the user's token (alone) to do this stuff if we're going to distinguish between the user and nova | |
| 14:34:16 | bauzas | dansmith: yup, and we need to round it back to the Manila team | |
| 14:34:28 | dansmith | yeah | |
| 14:38:02 | bauzas | thinking it more, the real problem is the fact that Manila exposes the export_location on the API for any user | |
| 14:38:35 | dansmith | well, for standalone usage it probably has to | |
| 14:38:36 | bauzas | sean-k-mooney: even with certs, the compute IP address would be shown in the export_location field of the manila share | |
| 14:38:42 | bauzas | tryue | |
| 14:38:47 | dansmith | however, I wonder if it should always only show a user their *own* exports | |
| 14:38:53 | bauzas | that's absolutely understandable from a standalone perspective | |
| 14:38:55 | sean-k-mooney | im not sure about that | |
| 14:38:59 | sean-k-mooney | but im in another meeting | |
| 14:39:01 | dansmith | that way, if nova uses its own user to create the export, the user would not be able to see it | |
| 14:39:04 | sean-k-mooney | so ill check back with this after | |
| 14:41:52 | bauzas | dansmith: another user, another project, but yeah | |
| 14:42:06 | bauzas | dansmith: it looks to me the granularity on CRUDs on per project | |
| 14:42:07 | dansmith | bauzas: well, either really | |
| 14:42:09 | dansmith | bauzas: we have per-user resources | |
| 14:42:15 | dansmith | but we need something | |
| 14:42:43 | dansmith | project would mean that the neutron user could see exports created by the nova user, which is probably not ever necessary | |
| 14:42:56 | dansmith | also not terrible, but.. | |
| 14:42:58 | bauzas | dansmith: well, we tested with Uggla, a devstack admin can create a share and add an access ACL, a devstack demo user can both delete the ACL and the share | |
| 14:43:16 | dansmith | wait, what? | |
| 14:43:22 | bauzas | that yeah | |
| 14:43:29 | dansmith | but why? | |
| 14:43:30 | bauzas | I'm saying again | |
| 14:43:35 | bauzas | f... no idea :) | |
| 14:43:55 | dansmith | that's fundamentally broken.. more than just "too coarse" right? | |
| 14:44:00 | bauzas | I'm devstack admin, I create a share and I add access to 1.2.3.4/32 | |
| 14:44:05 | bauzas | then, | |
| 14:44:10 | bauzas | I switch to demo | |
| 14:44:18 | dansmith | is manila in devstack configured with noauth or something/ | |
| 14:44:28 | bauzas | as demo, I can read that there is an existing share, I can read its ACL | |
| 14:44:37 | bauzas | and I can delete both | |
| 14:44:50 | dansmith | if that's true, this should have been a security bug no? | |
| 14:45:00 | bauzas | dansmith: checking, but I think admin and demo share the same project | |
| 14:45:31 | dansmith | oh, then that explains it, but.. they shouldn't be right? | |
| 14:45:39 | dansmith | isn't admin in the admin project and demo in the demo project? | |
| 14:45:56 | bauzas | dansmith: I'm just verifying that with Uggla | |
| 14:46:33 | bauzas | dansmith: double-checking but yeah | |
| 14:53:12 | bauzas | so, we tested with both an admin and a demo user from the same project | |
| 15:11:30 | opendevreview | Merged openstack/placement master: Move implemented specs for Xena and Yoga release https://review.opendev.org/c/openstack/placement/+/853730 | |
| 16:07:09 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Don't rely on branched connections https://review.opendev.org/c/openstack/nova/+/880663 | |
| 16:07:09 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Remove unnecessary 'insert()' argument https://review.opendev.org/c/openstack/nova/+/880664 | |
| 16:07:10 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Pass parameters to sqlalchemy.text() as bindparams https://review.opendev.org/c/openstack/nova/+/880665 | |
| 16:07:10 | opendevreview | Stephen Finucane proposed openstack/nova master: fixup! db: Remove unnecessary 'insert()' argument https://review.opendev.org/c/openstack/nova/+/880666 | |
| 16:07:11 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Add missing args to sqlalchemy.Table https://review.opendev.org/c/openstack/nova/+/880667 | |
| 16:07:11 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Avoid relying on autobegin https://review.opendev.org/c/openstack/nova/+/880668 | |
| 16:07:12 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Remove test for DB special characters https://review.opendev.org/c/openstack/nova/+/880669 | |
| 16:08:31 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Remove unnecessary 'insert()' argument https://review.opendev.org/c/openstack/nova/+/880664 | |
| 16:08:31 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Pass parameters to sqlalchemy.text() as bindparams https://review.opendev.org/c/openstack/nova/+/880665 | |
| 16:08:32 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Add missing args to sqlalchemy.Table https://review.opendev.org/c/openstack/nova/+/880667 | |
| 16:08:32 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Avoid relying on autobegin https://review.opendev.org/c/openstack/nova/+/880668 | |
| 16:08:33 | opendevreview | Stephen Finucane proposed openstack/nova master: tests: Remove test for DB special characters https://review.opendev.org/c/openstack/nova/+/880669 | |
| 16:41:10 | opendevreview | Merged openstack/placement master: Update python testing as per zed cycle testing runtime https://review.opendev.org/c/openstack/placement/+/841690 | |
| 17:11:53 | opendevreview | Dan Smith proposed openstack/nova master: Remove silent failure to find a node on rebuild https://review.opendev.org/c/openstack/nova/+/880632 | |
| 17:11:53 | opendevreview | Dan Smith proposed openstack/nova master: Stop ignoring missing compute nodes in claims https://review.opendev.org/c/openstack/nova/+/880633 | |
| 19:30:10 | dansmith | gibi: sean-k-mooney: bauzas: re our conversation this morning, I've seen this on plenty of volume tests lately as well: https://04e1a81b9004b13f1732-58d59663991bfd1d0dc3ba3167f83a98.ssl.cf1.rackcdn.com/880632/2/check/nova-ceph-multistore/f6ebaaf/testr_results.html | |
| 19:30:23 | dansmith | i.e. several kernel crashes and, obviously, a failure to detach later | |
| 19:31:00 | dansmith | it's this kind of thing that makes me highly suspect of cirros | |
| 22:15:56 | sean-k-mooney | dansmith: if its the old cirror image form the 5.2 release | |
| 22:16:07 | sean-k-mooney | actully thts a differnt crash then i have seen in the past | |
| #openstack-nova - 2023-04-18 | |||
| 04:39:02 | frickler | 5,15 kernel should be new cirros. that failure very much looks like memory corruption to me | |
| 06:38:18 | opendevreview | Merged openstack/placement master: Bugtracker link update https://review.opendev.org/c/openstack/placement/+/876768 | |
| 07:50:07 | gibi | dansmith: I agree that kernel crash points to cirros. Obviously if we see such crashes whathever happens after it (ie detach failure) is probably a consequence of the crash. | |
| 07:50:23 | gibi | dansmith: in the original detach failure case there was no kernel crash, so this is definitely different | |
| 08:01:56 | kashyap | gibi: On that device-detach patch from the QEMU upstream, do we have a way to test it upstream? -- https://www.mail-archive.com/qemu-devel@nongnu.org/msg952944.html | |
| 08:02:25 | kashyap | gibi: I can create an RPM build of QEMU with that patch for Fedora ... if we there's a Fedora job that can test it | |
| 08:02:46 | kashyap | But probably a Ubuntu package is preferred, I guess | |
| 08:11:46 | gibi | I don't think we have a Fedora based job upstream :/ | |
| 08:12:51 | gibi | kashyap: but if you can build the fix to Fedora then you can at least try my libvirt only reproduction to see if that is fixed | |
| 08:13:48 | kashyap | gibi: Oh, right indeed. I'll give it a go this week and update you on the bug | |
| 08:13:57 | gibi | cool,thanks | |
| 10:40:03 | sean-k-mooney | gibi: bauzas i now want to add a new hw_vif_model specifically igb | |
| 10:40:05 | sean-k-mooney | https://www.qemu.org/docs/master/system/devices/igb.html | |
| 10:40:38 | sean-k-mooney | qemu added sriov virtualsiation support to it last year | |
| 10:41:05 | gibi | wut? that feels super useful for testing if it does what I think it does | |