-
Notifications
You must be signed in to change notification settings - Fork 522
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
logdog: Filter sensitive settings from API output #1777
logdog: Filter sensitive settings from API output #1777
Conversation
e04b29b
to
550212e
Compare
^ Fixed up the tests now that things are async. |
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.
It might not be obvious to a user that these settings were removed, which could lead someone astray if the issue under investigation is related to one of the sensitive settings. I'm not sure what to suggest though, this is the lesser of evils for sure.
Previously, `logdog` used the `exec` mode to run `apiclient` and dump everything from the datastore. Since a few settings now exist that can store sensitive data, we need to filter them from being written to disk. In order to make this change, `logdog` needed to be trained to understand the data it was gathering, in order to filter it. A new `settings` mode was added specifically for gathering settings. This mode uses the `apiclient` and `datastore` libraries to be able to de/serialize the output, filter it, and then serialize it back to disk.
550212e
to
4a6b40e
Compare
^ Implemented @tjkirch 's suggestions |
Good point, but I would agree this is the lesser of evils. A user should still be able to fetch these settings via apiclient if they're needed for troubleshooting. |
@@ -25,6 +27,15 @@ const COMMON_REQUESTS: &str = include_str!("../conf/logdog.common.conf"); | |||
/// The `logdog` log requests that are specific to the current variant. | |||
const VARIANT_REQUESTS: &str = include_str!("../conf/current/logdog.conf"); | |||
|
|||
/// Patterns to filter from settings output. These follow the Unix shell style pattern outlined | |||
/// here: https://docs.rs/glob/0.3.0/glob/struct.Pattern.html. | |||
const SENSITIVE_SETTINGS_PATTERNS: &[&str] = &[ |
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.
Should the cluster certificate be added to this list?
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 don't think so - I believe it's public.
Description of changes:
For now, we're filtering all
user-data
fields, the Kubernetes bootstrap token, and https proxy.Testing done:
vmware-k8s-1.20
with user data for both host containers, a bootstrap container with user data, and the required k8s token. Ran logdog and inspected the output, which did not contain any user data, or k8s token. Relevant sections below:Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.