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 linwin to list of supported PLATFORM_DIRS #7646

Merged
merged 2 commits into from Jun 24, 2022

Conversation

zhumo
Copy link
Contributor

@zhumo zhumo commented Jun 23, 2022

Problem

The two tables intel_me_info and secureboot are being marked as available for all platforms, when it should only be on linux and windows ("linwin"). It is correctly marked as linwin, but the documentation is not generating correctly.

This is because the genwebsitejson.py file does not contemplate linwin, so it just assumes every platform is supported.

This PR adds a linwin key to the PLATFORM_DIRS constant so that it will generate the correct tagging in the JSON.

For broader consideration

Should we not have that try-catch statement? It allows for silent failures like this, if something happens in the future. I'm happy to make that change, if you think this is a good idea.

directionless
directionless previously approved these changes Jun 23, 2022
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.

Thanks for digging into this!

I didn't test your PR, but the diff looks fine.

@directionless
Copy link
Member

Should we not have that try-catch statement? It allows for silent failures like this, if something happens in the future. I'm happy to make that change, if you think this is a good idea

I'm not sure what you mean by try-catch here. I think it would be reasonable to throw an error, but I'd do it via either a quick .get instead of [] or by checking for an null return and then throwing.

@zhumo
Copy link
Contributor Author

zhumo commented Jun 23, 2022

@directionless Sorry, not try-catch... try-except statement. See screenshot below

image

Basically, I think the lack of linwin as a key is a proximate cause for this issue, but imo the root cause is that we are try-excepting the "No Key" error and then automatically assuming that if there's no key, then the table is available for all platforms. I think that's how this linwin thing slipped through.

I don't know the history/context of why we are letting this error go through, but at first glance, I would think that we should let it fail so that someone will catch it and fix it. Glad to make that change if the team agrees.

@directionless
Copy link
Member

Oh! I see what you mean. Yeah, I'm familiar with the pattern, but I misunderstood your comment -- I didn't realize we already had one.

I would bet it's there so that the all case can be specified with a lack of of designation. I doubt there's a deeper reason, and I'm in favor of changing it. I agree its error prone. It seems reasonable to remove the try and fix whatever breaks.

* The try-except statement assumes that no key means the table is
  available everywhere, which is an error-prone assumption.
@zhumo
Copy link
Contributor Author

zhumo commented Jun 24, 2022

@directionless ok! I removed the catch statement. I ran the script twice: once as is and once with linwin commented out. It works as is and fails with KeyError when linwin is greyed out, which I think is expected behavior 👍

@directionless directionless merged commit b2c0f9f into osquery:master Jun 24, 2022
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