Skip to content

src: hide kMaxDigestMultiplier outside HKDF impl#46206

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
tniessen:hkdf-kmaxdigestmultiplier
Jan 16, 2023
Merged

src: hide kMaxDigestMultiplier outside HKDF impl#46206
nodejs-github-bot merged 1 commit intonodejs:mainfrom
tniessen:hkdf-kmaxdigestmultiplier

Conversation

@tniessen
Copy link
Member

@tniessen tniessen commented Jan 14, 2023

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

nodejs-github-bot commented Jan 14, 2023

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 tniessen force-pushed the hkdf-kmaxdigestmultiplier branch from 229b1e1 to 364d675 Compare January 14, 2023 12:06
@tniessen
Copy link
Member Author

tniessen commented Jan 14, 2023

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

nodejs-github-bot commented Jan 14, 2023

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

@anonrig anonrig Jan 14, 2023

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
Loading

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

nodejs-github-bot commented Jan 16, 2023

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.

5 participants

@tniessen @nodejs-github-bot @panva @bnoordhuis @anonrig