Skip to content
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

Update linux disk_encryption to recursively query parent crypt status #8052

Merged

Conversation

Micah-Kolide
Copy link
Contributor

This change will rely on #8037

Functionally this should minimize getting encryption status from parent devices, but without the constraint being added in block_devices this will not perform well.

Below is the process of acquiring crypt status and overall what is different.

  1. Query block devices all or by name constraint
  2. Generate encryption row for each queried block device
    • If current block device has encryption
      • Set the encryption row with those values
    • If current block device does not have valid encryption
      • If we have a parent block device defined
        • If we need to create the parent's encryption status
          • If we need to query the parent block device
            • Query the parent block device
          • Generate parent encryption status
        • If we have already generated an encryption status for the parent
          • Use already generated parent device's encryption status
      • If we do not have a parent i.e is a root device
        • Set unencrypted status for this device
  3. Add encryption row results to output if that device was a part of the initial query

This avoids sorting parents altogether, which I figured made sense here.

@Micah-Kolide Micah-Kolide requested review from a team as code owners June 6, 2023 17:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@directionless directionless added this to the 5.10.0 milestone Jun 8, 2023
@mike-myers-tob mike-myers-tob changed the title Update linux disk_encryption to recursively query parent crypt status Update linux disk_encryption to recursively query parent crypt status Jun 20, 2023
@mike-myers-tob mike-myers-tob added virtual tables Linux bug ready for review Pull requests that are ready to be reviewed by a maintainer labels Jun 20, 2023
sharvilshah
sharvilshah previously approved these changes Oct 3, 2023
Copy link
Member

@sharvilshah sharvilshah left a comment

Choose a reason for hiding this comment

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

I have not built/tested this branch locally, but the code/flow seems fine.

Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I walked through the code. I think it works, but could use a coupe more comments. And maybe we can swap in an early return and ditch the else.

osquery/tables/system/linux/disk_encryption.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/disk_encryption.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/disk_encryption.cpp Outdated Show resolved Hide resolved
osquery/tables/system/linux/disk_encryption.cpp Outdated Show resolved Hide resolved
directionless
directionless previously approved these changes Oct 3, 2023
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I think @Micah-Kolide is manually testing, but I think this is correct

directionless
directionless previously approved these changes Oct 3, 2023
@directionless directionless dismissed their stale review October 3, 2023 20:39

Micah says he's still tinkering

@Micah-Kolide Micah-Kolide force-pushed the micah/linux_disk_encryption_patch branch from c092925 to 65ab341 Compare October 6, 2023 15:21
@directionless
Copy link
Member

@Micah-Kolide and I spent a long time tinkering with this. And ultimately, while there are several issues around efficiency, this seems worth merging as is. But, notes for next time...

disk_encryption needs block_devices. And unfortunately block_devices does not support query context. (see #8152) By extension, disk_encryption does not support query context. Thus, it does not declare any indexing, and in theory it should be correct. However...

Somewhere in osquery/sqlite there appear to be two issues.

  1. sometimes when called as select * from foo where x in (a, b, c) foo is called 3 times. This feels suspect.
  2. when used in a join, as SELECT * FROM mounts m LEFT JOIN disk_encryption de ON m.device_alias = de.name; it it called once per mount. This makes for some horrific performance.

In both cases, playing with index, additional and cachable do not fundamentally change the outcome.

These feel like bugs in osquery, and I think I've seen them in the past. I do not think they should block this merge, as this is an improvement over the status quo.

@directionless
Copy link
Member

Ha. The issues I describe are exactly covered by #5379. I knew I'd seen them before

@Micah-Kolide Micah-Kolide deleted the micah/linux_disk_encryption_patch branch October 10, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Linux ready for review Pull requests that are ready to be reviewed by a maintainer virtual tables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants