Navigation Menu

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

Add cryptsetup_status table #734

Merged
merged 7 commits into from Jun 10, 2021

Conversation

directionless
Copy link
Contributor

@directionless directionless commented Jun 3, 2021

This creates a kolide_cryptsetup_status as a wrapper over the cryptsetup status command. Similar to #732, it's meant to aid in debugging and understanding

This depends on #731

@directionless directionless requested a review from blaedj June 3, 2021 19:36
@directionless directionless marked this pull request as ready for review June 3, 2021 19:36
@blaedj
Copy link
Contributor

blaedj commented Jun 8, 2021

Testing this out manually, it required that I find the name of a LUKS encrypted device (more annoying that it may sound), but seems to work!

@directionless
Copy link
Contributor Author

Testing this out manually, it required that I find the name of a LUKS encrypted device (more annoying that it may sound), but seems to work!

Yeah, I think that limitation is inherent. And I don't want the table to be clever.

tablehelpers.WithLogger(t.logger),
)

if len(requestedNames) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

In other tables, we've also checked that the constraint is =, and that's functionally all this table handles as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm.... You're not wrong, it feels like it would be better to check that the constraint.Operator == table.OperatorEquals But...

  1. This is using tablehelpers.GetConstraints to get a simple iterable. If we wanted to check the Operator, it should be in there.
  2. IIRC osquery doesn't pass anything other than = to table anyhow.

I'd be willing to think about how to have the helper be more clever about the Operator, since I think (2) is a bug. But I'm not sure what the desired outcome should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. IIRC osquery doesn't pass anything other than = to table anyhow.

Ah I didn't realize that, I thought all constraints were passed through, that feels like either a bug or undesirable limitation to me as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, this isn't necessarily a blocker, but I think consistent handling of the operators is one of those small things that makes the project better overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree about consistency. I suspect we should push handling into the helper, and have it return []string, error instead of just []string

Copy link
Contributor

@blaedj blaedj left a comment

Choose a reason for hiding this comment

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

There's some discussion about checking for constraint types, but that's not really a blocker. This lgtm

@directionless directionless merged commit 2b6f90e into kolide:master Jun 10, 2021
@directionless directionless deleted the seph/cryptsetup branch June 10, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants