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

doc: add extra step reporter pre-approval #44806

Merged

Conversation

RafaelGSS
Copy link
Member

As discussed in the #security-triagge (OpenJS channel). To avoid insufficient CVE fixes across Security Release, might make sense to request a reporter pre-approval.

cc: @nodejs/tsc

@nodejs-github-bot
Copy link
Contributor

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 27, 2022
lpinca
lpinca approved these changes Sep 27, 2022
@mhdawson
Copy link
Member

@RafaelGSS While that would be great, I think that might be a bit of an ask on the steward. It might require building on platforms that are not available to them and also quite a lot of work if there are many vulnerabilities.

I think instead sharing the patch in the H1 issue with the reporter would be more manageable.

@RafaelGSS
Copy link
Member Author

I think instead sharing the patch in the H1 issue with the reporter would be more manageable.

The problem is that sometimes the patch might not be so obvious and considering the fact of the Node.js codebase is complex, I doubt the reporter could validate the fix easily.

I've included that option as optional, because as you said, sometimes might not be feasible to share a binary with the reporter due to its architecture.

@RafaelGSS RafaelGSS force-pushed the doc/request-reporter-pre-approval branch from c25869e to 5860adc Compare Sep 27, 2022
@RafaelGSS RafaelGSS changed the title doc: add extra step - reporter pre-approval doc: add extra step reporter pre-approval Sep 27, 2022
@RafaelGSS RafaelGSS force-pushed the doc/request-reporter-pre-approval branch from 5860adc to e09fb87 Compare Sep 27, 2022
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.

I'm not comfortable with this change given our current process. The problem is that we won't have the binary available until its release time and we have locked CI.

@RafaelGSS
Copy link
Member Author

I'm not comfortable with this change given our current process. The problem is that we won't have the binary available until its release time and we have locked CI.

The suggestion is for the releaser to build the binary locally and send it to the reporter. As mentioned before, sometimes it might not be possible due to different architectures between the releaser and reporter (that's the reason this option is optional). But, I see this as a good step to avoid further insufficient security fixes.

@RafaelGSS RafaelGSS requested a review from mcollina Sep 29, 2022
@tniessen
Copy link
Member

Assuming this is motivated by a particular issue which I won't mention because this is a public discussion, I want to remark that the patches that were developed did address the respective report (for the most part). It was either much later or a different person that found a way to circumvent the fix in a way that was not described in previous reports. In these cases, the respective reporter might have agreed that a patch was sufficient because they themselves weren't aware of other special cases at that time.

We have also had reporters submit patches themselves that turned out to be insufficient or wrong.

That being said, when I found a vuln in OpenSSL, the maintainers did share all the patches with me and asked for feedback, which was a good experience for both sides, I think. In that particular case, it worked out nicely, but even then, I didn't have much to say about the patch because I am not that familiar with their code base. I could easily have missed a bug in the patch.

@RafaelGSS
Copy link
Member Author

Great feedback @tniessen, thanks!

To summarize, we have two proposals on the table (we can choose both, btw):

  1. Build the binary locally and send to the reporter
    • Pros:
      • the reporter may run a few tests to guarantee the vulnerability was fixed.
    • Cons:
      • the releaser and the reporter must be system compatible
      • other vulnerabilities can be created while fixing the reported one
  2. Share the patches before the release with the reports
    • Pros:
      • we might avoid creating new vulnerabilities through reporter code-review
    • Cons:
      • rarely, the reporter will have enough knowledge to understand/review changes in the Node.js codebase
      • currently, we don't have a good way to share the patches that affect more than 1 single file. We can obviously paste the whole diff in the report, but that won't make the review any easier.

As discussed in the #security-triagge (OpenJS channel).
To avoid insufficient CVE fixes across Security Release,
might make sense to request a reporter pre-approval.
@RafaelGSS RafaelGSS force-pushed the doc/request-reporter-pre-approval branch from e09fb87 to 2fd3a56 Compare Oct 1, 2022
@RafaelGSS
Copy link
Member Author

So, we've discussed this PR at the Collab Summit (nodejs/Release#785) and we've agreed to use both approaches. I've adjusted the content to also include the diff share. We are also evaluating the efforts to use our build system to build each patch and provide it to the reporter itself. But that's another topic.

cc: @nodejs/releasers

jasnell
jasnell approved these changes Oct 2, 2022
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

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 2, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 2, 2022
@nodejs-github-bot nodejs-github-bot merged commit deb9f5e into nodejs:main Oct 2, 2022
15 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in deb9f5e

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
As discussed in the #security-triagge (OpenJS channel).
To avoid insufficient CVE fixes across Security Release,
might make sense to request a reporter pre-approval.

PR-URL: #44806
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants