Security-review checkbox for node-test-pull-request & node-test-commit

[orangemocha]

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.

[bnoordhuis]

What purpose would that serve?

[orangemocha]

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]

+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]

@rvagg how does that apply to smoketesting? Should we be doing things in a more sand boxed fashion for citgm?

[rvagg]

@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]

@rvagg I would feel a lot better about that if it were possible.

[rvagg]

possible .. but someone has to do the work

[MylesBorins]

That is something I would be open to exploring... although I may need some mentorship with figuring out how we orchestrate our systems

[rvagg]

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]

I'm busier than normal but lets try and schedule something. Ping me on IRC and we'll suss it out.

[orangemocha]

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]

Closing as the original issue has been resolved.