[Backport 1.x] Bumps node-forge from v0.10.0 to v1.3.0

[ZilongX]

Signed-off-by: Zilong Xia zilongx@amazon.com

Description

• Resolves CVE-2022-24771, CVE-2022-24772, CVE-2022-24773, CVE-2022-0122 by bumping package node-forge up from v0.10.0 to v1.3.0

• This PR is to backport same fixes applied to branch 2.0.0 back to branch 1.x, which is a combination of

• • Upgrades node-forge from v0.10.0 to v1.2.1 #1239
• • Bumps node-forge from v1.2.1 to v1.3.0 #1369

• Based on https://github.com/digitalbazaar/forge/blob/v1.2.1/CHANGELOG.md of package node-forge, there are breaking changes introduced in version v1.0.0 yet it has been confirmed that none of them apply to Dashboards repository.
  (Upgrades node-forge from v0.10.0 to v1.2.1 #1239)

• Upgrades @elastic/request-crypto from 1.1.4 to 2.0.0 which has a downstream dependency on node-forge.

• • @elastic/request-crypto uses node-jose@2.0.0 which still depends on node-forge@0.10.0 so we need a manual resolution for node-jose@2.1.0.

Issues Resolved

Resolves #1112
Resolves #1134
Resolves #1365
Resolves #1366
Resolves #1367

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

[joshuarrrr]

there are breaking changes introduced in version v1.0.0 yet it has been confirmed that none of them apply to Dashboards repository.

Unfortunately, that's not sufficient under our current definition of semantic versioning. Because of the current shared dependency model, plugins are allowed to treat explicit package.json dependencies as a defacto public API - they can assume that the dependencies defined there (and their methods) can be used directly. (We don't want things to work that way, and are working to change it!)

Because v1.0.0 of node-forge has breaking changes (of removed and changed APIs), we can't backport it to a minor or patch version branch (1.x, in this case). There could be community or private plugins targeting 1.3 versions of OSD that directly use deprecated node-forge methods that would be broken by this change.

[ZilongX]

there are breaking changes introduced in version v1.0.0 yet it has been confirmed that none of them apply to Dashboards repository.

Unfortunately, that's not sufficient under our current definition of semantic versioning. Because of the current shared dependency model, plugins are allowed to treat explicit package.json dependencies as a defacto public API - they can assume that the dependencies defined there (and their methods) can be used directly. (We don't want things to work that way, and are working to change it!)

Because v1.0.0 of node-forge has breaking changes (of removed and changed APIs), we can't backport it to a minor or patch version branch (1.x, in this case). There could be community or private plugins targeting 1.3 versions of OSD that directly use deprecated node-forge methods that would be broken by this change.

@joshuarrrr

So I just did a quick search (https://github.com/search?q=org%3Aopensearch-project+node-forge&type=code) and confirmed that (at least) within current org the node-forge is only consumed by OpenSearch-Dashboards so there won't be any breaking chains in OSD.

Speaking of community or private plugins, do you have any real examples of such cases for node-forge or just a guess in general ? Since we have a bunch of customers actively consuming v1.3 and suffering from exposures to the CVEs as mentioned in this fix, this kind of rejection without proof does not make sense to us.

[ashwin-pc]

@ZilongX I dont think the callout is about whether or not someone uses the dependency, but whether we want to stick to the definition of semver or not. Given the tight coupling that exists today between OSD and plugins when it comes to dependencies, we have to treat any direct dependency upgrade that changes its api as a breaking change since we cannot guarantee that a plugin for OSD will not depend on that api. And changing that api is technically a breaking change. Since one of our org tenants is to follow semver we need to apply that bar here too.

This should no longer be an issue once we decouple the tight dependency between plugins and OSD for direct dependencies, but as it stands right now, thats not the case unfortunately.

[joshuarrrr]

Speaking of community or private plugins, do you have any real examples of such cases for node-forge or just a guess in general ?

No, given our extensibility model it's impossible to know or asses all consumers of the package to see how they use it. You're correct that it's nothing specific to this package; rather it's a conservative approach that we've taken to semantic versioning in general, which optimizes for preventing surprising breakages.

[seraphjiang]

https://semver.org/#if-even-the-tiniest-backwards-incompatible-changes-to-the-public-api-require-a-major-version-bump-wont-i-end-up-at-version-4200-very-rapidly

Do we follow semver strictly today?

if so, should we bump up major version not minor if any dependencies has break change even we are not impacted?

[ashwin-pc]

@seraphjiang AFAIK yes we do. Thats why we have not backported similar changes and had to wait until 2.0. e.g. node 14 upgrade

[seraphjiang]

@seraphjiang AFAIK yes we do. Thats why we have not backported similar changes and had to wait until 2.0. e.g. node 14 upgrade

Thanks @ashwin-pc for clarification.

I have a question, i'd like to discuss. whether we should treat downstream dependency break change as our own break change.
IMHO, if dependencies break change doesn't impact our's compatibility, we don't treat it as our break change.

e..g
kibana 6.0 use node 6.11.5
Kibana 6.3 use node 8.11.4
Kibana 6.6 use node 10.15.2

there are breaking change from node 6, node 8, to node 10, however, it doesn't mean it will cause upstream break change. and not necessary to bump up version just because downstream dependencies declare they has break change.

@kavilla @AMoo-Miki @mihirsoni @zengyan-amazon @ashwin-pc @joshuarrrr @dblock @seanneumann

[joshuarrrr]

kibana 6.0 use node 6.11.5
Kibana 6.3 use node 8.11.4
Kibana 6.6 use node 10.15.2

Thanks @seraphjiang - this is a good example of how our versioning approach differs from how Kibana handled versioning. Because of the reasons discussed upstream, each of those node version bumps would only go in a major OpenSearch Dashboards release, not in a minor release.

it doesn't mean it will cause upstream break change

True, but there's also no way to guarantee that it won't cause a breaking change, which is why we take the conservative approach.

[seraphjiang]

kibana 6.0 use node 6.11.5
Kibana 6.3 use node 8.11.4
Kibana 6.6 use node 10.15.2

Thanks @seraphjiang - this is a good example of how our versioning approach differs from how Kibana handled versioning. Because of the reasons discussed upstream, each of those node version bumps would only go in a major OpenSearch Dashboards release, not in a minor release.

it doesn't mean it will cause upstream break change

True, but there's also no way to guarantee that it won't cause a breaking change, which is why we take the conservative approach.

if there is no break change to customer, there is not necessary to bump up major. Node version bump should not be the reason to bump up OSD version.

as far as i know, we run bwc test, if that test works, means there is no break change in OSD itself.

I'm OK with what already happened, but in the future we should not bump up major version because dependencies has break change.

[kavilla]

Closing for now as this seems to break semver for developers. Please re-open if any concerns.