Skip to content

PM-32077 - Add override to KDF iterations from RustSdk for Seeder#7225

Merged
theMickster merged 5 commits intomainfrom
seeder/resolve-kdf-iterations-bug
Mar 17, 2026
Merged

PM-32077 - Add override to KDF iterations from RustSdk for Seeder#7225
theMickster merged 5 commits intomainfrom
seeder/resolve-kdf-iterations-bug

Conversation

@theMickster
Copy link
Contributor

@theMickster theMickster commented Mar 16, 2026

🎟️ Tracking

Fix KDF Iterations to Meet Minimum Threshold

📔 Objective

Update the Seeder to allow a user to override the 5000 KDF Iterations.
The new option allows for a user to set higher thresholds while still allowing for our automated tests to function as they did before.

🧪 Testing

CLI Override Verification (50 users, 500 ciphers)

# 600k iterations
time dotnet run -- organization -n E2eOrg -d e2e.example -u 50 -c 500 --kdf-iterations 600000 --mangle
# 29.410 total

# 100k iterations
time dotnet run -- organization -n E2eOrg -d e2e.example -u 50 -c 500 --kdf-iterations 100000 --mangle
# 28.712 total

# Default (5k iterations)
time dotnet run -- organization -n E2eOrg -d e2e.example -u 50 -c 500 --mangle
# 32.212 total
--kdf-iterations DB KdfIterations Users Wall Clock
600000 600,000 48 29.4s
100000 100,000 48 28.7s
(default 5k) 5,000 48 32.2s

KDF iteration count has zero measurable impact on seeding performance on my local machine (Claude estimates that wall-clock time is dominated by RSA keypair generation and MSSQL bulk insert, not PBKDF2).

Going from 5,000 → 600,000 iterations (120x increase) adds no detectable time even at 50 users.

@theMickster theMickster requested a review from a team as a code owner March 16, 2026 10:49
@theMickster theMickster added the ai-review Request a Claude code review label Mar 16, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

Logo
Checkmarx One – Scan Summary & Details0f08c32c-2923-450a-bfb9-866811e8c1a4

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.53%. Comparing base (e5fec6a) to head (1c63785).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7225      +/-   ##
==========================================
- Coverage   61.58%   57.53%   -4.06%     
==========================================
  Files        2032     2032              
  Lines       89476    89476              
  Branches     7957     7957              
==========================================
- Hits        55108    51478    -3630     
- Misses      32441    36153    +3712     
+ Partials     1927     1845      -82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

We use 5k iterations rather than 600k due to the inherit slowness of PBKDF. If you seed large data sets with a high iteration count it will be very slow. This is the same reason we don't generate private keys.

If you want this, we need to ensure existing tests like the large organization performance test still have ways to run efficiently.

@theMickster
Copy link
Contributor Author

theMickster commented Mar 16, 2026

We use 5k iterations rather than 600k due to the inherit slowness of PBKDF. If you seed large data sets with a high iteration count it will be very slow. This is the same reason we don't generate private keys.

If you want this, we need to ensure existing tests like the large account performance test still have ways to run efficiently.

🤔 I didn't see a terrible performance hit when testing locally which is why I raised the pull request without a configurable override. I can work to include the override as part of the entire solution.

Where is the large account performance test that you mentioned? Will you drop me a link to it?

UPDATE: Matt Gibson and I connected today about a number of Seeder items. Having the full picture of the work; we're going to invert this so we have a default of 5000 KDFs and a configurable override for higher iterations.

@theMickster theMickster marked this pull request as draft March 16, 2026 11:40
@theMickster theMickster force-pushed the seeder/resolve-kdf-iterations-bug branch from efd30de to ab7c592 Compare March 16, 2026 14:46
@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@theMickster theMickster added ai-review-vnext and removed ai-review Request a Claude code review labels Mar 16, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

Code Review: PR #7225

Overall Assessment: APPROVE

This is a re-review after commit 1c63785b5 ("Add argument validation to the Seeder CLI"). The previous review flagged one IMPORTANT finding: missing minimum validation for --kdf-iterations in the seed command (SeedArgs.cs). This commit directly addresses that finding by adding the validation, matching the existing validation in OrganizationArgs.cs.

All prior findings are resolved. No new findings.

The full PR cleanly threads a configurable kdfIterations parameter through:

  • CLI layer (OrganizationArgs.cs, SeedArgs.cs) with >= 5000 validation
  • Preset system (SeedPreset.cs, preset.schema.json) with JSON schema bounds (5000-1000000)
  • Pipeline DI (SeederSettings, SeederContextExtensions.cs, RecipeOrchestrator.cs)
  • Step execution (CreateOwnerStep.cs, CreateRosterStep.cs, CreateUsersStep.cs)
  • Factory (UserSeeder.cs)
  • Rust FFI (RustSdkService.cs, lib.rs) with zero-value guard

@theMickster theMickster changed the title PM-32077 - Remove hardcoded KDF iterations from RustSdk for Seeder PM-32077 - Add override to KDF iterations from RustSdk for Seeder Mar 16, 2026
@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Overall Assessment: APPROVE

Re-reviewed after commit 1c63785 which adds the previously flagged KDF iteration validation to the seed command. The fix correctly mirrors the organization command validation, guarding against values below 5000 when explicitly provided. All prior findings are resolved.

Code Review Details

No outstanding findings. The PR cleanly threads a configurable kdfIterations parameter from CLI args through the preset system, pipeline DI, step execution, and Rust FFI layer. Validation is consistent across both CLI entry points (organization and seed commands), the JSON schema enforces bounds for preset files, and the Rust layer gracefully handles zero-iteration input.

@bitwarden bitwarden deleted a comment from claude bot Mar 16, 2026
@sonarqubecloud
Copy link

@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Overall Assessment: APPROVE

Re-reviewed after commit 1c63785 which adds the previously flagged KDF iteration validation to the seed command. The fix correctly mirrors the organization command validation, guarding against values below 5000 when explicitly provided. All prior findings are resolved.

Code Review Details

No outstanding findings. The PR cleanly threads a configurable kdfIterations parameter from CLI args through the preset system, pipeline DI, step execution, and Rust FFI layer. Validation is consistent across both CLI entry points (organization and seed commands), the JSON schema enforces bounds for preset files, and the Rust layer gracefully handles zero-iteration input.

@theMickster theMickster requested a review from MGibson1 March 16, 2026 18:19
@theMickster theMickster marked this pull request as ready for review March 16, 2026 18:19
UserKeys? keys = null,
string? password = null)
string? password = null,
int kdfIterations = 5_000)
Copy link
Member

Choose a reason for hiding this comment

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

This method really needs xml docs to help unwrap parameter actions. With this latest, we need to call out that if keys are provided, the kdfIterations must match whatever generated those keys or nothing will decrypt properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed; I will replace the input params with one of the POCO Dtos this week

throw new ArgumentException("--preset must be specified. Use --list to see available presets.");
}

if (KdfIterations.HasValue && KdfIterations.Value < 5_000)
Copy link
Member

Choose a reason for hiding this comment

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

technically there's a maximum as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it approaching infinity ;-)

@Hinton
Copy link
Member

Hinton commented Mar 17, 2026

The performance test is OrganizationUsersControllerPerformanceTests. We've run it with upwards of 80k users and there is a significant difference between 600k and 5k iterations.

@theMickster theMickster merged commit cfdd6df into main Mar 17, 2026
79 checks passed
@theMickster theMickster deleted the seeder/resolve-kdf-iterations-bug branch March 17, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants