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.

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

src: hide kMaxDigestMultiplier outside HKDF impl #46206

Merged
merged 1 commit into from Jan 16, 2023
Merged

src: hide kMaxDigestMultiplier outside HKDF impl #46206

merged 1 commit into from Jan 16, 2023

Conversation

tniessen
Copy link
Member

There is no reason to expose this constant outside of the HKDF implementation, especially with such a generic name.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jan 14, 2023
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
bnoordhuis
bnoordhuis approved these changes Jan 14, 2023
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but perhaps put it in HKDFTraits or even HKDFTraits::AdditionalConfig()?

(I also wonder whether it's correct. Length can't be > digest size * 255? Why? Not self-evident.)

@panva
Copy link
Member

panva commented Jan 14, 2023

I also wonder whether it's correct.

It may remain not self-evident but it is as per RFC 5869

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
src: hide kMaxDigestMultiplier outside HKDF impl
364d675 
There is no reason to expose this constant outside of the HKDF
implementation, especially with such a generic name.
@tniessen
Copy link
Member Author

LGTM but perhaps put it in HKDFTraits or even HKDFTraits::AdditionalConfig()?

Done, thank you.

(I also wonder whether it's correct. Length can't be > digest size * 255? Why? Not self-evident.)

I added an explaining comment. The limit arises from HKDF-Expand:


   The output OKM is calculated as follows:

   N = ceil(L/HashLen)
   T = T(1) | T(2) | T(3) | ... | T(N)
   OKM = first L octets of T

   where:
   T(0) = empty string (zero length)
   T(1) = HMAC-Hash(PRK, T(0) | info | 0x01)
   T(2) = HMAC-Hash(PRK, T(1) | info | 0x02)
   T(3) = HMAC-Hash(PRK, T(2) | info | 0x03)
   ...

   (where the constant concatenated to the end of each T(n) is a
   single octet.)

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 14, 2023
@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/48987/

anonrig
anonrig approved these changes Jan 14, 2023
View reviewed changes
src/crypto/crypto_hkdf.cc
// HKDF-Expand computes up to 255 HMAC blocks, each having as many bits as the
// output of the hash function. 255 is a hard limit because HKDF appends an
// 8-bit counter to each HMAC'd message, starting at 1.
constexpr size_t kMaxDigestMultiplier = 255;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It might be better suited to make this a static, but just as suggestion.

Copy link
Member Author

@tniessen tniessen Jan 15, 2023 •
edited

Choose a reason for hiding this comment

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

It was static before and I intentionally removed the keyword. Could you elaborate on why static would be beneficial here?

My understanding is that since kMaxDigestMultiplier is not ODR-used, not making it static allows the compiler to completely eliminate any storage space for it, but I might be wrong about this.

This was referenced Jan 15, 2023
Open
Open
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2023
@nodejs-github-bot nodejs-github-bot merged commit bcc2d58 into nodejs:main Jan 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in bcc2d58

@github-actions github-actions bot mentioned this pull request Jan 17, 2023
Open
17 tasks
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@tniessen @RafaelGSS
src: hide kMaxDigestMultiplier outside HKDF impl
93bd469 
There is no reason to expose this constant outside of the HKDF
implementation, especially with such a generic name.

PR-URL: nodejs#46206
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
This was referenced Jan 18, 2023
Open
Open
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@tniessen @RafaelGSS
src: hide kMaxDigestMultiplier outside HKDF impl
9d55a1f 
There is no reason to expose this constant outside of the HKDF
implementation, especially with such a generic name.

PR-URL: #46206
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@tniessen @juanarbol
src: hide kMaxDigestMultiplier outside HKDF impl
c7abebf 
There is no reason to expose this constant outside of the HKDF
implementation, especially with such a generic name.

PR-URL: #46206
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@tniessen @juanarbol
src: hide kMaxDigestMultiplier outside HKDF impl
3ce39bb 
There is no reason to expose this constant outside of the HKDF
implementation, especially with such a generic name.

PR-URL: #46206
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panva panva panva approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tniessen @nodejs-github-bot @panva @bnoordhuis @anonrig