Repository navigation
Security-review checkbox for node-test-pull-request & node-test-commit #342
Description
At the last Build WG meeting, we decided to add such a checkbox.
It should be fairly uncontroversial, but in case anyone wants to provide feedback here's the text that I would add:
[ ] I have reviewed *the latest version of* these changes and I am sure that they don’t contain any code that could compromise the security of the CI infrastructure.
Activity
bnoordhuis commentedon Feb 25, 2016
What purpose would that serve?
orangemocha commentedon Feb 25, 2016
To raise awareness with collaborators that our CI is not a fully-sandboxed system, and that malicious code in a pull request could compromise the infrastructure. @rvagg has suggested that there might be a lack of such awareness with most collaborators. This is a reminder at the right place and time.
rvagg commentedon Feb 26, 2016
+1 if we make it a required check. @bnoordhuis because CI is so easy, everyone's just used to throwing code at it, often without even looking at the code being submitted but just because it's something that needs to be done. This is just an attempt to remind them that when submitting code to CI you are taking responsibility for what is sent to our build slaves for executation.
MylesBorins commentedon Feb 26, 2016
@rvagg how does that apply to smoketesting? Should we be doing things in a more sand boxed fashion for citgm?
rvagg commentedon Feb 26, 2016
@thealphanerd smoke testing opens a vector via the packages being smoke tested, basically anything you can get into one of those packages you can get onto our test infrastructure. So it's our responsibility to make sure the list of packages is reputable at least. It's hard to sandbox smoke testing without a lot more server resources to run them on but that would be an ideal. Another option we can consider is to separate out a smoke testing user on the servers we're using so that the impact is somewhat contained.
MylesBorins commentedon Feb 26, 2016
@rvagg I would feel a lot better about that if it were possible.
rvagg commentedon Feb 26, 2016
possible .. but someone has to do the work
MylesBorins commentedon Feb 26, 2016
That is something I would be open to exploring... although I may need some mentorship with figuring out how we orchestrate our systems
rvagg commentedon Feb 26, 2016
You'd probably need to lean on @jbergstroem who has overhauled the setup of all our slaves, I think he's a bit busy though
jbergstroem commentedon Feb 26, 2016
I'm busier than normal but lets try and schedule something. Ping me on IRC and we'll suss it out.
orangemocha commentedon Mar 2, 2016
The change is now live. @nodejs/collaborators : there is an additional checkbox parameter when launching node-test-pull-request or node-test-commit. It reads:
[ ]
I have reviewed *the latest version of* these changes and I am sure that they don’t contain any code that could compromise the security of the CI infrastructure....and it's intended as a reminder for you to make sure that the change that you are testing doesn't contain any malicious code that could compromise our Jenkins slaves.
orangemocha commentedon Mar 4, 2016
Closing as the original issue has been resolved.