@Ishan - not necessarily, a committer has to explicitly approve running the GHA checks for first time contributors. So a random person with a drive-by PR won't be automatically handed the keys to our infra. I believe we can set that to needing approval for each run by a non-committer, or maybe that's already set, somebody would need to dig through GH configs and verify (or experiment).
I think that would still run into issues with my second case. Even if we allow 'gradlew test' in the list of allowed commands (which I think we would have to), what's stopping a user from creating a new test (or modifying an existing one) to do malicious things and then execute it via the test runner. It doesn't matter if they're launching bitcoin miners on their own machine - using our tests to do that would be a very indirect and inefficient way to do it. But giving people access to unfettered compute resources suddenly makes for an enticing target. I'm not sure if I'm being clear enough with my description, so let me try an example. `gradlew test` will run all tests. The attacker creates a new test "DoBadThingsTest" that will connect out to the internet and do the previously described bad things. They launch crave run from their machine with the changed code, and... does this upload their current code state to crave to run the tests? This is probably possible to do safely with sufficient isolation - gradle needs access to a specific set of known hosts to download dependencies, and the tests need network connectivity to talk to the multiple Solr servers that we launch during testing, but otherwise I don't think there's a need for external traffic. This is hard to do correctly and completely, but would essentially limits the threat model to a DoS. Anyway, it's not my place to tell Crave how to run their business, and I don't mean to turn down their generous contribution with a heavy-handed security audit. It's a very nice thing that they are offering and I think we should take advantage of it while it is available. Mike On Thu, Jan 26, 2023 at 9:18 AM Ishan Chattopadhyaya < ichattopadhy...@gmail.com> wrote: > Though, if a malicious user creates a PR that executes harmful code, that > PR will also get executed (via "gradlew test"), right? > > On Thu, 26 Jan, 2023, 8:36 pm Yuvraaj Kelkar, <yuvr...@gmail.com> wrote: > > > Hm. I see your point. The first solution I thought of is a bit blunt, but > > it will work: We disable command line arguments when using ZeroConf. Only > > preset commands are allowed. > > Also, to allow the Solr GHA to run whatever commands needed not just for > > now, but also for any future changes, we set up regular (not ZeroConf) > > access for the GHA runner. > > This way GHA always has the ability to run any commands - but it is > > strictly controlled by review requests. > > Regular developers only get to use the commands that are approved by the > > Solr community leaders. > > > > Would that work? > > Thanks, > > -Uv > > > > On Jan 26 2023, at 8:09 am, Mike Drob <md...@apache.org> wrote: > > > Having massive infrastructure to run PRs is pretty cool. > > > > > > I'm worried about letting arbitrary people run code on these > > > machines though - a single 'crave run -- mine_bitcoin.exe' would ruin > the > > > system for everybody, or it's not hard to imagine a slightly more > > indirect > > > case where an attacker adds a test that launches an undesirable process > > and > > > runs crave. What safeguards exist to protect against this? At least > with > > > GHA we have to approve non-committers tests to run, but opening it up > to > > > local command line access sounds very broad. > > > > > > Mike > > > On Wed, Jan 25, 2023 at 10:34 PM Ishan Chattopadhyaya < > > > ichattopadhy...@gmail.com> wrote: > > > > > > > This is very cool. Thanks for working on this, David. Can multiple > > > > developers execute their tests at the same time? > > > > > > > > On Thu, 26 Jan, 2023, 5:07 am Noble Paul, <noble.p...@gmail.com> > > wrote: > > > > > > > > > This is interesting. > > > > > > > > > > So, if the PR is merged , we will have the full test running on > > crave.io > > > > > for every PR raised? > > > > > > > > > > On Thu, Jan 26, 2023 at 9:22 AM David Smiley <dsmi...@apache.org> > > wrote: > > > > > > > > > > > We haven't been running all our tests in GitHub Actions (i.e. PR > > > > > > validation) because it was too time consuming to do so. I don't > > recall > > > > > how > > > > > > slow it was when someone last tried; it's probably better now but > > still > > > > > > slow. To make up for this, there is a GHA only for SolrJ if a PR > > > > touches > > > > > > SolrJ. > > > > > > > > > > > > There's now a PR here to introduce a new GHA that builds on > > Crave.io > > > > on a > > > > > > beefy machine: https://github.com/apache/solr/pull/1303 The PR > > > > > validation > > > > > > took 11 minutes which is similar to the amount of time it took a > > GHA to > > > > > > just do precommit checks -- 10 minutes :-) > > > > > > I think we can remove the SolrJ specific GHA as it'll be > redundant. > > > > > > > > > > > > Furthermore, anyone can use this to run tests from the > convenience > > of > > > > > your > > > > > > laptop at the CLI while you are in the middle of any change > > (doesn't > > > > > matter > > > > > > what you have committed or not, pushed or not). To do so, run: > > crave > > > > run > > > > > > -- './gradlew localSettings && ./gradlew --max-workers=`nproc` > > > > > > -Ptests.jvms=48 test' > > > > > > > > > > > > Yeah that's long. There is a discussion in JIRA underway that may > > lead > > > > > to > > > > > > eliminating the "localSettings" step if, for example, it's moved > > to a > > > > > bash > > > > > > script executed by the gradle wrapper (my proposal). I should > also > > be > > > > > able > > > > > > to configure crave with a default run configuration with this > > baked in. > > > > > > I'll post an update when I'm able to do that. > > > > > > > > > > > > ~ David Smiley > > > > > > Apache Lucene/Solr Search Developer > > > > > > http://www.linkedin.com/in/davidwsmiley > > > > > > > > > > > > > > > > > > > > > -- > > > > > ----------------------------------------------------- > > > > > Noble Paul > > > > > > > > > > > > > > > > >
