FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v18.x backport] crypto: use WebIDL converters in WebCryptoAPI by panva · Pull Request #46252 · nodejs/node · GitHub

/ node Public

[v18.x backport] crypto: use WebIDL converters in WebCryptoAPI - #46252

Closed
panva wants to merge 2 commits into
nodejs:v18.x-stagingfrom
panva:backport-46067-to-v18.x
Closed

[v18.x backport] crypto: use WebIDL converters in WebCryptoAPI#46252
panva wants to merge 2 commits into
nodejs:v18.x-stagingfrom
panva:backport-46067-to-v18.x

Conversation

panva commented Jan 18, 2023
edited
Loading

Copy link
Copy Markdown
Member

Backports #46067 (and its followup #46563), which is semver-major PRs that contain breaking changes and should be released in the next major version. chilling in main until the 20.0.0 release in April, to v18.x where WebCryptoAPI is still experimental Issues and PRs related to experimental features. .

This backport is in a bit of a unique situation. Between the collaborators I've talked to it is a coin toss whether this can be backported right away or when it first lands in a current 20.0.0, some think that due to the experimental stability of WebCryptoAPI on 18.x this is fair game straight away, some think it should still be released on current before being backported to LTS. Noone has suggested this cannot be backported, the question only seems to be when to do it. The PR improves interoperability with other Web Crypto API implementations.

/discuss - blocked PRs that are blocked by other issues or PRs. until we have a consensus.


blocked PRs that are blocked by other issues or PRs. , waiting with the backport until the changes land on a v20.x release

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x labels Jan 18, 2023
panva force-pushed the backport-46067-to-v18.x branch from 982fd69 to b1aa264 Compare January 18, 2023 15:03
panva added crypto Issues and PRs related to the crypto subsystem. webcrypto experimental Issues and PRs related to experimental features. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 18, 2023
panva marked this pull request as ready for review January 18, 2023 15:04
panva added blocked PRs that are blocked by other issues or PRs. notable-change PRs with changes that should be highlighted in changelogs. labels Jan 18, 2023
panva changed the title [v18.x] crypto: use WebIDL converters in WebCryptoAPI [v18.x backport] crypto: use WebIDL converters in WebCryptoAPI Jan 19, 2023

panva commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/lts

BethGriggs commented Jan 19, 2023
edited
Loading

Copy link
Copy Markdown
Member

As it's experimental, we can land breaking changes in an existing release line if we feel it's appropriate. I think it would be good to honour the typical 'land in a Current release before backporting to LTS' flow, though. That would give some feedback just in case it had unexpected impact.

I'd suggest opening a backport to land it in v19.x-staging, which would hopefully ship in an upcoming Node.js 19 release (even this week, if there's consensus). If there are no concerns after it has been released in 19, we could decide to backport and ship in subsequent 18 release.

panva commented Jan 19, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

I'd suggest opening a backport to land it in v19.x-staging, which would hopefully ship in an upcoming Node.js 19 release (even this week, if there's consensus). If there are no concerns after it has been released in 19, we could decide to backport and ship in subsequent 18 release.

We've declared WebCryptoAPI stable from 19.x forward (because it's finally global as it should be) which is why this PR is waiting for 20.x to be part of a release.

I am confident the risk of user code breakage would be minimal even in 19.x but we have the semver major rules for a reason.

panva commented Jan 19, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

As it's experimental, we can land breaking changes in an existing release line if we feel it's appropriate. I think it would be good to honour the typical 'land in a Current release before backporting to LTS' flow, though. That would give some feedback just in case it had unexpected impact.

So we'd keep this backport PR blocked PRs that are blocked by other issues or PRs. until april when the first 20.x goes out, then have it part of the next 18.x active lts release.

Copy link
Copy Markdown
Member

As @BethGriggs mentions, our default position is that things should go out in a current release first before landing on LTS. That is not an absolute rule but does mean that arguments would need to be made to justify deviating from the default position.

As it's experimental, we can land breaking changes in an existing release line if we feel it's appropriate. I think it would be good to honour the typical 'land in a Current release before backporting to LTS' flow, though. That would give some feedback just in case it had unexpected impact.

So we'd keep this backport PR blocked PRs that are blocked by other issues or PRs. until april when the first 20.x goes out, then have it part of the next 18.x active lts release.

If you're okay with waiting this would be my preferred option for LTS.

panva commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

Yes, that is okay!

Copy link
Copy Markdown
Member

This is only even a question because the PR that marked WebCryptoAPI as stable hasn’t been backported. Why hasn’t it been? It’s weird to keep an API experimental in an older release line when it’s transitioned to stable in the current line. We really shouldn’t be in a position to be able to ship breaking changes on an older release line but not the current one.

panva commented Jan 19, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

This is only even a question because the PR that marked WebCryptoAPI as stable hasn’t been backported. Why hasn’t it been?

That I can explain. The reason is that we aligned Web Crypto API being stable with it also being available as a global (i.e. how it should be obtained per spec), that happened in 19.x and cannot be backported.

jasnell commented Jan 20, 2023
edited
Loading

Copy link
Copy Markdown
Member

@BethGriggs

As it's experimental, we can land breaking changes in an existing release line if we feel it's appropriate.

100%

Also, just to expand on the point a bit. If we have an experimental feature in release X that graduates to stable in release X+1, the feature still remains experimental in release X indefinitely and is always subject to the same semver-immunity rules unless the decision is made to also graduate the feature to stable in that release line.

