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

Fix/jenkins credentials #1029

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zipkid
Copy link
Member

@zipkid zipkid commented Feb 28, 2022

Pull Request (PR) description

When updating credentials or using the puppet debug flag credentials leak to the puppet log.
This PR is an attempt to avoid this without requiring the user to apply the Sensitive data type to all parameters.

lib/puppet/type/jenkins_credentials.rb Outdated Show resolved Hide resolved
Puppet.debug("#{sname} stdin:\n#{input}")
# This Puppet.debug shows the JSON with changed credentials in plain text.
# ToDo: Remove this debug output OR mask the credentials
# Puppet.debug("#{sname} stdin:\n#{input}")
Copy link
Member

Choose a reason for hiding this comment

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

If this contains credentials, you should note that it's written to a file just below and there's a chmod 644 rescue block, which makes those credentials world readable.

It looks like that is designed for the case where the jenkins user is not yet available. I wonder if that workaround should be removed and instead hard fail.

It should also be noted that there's no way to ensure the file really is removed. If execute_with_retry raises an exception it's not removed. That means in some cases the credentials could remain world readable until $TMPDIR is cleaned.

Copy link
Member Author

Choose a reason for hiding this comment

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

At least this would stay on the server and not be shipped off to something like The Foreman where many more can see the credentials.

@root-expert
Copy link
Member

Hello,

I'm not a fan of this solution, that places a lot of code in the provider to fix a issue that there's already a solution for it.
IMHO if the user wants to hide credentials/sensitive info from leaking to the logs, then they should use the proper solution, Sensitive data type, instead of relying to hacky workarounds.

If, for some reason, the module doesn't support Sensitive data types that's something that needs to be added instead of using workarounds.

@zipkid
Copy link
Member Author

zipkid commented Mar 14, 2022

We looked into solving this via the sensitive datatype.

Because of the multiple implementation types and the many different credentials we use in Jenkins this is very impractical.
For each implementation type we need a different hiera sensitive setting.
Because we pass all credentials in a hash that requires a setting for each var.

This 'solution' is works for everyone & always, also for people who forget about or don't know the sensitive data type.

I understand your objection and would agree if this was just 1 password.

Regards,

Stefan Goethals.

@kenyon
Copy link
Member

kenyon commented Mar 14, 2022

@zipkid you can use regexp in the lookup_options, would that work for converting to Sensitive? Might have to arrange your data structure differently, not sure how it looks. https://puppet.com/docs/puppet/7/hiera_merging.html#setting_lookup_options_to_refine_the_result_of_a_lookup-lookup-options-format

@zipkid
Copy link
Member Author

zipkid commented Mar 15, 2022

@kenyon We have tried, believe me. The problem is that sensitive does not work in hiera on members of a hash.

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

4 participants