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

Don't log the agent configuration changes #274

Conversation

Projects
None yet
3 participants
@martinisoft
Copy link
Contributor

martinisoft commented Feb 10, 2016

This will work on any Chef Client 11.14+ by suppressing the diff output in the logs to prevent API credentials from leaking into logs. This could go even farther if needed by using the new ability in Chef to node.rm('datadog', 'api_key') the key from the node data before it is saved back to the Chef server.

Don't log the agent configuration changes
This will work on any Chef Client 11.14+ by suppressing the diff output in the logs to prevent API credentials from leaking into logs
@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Feb 10, 2016

As an additional note, this code will also work on Chef Clients earlier than 11.14 with the if guard.

@miketheman miketheman added the optimize label Mar 12, 2016

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Mar 12, 2016

@martinisoft Thanks for the patch - I am curious why you would evaluate the ChefGem resource for its inclusion of sensitive, when this is a template resource - can you elaborate?

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 12, 2016

@miketheman sensitive is a common resource option so we have to sniff out the option from the Chef Gem itself to see if it includes that as an instance method. You'll see it logged as "diff output suppressed" when a given resource is marked as sensitive. So this includes file and template resources.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Mar 12, 2016

@martinisoft Thanks for explaining.
I think I'm not getting it - since when attempting to use chef-shell to suss out behavior here, I can't seem to find the method used to determine behavior.

chef (12.7.2)> Chef::Resource::ChefGem.instance_methods(false).include?(:sensitive)
 => false
chef (12.7.2)> Chef::Resource::ChefGem.instance_methods(false)
 => [:gem_binary, :gem_binary=, :compile_time, :compile_time=, :after_created]

I think the module under inspection is not the Chef gem, rather the chef_gem resource - which doesn't have a notion of sensitive, as it inherits from class ChefGem < Chef::Resource::Package::GemPackage.

Pursuing the ancestors path, we can see that none of the declared resources have this instance_method, other than the main Resource - where it is defined as a common

chef (12.7.2)> Chef::Resource::Template.instance_methods(false).include?(:sensitive)
 => false
chef (12.7.2)> Chef::Resource::File.instance_methods(false).include?(:sensitive)
 => false
chef (12.7.2)> Chef::Resource::Execute.instance_methods(false).include?(:sensitive)
 => false
chef (12.7.2)> Chef::Resource.instance_methods(false).include?(:sensitive)
 => true

Turning it slightly around, we can see:

chef (12.7.2)> Chef::Resource::Template.instance_methods(true).include?(:sensitive)
=> true

So it may appear that this would be the best approach, as we interrogate the Template module for all of the instance_methods it has available to it to find :sensitive - which it gets from inheriting from Chef::Resource.

Does that make sense?

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 12, 2016

@miketheman Makes a lot of sense actually. Just did a little testing with Chef 11.10 and 11.14 and I see your point. I'll update my PR shortly. 👍

Use better feature detection
This feature was introduced with Chef 11.14 so we're checking the resource instance methods since it was added to the File, Template, and Execute resources.
@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 12, 2016

@miketheman Have updated the change. I did some testing locally and it looks like the Template instance methods will return a false positive there so I had to use the Resource class to detect the sensitive attribute.

On Chef 11.10.4

chef > Chef::Resource::Template.instance_methods(true).include?(:sensitive)
 => true
chef > Chef::Resource.instance_methods(false).include?(:sensitive)
 => false
@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Mar 12, 2016

👍 Thanks! We'll add this to the merge range for the next minor release cycle.

@miketheman miketheman added this to the Next minor milestone Mar 12, 2016

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 12, 2016

@miketheman Sounds good. Did you want me to also add in the node.rm stuff as well to remove the api key from the node data?

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Mar 12, 2016

@martinisoft I think that's a great optimization, I think that's best placed in its own PR - it might be best to wait for the next Major revision for that, since it removes something from the node's saved attributes (hopefully nobody else uses that, but you never know...).

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 12, 2016

@miketheman Fair enough. 👍

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Mar 16, 2016

@miketheman So I completely forgot the LWRP and just included that as well on the file and template resources called as well because secrets can get exposed through there as well.

@iiro

This comment has been minimized.

Copy link

iiro commented Mar 23, 2016

This is superb stuff! Glad I found this as credentials are all over logs now...

@miketheman miketheman merged commit f6010ca into DataDog:master Apr 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.