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

mhdawson · 2018-07-19T21:51:16Z

Some recent dicsussions point to it being
a good idea to require reviews from the security-wg
for security related PRs. See #21766

Add this requirement to the collaborator guide.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-07-19T21:51:17Z

@mhdawson build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/317/pipeline

mhdawson · 2018-07-19T21:52:07Z

Pushed button a bit early, running tests now, tests complete and ok.

Some recent dicsussions point to it being a good idea to require reviews from the security-wg for security related PRs. See nodejs#21766 Add this requirement to the collaborator guide.

vsemozhetbyt · 2018-07-19T22:32:18Z

COLLABORATOR_GUIDE.md

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


cryptograpy -> cryptography

Trott

I'm sorry to register opposition to this approach but I don't think this text is the way we want to go.

It's not clear to me what a "security API" is. Is that something like Buffer.from(). If not, what is a "security API"?
I'm opposed to adding a new complication to landing stuff. If we want to make it more difficult for problems to slip through the cracks, let's simplify the rules rather than add yet another rule that will be ignored. (We break our policies every single day and I do a lot of biting my tongue about it.) A simplification might be that semver-minor requires two TSC approvals the way semver-major does. If that's not a simplification, at least it does not add new complications. Another simplification might be to simply require all PRs to be open for 72 hours (except for fast-track PRs). No more figuring out if it's a "weekend" or not (time zones anyone?). That will give people more time to go through new PRs that might be of interest, and more time for people to @-mention interested parties and stakeholders.
I'd be 💯 on a guideline that strongly recommended notifying Security WG via @-mention. And if we go that route, then we don't have to clarify what a security API is. Not sure something is a security API? Well, when in doubt, @-mention Security WG and let them figure out if it's relevant to their concerns or not.

joepie91 · 2018-07-20T06:35:01Z

I'm opposed to adding a new complication to landing stuff.

I don't feel that this adds any complication, exactly. Rather, it just adds an extra step to the process that can be paraphrased as "ensure that it's been seen by the people who are subject matter experts on the kind of changes being made".

That seems like a pretty essential part of the process to me, considering the long-term impact of getting it wrong. We're still, as an ecosystem, collectively dealing with the fallout of new Buffer.

It's not clear to me what a "security API" is. Is that something like Buffer.from(). If not, what is a "security API"?

That's a reasonable question. This is probably always going to remain fuzzy to some degree, but I think a good first approximation is "changes where getting it wrong could have a security impact somehow", to be determined by those reviewing the changes.

To relate this back to your Buffer example; the original new Buffer as well as the current Buffer.allocUnsafe (and its slow variant) would need security review, considering that they could potentially expose chunks of memory. Buffer.from would not, since there's presumably no security impact in getting it wrong.

Possibly we should reword the change to "cryptography APIs and changes with potential security impact", or a wordier variation of that? I agree that "cryptography/security APIs" is rather vague, and will probably be misinterpreted.

MylesBorins

As this appears to expand the charter of the security-wg I feel like this is something that should also land in the charter and be approved by the TSC

ChALkeR · 2018-07-20T09:00:10Z

@Trott

I share your concerns about already breaking our policies and that we should try to see why those are broken and fix them.

require all PRs to be open for 72 hours (except for fast-track PRs)

It might be a good idea to give more time for review.

One more idea for solving the «we don't wait enough time» — a bot that would ensure that we wait appropriate time via GitHub checks API should be great. Having that, we might also install more complex rules, e.g. wait more time for semver-minor and semver-major requests, enforce TSC approval, etc.

A simplification might be that semver-minor requires two TSC approvals the way semver-major does.

+1 on this one, we have enough people in the TSC for that. Also could be enforced by a bot, I believe.

That said, I would still prefer something among these lines to be adopted (for security reasons), and I ultimately believe that solving the «too hard to keep track of our rules» problem is orthogonal and should be solved by other means. I believe that the evidience we have justifies that.

It's not clear to me what a "security API" is. Is that something like Buffer.from(). If not, what is a "security API"?

What @joepie91 said above («changes where getting it wrong could have a security impact somehow») is good, but part of the problem is that detemining that is hard. Perhaps that can include something like «any semver-minor or semver-major changes to the crypto module, anything labelled as security, and anything else on any Collaborator judgment (i.e. where improper usage could cause security impact — should be labelled as security)».

«Any semver-minor or semver-major changes to the crypto module» might seem too broad and excessive (as most of those are not significant), but there were only 14 such PRs in the whole 10.x changelog, out of 1275. Including those that went into 10.0.0 release.
So I don't think that this would add any significant burden.

I'd be 💯 on a guideline that strongly recommended notifying Security WG via @-mention.

Yes, that is also a great idea and could be done independently in a broader range of situations. But that alone wouldn't suffice without a strict requirement, imo.

ChALkeR · 2018-07-20T09:11:01Z

@MylesBorins

As this appears to expand the charter of the security-wg I feel like this is something that should also land in the charter and be approved by the TSC

Can that be resolved by allowing to override that rule with a TSC consensus or vote instead?
I.e. «a review from the Security WG or a TSC vote». I feel like that should be added just in case, and don't expect that it would be used without a very good reason.

All what remains could fall under «Recommend security improvements for the core Node.js project» and «Define and maintain security policies and procedures for the core Node.js project» which are already in the charter. We could refine that, but I don't view this as a blocker.

jasnell · 2018-07-20T15:22:50Z

I agree that this is something we should have the TSC discuss before progressing.

mhdawson · 2018-07-30T22:46:31Z

Agreed that this needs TSC discussion. I had thought I'd added a @nodejs/tsc ping but looks like I missed that. Will track conversation and then if makes sense add to agenda for next week.

mhdawson · 2018-07-31T19:57:33Z

Reading through the specific comments.

I'd be personally ok with 2 TSC approvals being required for all SemVer minor commits but that is probably a bigger ask than the original suggestion.
I like @ChALkeR suggestions on how to make the definition more concrete:

Perhaps that can include something like «any semver-minor or semver-major changes to the crypto module, anything labelled as security, and anything else on any Collaborator judgment (i.e. where improper usage could cause security impact — should be labelled as security)».

I don't think the TSC indicating that reviews from a WG/team are required should need a change to the charter. The TSC still has ownership of the review process and could remove that requirement without agreement from the Security WG. It is TSC/collaborator base decision to adopt the suggested review process and I don't think it transfers any formal authority/responsibility in doing so.
A middle ground might be to require 2 TSC approvals for PRs that add or change cryptograpy/security APIs, along with the requirement that there be a @mention to the security wg. We can then ask TSC members to only approve once we've had enough input from the security-wg and trust that will ensure enough review.
I agree that if we don't follow our existing rules then it is orthogonal and we should start a discussion on how to fix that as opposed to conflicting that with this discussion.
I personally feel there are times that its ok to slow things down a bit, particularly where the potential downsides are high. We don't want to do that in all cases but I think when adding new security APIs or features to existing ones, I'd rather we do what we can to make sure we get them right versus experimenting and cleaning up later.

Trott · 2018-08-01T05:13:00Z

I agree that if we don't follow our existing rules then it is orthogonal

FWIW, IMO this sort of thing is not orthogonal to our failure to keep track of and follow our policies. It is a root cause of it. Creating different requirements for different APIs is going to naturally and perhaps inevitably result in those sorts of problems. This is especially true when some of the APIs (like "security APIs") are difficult to define in practical terms. It seems like a necessarily "I know it when I see it" kind of thing. (Lots of things are that way necessarily.)

Trott · 2018-08-01T05:16:13Z

Working Groups often go through periods of inactivity or even effectively stop existing altogether. If there is to be WG veto power on code changes, it needs to be an active veto. To stop something, they need to do something. The proposal here invests a WG with the power to veto something by doing nothing. That seems unwise to me.

Here's a multi-pronged counter-proposal:

Identify certain files that will result in a @-mention ping of the Security WG by our GitHub bot when one or more of those files are modified in a PR.
Consider an effort to onboard more Security WG folks as Collaborators.
Add material to the COLLABORATOR_GUIDE and elsewhere encrouaging the pinging of the Security WG for relevant changes.
The Security WG can undertake an initiative to make sure they review potentially-relevant pull requests and flag things. ("Potentially-relevant" may just mean reviewing all PRs that modify files in lib or src.)
Clarify the scope of fast-tracking. I'd propose that we allow Collaborators to use the existing fast-track approach for appropriate changes in test/doc/tools. However, fast-tracking in lib/src should require two TSC approvals rather than two Collaborator approvals. Basically, we should never fast-track in lib/src unless there are extraordinary circumstances.
Change the 48/72 hour rule to a 72 hour rule. Keep it simple, and give everyone a little extra time. No one has to figure out if it's a weekend somewhere in the world. 72 hours all the time.

Trott · 2018-08-01T05:22:01Z

Oh, another one...

Consider an effort to recruit appropriate folks from @nodejs/collaborators into the Security WG.

MylesBorins · 2018-08-01T06:39:47Z

So at the risk of being pedantic... security !== crypto

It is possible that improper crypto can lead to security vulnerabilities, but afaik while the security-wg maty have crypto experts it is not a requirement of being part of the group.

I would much rather see something to empower the security-wg to have the ability to flag things that are security concerns... this may be crypto, it may be http2, it may be the debugger (e.g. how we expose ports).

edit:

FWIW I think we should explore spinning up a crypto team to work on a long term plan for ongoing support of crypto in core to help avoid security concerns... such a group could eventually be chartered by the TSC.

edit 2:

it is also possible that cryptography could be a team under the security wg, with limited membership. Similar to what we currently do with Release / releasers / LTS.

all crypto team and in sec-wg but not all sec-wg members are on crypto team

joepie91 · 2018-08-01T10:16:54Z

@Trott: We have basically two failure modes to pick from here:

Require active review; the failure mode is that a potentially-security-impacting change gets blocked on people from a WG not being around, until those people return.
Allow passive veto; the failure mode is that if people from a WG are not around, a change that negatively impacts security gets passed quietly, has impact on the ecosystem, and then becomes very difficult to revert or resolve afterwards; in some cases it potentially being impossible to reverse the damage.

I would argue that the first failure mode is far preferable where potentially-security-impacting changes are concerned. There's no review model that is entirely without downsides, but requiring active review at least does not leave difficult-to-resolve ecosystem damage on something as critical as security.

Trott · 2018-08-01T20:15:19Z

@joepie91 The failure modes argument is, of course, a great point. I'm not sure it tips the scales for me--although it might--but I certainly won't try to dispute it.

AFAICT, this whole thing is mostly due to #20816. So I do want to point out that that PR did get a ✅from a Security WG member, so this policy change wouldn't have prevented that. (Of course, for some, that may argue for a stronger version of this policy.)

mhdawson · 2018-08-01T22:19:17Z

@MylesBorins you make some good points about things being potentially broader. I think it would be good though to see if we can get consensus on something more limited (for example crypto additions) as a first step, and once we are comfortable with that then look at other ways to improve/have a better safety net overall on the security front.

mhdawson · 2018-08-01T22:20:46Z

As to the discussion the failure mode, I'm definitely in favor of erring on the side of slowing things down a bit. That would not be the case in other areas, but for security I think its justified.

Trott · 2018-08-01T22:31:42Z

Oh, another suggestion along the tools-not-rules line of thinking:

Have node-core-utils warn when you're about to land changes to crypto.js (and maybe a few other files) if Security WG has not been pinged in the PR. This would be similar to what it does now when it sees that changes have been pushed since the last CI run, etc.

This and the GitHub bot suggestion have the advantage of being things that can't be forgotten about.

mhdawson · 2018-08-02T22:56:55Z

I like the suggestions of having node-core-utils warn, but not sure that removes the reason/benefit of having requirement for additional reviewers.

mhdawson · 2018-08-02T22:58:01Z

@nodejs/tsc would be good to have a few more voices in the discussion.

jasnell · 2018-08-03T22:33:20Z

COLLABORATOR_GUIDE.md

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


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?

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.

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.

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".

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.

tniessen · 2018-08-04T10:01:25Z

COLLABORATOR_GUIDE.md

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


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

tniessen · 2018-08-04T17:02:54Z

An alternative idea for discussion:
The team @nodejs/security (or @nodejs/security-wg, I don't know) should be pinged on all relevant PRs (let's say semver-minor / semver-major crypto PRs, security-related PRs etc.), which gives them the usual 48/72 hour window to express concerns. This way we avoid the formality of having to block on an approval from a team and still make sure to include the security WG in these decisions. If a PR author does not ping the security team in these cases (e.g. because they are not a collaborator, don't think it's necessary or simply forget it), other collaborators are encouraged to ping the security team as soon as possible.

