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

Introduce configurable pbkdf2 iteration limit #1471

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 4, 2023

Introduces the ability for workerd embedders to configure the max iteration limit for KDFs. Removes the limit for workerd by default. Keeps the current limit of 100,000 when not configured.

Refs: #1346

@jasnell jasnell requested review from a team as code owners December 4, 2023 22:33
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Will this be configurable in the workerd configuration? Want to make sure whatever limit ends up being set in production is replicated locally, so tests/development fail early rather than when users deploy their Workers.

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2023

Not currently. That's a good idea tho. The LimitEnforcer implementation that is used by default in workerd is a non-op currently with none of the production-enforced limits in place. It wouldn't be that difficult to make it something someone could configure tho. That said, I think that's a separate PR.

@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch 2 times, most recently from 36ceaae to 974e3cb Compare December 5, 2023 22:59
src/workerd/jsg/limits.h Outdated Show resolved Hide resolved
src/workerd/jsg/limits.h Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch from 974e3cb to 6980ce0 Compare December 8, 2023 22:02
@jasnell
Copy link
Member Author

jasnell commented Dec 8, 2023

Updated to:

  • rename the check to make it specific to pbkdf2
  • move the limit check off jsg::IsolateLimitEnforcer. That type is still in place to make it easy to access the limit enforcer without needing to have an IoContext... so the configurable limit can be enforced easily when we're not in an io context.

src/workerd/jsg/limits.h Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch 3 times, most recently from 804c596 to c5b4190 Compare December 15, 2023 21:32
src/workerd/jsg/setup.c++ Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch 2 times, most recently from 7f5bd89 to 3d9b90b Compare December 18, 2023 15:01
src/workerd/jsg/jsg.h Outdated Show resolved Hide resolved
src/workerd/io/worker.h Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch 3 times, most recently from f321bb2 to b5bbf5a Compare December 21, 2023 02:19
Introduces the ability for workerd embedders to configure the
max iteration limit for KDFs. Removes the limit for workerd by
default. Keeps the current limit of 100,000 when not configured.
@jasnell jasnell force-pushed the jsnell/configurable-pbkdf2-iteration-limit branch from b5bbf5a to 12bc98a Compare December 21, 2023 02:20
@@ -1251,6 +1255,12 @@ Worker::Script::~Script() noexcept(false) {
impl = nullptr;
}

const Worker::Isolate& Worker::Isolate::from(jsg::Lock& js) {
auto ptr = js.v8Isolate->GetData(3);
Copy link
Member

Choose a reason for hiding this comment

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

We should define an enum somewhere of our embedder data indices so we don't accidentally reuse noe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Already had a note to introduce that in a separate PR

@jasnell jasnell merged commit 650c09b into main Dec 23, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants