19:59:59 <nitzmahone> #startmeeting Ansible Windows Working Group
19:59:59 <zodbot> Meeting started Tue May 10 19:59:59 2022 UTC.
19:59:59 <zodbot> This meeting is logged and archived in a public location.
19:59:59 <zodbot> The chair is nitzmahone. Information about MeetBot at https://fedoraproject.org/wiki/Zodbot#Meeting_Functions.
19:59:59 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:59:59 <zodbot> The meeting name has been set to 'ansible_windows_working_group'
20:00:03 <nitzmahone> bah
20:00:04 <briantist> o/
20:00:05 <jborean93> o/
20:00:08 <nitzmahone> #chair jborean93
20:00:08 <zodbot> Current chairs: jborean93 nitzmahone
20:00:11 <nitzmahone> #info agenda https://github.com/ansible/community/issues/644
20:01:12 <briantist> anything else you all need from me on https://github.com/ansible-collections/community.windows/pull/391 and https://github.com/ansible-collections/community.windows/pull/392 ?
20:01:31 <nitzmahone> Yeah, thinking about it a little more and playing with a couple things, I'm worried that at least the preview-build-on-PR functionality introduces a bit of a security hole
20:01:37 <jborean93> I've yet to look at it sorry
20:02:34 <briantist> ah, I thought a LOT about the security aspect of that; the short of it is that `pull_request_target` is what prevents a contributor from being bale to run arbitrary code with a privileged token
20:02:39 <briantist> if that was your concern
20:03:40 <briantist> with that event, the workflow file is always loaded from the target branch, not from the PR (it makes it a PITA to test changes to such a workflow)
20:04:03 <nitzmahone> Well, that, but more the fact that the token that's already there has to have write-access to the repo, so IIUC a malicious PR could commit to any branch.
20:05:22 <jborean93> would it better for the first case to just remove the PR preview aspect and just have the doc build on merge side?
20:05:29 <briantist> right so that's what `pull_request_target` prevents. The token in that event has write access, but the PR author cannot change the contents of that workflow in their PR; this is actually why that event gets a full token, like a pus event would
20:05:51 <briantist> a regular `pull_request` event on the other hand, from a fork, gets a read-only token
20:06:43 <nitzmahone> Right but since we're executing code from inside the repo, IIUC a malicious PR can still run custom code from, eg, a test without a modified workflow
20:07:20 <briantist> we aren't exexuting code form the repo except in the docs _build_ part, which happens in a separate workflow that explicitly has read only permission (set in both the calling and called workflow)
20:07:27 <nitzmahone> I didn't go so far as to PoC an exploit, but I don't see how it could be avoided with the ability to commit to the repo
20:07:54 <briantist> and since the parent workflow runs in `pull_request_target`, the PR author cannot modify it, because that workflow only ever gets loaded from the target branch (`main`)
20:08:37 <nitzmahone> I think that might've been the part I was missing with the token permissions
20:08:49 <briantist> in other words, the PR author can't modify the workflows that have a writable token, and the workflow which loads artibtrary (PR author's) code, runs explicity with read-only permissions
20:09:18 <briantist> the part that runs with write, never even checks out the PR code
20:09:29 <briantist> it only downloads the artifact that contains the HTML files from the build
20:09:59 <briantist> this is  a great article about `pull_request_target` and the specific risks: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
20:10:42 <briantist> we avoid these by splitting build from publish/pr comments; build needs PR contents, but it runs read-only. publishing and PR commenting need write, and they don't use PR contents
20:11:13 <briantist> and the workflows themselves do not run from the PR, they always run from target (main)
20:11:59 <nitzmahone> Cool, just even talking about it here closed a couple of gaps for me- I'll look over the article and try a few more things, but unless I come up with something weird, I think we'll be OK then
20:12:02 <jborean93> is there a concern that these preview builds "clog" up the git history or does github do something special to prune all that in the docs branch?
20:12:50 <nitzmahone> I'd also want some caution tape around the stuff in the workflow like "don't F with this to avoid granting PRs a way to corrupt the repo"
20:13:11 <briantist> jborean93: that is a concern of mine, yes, there is an option in the github action we use to publish to make an orphan commit to prevent that, the only problem is that it doesn't work correctly with the subdirectory option we're using (yet): https://github.com/peaceiris/actions-gh-pages/issues/455
20:13:30 <briantist> the author is looking to make that work in the next version of the action, so I look forward to it
20:13:48 <nitzmahone> I'd also probably prefer to change the default to R/O tokens on the project and just opt-in as necessary- it's currently defaulted to r/w token
20:13:58 <briantist> the good news is that while it will be a bunch of commits, the contents are nearly identical in all of them, so should be very compressed/deduped in git
20:13:59 <nitzmahone> (which we should rarely need except for this ATM)
20:14:22 <briantist> nitzmahone: if you change to default to r/o in the repository, a workflow can't opt-in at all unfortunately
20:14:51 <briantist> PRs that run against `pull_request` (not `pull_request_target`) workflow events automatically get a read-only token that can't be upgraded to write
20:15:01 <briantist> for the exact reason of arbitrary execution risk
20:15:20 <nitzmahone> Boo- this is an area I've not spent a lot of time on. We try pretty hard to avoid secrets and/or self-commit capabilities, so I'm not fully up to speed on the capabilities :(
20:15:29 <briantist> however, itis very good practice to use the `permissions:` option on every workfllow to drop to least privilege
20:15:41 <briantist> and all the github-build stuff and the sample consuming workflows I  have do just that
20:16:05 <briantist> no worries.. it's a bit of a wormhole.. I have looked deeply into this stuff as a result of building this docs stuff
20:17:19 <nitzmahone> OK- I'll poke at it a bit more to make sure I'm comfortable with the robustness of the protection and unless I find something glaring, will +1 and/or merge the PR for c.w at that point. Still want to wait on a.w for awhile to see how things go with this one though
20:17:22 <briantist> my original version when it wa sjust in my own repo, was all in one workflow, and I only built the collections docsite, not the full plugins and and all that, solely because I needed a write token to post a PR comment, and I dodn't want to risk execution by way of docs build (modules and plugins use AST parsing, but doc fragments are imported/executed)
20:17:48 <briantist> splitting into separate workflows for build/publishing was necessary to constrain the token permissions in a way that's safe
20:18:09 <briantist> ok sounds good, always around to hash it out and answer any questions
20:18:24 <nitzmahone> Cool- thanks for your patience with my tin-foil-hattedness ;)
20:18:46 <briantist> it's good you're asking! and believe me, if you find something I missed, I want to know about it
20:18:56 <briantist> for my collection and the others who are or will be using it
20:19:15 <nitzmahone> Worst case, we just shut off the PR publishing- there's still a ton of value IMO even just on tag/merge
20:20:17 <briantist> yeah that's a possibility for sure; for me the PR part is most valuable, because it's fast and otherwise missing feedback on contributions but starting with push only is viable too
20:20:19 <nitzmahone> (but also see the value of the PR builds as well)
20:21:04 <briantist> if you want to set aside some dedicated time at some point to deep dive on it, I'd be happy to make time for it
20:21:49 <briantist> async or even synchronous over IRC, zoom, etc.
20:22:59 <nitzmahone> (sorry, my computer just hung, rebooting)
20:23:58 <jborean93> I've got nothing else to add for that, I'm hoping to create a new release of the collections sometime this week
20:24:58 <nitzmahone> grr- maybe ~once a month or so my SSD will just go to lunch and read/write timeout entirely, progressively grinding the machine to a halt
20:25:45 <briantist> oh yikes that sounds scary
20:25:54 <nitzmahone> Yeah, I mainly just want to try a couple of ideas for executing PR code in the docs build- especially with Python it's unfortunately very easy to do
20:26:21 <briantist> jborean93: it might be good to at least get the push portion in before the release, so a tag version gets published, but it's not critical
20:26:22 <nitzmahone> So far it's always come back, and it's been doing it since the machine was new, so 🤞
20:26:43 <briantist> I could modify the PR to be push only for now if that's desirable
20:27:09 <briantist> nitzmahone: that sounds awesome! would love to see some hack attempts 😈
20:27:11 <nitzmahone> I'll leave that choice up to jborean93- I'm mostly comfortable with that at this point
20:27:30 <nitzmahone> The PR bits are what make me 😨
20:27:41 <briantist> understandable :)
20:28:35 <jborean93> I'll think about it, was planning on just doing the new release and working on the docs bits once that is out
20:29:13 <nitzmahone> Anyway, nothing else from me ATM- just digging out from an unexpected extension of last week's PTO due to Alaska Airlines' "rolling blackout" flight cancellations
20:29:26 <briantist> jborean93: ok, np, I'll leave it alone for now
20:29:58 <briantist> I got nothing else Windows related
20:31:04 <nitzmahone> Cool, guess we'll wrap up for this week then- til next week... Thanks all!
20:31:13 <briantist> nitzmahone: feel free to put some PRs up on https://github.com/ansible-collections/community.hashi_vault/ to test the boundaries of the PR docs build (so you don't have to set up something else), might want to disable the ansible-test workflows in your PR though (just delete 'em hehe) just to speed it up
20:32:21 <nitzmahone> Yeah, might try that- basically though, if I can get code to execute under the docs build part of the workflow, that's pretty much fatal IMO, yeah?
20:32:31 <nitzmahone> (from a PR)
20:32:46 <briantist> yes, if it executes under one of the jobs that has write access
20:33:07 <nitzmahone> It's too bad we can't limit a token to one branch :(
20:33:09 <briantist> or if you can escalate the read-only jobs to get write access (but that'd be a CVE in GHA itself)
20:33:39 <nitzmahone> I wasn't clear exactly what the scope of the `pages` permission was
20:34:29 <nitzmahone> (eg, just API access, or if it gave a proxied way to commit to the configured pages branch without full write permission)
20:34:31 <briantist> btw I should add to clarify, these restrictions are rather moot for anyone who has write access to the repo, so PRs that come from branches on the repo itself have full permissions too, it's PRs from forks that are restricted
20:35:16 <nitzmahone> Yeah, I'm not worried about those- if you can write to the repo, well, you can write to the repo- no extra points for obfuscation... :D
20:35:27 <briantist> you need `contents: write` to commit; I don't know exactly what the `pages` scope does, but I don't think I'm using it anyway
20:35:58 <briantist> https://github.com/ansible-community/github-docs-build/blob/main/.github/workflows/_shared-docs-build-publish-gh-pages.yml#L46-L47
20:36:55 <nitzmahone> Yeah, I was just looking to see if `pages` access maybe provided a more limited way to do that without directly allowing commit access to the token, but haven't played with it at all yet
20:38:00 <briantist> I see I see, to my knowledge it doesn't, committing is the only way to get content into the pages branch, good idea to poke that area though
20:39:21 <nitzmahone> Yeah, I assume the same, but want to verify
20:39:51 <briantist> excellent, thanks for the extra scrutiny, I really appreciate it actually :)
20:40:10 <nitzmahone> No worries- will hopefully find some time to poke at that this week. Thanks again for everything!
20:40:21 <nitzmahone> Til next week...
20:40:24 <nitzmahone> #endmeeting