Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39800 +/- ##
==========================================
+ Coverage 66.26% 66.27% +0.01%
==========================================
Files 2438 2439 +1
Lines 195263 195404 +141
Branches 8538 8538
==========================================
+ Hits 129384 129511 +127
+ Misses 54165 54158 -7
- Partials 11714 11735 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ksykulev
left a comment
There was a problem hiding this comment.
Not saying this is the incorrect approach. But would it be possible to use MutateSoftwareOnIngestion?
I know we did some munging of EAP versions: #22723
We also did some grabbing of release notes for versions on mac office:
#30082 (can't grab jetbrains website/docs on every software ingest)
I haven't specifically looked at how the mapping works for jetbrains versions. I assume the reason for the additional query is because a static hardcoded list would be out of the question, and the maintenance would be a nightmare?
| `, | ||
| Platforms: []string{"windows"}, | ||
| DirectIngestFunc: directIngestSoftware, | ||
| Discovery: discoveryTable("programs"), |
There was a problem hiding this comment.
I see in the description it says Requires fleetd". I assume this is in reference to file_contents which is a fleetd extension table. Should we add it as one of the tables in Discovery?
When we added macos_codesign and macos_executable_sha256 we added the fleetd extension tables as part of the Discovery query. I believe this is because some users might have old versions of orbit after the server is upgraded so it could case breakages (I can't remember the exact mechanism here).
There was a problem hiding this comment.
great catch! i updated the discovery query to use the file_contents table
| END | ||
|
|
||
| WHERE p.publisher LIKE '%JetBrains%' | ||
| AND p.name NOT LIKE '%Toolbox%' |
There was a problem hiding this comment.
query looks good though! 👍
|
|
||
| - Discovery query: | ||
| ```sql | ||
| SELECT 1 FROM osquery_registry WHERE active = true AND registry = 'table' AND name = 'programs' |
There was a problem hiding this comment.
WARNING: Discovery query mismatch with the actual code
I noticed this documentation says the discovery query checks for the programs table, but the actual code at queries.go uses discoveryTable("file_contents"), which checks for the file_contents table instead. Since file_contents is the fleetd-specific table that might not be available, that's the one that really needs to be discovered. The programs table is always present on Windows.
It would be wonderful if we could update this to match what the code actually does:
| SELECT 1 FROM osquery_registry WHERE active = true AND registry = 'table' AND name = 'programs' | |
| SELECT 1 FROM osquery_registry WHERE active = true AND registry = 'table' AND name = 'file_contents' |
| // windows_jetbrains uses the version contained in the product-info.json file as exe installers | ||
| // provide an unconvertible build number in the programs table not used in vulnerability matching. | ||
| "windows_jetbrains": { | ||
| Description: "A software override query[^1] to append 'JetBrains' to the name of JetBrains Toolbox-managed applications on Windows. Requires `fleetd`", |
There was a problem hiding this comment.
SUGGESTION: Description doesn't match what the query does
The description says "to append 'JetBrains' to the name," but the query doesn't actually append anything to the name — it selects p.name AS name unchanged. The real purpose, as your helpful comment above explains, is to fix the version number by reading from product-info.json. It might be nice to update the description so it reflects what the query truly does — something like "A software override query to use the correct version from product-info.json for JetBrains Toolbox-managed applications on Windows."
This same description also appears in the docs file, so both would want updating together.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Well, I've taken a careful look through this PR, and I'm happy to say it all looks quite thoughtful and well-done. The new JetBrains software override query correctly selects the same columns expected by the Windows software ingestion pipeline, the Files Reviewed (4 files)
|
Ian and I both looked at a mutation approach, but unfortunately couldn't find a reliable approach
I chatted with Sharon on the release notes approach, but ultimately decided it was overkill for this case (only affects exe installers on windows)
that's the thought, hoping this approach will be maintenance free |
|
@getvictor need a codeowner approval here for host vitals docs |
sharon-fdm
left a comment
There was a problem hiding this comment.
I only reviewed
docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
LGTM.
Relying on existing approval for the rest.
This reverts commit a3bafe8.
Related issue: Resolves #26405
Checklist for submitter
Found on disk metadata to use that conforms to the desired version scheme of Jetbrains products.
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Added/updated automated tests
QA'd all new/changed functionality manually