Skip to content

crypto: add KMAC Web Cryptography algorithms #59647

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 6, 2025
Merged

crypto: add KMAC Web Cryptography algorithms #59647

merged 3 commits into from
Sep 6, 2025

Conversation

panva
Copy link
Member

@panva panva commented Aug 27, 2025

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/web-infra

@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. labels Aug 27, 2025
@panva panva added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. experimental Issues and PRs related to experimental features. webcrypto dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Aug 27, 2025
@panva panva force-pushed the webcrypto-kmac branch 5 times, most recently from 5e3754e to f227d31 Compare August 27, 2025 14:39
@panva panva requested review from jasnell and tniessen August 27, 2025 15:23
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov Codecov
Copy link

codecov bot commented Aug 27, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 82.37082% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.92%. Comparing base (6428e2e) to head (7c0c52a).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_kmac.cc 56.77% 30 Missing and 21 partials ⚠️
lib/internal/crypto/mac.js 95.12% 4 Missing ⚠️
src/crypto/crypto_kmac.h 33.33% 2 Missing ⚠️
lib/internal/crypto/webidl.js 98.38% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59647      +/-   ##
==========================================
- Coverage   89.95%   89.92%   -0.03%     
==========================================
  Files         667      669       +2     
  Lines      196813   197473     +660     
  Branches    38425    38594     +169     
==========================================
+ Hits       177038   177584     +546     
- Misses      12200    12282      +82     
- Partials     7575     7607      +32
Files with missing lines Coverage Δ
lib/internal/crypto/keys.js 95.21% <100.00%> (+0.01%) ⬆️
lib/internal/crypto/util.js 95.43% <100.00%> (+0.13%) ⬆️
lib/internal/crypto/webcrypto.js 96.30% <100.00%> (+0.06%) ⬆️
src/node_crypto.cc 81.81% <ø> (ø)
lib/internal/crypto/webidl.js 98.76% <98.38%> (-0.09%) ⬇️
src/crypto/crypto_kmac.h 33.33% <33.33%> (ø)
lib/internal/crypto/mac.js 92.56% <95.12%> (+0.56%) ⬆️
src/crypto/crypto_kmac.cc 56.77% <56.77%> (ø)

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@panva panva added the review wanted PRs that need reviews. label Aug 31, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@panva
Copy link
Member Author

panva commented Sep 5, 2025

cc @nodejs/crypto @nodejs/security-wg please review

addaleax
addaleax reviewed Sep 5, 2025
View reviewed changes
deps/ncrypto/ncrypto.h
private:
DeleteFnPtr<EVP_MAC_CTX, EVP_MAC_CTX_free> ctx_;
};
#endif // OPENSSL_VERSION_MAJOR >= 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really feels like these pointer classes should become C++ templates at some point 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's been a long standing todo to get back to clean that up but haven't been able to get to it. They 100% should become templates.

@panva
Copy link
Member Author

panva commented Sep 5, 2025

@addaleax thank you, I believe I've addressed what's addressable, see the fixup! commit

addaleax
addaleax approved these changes Sep 5, 2025
View reviewed changes
@panva @addaleax
Update src/crypto/crypto_kmac.cc
7c0c52a 
Co-authored-by: Anna Henningsen <[email protected]>
@panva panva added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 6, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2025
@nodejs-github-bot nodejs-github-bot merged commit 14c68e3 into nodejs:main Sep 6, 2025
64 of 65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 14c68e3

@panva panva deleted the webcrypto-kmac branch September 6, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. 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. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@panva @nodejs-github-bot @jasnell @addaleax