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: describe sec team membership and policy #56

Closed
wants to merge 5 commits into from
Closed

process: describe sec team membership and policy #56

wants to merge 5 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Oct 12, 2017

as discussed in #55, attempt to describe privacy policy for sec team

@sam-github
Copy link
Contributor Author

related discussion: nodejs/TSC#358 (comment)

Members of the security teams should indicate that they accept the privacy
policies by PRing their acceptance to this file.

## Team that triages security reports
Copy link
Member

Choose a reason for hiding this comment

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

Should we explicitly define the scope of the security team to nodejs core only?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, we probably need 2 of these. One for Node core and then a different one for modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was we would document here the current team and practice. Once we have a thirdparty triage team and process, we can add that. But, I can add more future looking wording if that works.

Choose a reason for hiding this comment

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

So then does this doc explicitly cover only security reports and issues in node-core? If so, +1 to adding wording in the doc to making it explicit- and agree with @mhdawson that in the future there are probably separate teams for core vs modules

Copy link
Member

Choose a reason for hiding this comment

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

@sam-github I agreed, probably just good to make it clear that in this one its focused on core

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to explicitly defining core vs. module vulnerabilities.


See [nodejs-private/node-private](https://github.com/nodejs-private/node-private).

TBD - why is this list not the same as the team with access to security issues?
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be the same even if it is not currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just compared the lists.

@ejratl, @grnd, @mscdex, @shigeki, and @trevnorris are on the "access to security issues" list. They are not in nodejs-private/node-private, but all have pending invitations (it's a separate GitHub org).

@hackygolucky and @mrhinkle are included in nodejs-private/node-private but not the security issues list. They are probably included in nodejs-private because of Foundation activity.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, @hackygolucky and @mrhinkle must retain access from the foundation side of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcollina, you said you had access to issues, but not node-private, but that isn't what @cjihrig is seeing above. comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcollina wasn't listed here, so I didn't check his name. I can confirm he is not a member of node-private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, would he have access to the security issues as a side-effect of being on the TSC? Its quite possible my list is incomplete, I literally took the list from https://github.com/orgs/nodejs/teams/security/members, the folks in at-nodejs/security, but its possible there are more people with access than that team.

@sam-github
Copy link
Contributor Author

/to @nodejs/security , who this effects.

@@ -0,0 +1,63 @@
# Node.js Security Team

Node.js security teams are expected to keep all information that they have

Choose a reason for hiding this comment

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

Did you mean "Node.js security team members" here?

@sam-github
Copy link
Contributor Author

Updated to address comments.

Node.js security team members are expected to keep all information that they have
privileged access to by being on the team completely private to the team. This
includes notifying anyone outside the team of issues that have not yet been
disclosed publically, including the existence of issues, expectations of
Copy link
Member

Choose a reason for hiding this comment

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

s/publically/publicly/


## Team with access to private security patches

See [nodejs-private/node-private](https://github.com/nodejs-private/node-private).
Copy link
Member

Choose a reason for hiding this comment

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

Since this can't be seen publicly, should it be enumerated here, as with the other teams?

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

@@ -0,0 +1,92 @@
# Node.js Security Team

Copy link
Member

Choose a reason for hiding this comment

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

I am assuming this is intended specifically to describe the security team for Node.js core rather than the third party modules process, correct? If so, it would be good to state that up front.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is will eventually describe all the Node.js Security Teams, as the same privacy policy will apply to all of them.

The third party module process and teams are not yet listed, because they don't exist (yet).

@@ -0,0 +1,92 @@
# Node.js Security Team

Node.js security team members are expected to keep all information that they have
Copy link
Member

Choose a reason for hiding this comment

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

s/Node.js security team/Node.js Core Security Team


Node.js security team members are expected to keep all information that they have
privileged access to by being on the team completely private to the team. This
includes notifying anyone outside the team of issues that have not yet been
Copy link
Member

Choose a reason for hiding this comment

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

This includes not notifying ... ?


List is from ["security" alias](https://github.com/nodejs/email/blob/master/iojs.org/aliases.json).

## Team with access to security issues
Copy link
Member

Choose a reason for hiding this comment

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

So thinking about this particular list... I think we should manage this differently. Within the nodejs-private GitHub organization, only the Triage team above should have general access to everything. When a new security vulnerability is reported, a new private fork repo within nodejs-private is created by a member of the triage team, and a new GitHub team specific to that one repo is created by the Triage team member. That specific team can pull in any other collaborator/contributor is necessary to address that one problem so long as they agree to the confidentiality requirement. Once the issue is resolved and the fix lands in nodejs/node master, the individual fix repo in nodejs-private is archived and the GitHub team is dissolved.

This accomplishes several things:

  1. We have targeted and specific control over who has access to the specific security vulnerability details
  2. Each security issue will have it's own issues/PR tracker
  3. We don't have to maintain a list like the one below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the generalities of what you describe above, but the current list is a statement of fact, not of how it could be. Adoption of new tooling, proposed in nodejs/TSC#344 and not generating much interest, would cause a change in the process. For example, some of what you describe above is specific to the tooling used, once @vdeturckheim has made more progress on #57 , it may be easy to demonstrate the process, and propose it to be used for node core - at which point these specific teams may cease to exist, but the privacy policy will continue to be relevant.

List is from [nodejs/teams/security](https://github.com/orgs/nodejs/teams/security/members).

## Team with access to private security patches

Copy link
Member

Choose a reason for hiding this comment

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

So there need to be explicitly defined groups here:

We have:

  1. The Triage Team -- the small group that looks at initial reports and determines who the fix team should be.
  2. The Fix Team -- the small group of Collaborators with access to the private repo for fixing a specific issue
  3. The Review Team -- the slight larger group of trusted Collaborators who review and validate a fix immediately before release.

The group below should essentially be the Review Team. They may not be the ones actually working on the fix, and may not get the full details of the vulnerability right away, but they are expected to review and sign off on the fix before it goes out the door. Personally, I think the Review Team should be limited strictly to TSC members. If non-TSC members are required to help resolve an issue, they should be brought in on the Fix Team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In WG meetings, we felt that proposals to change the current process for core would best be raised after #54 is done, so changes to the core process can be proposed in the context of the tooling to be used. Just a suggestion, of course the TSC can change the sec process and team arrangement at any time. If you would like to kick off that discussion immediately I suggest opening a separate issue or PR.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

This is a good start. I would like to see it evolve further before landing tho.

@mhdawson
Copy link
Member

@jasnell I think @sam-github was trying to document as it is today..... It would be nice to let him achieve that and then we can work on what the process should be. Your last comment is not very specific in terms of what you'd want to have changed.

@jasnell
Copy link
Member

jasnell commented Oct 24, 2017

Your last comment is not very specific in terms of what you'd want to have changed

But my other comments should be :-)

I'm all for documenting current practice so long as we make progress on evolving from there.

@mhdawson
Copy link
Member

@jasnell so your specific comments seem to be around how we should manage things as opposed to the current state. I guess I'm asking if you believe we need to figure out that target state before landing as that is how I interpreted your comments and the request to change before this gets landed.

@sam-github
Copy link
Contributor Author

OK, I wasn't clear enough in my intentions for this.

This PR addresses two specific issues that came up during the informal meeting at NINA 2017:

  1. There is no documented privacy policy for the members of the existing teams.
  2. There is an existing set of security teams in Node.js, but the existence of those teams is not well-known, and the membership of those teams is not widely known (often the github pages that show the membership can only be accessed by the members).

This PR is not attempting to do the following things, though they are worthwhile and almost certainly necessary, eventually:

  • propose new teams that do not exist at this moment
  • propose new processes (other than the privacy policy, but that's not really a process)
  • unify any github setup (for example, to enforce that certain teams have exactly the same members)
  • propose any changes to policy on who are members of each team
  • etc.

From my point of view, the privacy expectation is the only important aspect of this PR, the list of members in the PR are pure statements of fact (though I believe I may be mis-reading github permissions), included so that it is clear who the policy should apply to.

@jasnell jasnell dismissed their stale review October 24, 2017 16:40

Ok. Given that this is only intended to document current process.

@sam-github
Copy link
Contributor Author

@jasnell yes, the current process --- EXCEPT for the privacy policy. That is undocumented and not widely agreed on ATM. The TSC will need to find a way to ensure that all the listed team members agree to be bound by the privacy policy.

If the teams were just forming, I'd think that would happen using the typical process of a new member PRing their name to the membership list under the policy statement. But the teams exist already, so not too sure how to do this. Its under the TSCs direction.

Do you have any ideas on this?

@mhdawson
Copy link
Member

I think this may be ready to land with the understanding that it docs what we have now, and that more work is needed to discuss update to reflect what we would like it to be, Anybody have outstanding objections ? @thefourtheye @jasnell @cjihrig @vdeturckheim @digitalinfinity

@vdeturckheim
Copy link
Member

In current form, lgtm

mhdawson pushed a commit that referenced this pull request Oct 27, 2017
PR-URL: #56
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@mhdawson
Copy link
Member

Landed as 129aaea

@mhdawson mhdawson closed this Oct 27, 2017
@sam-github
Copy link
Contributor Author

Thanks Michael.

@sam-github sam-github deleted the doc-security-privacy-expectation branch October 28, 2017 04:32
patrickm68 added a commit to patrickm68/security-wg-process that referenced this pull request Sep 14, 2023
PR-URL: nodejs/security-wg#56
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
mattstern31 added a commit to mattstern31/security-wg-process that referenced this pull request Nov 11, 2023
PR-URL: nodejs/security-wg#56
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
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.

9 participants