| *** dmsimard has quit IRC | 06:34 | |
| *** dmsimard has joined #openstack-infra-incident | 06:42 | |
| *** panda|off is now known as panda | 09:40 | |
| *** myoung has joined #openstack-infra-incident | 12:17 | |
| *** myoung is now known as myoung|ruck | 12:18 | |
| *** rlandy has joined #openstack-infra-incident | 12:31 | |
| *** panda is now known as panda|lunch | 13:05 | |
| *** panda|lunch is now known as panda | 13:35 | |
| *** rosmaita has joined #openstack-infra-incident | 15:59 | |
| *** rlandy is now known as rlandy|afk | 17:29 | |
| *** myoung|ruck is now known as myoung|afk | 18:01 | |
| *** myoung|afk is now known as myoung|biab | 18:01 | |
| -openstackstatus- NOTICE: Most jobs in zuul are currently failing due to a recent change to zuul; we are evaluating the issue and will follow up with a recommendation shortly. For the moment, please do not recheck. | 18:17 | |
| *** ChanServ changes topic to "Most jobs in zuul are currently failing due to a recent change to zuul; we are evaluating the issue and will follow up with a recommendation shortly. For the moment, please do not recheck." | 18:17 | |
| *** panda is now known as panda|off | 18:20 | |
| *** ChanServ changes topic to "Situation normal | OpenStack Infra team incident handling channel | Find us in #openstack-infra for more stimulating conversation" | 18:40 | |
| -openstackstatus- NOTICE: Zuul has been restarted without the breaking change; please recheck any changes which failed tests with the error "Accessing files from outside the working dir ... is prohibited." | 18:40 | |
| corvus | hi | 18:55 |
|---|---|---|
| tobiash | hi | 18:55 |
| dmsimard | o/ | 18:56 |
| corvus | i can't concentrate on something important like a security bugfix with more that one conversation going one | 18:56 |
| corvus | so... to recap | 18:56 |
| corvus | http://logs.openstack.org/22/551822/1/gate/openstack-tox-py27/623f25c/job-output.txt.gz#_2018-03-12_18_05_21_245800 | 18:56 |
| corvus | http://logs.openstack.org/22/551822/1/gate/openstack-tox-py27/623f25c/ara/file/71b64ee4-f5b4-49ef-aed3-419a39127f07/#line-4 | 18:56 |
| corvus | and https://review.openstack.org/552110 is a proposed fix | 18:57 |
| corvus | tobiash: i wonder whether that will actually fix it though | 18:58 |
| tobiash | corvus: nope | 18:58 |
| tobiash | I don't think anymore | 18:58 |
| tobiash | well it solves one part I think | 18:58 |
| corvus | find_needle is going to find .../playbook_0/project/.... | 18:58 |
| corvus | and playbook_0 isn't under work/, so it will fail the path check | 18:58 |
| tobiash | but currently the symlink makes the check for .../work/... work for the repo under test | 18:58 |
| corvus | tobiash: yes, and any untrusted repo involved in testing (ie, via depends-on) | 18:59 |
| tobiash | so they are located in /work/src... | 18:59 |
| tobiash | right? | 18:59 |
| corvus | tobiash: yes | 18:59 |
| tobiash | so the symlink actually makes that work | 19:00 |
| tobiash | so if this assumption is correct, 552110 should break tox-remote | 19:00 |
| corvus | nice | 19:00 |
| corvus | so yeah, the hard link will both (a) not fix the trusted context, and (b) further break the untrusted context | 19:01 |
| *** rlandy|afk is now known as rlandy | 19:01 | |
| tobiash | I think we need a combination of hard link and allow the playbook dir | 19:02 |
| tobiash | but with the current implementation it'll be more complicated to make reading from playbook dir work and restrict writes to work | 19:02 |
| corvus | tobiash: or we could leave the symlinks and allow trusted/ | 19:02 |
| corvus | (but only for reading, not writing) | 19:03 |
| tobiash | I think we still need to split between reading and writing | 19:03 |
| corvus | tobiash: oh, because we're overriding find_needle, and we don't know whether find_needle is being used for src or dest...? | 19:03 |
| tobiash | and then we;re safer with hard links | 19:03 |
| tobiash | yah | 19:03 |
| fungi | is there any reason to disallow reading of files under trusted/? content we want to avoid getting copied/exposed? | 19:04 |
| corvus | fungi: no, trusted/ only contains git repos, so should be okay. | 19:04 |
| corvus | the playbook directory gets secrets.yaml. it still may be okay to allow that, but will require a bit more thought (like, it's probably okay to read any file in the *current* playbook directory, but we shouldn't allow reading from *other* playbook directories) | 19:05 |
| corvus | for reference: http://git.openstack.org/cgit/openstack-infra/zuul/tree/zuul/executor/server.py#n290 | 19:05 |
| tobiash | ok, so allowing trusted/ is most easiest | 19:06 |
| corvus | tobiash: but we should only do so for reads :/ | 19:07 |
| tobiash | but I have no good idea how to differenciate that currently | 19:07 |
| tobiash | without forking the complete modules | 19:07 |
| corvus | tobiash: could we do something like the old version of the patch checking, where we explicitly pull out the arguments and call find_needle ourselves? | 19:08 |
| corvus | s/patch/path/ | 19:08 |
| tobiash | corvus: maybe there is a possibility: | 19:10 |
| tobiash | https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/action/copy.py#L448 | 19:11 |
| tobiash | just noticed that the args are in self | 19:11 |
| tobiash | as well as find_needle | 19:11 |
| corvus | so we can compare the argument to figure out which is which? | 19:12 |
| corvus | if needle==src permit trusted | 19:12 |
| *** myoung|biab is now known as myoung|ruck | 19:13 | |
| tobiash | hrm, actually I think find_needle is never used for writing | 19:13 |
| corvus | some modules will need some custom comparators there -- like the script module manipulates 'source' before it passes it to find_needle, but that work was already done.... | 19:13 |
| tobiash | find_needle doesn't make sense for specifying a dest | 19:13 |
| corvus | now that you say that, it makes perfect sense | 19:13 |
| tobiash | so in find_needle we can always allow trusted | 19:13 |
| tobiash | just need to adjust the path checker with a allow_trusted=False | 19:14 |
| corvus | yeah, so find_needle can use that, and the existing dest patch checks will not | 19:15 |
| tobiash | cool | 19:17 |
| corvus | i need to grab some food now; i can help with some test cases, etc, when i get back | 19:28 |
| tobiash | remote: Allow trusted for find_needle https://review.openstack.org/552127 | 19:31 |
| tobiash | without test case yet | 19:31 |
| corvus | tobiash: good news! the hard lik change failed tests :) | 20:06 |
| tobiash | Yeah, just looked at that | 20:07 |
| corvus | tobiash: though it looks like it was because the hardlink operation itself failed | 20:08 |
| tobiash | Yah | 20:08 |
| corvus | tobiash: what can i help with? | 20:10 |
| tobiash | corvus: what do you think about the approach in 552127? | 20:12 |
| corvus | tobiash: lgtm | 20:13 |
| tobiash | corvus: latest ps of 552127 adds a skelleton of a tes for that | 20:16 |
| tobiash | it probably won't work yet | 20:17 |
| tobiash | but I think at least something similar could mimick the trusted repo problem | 20:17 |
| corvus | tobiash: i'm running that test locally now | 20:21 |
| corvus | tobiash: the patch has a typo, but also, the new test fails | 20:21 |
| tobiash | just updated | 20:22 |
| corvus | "msg": "Accessing files from outside the working dir /tmpfs/tmpt77h7s_d/zuul-test/c45c9f22f10144d28433c8ac1acd3583/work is prohibited. Invalid path: /tmpfs/tmpt77h7s_d/zuul-test/c45c9f22f10144d28433c8ac1acd3583/trusted/project_0/review.example.com/common-config/roles/common-copy/files/common-file" | 20:22 |
| corvus | is what i got | 20:22 |
| tobiash | ok, ran it too now | 20:23 |
| tobiash | I got that too | 20:23 |
| corvus | found the bug | 20:24 |
| corvus | tobiash: left comments on change | 20:24 |
| corvus | on ps3 | 20:24 |
| tobiash | ah, so the leading / breaks it? | 20:25 |
| corvus | it passes with that fix. and i think that helps confirm that it's a working test. | 20:25 |
| corvus | tobiash: yeah, it ends up as '/trusted' | 20:25 |
| tobiash | oh cool, local test worked :) | 20:25 |
| corvus | https://docs.python.org/2/library/os.path.html#os.path.join | 20:25 |
| tobiash | that was easier than expected | 20:25 |
| corvus | " If a component is an absolute path, all previous components are thrown away and joining continues from the absolute path component." | 20:25 |
| tobiash | ah, didn't read the docs ;) | 20:26 |
| dmsimard | Sorry for not being more helpful, I get lost quickly in the Zuul speculative things. I'll cheer on from the sidelines. | 20:27 |
| tobiash | but at least the leading / proved that the test case is correct :) | 20:28 |
| corvus | dmsimard: keep asking questions :) | 20:28 |
| corvus | tobiash: agreed | 20:28 |
| tobiash | so I should pretend the leading / was intended ;) | 20:29 |
| dmsimard | corvus: yeah, I ask a lot of questions and the intent is not always to get an answer, it's often to make people think and hopefully spark ideas :D | 20:30 |
| dmsimard | I'll take a look at the patch and see if I have any useful input | 20:31 |
| * dmsimard gets 500 errors looking up the ansible github repo.. | 20:32 | |
| dmsimard | Could/should we overload "find_file_in_search_path" ? https://github.com/ansible/ansible/blob/5c7f203c33b60de98cc30d5f04150e2bacfa4a84/lib/ansible/plugins/lookup/__init__.py#L111 | 20:35 |
| dmsimard | I feel like that's sort of the intent with the work we're doing in paths.py but I'm not sure where/how that file ends up being used | 20:36 |
| dmsimard | I admit it's kind of tricky to try and prevent any/all modules from accessing things it's not supposed to | 20:41 |
| dmsimard | Maintaining our own module forks with "embedded" security checks doesn't scale very well though | 20:41 |
| dmsimard | I wonder what's the plan for that moving forward, especially with the talks around supporting different versions of Ansible but that's a question for #zuul :) | 20:42 |
| corvus | dmsimard: it looks like our lookup plugins either call find_file_in_search_path and checking the value before handing off to the originals, or override a lower-level method and perform a check there | 20:47 |
| corvus | dmsimard: we might be able to save some code and simplify things by overriding that in the same way we're doing find_needle | 20:48 |
| fungi | if memory serves, there are ongoing discussions upstream about integrating some of our safety/security work | 20:48 |
| fungi | though i don't know which parts | 20:48 |
| dmsimard | fungi: the more we can push upstream as appropriate, the better indeed | 20:48 |
| corvus | dmsimard: also, that might allow those plugins to benefit from the ability to read files in their own directories like we're about to get with the find_needle thing | 20:49 |
| dmsimard | I talked with upstream about making ansible_search_path configurable a few weeks ago | 20:49 |
| corvus | dmsimard: if you want to hack on that, i think it could be beneficial | 20:49 |
| corvus | dmsimard: at the moment, i don't see a vulnerability there, but also, if you wanted to double check and audit those, that would be good too :) | 20:50 |
| dmsimard | corvus: ansible_search_path, as I understand it, defines a hierarchy where Ansible tries to find things -- for example when using include_vars | 20:50 |
| dmsimard | It doesn't prevent Ansible from looking at a place if you tell it to look there explicitely though | 20:51 |
| dmsimard | but I could see a "feature" where ansible_search_path could be enforced, perhaps ? | 20:51 |
| dmsimard | It's probably a bit more complicated than that, because you need something that'd prevent, say, the command module to do a "cat /etc/shadow" | 20:52 |
| dmsimard | There's a feature they call "command nanny" (I think?), it's the stuff that tells you "you should use the yum module instead of putting yum in a command" for example. Maybe we could hook into that, I'm not sure how it works. | 20:53 |
| dmsimard | Hmm, it's basically this. I remember it being something else: https://github.com/ansible/ansible/blob/5a80375be998a854013c4237e3494bc4a3a438fd/lib/ansible/modules/commands/command.py#L124-L153 | 20:55 |
| corvus | we seem to be winding down here, let's move further discussion back to #zuul | 21:04 |
| fungi | thanks! | 21:06 |
| *** SergeyLukjanov has quit IRC | 21:52 | |
| *** SergeyLukjanov has joined #openstack-infra-incident | 21:55 | |
| *** myoung|ruck is now known as myoung|bbl | 22:16 | |
| *** rlandy is now known as rlandy|bbl | 23:12 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!