Trott · 2018-08-04T20:03:12Z

(I hasten to add that there are people on the Security WG now who are extremely knowledgable about security and Node.js. Didn't mean to imply that it consists primarily of people who aren't or anything like that!)

tniessen · 2018-08-04T20:05:08Z

@Trott I completely agree, and even if we assume that there is a group of "security experts", that doesn't mean that they are also "crypto experts".

So, my question would then be: which team or WG does consist of people who specialize in the security aspect of Node.js core?

@joepie91 That would be the security WG (as far as I know, not entirely sure anymore), but being a member of that WG still doesn't require you to be a professional in that area.

Is there a particular reason you're making a distinction between the API design and the implementation? I don't immediately see why they'd require two different sets of people looking at them, but perhaps there's something I'm missing.

I think there are some differences, though I might be wrong here. To review the API design, one doesn't need to fully understand all the native code, OpenSSL routines etc., but one should probably be familiar with security, cryptography, JavaScript, the Node.js ecosystem and established conventions.
To review the implementation from a security perspective, it's good to have a non-trivial amount of knowledge about Node.js internals, C++, OpenSSL, JavaScript, cryptography, security, maybe even the build process and supported architectures.

Side note: We are effectively already kind of requiring reviews from the people in @nodejs/crypto since they are more or less the only people who frequently review crypto PRs, and semver-major PRs also need to be approved by the TSC.

bengl · 2018-08-04T20:15:31Z

It seems that non-working-group teams are typically used as groups to ping on specific issues and PRs related to their specialties. In this case, I think if the process is modified to require the approval of folks on specific teams, then it should be the non-working-group teams (i.e. @nodejs/crypto and/or @nodejs/security) rather than @nodejs/security-wg, since the WG's purpose is primarily defining security policy, rather than being a collective of expertise with respect to security-related code.

ChALkeR · 2018-08-04T21:11:25Z

@joepie91 (cc @Trott) @nodejs/security is in a bit inconsistent state atm, as it's noted in the policy. See members list. The most close approximization of it would be "those with access to private security issues/prs in Node.js core". It is mostly formed from the @nodejs/tsc and the @nodejs/release teams as standing members (though individual discussions can include people from outside). Basically, things happen there when a security issue is found or reported.

@nodejs/security-wg, on the other hand, is a separate autonomus working group, which charter is here. It deals with overall security in the ecosystem, policies, and more public stuff like that, also private reports to the nodejs-ecosystem packages on HackerOne.

Of those, I don't think that this falls into @nodejs/security area. Relying on the @nodejs/security for this would just put more load on the @nodejs/security and would result in no effect (i.e. no additional input) compared that just relying on the TSC (@nodejs/security is mostly TSC).

We need more outside expertise on this, especially from people that are dealing with ecosystem stuff, e.g. which analyze issues in ecosystem packages and deal with ecosystem reports.

I would rely on @nodejs/security-wg more here.

@Trott, questioning @nodejs/security-wg membership is not a good argument here, in my opinion.

There is no requirement that members of the Security WG have expertise in security and Node.js. It is entirely plausible that people can get added simply for being interested and willing to contribute valuable work on documents, policies, etc. that may not require specific security expertise.
I think it's also clear that there are knowledgable security folks on the project who are not in the WG.
So it's not necessarily true that a member of the Security WG is a security expert. And it's certainly not true that all the security experts on the project are in the WG.

Everything said in that citation is both obvious and can be applied to any other group. That is why it's not an argument (imo).

What matters is, in my opinion:

Requesting input from @nodejs/security-wg for everything with potential significant security impact is better than not doing so, even if that would induce additional wait. In my opinion, the additonal wait shouldn't be a significant issue though for two reasons: @nodejs/security-wg is pretty active (at least certain members of it) and there shouldn't be many issues where we would need that (see the security label as a reference, I tend to use on anything with potential security impact).
Requesting additional input from @nodejs/security-wg should give better effect on those issues than requesting input from @nodejs/security, for the reasons stated above.

Upd: Re: @nodejs/security-wg having members that are there for just docs and so - perhaps it would be better to ask them if that's the case, and if that is - perhaps ask the WG to form a sub-group with people that should be mentioned in outside discussions like this?

ChALkeR · 2018-08-04T21:38:16Z

@Trott (cc @MylesBorins) Do I understand your concerns correct that the main part of the problem is blocking against the Security WG review and that it's not entirely clear who exactly and how exactly does that?

Perhaps a somewhat more flexible way that would not require indefinite blocks would be to:

Mention @nodes/security-wg to every security-labelled PR (see proposal for criterias above, as cited here).
Give them at least 72 hours after the team is mentioned (do not land until that).
Consider all input from the @nodejs/security-wg as input from collaborators for issues that any collaborator labeled as security -- i.e. block on all -1s from them until the disagreement is resolved or that gets escalated and resolved on the TSC level.

@joepie91 @Trott @MylesBorins @mhdawson Thoughts?

tniessen · 2018-08-04T22:00:28Z

@ChALkeR That's roughly what I suggested in #21896 (comment), @joepie91 believes this might lead to things landing without being properly reviewed by the security WG since we aren't blocking on it.

ChALkeR · 2018-08-04T22:12:22Z

@tniessen @joepie91 Ok, I see the concern here. The problem is that we don't want potential security-affecting changes to slip by fast without proper review. The other problem is that people could ignore non-important changes and avoid giving reviews becaue they don't want to bother with testing out the code, for example, and we don't want to block indefinitely in that situation.

Perhaps we could require "tagged+cc for 72h + approval from security-wg member OR tagged+cc for 120h + approval from a TSC member" then? That should both give more time, not block indefinitely, and encourage pinging security-wg more actively.

Trott · 2018-08-04T22:53:10Z

Everything said in that citation is both obvious

@ChALkeR Previous conversation in this pull request made it clear (IMO anyway) that it was not obvious to everyone that there is no requirement that members of the Security WG have expertise in security.

@Trott, questioning @nodejs/security-wg membership is not a good argument here, in my opinion.

Agreed. I wrote: "In and of itself, that might not be a reason to oppose this proposal. But hopefully exposing different understandings and assumptions can help clarify issues and achieve a resolution sooner."

I didn't expect the membership stuff to persuade anyone of anything as much as I thought it might facilitate clearer communication between the two points of view as there seemed to be different assumptions.

Another thing that is obvious to a lot of us but maybe not obvious to everyone: Currently, no other group has special review authority as is being proposed here. Again, not an argument against it: You can make the case that security concerns deserve special treatment that doesn't apply elsewhere in the code base. But I think the fact that this is unprecedented may explain some of the pushback.

richardlau · 2018-08-04T23:35:17Z

Another thing that is obvious to a lot of us but maybe not obvious to everyone: Currently, no other group has special review authority as is being proposed here. Again, not an argument against it: You can make the case that security concerns deserve special treatment that doesn't apply elsewhere in the code base. But I think the fact that this is unprecedented may explain some of the pushback.

I recall there was some discussion some time ago about streams PRs requiring sign off from nodejs/streams. It was never formalized -- I don't recall why.

Edit: Proposal: Require Streams WG to sign off on streams PRs

lirantal · 2018-08-07T06:23:07Z

I agree with all of @ChALkeR arguments, and believe that we should also follow the same with @nodejs/crypto, thereby covering both the ecosystem impact/policies and expertise in this domain.

mhdawson · 2018-08-07T21:03:05Z

@Trott I'll point you to: https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md
which requires sign off from the N-API team. (Note there was a ping from TSC input before that landed)
Not that having a precedent justifies applying it in a different situation. I also agree that this is still something new and warrants sufficient discussion (which it seems to be getting :)) to come to the right answer.

There seem to be 3 different discussions:

Should there be additional review requirements for security-related PRs, even if this slows them down a bit?
How do we properly identify "security-related PRs", using change to crypto was one attempt at that
If the answer to 1) is yes, then which group is the right group to do the review?

It may make sense to tackle the discussion in that order as they each depend on agreement on the previous one.

My personal feeling is that extra review is warranted for SemVer minor security PRs and I'd be in favor or a broader versus narrower review. In my mind we want to get as many eyes on the PR as opposed to arguing if a particular group is qualified or not. We generally trust people to review in the areas they are competent in (whether that be API design, implementation etc.) So if there are multiple potential groups/team then we should require the all be notified and even possibly require some approval from them. When we talk about 72hours for review I'm fine for most PRs, but I think we can/should take a bit more time in some cases and this is one of them. Otherwise its just too easy to miss out on what may be very valuable input.

Trott · 2018-08-07T23:19:32Z

@mhdawson So, subtle but crucial ambiguity: It's not clear to me if that n-api doc and this security/crypto proposal here require sign-off from WG/team members who are also Collaborators or if sign-off from any member of the team is sufficient. At face value, it would seem to be any member, but sign-off from non-Collaborators is sufficiently novel that it should probably be made explicit if that is the intention.

Trott · 2018-08-07T23:32:44Z

Regarding the n-api doc: #21877 (comment)

mhdawson · 2018-08-08T22:30:53Z

@Trott I assume you mean "core" collaborators. It was not intended to be limited to people who were both core collaborators and members of the security-wg or team.

I agree its good to clarify, before I do that do you see that as more problematic?

I'd like to figure out if we are trending towards agreement that we should have additional review or we still don't have any agreement on that front.

gireeshpunathil · 2018-08-09T04:37:42Z

Adhereing to the standard review and landing process for node PRs (at least one approval from core collaborator; x hours before landing; weekend considerations etc.) + additional review from members of WG when required, where they may not be a core collaborator looks good to me. By leveraging WG member's expertise and insights, we are only strengthening our PRs.

Trott · 2018-08-09T05:22:10Z

Adhereing to the standard review and landing process for node PRs (at least one approval from core collaborator; x hours before landing; weekend considerations etc.) + additional review from members of WG when required,

@gireeshpunathil I see your point for sure, but how many variations on the standard review and landing process before it's too unwieldy and everyone routinely ignores the variations? If this lands and if one accepts the N-API one as binding policy, that means we now have three different requirements depending on what kind of changes you're making. I would argue that three may already be too many in that people will forget about the policy and violate it.

(Conceivably, any issues there could be solved via tooling. If we get the commit queue together, it can enforce restrictions on what can land, although it's a bit difficult to imagine how the requirements in this particular proposal could be followed algorithmically. What constitutes a "security API" is not easy to define with precision.)

Trott · 2018-08-09T05:26:42Z

I'd like to figure out if we are trending towards agreement that we should have additional review or we still don't have any agreement on that front.

@mhdawson I'm going to punt and say let's see what other folks who have expressed concerns (specifically about Security WG review authority) are thinking: @tniessen @jasnell @MylesBorins @bengl

gireeshpunathil · 2018-08-09T06:16:29Z

@Trott - acknowledge on the issues at ground (per-PR policy eneforcement). But given the intent (tightening the security posture of our project) how about we move forward with this and improve over time? I see you already ping WGs for PRs based on perceived security implications.

tniessen · 2018-08-09T08:53:55Z

I share @Trott's concern that we are fragmenting our policies here, and I am still not sure whether this is the best way to go. I won't object to this decision and suggest we try it out. If it does not improve the situation or blocks PRs due to lack of approval, we can still revert the decision.

mhdawson · 2018-08-10T21:12:34Z

Got some input from @MylesBorins today. Will take a cut at an updated version when I get back from holiday next week.

mhdawson · 2018-08-29T18:58:50Z

@MylesBorins updated based on our discussion, please take a look.

SomeoneWeird · 2018-08-30T05:24:25Z

COLLABORATOR_GUIDE.md

+For Semver-minor commits changing cryptography APIs, they must have
+an approval from at least one member from the crypto team.
+
+For Semver-minor commits changing Security API's(other than those


API's -> APIs like it is elsewhere?

MylesBorins · 2018-09-04T17:26:30Z

COLLABORATOR_GUIDE.md

+For Semver-minor commits changing cryptography APIs, they must have
+an approval from at least one member from the crypto team.
+
+For Semver-minor commits changing Security API's(other than those


this seems extremely broad. What are our security APIs?

bengl · 2018-09-04T17:44:52Z

COLLABORATOR_GUIDE.md

@@ -378,6 +379,24 @@ 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 APIs and Security APIs


Does tls fall under "security" or "crypto", and if it's "crypto" then are there any "security APIs" at all?

jasnell · 2018-10-19T22:11:50Z

What is the status on this?

Trott · 2018-11-20T23:00:07Z

I'm going to close this one and label it stalled, but feel free to comment or re-open if you intend to continue discussion to make the case for it.

doc: add req for sec wg review in some cases

260f937

Some recent dicsussions point to it being a good idea to require reviews from the security-wg for security related PRs. See nodejs#21766 Add this requirement to the collaborator guide.

mhdawson force-pushed the sec-review branch from 27010b8 to 260f937 Compare July 19, 2018 22:08

vsemozhetbyt reviewed Jul 19, 2018

View reviewed changes

Trott requested changes Jul 20, 2018

View reviewed changes

MylesBorins suggested changes Jul 20, 2018

View reviewed changes

ChALkeR added doc Issues and PRs related to the documentations. security Issues and PRs related to security. meta Issues and PRs related to the general management of the project. labels Jul 22, 2018

jasnell reviewed Aug 3, 2018

View reviewed changes

tniessen reviewed Aug 4, 2018

View reviewed changes

squash: address comments

a4850f4

SomeoneWeird reviewed Aug 30, 2018

View reviewed changes

MylesBorins reviewed Sep 4, 2018

View reviewed changes

bengl reviewed Sep 4, 2018

View reviewed changes

Trott added the stalled Issues and PRs that are stalled. label Nov 20, 2018

Trott closed this Nov 20, 2018

mhdawson deleted the sec-review branch September 30, 2019 13:13

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

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

Conversation

mhdawson commented Jul 19, 2018 • edited Loading

Checklist

nodejs-github-bot commented Jul 19, 2018

mhdawson commented Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

joepie91 commented Jul 20, 2018 • edited Loading

MylesBorins left a comment

Choose a reason for hiding this comment

ChALkeR commented Jul 20, 2018 • edited Loading

ChALkeR commented Jul 20, 2018

jasnell commented Jul 20, 2018

mhdawson commented Jul 30, 2018

mhdawson commented Jul 31, 2018

Trott commented Aug 1, 2018 • edited Loading

Trott commented Aug 1, 2018

Trott commented Aug 1, 2018

MylesBorins commented Aug 1, 2018 • edited Loading

joepie91 commented Aug 1, 2018

Trott commented Aug 1, 2018 • edited Loading

mhdawson commented Aug 1, 2018

mhdawson commented Aug 1, 2018

Trott commented Aug 1, 2018

mhdawson commented Aug 2, 2018

mhdawson commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Aug 4, 2018

Trott commented Aug 4, 2018

tniessen commented Aug 4, 2018

bengl commented Aug 4, 2018

ChALkeR commented Aug 4, 2018 • edited Loading

ChALkeR commented Aug 4, 2018 • edited Loading

tniessen commented Aug 4, 2018

ChALkeR commented Aug 4, 2018 • edited Loading

Trott commented Aug 4, 2018

richardlau commented Aug 4, 2018 • edited Loading

lirantal commented Aug 7, 2018

mhdawson commented Aug 7, 2018

Trott commented Aug 7, 2018

Trott commented Aug 7, 2018

mhdawson commented Aug 8, 2018

gireeshpunathil commented Aug 9, 2018

Trott commented Aug 9, 2018

Trott commented Aug 9, 2018

gireeshpunathil commented Aug 9, 2018

tniessen commented Aug 9, 2018

mhdawson commented Aug 10, 2018

mhdawson commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 19, 2018

Trott commented Nov 20, 2018

mhdawson commented Jul 19, 2018 •

edited

Loading

mhdawson commented Jul 19, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

joepie91 commented Jul 20, 2018 •

edited

Loading

ChALkeR commented Jul 20, 2018 •

edited

Loading

Trott commented Aug 1, 2018 •

edited

Loading

MylesBorins commented Aug 1, 2018 •

edited

Loading

Trott commented Aug 1, 2018 •

edited

Loading

Trott Aug 4, 2018 •

edited

Loading

ChALkeR commented Aug 4, 2018 •

edited

Loading

ChALkeR commented Aug 4, 2018 •

edited

Loading

ChALkeR commented Aug 4, 2018 •

edited

Loading

richardlau commented Aug 4, 2018 •

edited

Loading