-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix hardware UUID caching #8616
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
Fix hardware UUID caching #8616
Conversation
2df7187 to
77449ae
Compare
getvictor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, with comments.
osquery/core/system.cpp
Outdated
|
|
||
| if (!checked_hardware_uuid) { | ||
| // Attempt to get the hardware UUID from the system. | ||
| std::string hardware_uuid = osquery::generateHostUUID(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. The function names make the code hard to read/maintain. The generateHostUUID function actually tries to get the UUID from hardware, and does not generate. While this function either gets from hardware, gets from DB, or generates. It would be nice to have better names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok updating this, it's only called from one spot.
77449ae to
835552b
Compare
835552b to
babae5c
Compare
sgress454
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getvictor updated generateHostUUID to getHardwareUUID as suggested, and addressed other comment.
getvictor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
For #7509
For fleetdm/fleet#28535
Details
This PR updates the logic for generating, storing and returning host identifiers when
--host-identifier=uuidis used. Previously, once a UUID was determined (either by getting a valid hardware UUID from the device, or failing that, creating a random UUID) it was stored in the local db and returned henceforth on every call togetHostUUID(). With this update, the logic becomes:getHostUUID()is called.This handles the most pertinent cases of 1) moving an osquery db between two hosts with different hardware UUIDs and 2) the hardware UUID of a host changing (e.g. due to a motherboard being swapped out). It also maintains the behavior of randomly-generated UUIDs persisting across osquery restarts (provided that no hardware UUID is found) so that VMs using
--host-identifier=uuidget a consistent UUID.The behavior when moving from a system with a hardware UUID to a system without one (or moving between two VMs) may still be considered sub-optimal, but those use-cases don't seem as problematic and I'm not sure what a good solve would be.
Testing
I tested this manually with:
and manipulating the db using
ldb. I also simulated VM usage by temporarily adding my laptop's UUID to the "placeholders" list.I'd love to add a unit test for the new logic but the only way I can think to do that would be to change
getHostUUIDto use dependency-injection so I could mockgenerateNewUUID(),getDatabaseValue()andsetDatabaseValue(). I haven't seen that pattern elsewhere in osquery though.