build,src: add tag/property for security releases

[richardlau]

Define a new NODE_VERSION_IS_SECURITY_RELEASE macro in
src/node_version.h that indicates if the release is a security
release. Update the release documentation.

Add a corresponding new property to process.release and include
it in report and trace events output.

Fixes: nodejs/Release#437

cc @nodejs/security-release

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3463

[richardlau]

These are the build changes in core required to support nodejs/Release#437. Corresponding changes are required in the tool that builds the index files: nodejs/nodejs-dist-indexer#8

cc @nodejs/security-wg

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23003/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23004/ (✔️)

[refack]

Is there a "homolog" of such a flag in other language runtimes?

[richardlau]

Is there a "homolog" of such a flag in other language runtimes?

☝️ @bnb, as you raised the requirement.

[rvagg]

What does it mean for a node process to have an internal process.release.security===true or false? How does that model make sense for a running process? "This process is a security process", "this process only exists in its current form because it was fixing a bug in the last version of this process that was a security release". And what does it mean for a C++ compiler to encounter a macro that says "this code is a security release"? Why should it make any difference at either level?

This information doesn't relate to a process and it only relates to the codebase in a temporal sense of "this commit is better than an older commit because of <security reasons>" but even that doesn't extend into compiler or runtime land, it's just irelevant and serves no purpose. Or does it? Is there a consumption model of this that makes sense beyond metadata for the generation of index.json and index.tab?

I don't mind the idea of being able to report it in index.json/index.tab though.

Here's two alternatives:

1. Adjust dist-indexer to look at the commit message for the release tag. We've always had a lose requirement for including text that indicates a security release. I think all of mine start with "This is a security release." (matching "security release" isn't quite good enough unfortunately). That could just be codified into release docs.

2. Use existing information available in https://github.com/nodejs/security-wg/blob/master/vuln/core/ to figure this out. I've been making a point of being very clear with version information in all of the most recent additions so you can tell what versions are impacted.

e.g. from 59 for CVE-2018-12123

  "vulnerable": "6 || 8 || 10 || 11",
  "patched": "^6.15.0 || ^8.14.0 || ^10.14.0 || ^11.3.0",

6.15.0, 8.14.0, 10.14.0, 11.3.0 are all clearly security releases.

The trick is connecting this with the consumer. I've been inclined to say that it's up to consumers to put these pieces together, and ideally downstream packagers and service providers to provide a nice format for it (e.g. Snyk saying "hey, you're running a vulnerable version of Node" by having this information available). It's true that in the past, those vuln/core JSON files haven't been put in place until after releases were out, so it wouldn't have been possible for index.json and index.tab to have accurate information at that moment (it could be made accurate with a later re-run). But maybe that's a matter of process too.

[richardlau]

Adjust dist-indexer to look at the commit message for the release tag. We've always had a lose requirement for including text that indicates a security release. I think all of mine start with "This is a security release." (matching "security release" isn't quite good enough unfortunately). That could just be codified into release docs.

@rvagg I've just submitted nodejs/nodejs-dist-indexer#9 that does this. If that's the preferred approach we can close this PR and just submit a release docs update to this repository to codify the "This is a security release." in commit message convention.

[jasnell]

I'm -1 on adding a runtime property here because the value is likely limited and there is a non-zero chance of it being used in less-than-desirable ways.

[Trott]

I share the concerns raised by others, and I am especially concerned that this will be routinely misunderstood as "This process is running in some sort of extra-secure mode" like a jailed process or something.

I'm going to go ahead and close this out since it seems like your alternative approach is likely to happen and this is unlikely to happen. If you think I'm acting too soon, by all means, re-open it. (I'm just taking a break from something else to do some traiging/landing for a few minutes, and this is the thing I'm choosing to close out.)

[richardlau]

I share the concerns raised by others, and I am especially concerned that this will be routinely misunderstood as "This process is running in some sort of extra-secure mode" like a jailed process or something.

I'm going to go ahead and close this out since it seems like your alternative approach is likely to happen and this is unlikely to happen. If you think I'm acting too soon, by all means, re-open it. (I'm just taking a break from something else to do some traiging/landing for a few minutes, and this is the thing I'm choosing to close out.)

No worries. This PR has served it purpose, which was to try to get nodejs/Release#437 moving to some form of resolution.

I've opened #27643 as a counterpart to nodejs/nodejs-dist-indexer#9.