[PM-20578] Risk Insights report from DB #15321
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #15321 +/- ##
==========================================
- Coverage 36.86% 36.82% -0.04%
==========================================
Files 3228 3230 +2
Lines 93388 93490 +102
Branches 14056 14069 +13
==========================================
+ Hits 34426 34431 +5
- Misses 57536 57633 +97
Partials 1426 1426 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| reportContentEncryptionKey, | ||
| ); | ||
|
|
||
| const wrappedReportContentEncryptionKey = await this.encryptService.wrapSymmetricKey( |
There was a problem hiding this comment.
Comment: Nice work! I think this adequately addresses the immediate key-rotation concern from the encryption side. Thank you for addressing it!
| const reportDataInJson = JSON.parse(riskInsightsReportResponse.reportData); | ||
| const reportEncrypted = reportDataInJson.data; | ||
| const wrappedReportContentEncryptionKey = reportDataInJson.key; |
There was a problem hiding this comment.
Issue (blocking):
As per the suggestion in the other comment, we should not encode to json here, and store the key on a separate column / have it separately in the API response, to both gain type safety/validate inputs, but also to make the updates cheaper from the database perspective.
| const reportDataInJson = JSON.parse(riskInsightsReportResponse.reportData); | |
| const reportEncrypted = reportDataInJson.data; | |
| const wrappedReportContentEncryptionKey = reportDataInJson.key; | |
| const reportEncrypted = riskInsightsReportResponse.reportData; | |
| const wrappedReportContentEncryptionKey = riskInsightsReportResponse.reportKey; |
There was a problem hiding this comment.
I will implement this in the next PR. I think your point about separating the content and key to facilitate key rotation is a very good point.
| totalMembers: 0, | ||
| totalAtRiskMembers: 0, | ||
| totalApplications: 0, | ||
| totalAtRiskApplications: 0, | ||
| totalCriticalApplications: 0, |
There was a problem hiding this comment.
Question:
| totalMembers: 0, | |
| totalAtRiskMembers: 0, | |
| totalApplications: 0, | |
| totalAtRiskApplications: 0, | |
| totalCriticalApplications: 0, |
I think the server-side no longer uses them (https://github.com/bitwarden/server/blob/2af4e9ccfa7c3848ca9f4b82adf9dad01268736f/src/Core/Dirt/Reports/ReportFeatures/Requests/AddOrganizationReportRequest.cs#L5) due to the potential vulnerability reported on the previous instance of the PR.
Can we remove these? Did we otherwise address this?
There was a problem hiding this comment.
yes, this can be removed. I didn't realize this was there. This has been addressed and we are not saving a summary in the DB. The summary is a part of the encrypted data.
| const riskInsightReport = { | ||
| organizationId: organizationId, | ||
| date: new Date().toISOString(), | ||
| reportData: JSON.stringify(reportDataWithWrappedKey), |
There was a problem hiding this comment.
| reportData: JSON.stringify(reportDataWithWrappedKey), | |
| reportData: reportEncrypted.encryptedString, | |
| reportKey: wrappedReportContentEncryptionKey.encryptedString, |
Issue (blocking):
On ciphers, we store the key in a separate column. See:
We should:
Change this to not stingify "reportDataWithWrappedKey". The request should probably look as follows:
{
organizationId: organizationId,
date: new Date().toISOString(),
reportData: reportEncrypted.encryptedString,
reportKey: wrappedReportContentEncryptionKey.encryptedString,
...
}
the server-side should validate both EncStrings in the request model. Here is how we do that for ciphers for example:
https://github.com/bitwarden/server/blob/2af4e9ccfa7c3848ca9f4b82adf9dad01268736f/src/Api/Vault/Models/Request/CipherRequestModel.cs#L29
This improves API type safety because the encstrings are validated.
Additionally, when KM gets towards implementing key-rotation for organizations, we need to re-encrypt all wrappedReportContentEncryptionKey entries. While this now no longer requires sending over the re-encrypted reportData over the network, this, as currently designed, still requires the server to read and parse the stored JSON, and replace the key in it. Having it on a separate column means only the column is replaced.
Especially when we get to larger organizations, and them having to rotate many (hundreds or thousands? of reports), each of which are 60KB+ in the sample I have seen, we cannot read all JSON entries from the database, so moving the key to as separate column is blocking.
So, to summarize, we need to split out the key in the request model to a separate field, and on the server side add a new column for the key.
There was a problem hiding this comment.
We will implement this. This makes a lot of sense.
Banrion
changed the base branch from
main
to
dirt/feature/risk-insights-saving-reports
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-20578
📔 Objective
Persist the Risk insights report to the database and retrieve from DB, unless, there is no data in the DB or there is a specific request to refresh.
📸 Screenshots
Default report - retrieve from DB
Report after the
Refreshwas requested⏰ 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