It’s weird to keep an API experimental in an older release line when it’s transitioned to stable in the current line. We really shouldn’t be in a position to be able to ship breaking changes on an older release line but not the current one.

I don't consider this weird at all. For instance, hypothetically if we found a major security bug in a stable API that was still experimental in an older release line we could easily decide to only fix the issue in the current stable line and simple remove the experimental API in the older line to make things easier. We could (and have) graduated experimental features in older release lines but there's absolutely nothing that says we should.

Copy link
Copy Markdown
Member

It’s weird to keep an API experimental in an older release line when it’s transitioned to stable in the current line. We really shouldn’t be in a position to be able to ship breaking changes on an older release line but not the current one.

I don’t consider this weird at all. For instance, hypothetically if we found a major security bug in a stable API that was still experimental in an older release line we could easily decide to only fix the issue in the current stable line and simple remove the experimental API in the older line to make things easier. We could (and have) graduated experimental features in older release lines but there’s absolutely nothing that says we should.

What I think is weird is that from the user’s perspective, features should progress in the same order across lines. So if an API lands enhancements A, B and C in 19.x, the logical expectation is that the backports would be A, B and C, and in that order, in the older lines. It’s not wrong to do otherwise, but it’s unusual and the deviation from common practice could lead to bugs. For example, someone writing a version check for this API will need to write something like “if version > 18.10.0 or version > 20.0.0” and just skip 19, which looks wrong and could lead to confusion in a code review. Or worse, if a later feature lands, users might assume that an earlier feature is already also present when it’s missing from that release line. It’s just a potential landmine for users if they’re not careful and don’t read the docs closely enough.

I don’t think this is a big enough worry to not backport, if we think that the backport is very valuable, but I think we should default to the pattern of enhancements landing in sequence and never having something “skip” a release line like we’d be skipping 19 here. That should be the normal practice, and doing otherwise would be the exception that we should try to do rarely, when the feature is really valuable; and the docs should emphasize the deviation from typical practice, noting that a particular feature might be in 18 and 20 but not 19, and that this isn’t a mistake or bug.

Copy link
Copy Markdown
Member

The reason is that we aligned Web Crypto API being stable with it also being available as a global (i.e. how it should be obtained per spec), that happened in 19.x and cannot be backported.

So making the API available as a global would be a breaking change for 18? Adding a new global is semver-major?

panva commented Jan 20, 2023

Copy link
Copy Markdown
Member Author

The reason is that we aligned Web Crypto API being stable with it also being available as a global (i.e. how it should be obtained per spec), that happened in 19.x and cannot be backported.

So making the API available as a global would be a breaking change for 18? Adding a new global is semver-major?

I'm not sure in general but in this case, yes, because the crypto global conflicted with REPL's crypto being the node:crypto module, same for -p print and -e eval node options. The risk of breaking user tooling was considerable.

panva commented Jan 20, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

@GeoffreyBooth When odd releases reach EOL it is the case for odd-1 LTS backports to skip the odd-EOL.

When 20.x gets out the door 19.x goes into Maintenance for a few months until it is EOL. When 19.x reaches EOL 20.x will still be Current and at the same 18.x will still be Active LTS.

What I mean to say is that this skip would be "natural" if a change (not a major one) came after 19.x EOL but before 20.x became LTS.

I do think this backport is valuable, with 18 LTS having a proper LTS lifespan it does make sense to me that down the line, 18 and 20 behave the same in this regard (omitting the difference in how the Web Crypto API module is obtained). If that happens right when 20.x gets released, now, or when 19.x is EOL doesn't make that big of a difference to me.

panva force-pushed the backport-46067-to-v18.x branch from 0303a54 to c12ff2c Compare March 8, 2023 07:52
panva added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 3, 2023
panva added 2 commits April 18, 2023 14:42
WebCryptoAPI functions' arguments are now coersed and validated as per
their WebIDL definitions like in other Web Crypto API implementations.
This further improves interoperability with other implementations of
Web Crypto API.

PR-URL: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
PR-URL: nodejs#46563
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#46252
panva force-pushed the backport-46067-to-v18.x branch from c12ff2c to b703389 Compare April 18, 2023 12:42
panva removed the blocked PRs that are blocked by other issues or PRs. label Apr 18, 2023
panva marked this pull request as ready for review April 18, 2023 17:30
panva added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

panva commented Apr 19, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/backporters barring any https://github.com/nodejs/node/labels/v20%2Ex issues (that I'll be on the lookout for) this is now ready.

panva removed the notable-change PRs with changes that should be highlighted in changelogs. label May 15, 2023
danielleadams pushed a commit that referenced this pull request May 17, 2023
WebCryptoAPI functions' arguments are now coersed and validated as per
their WebIDL definitions like in other Web Crypto API implementations.
This further improves interoperability with other implementations of
Web Crypto API.

PR-URL: #46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: #46252
danielleadams pushed a commit that referenced this pull request May 17, 2023
PR-URL: #46563
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #46252

Copy link
Copy Markdown
Contributor

Landed in 5dc0fee...075d9ee

panva deleted the backport-46067-to-v18.x branch May 17, 2023 03:29
aduh95 pushed a commit to aduh95/node that referenced this pull request Feb 18, 2025
WebCryptoAPI functions' arguments are now coersed and validated as per
their WebIDL definitions like in other Web Crypto API implementations.
This further improves interoperability with other implementations of
Web Crypto API.

PR-URL: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
aduh95 pushed a commit to aduh95/node that referenced this pull request Feb 18, 2025
PR-URL: nodejs#46563
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#46252
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. webcrypto

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL