Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: update Node.js sec membership policy #558

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Jul 4, 2019

The policy was spread across two files, had mild contradictions (one doc
said membership was confirmed by the TSC, the other said by it was
confirmed by the current members), referenced the unused github issue
tracker (HackerOne is used now), and had lots of mention of the
@nodejs/security team, which does not in itself have access to ANY
private issues or patches (I removed those refs).

The policy is essentially unchanged, just more readable, and in one file
so it shouldn't drift out of sync as easily.

@sam-github
Copy link
Contributor Author

@nodejs/tsc @nodejs/security There is something I'm not clear on, is everyone who has access to the Node.js program in H1 on the Triage team? All those people have the ability to triage issues, but I'm not sure that you all will triage issues, and want to triage issues. A number of the TSC members may have access to H1 because of a responsibility to be aware of activity, but not out of an intention to be actively involved. Does it even matter? Maybe not, but if none of the "triage team" is actually triaging, things will go badly. Then again, perhaps that's best dealt with in the weekly TSC meetings, if there are open vulns that are not getting dealt with.

I bother bringing it up because the old triage team was small, and with H1, its now quite large, and its not obvious that the everyone who accepted the H1 invitation realized they were implicitly joining the triage team.

I'm a big fan of simplifying the complexity of the team structure. It does appear there are unavoidably, at this moment, 3 actual resources, and corresponding lists of people with access:

  • H1's nodejs program
  • https://github.com/nodejs-private/node-private, where we prepare patches that cannot be seen in the public yet, which is larger than the H1 list because it includes releasers
  • @nodejs/security, used to attract people's attention on github (and yes, I'm tagging you now on purpose)

It is possible that we can force the latter two github lists to have the same members (manually, or automatically, don't we have some tools somewhere for updating markdown membership lists from github APIs?), in which case, we only have to document two lists: triage team and security team.

What should we do here?

@sam-github
Copy link
Contributor Author

and to further flood mailboxes, sorry:

realized they were implicitly joining the triage team.

The triage team policy used to be that everyone individually PRed their name here, indicating acceptance of the privacy policy. I'm not sure that happened for the TSC when they got H1 access. I'm OK with removing the process, or following the process, but we should probably do one or the other.

@richardlau
Copy link
Member

It is possible that we can force the latter two github lists to have the same members (manually, or automatically, don't we have some tools somewhere for updating markdown membership lists from github APIs?), in which case, we only have to document two lists: triage team and security team.

Yes: https://github.com/nodejs/node-core-utils/blob/master/docs/ncu-team.md

@sam-github sam-github force-pushed the link-node-triage-team branch from 2e07e41 to 0ba7cda Compare July 8, 2019 16:46
@sam-github sam-github marked this pull request as ready for review July 8, 2019 17:36
@sam-github sam-github force-pushed the link-node-triage-team branch from 0ba7cda to 4bfdba9 Compare July 8, 2019 17:58
@sam-github
Copy link
Contributor Author

PTAL, now lists only the two teams, the one with access to H1, and the one with access to nodejs-private/node-private (updated to use ncu-team for keeping synced).

@Trott @mhdawson

@Trott
Copy link
Member

Trott commented Jul 8, 2019

PTAL, now lists only the two teams, the one with access to H1, and the one with access to nodejs-private/node-private (updated to use ncu-team for keeping synced).

@Trott @mhdawson

Taking a quick look, seems OK to me. I'm not on Security WG so I'll leave it to actual members to green-check this, but 👍.

@sam-github
Copy link
Contributor Author

@Trott You are among the correct people to review this, IMO. While the process wording needs updating because triage moved to H1, it is the sole responsibility of the TSC to determines the membership of the Node.js triage team, see https://github.com/nodejs/security-wg/blob/master/processes/security_team_membership_policy.md

@nodejs/TSC, PTAL

@sam-github sam-github force-pushed the link-node-triage-team branch 2 times, most recently from f5f47b7 to 57df1e6 Compare July 8, 2019 19:52
@sam-github sam-github changed the title doc: link to the Node.js triage team process: update Node.js sec membership policy Jul 8, 2019
@sam-github
Copy link
Contributor Author

While the process wording needs updating because triage moved to H1

... I updated the process wording, by moving its text into the same file as the membership privacy policy/membership list. So, that's all cleaned up now.

@sam-github
Copy link
Contributor Author

@nodejs/tsc, PTAL - it is TSC's preexisting responsiblity to appoint sec team members, I updated the lists of members, and made minor changes to the policy falling out of the HackerOne use, which moved the location of issues out of github (so access to it is not controlled any longer by a github team).

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with two tiny typographic changes to fix GitHub markdown rendering.

@sam-github sam-github force-pushed the link-node-triage-team branch from fc076f8 to d06ca0b Compare July 8, 2019 22:50
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

mhdawson commented Jul 9, 2019

Some background I think that is relevant:

  1. The suggestion was made to obsolete the private github repo that was used to manage discussion of vulnerabilities to force all discussion to H1
  2. All TSC members previously had access to the private github repo
  3. All TSC members were therefore invited to H1 so that they'd continue to have insight into the discussions on vulnerabilities.

I don't think there was any intention to change the composition of the triage team by adding the TSC members to H1. ie it still makes sense to have people PR themselves into a list that identifies who the core security triage team is.

@sam-github
Copy link
Contributor Author

@mhdawson I am also unclear about who is a member of the triage team, see my questions here: #558 (comment)

The triage team was a mailing list of people who received notification of new vuln reports. That mailing list no longer exists.

With H1, everyone who is a member of the H1 program gets notifications, but I agree, its not clear which (if any) of them is on the triage team.

I'd add an extra section specific to triage, but I don't know who the current team is. Perhaps a re-call for volunteers should be made at the next TSC meeting?

@mhdawson
Copy link
Member

mhdawson commented Jul 9, 2019

I agree re-validating who is currently on the core triage team would make sense.

@mcollina, @MylesBorins, @jasnell as well as yourself are the people who I've seen doing that work recently (apologies in advance to those that I've missed as this is just based on my recent memory of activity notifications I've gotten for H1).

Maybe start with this list and then confirm those members and ask if we've missed anybody? I suspect @mcollina, @MylesBorins, @jasnell will be able to fill in the gaps.

@mcollina
Copy link
Member

Everybody on the TSC has access to H1 because it’s there that the main conversation happen about vulnerabilities. Initial triaging is facilitates by H1 staff, so it’s not a burden.

The current problem is a) deciding what to do for the borderline cases and b) fixing protocol-relates issues (http, http2, etc). Some of those are extremely hard, and there are not enough people engaged in the process.

@sam-github
Copy link
Contributor Author

@mhdawson See @mcollina 's comment above, H1 is responsible for initial triage, something node used to do itself. I doesn't appear we need an initial triage team anymore.

@sam-github
Copy link
Contributor Author

@mhdawson PTAL. Triage team is restored to its previous value.

@sam-github sam-github force-pushed the link-node-triage-team branch from 16d6205 to e684eef Compare July 11, 2019 17:40
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sam-github
Copy link
Contributor Author

FTR, removing @mhdawson from triage, he shouldn't have been there, he only had access to H1. See #414 (comment)

The policy was spread across two files, had mild contradictions (one doc
said membership was confirmed by the TSC, the other said by it was
confirmed by the current members), referenced the unused github issue
tracker (HackerOne is used now), and had lots of mention of the
@nodejs/security team, which does not in itself have access to ANY
private issues or patches (I removed those refs).

The policy is essentially unchanged, just more readable, and in one file
so it shouldn't drift out of sync as easily.
@sam-github sam-github force-pushed the link-node-triage-team branch from cd10414 to dfc7526 Compare July 11, 2019 19:07
@sam-github sam-github merged commit 13281e1 into master Jul 11, 2019
@sam-github sam-github deleted the link-node-triage-team branch July 11, 2019 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants