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

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

Closed
wants to merge 2 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Jan 18, 2023

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot 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 Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jan 18, 2023
panva added a commit to panva/node that referenced this pull request Jan 18, 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: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
@panva 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 panva marked this pull request as ready for review January 18, 2023 15:04
@panva 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 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
Copy link
Member Author

panva commented Jan 19, 2023

cc @nodejs/lts

@BethGriggs
Copy link
Member

BethGriggs commented Jan 19, 2023

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
Copy link
Member Author

panva commented Jan 19, 2023

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
Copy link
Member Author

panva commented Jan 19, 2023

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.

@richardlau
Copy link
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
Copy link
Member Author

panva commented Jan 19, 2023

Yes, that is okay!

@GeoffreyBooth
Copy link
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
Copy link
Member Author

panva commented Jan 19, 2023

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
Copy link
Member

jasnell commented Jan 20, 2023

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

@GeoffreyBooth
Copy link
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.

@GeoffreyBooth
Copy link
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
Copy link
Member Author

panva commented Jan 20, 2023

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
Copy link
Member Author

panva commented Jan 20, 2023

@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 added a commit to panva/node that referenced this pull request Feb 2, 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: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
@panva panva marked this pull request as draft March 5, 2023 08:09
panva added a commit to panva/node that referenced this pull request Mar 5, 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: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
panva added a commit to panva/node that referenced this pull request Mar 5, 2023
PR-URL: nodejs#46563
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#46252
panva added a commit to panva/node that referenced this pull request Mar 8, 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: nodejs#46067
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Backport-PR-URL: nodejs#46252
panva added a commit to panva/node that referenced this pull request Mar 8, 2023
PR-URL: nodejs#46563
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#46252
@panva panva added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 3, 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: 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 panva removed the blocked PRs that are blocked by other issues or PRs. label Apr 18, 2023
@panva panva marked this pull request as ready for review April 18, 2023 17:30
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@panva
Copy link
Member Author

panva commented Apr 19, 2023

cc @nodejs/backporters barring any v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. issues (that I'll be on the lookout for) this is now ready.

@panva 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
@danielleadams
Copy link
Contributor

Landed in 5dc0fee...075d9ee

@panva panva deleted the backport-46067-to-v18.x branch May 17, 2023 03:29
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. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants