Skip to content

doc: add req for sec wg review in some cases #21896

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.

Sign up for GitHub

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

Closed
mhdawson wants to merge 2 commits into from
Closed

doc: add req for sec wg review in some cases #21896

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
260f937
doc: add req for sec wg review in some cases
mhdawson Jul 19, 2018
a4850f4
squash: address comments
mhdawson Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions COLLABORATOR_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements)
- [When Breaking Changes Actually Break Things](#when-breaking-changes-actually-break-things)
- [Reverting commits](#reverting-commits)
- [Additions to the Cryptography and Security APIs](#additions-to-the-cryptography-and-security-apis)
- [Introducing New Modules](#introducing-new-modules)
- [Deprecations](#deprecations)
- [Involving the TSC](#involving-the-tsc)
Expand Down Expand Up @@ -378,6 +379,16 @@ multiple commits. Commit metadata and the reason for the revert should be
appended. Commit message rules about line length and subsystem can be ignored.
A Pull Request should be raised and approved like any other change.

### Additions to the Cryptography and Security APIs

Semver-minor commits that add or change cryptograpy/security APIs should be
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Jul 19, 2018

Choose a reason for hiding this comment

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

cryptograpy -> cryptography

Copy link
Copy Markdown
Member

@tniessen tniessen Aug 4, 2018

Choose a reason for hiding this comment

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

Is semver-major ⊂ semver-minor? Otherwise, I don't see a reason not to include semver-major here.

treated with extra care. Due to the potential impact, it is important that
these APIs be constructed to reduce the potential for incorrect
usage.

These commits must have an approval from at least one member from the
[security working group](https://github.com/nodejs/security-wg).
Copy link
Copy Markdown
Member

@jasnell jasnell Aug 3, 2018

Choose a reason for hiding this comment

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

Like others, I am fine with the recommendation around taking increased care, but I'm not sure if the Security-WG is the correct venue. The @nodejs/cryto team already exists for coverage of the crypto and openssl related items, and the @nodejs/security team already handles Node.js core security related items. Perhaps an alternative here would be to require approval from at least one member of the Security WG or either @nodejs/crypto or @nodejs/security?

Copy link
Copy Markdown
Contributor

@joepie91 joepie91 Aug 4, 2018

Choose a reason for hiding this comment

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

I was under the impression that the Security WG and @nodejs/security were one and the same thing. Is this not the case?

Other than that, requiring approval from either the crypto or security team seems like a good idea.

Copy link
Copy Markdown
Member

@tniessen tniessen Aug 4, 2018

Choose a reason for hiding this comment

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

I was under the impression that the Security WG and @nodejs/security were one and the same thing. Is this not the case?

@joepie91 Not really, there is @nodejs/security and @nodejs/security-wg.

Copy link
Copy Markdown
Member

@Trott Trott Aug 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I was under the impression that the Security WG and @nodejs/security were one and the same thing. Is this not the case?

They are not the same thing. I suggested not naming the WG "Security" to avoid exactly this kind of confusion. I don't know what I would name it though. Maybe "Security-and-Something-Else WG" or "Something-Else and Security WG".

Copy link
Copy Markdown
Member

@Trott Trott Aug 4, 2018

Choose a reason for hiding this comment

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

Oh, I guess I should explain the difference. Or at least provide links. security-wg is the Security Working Group consisting largely of interested parties but not necessarily (or even largely) people who do a lot of work themselves on Node.js core. More details are at https://github.com/nodejs/security-wg.

Meanwhile, security is the team briefly described at https://github.com/nodejs/security-wg/blob/master/processes/security_team_membership_policy.md. That team existed before security-wg.


### Introducing New Modules

Semver-minor commits that introduce new core modules should be treated with
Expand Down