Should we enable Snyk PR integration? #1094

Open
phillipj opened this Issue Jan 5, 2017 · 16 comments

Projects

None yet

8 participants

@phillipj
Member
phillipj commented Jan 5, 2017

In August w/#841 we introduced Snyk to the project, but unfortunately wasn't able to turn on the PR integration of Snyk. That's cause it used to require private organisation wide access, which is a big no-no for the nodejs organisation.

I've been informed by @guypod that Snyk recently has made it possible to only provide public organisation wide access. AFAIK such permissions could be allowed by the nodejs org, @nodejs/ctc please correct me if I'm wrong.

@nodejs/website any thoughts on trying to enable this integration?

More about Snyk PR/alerts @ https://snyk.io/features.

@guypod
guypod commented Jan 9, 2017 edited

Much of this integration's goal is to keep projects free of vulnerable dependencies continuously, but it's worth noting some projects under @nodejs use vulnerable packages today, as you can see in these test reports:
https://snyk.io/org/guypod/test/github/nodejs/iojs.org
https://snyk.io/org/guypod/test/github/nodejs/foundation.nodejs.org
https://snyk.io/org/guypod/test/github/nodejs/nodejs-zh-tw

This integration will make it easy to find out about these issues and fix them with a click with the right upgrades and patches.

@Fishrock123
Member

AFAIK such permissions could be allowed by the nodejs org, @nodejs/ctc please correct me if I'm wrong.

That sounds correct.

@guypod
guypod commented Jan 9, 2017

@Fishrock123 they indeed all see to have pretty old commits, just wasn't sure if they're still deployed somewhere.
I did note nodejs-ko seems more recently updated and has vuln deps: https://snyk.io/test/github/nodejs/nodejs-ko/

@phillipj
Member

Since no objects has been made, I'm trying to get this setup. For reference I'm adding a couple of screenshots showing what I'm seeing. The first grant access screen (1) looks promising. Not so much for the request nodejs organization permissions modal (2) tho which is really misleading if it's true that Snyk won't actually request access to private org data.

I've requested permissions from the nodejs org. Lets see if the org admins will grant it or not.

Screenshot 1


screen shot 2017-01-12 at 09 51 31

Screenshot 2


screen shot 2017-01-12 at 09 51 56

@guypod
guypod commented Jan 12, 2017

@phillipj I believe the private information in this statement only refers to the list of users in the org and perhaps some other metadata. The team is confirming that's accurate right now.

As you can see in the first screen, we're definitely not requesting (nor getting) access to private repos.

Last note - we request write access only because that's the only way that allows us to set a webhook. We never commit code to the repo otherwise, only submit fix pull requests.

@phillipj
Member

@phillipj I believe the private information in this statement only refers to the list of users in the org and perhaps some other metadata. The team is confirming that's accurate right now.

That might still be something we don't want. IIRC @ChALkeR might have some concerns about exposing org users?

Last note - we request write access only because that's the only way that allows us to set a webhook.

If we were able to add that webhook manually ourselfs, would we avoid this issue entirely?

@targos
Member
targos commented Jan 12, 2017

The new GitHub integrations API has a more fine-grained permission system (and automatically creates a webhook).

@adrukh
adrukh commented Jan 12, 2017

If we were able to add that webhook manually ourselfs, would we avoid this issue entirely?

@phillipj the webhook creation needs to come from our server, as we add a secret to sign posted webhooks so we can trust their source to be GitHub.

@bnoordhuis
Member

Denied, sorry. Access to 'private data' is too coarse.

By signing up you agree to our terms & conditions: https://snyk.io/policies

This might be problematic. I don't think I can unilaterally agree to the T&C for the nodejs organization. Probably has to be brought up at a CTC or TSC meeting.

@jasnell
Member
jasnell commented Jan 12, 2017

Agree that the access to the "private organizational data" is far too vague to approve this. Our policy is to prevent access to private data by default. It needs to be clarified exactly what private data Snyk would have access to and the CTC or TSC would need to review and approve it.

@guypod
guypod commented Jan 12, 2017

All, thanks for looking into this.
I believe the second screen (requesting approval) simply has a somewhat misleading text from GitHub. The permissions we request are in the first screenshot, and clearly outlined. They contain asking for access to private email address, hence the use of the word private in the second prompt.

Perhaps one of the admins on the repo, who can choose to grant the permissions locally, could browse to https://snyk.io/add and attempt to grant the (public only) access there? This should skip the second prompt which, as I mentioned, is simply a bit poorly phrased...

@mikeal
Member
mikeal commented Jan 12, 2017

As a general rule we don't enable org wide access to external applications for security reasons. We have sensitive information in private repositories.

Being that there are only a few repos that we want to enable Snyk can we just configure a proper hook per repository?

@guypod
guypod commented Jan 12, 2017

@mikeal, the hook needs to be setup by our system as it includes a shared secret to verify requests.

However, you can easily control access further by creating a user that has access only to specific repos and integrate through that user. Would that be a valid way to set it up?

@sotayamashita sotayamashita referenced this issue in sotayamashita/nodejs.org Jan 13, 2017
Open

Try textlint into Nodejs.org #1

@phillipj
Member

Perhaps one of the admins on the repo, who can choose to grant the permissions locally, could browse to https://snyk.io/add and attempt to grant the (public only) access there?

No repos under the nodejs org is listed there. Not under the "other repositories" tab either.

@guypod
guypod commented Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment