[PM-21075] Add crypto abstraction layer for database seeder #6114
Conversation
- Introduce ISeederCryptoService interface for crypto operations - Implement C# crypto service with proper Bitwarden encryption - Add RustSeederCryptoService stub for future SDK integration - Fix authentication by implementing correct double-hashing - Add vault item seeding capabilities - Update all recipes to use dependency injection - Add feature flag for implementation switching - Comment out RustSdk file copy operations to prevent build errors - Currently supports SQL Server and PostgreSQL
|
New Issues (17)Checkmarx found the following issues in this Pull Request
Fixed Issues (12)Great job! The following issues were fixed in this Pull Request
|
There was a problem hiding this comment.
I stopped about half-way, but there are solution-wide capabilities your AI generator wasn't aware of that replace a lot of this -- code in test and utility projects mainly to handle database configuration, data protection, and so on. This can thin out a lot and focus on the recipes utilizing the SDK. We'll need to get its invocation smoothed out too.
.gitignore
Outdated
| NativeMethods.g.cs | ||
| util/RustSdk/rust/target |
There was a problem hiding this comment.
❌ You should be able to use the same ignore you set up below I'd think.
| @@ -133,6 +133,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Seeder", "util\Seeder\Seede | |||
| EndProject | |||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DbSeederUtility", "util\DbSeederUtility\DbSeederUtility.csproj", "{17A89266-260A-4A03-81AE-C0468C6EE06E}" | |||
| EndProject | |||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "RustSdk", "util\RustSdk\RustSdk.csproj", "{D1513D90-E4F5-44A9-9121-5E46E3E4A3F7}" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
The RustSdk project was part of @Hinton's SDK integration work
| @@ -9,7 +9,7 @@ namespace Bit.Api.IntegrationTest.AdminConsole.Controllers; | |||
|
|
|||
| public class OrganizationUsersControllerPerformanceTest(ITestOutputHelper testOutputHelper) | |||
| { | |||
| [Theory(Skip = "Performance test")] | |||
| [Theory()] | |||
There was a problem hiding this comment.
❌ Not sure why this was done.
There was a problem hiding this comment.
The test did not work with the new injection, so skipped it for build
| .idea/ | ||
|
|
||
| # macOS | ||
| .DS_Store |
There was a problem hiding this comment.
⛏️ Newlines at the end of all files.
There was a problem hiding this comment.
Should be fixed now
| services.AddSingleton<IPasswordHasher<User>, PasswordHasher<User>>(); | ||
|
|
||
| // Register ISeederCryptoService with feature flag support | ||
| // TODO: When Rust SDK is ready, check configuration to switch implementations |
There was a problem hiding this comment.
❌ If we're gonna move forward with a PR like this, we are gonna use the SDK.
There was a problem hiding this comment.
Agreed, just wanted to move closer to that point with this work
| /// <summary> | ||
| /// Factory for creating configured service providers for seeder operations | ||
| /// </summary> | ||
| public static class ServiceProviderFactory |
There was a problem hiding this comment.
❓ I'm guessing this is generated? We'd want to follow patterns we use in other projects to access a database and this is wildly different.
There was a problem hiding this comment.
Was having issues with dataprotection, sort of a workaround at the moment, will look closer
There was a problem hiding this comment.
You probably don't need data protection as it's backwards compatible.
| "globalSettings": { | ||
| "databaseProvider": "postgres", | ||
| "postgreSql": { | ||
| "connectionString": "Host=localhost;Database=vault_dev;Username=postgres;Password=ComplexPassword#123;Include Error Detail=true" |
There was a problem hiding this comment.
❌ See the dev/ folder and the contributing docs for example on how we set defaults in these and then use secrets. We will want this utility to utilize all the central practices.
There was a problem hiding this comment.
Was work around also, will work on regular secrets population
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Create identities | ||
| var identities = CipherSeeder.SampleData.Identities.OrderBy(_ => random.Next()).Take(identityCount); |
Check warning
Code scanning / Checkmarx One
Use of Cryptographically Weak PRNG
| } | ||
|
|
||
| // Create secure notes | ||
| var notes = CipherSeeder.SampleData.SecureNotes.OrderBy(_ => random.Next()).Take(noteCount); |
Check warning
Code scanning / Checkmarx One
Use of Cryptographically Weak PRNG
| var random = new Random(); | ||
|
|
||
| // Create shared login items | ||
| var logins = CipherSeeder.SampleData.Logins.OrderBy(_ => random.Next()).Take(sharedLoginCount); |
Check warning
Code scanning / Checkmarx One
Use of Cryptographically Weak PRNG
- Re-add Skip attribute to performance test - Clarify SDK integration TODO comment - Clean up gitignore patterns - Add project-specific gitignore for RustSdk - Fix file formatting
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-21075
📔 Objective
This PR adds a crypto abstraction layer to the database seeder, enabling Bitwarden encryption
for seeded data and preparing for future Rust SDK integration.
Key improvements:
📝 Implementation Details
ISeederCryptoServiceinterface to abstract crypto operationsRustSeederCryptoServicestub showing one possible integration approachSeeder:UseRustSdk) for future implementation switching📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes