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

Use curl netrc file instead of --user #399

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

alexjfisher
Copy link
Member

This stops credentials appearing in the process list and failed
downloads won't result in the credentials being logged in puppet reports
and agent log output.

Fixes #397

@alexjfisher
Copy link
Member Author

I intend to fixup/rewrite the tests after getting some initial feedback.

@alexjfisher
Copy link
Member Author

Should probably implement a similar fix for the wget provider. Annoyingly, wget can also use netrc files, but it appears you can't actually specify a path to such a file. It only looks in ~/.netrc (which we can't overwrite!)

@alexjfisher
Copy link
Member Author

You can use a WGETRC environment variable.
https://www.gnu.org/software/wget/manual/html_node/Wgetrc-Location.html

Since the implementation wouldn't have too much in common, and since you have to go out of your way to select wget as the provider, I'll probably just leave this for a separate PR.

@igalic
Copy link
Contributor

igalic commented Jan 26, 2020

and we can't use Sensitive type in a puppet type?

@alexjfisher
Copy link
Member Author

Type properties automatically work with Sensitive types. When puppet syncs the property it just knows to redact the change. But password is a parameter. (You can also define a property to be Sensitive always even if the user provides a normal string. See puppetlabs/puppet@f62a592)

With parameters, you can pass a Sensitive value if you like and it will work. But you'll get a warning like:

Unable to mark 'password' as sensitive: password is a parameter and not a property, and cannot be automatically redacted.

I suppose you could suppress the warning by converting the parameter into a property that is fudged to always be in sync (like in puppetlabs/puppetlabs-stdlib#786)

But since we don't (in this fix of this provider), pass the password to the command being executed, we don't have to do something crazy like in https://github.com/puppetlabs/puppetlabs-vcsrepo/pull/416/files

@alexjfisher
Copy link
Member Author

By switching to using a netrc file, I think we avoid all the hassle of having to conditionally redact command output by calling Puppet::Util::Execution directly instead of using the autogenerated helper. If we couldn't have used netrc, then I don't think I'd even check to see if password was Sensitive, but always consider it so.
I'm also guessing if you Puppet::Util::Execution.execute("curl $PARAMS", sensitive: true) you end up redacting the command error output completely. This would be very helpful to users.

@alexjfisher
Copy link
Member Author

I'm unclear as to why in https://github.com/puppetlabs/puppetlabs-vcsrepo/pull/416/files#diff-574463866055cc3f49ee013638f6652aR81 they have to call unwrap. If I set archive's password parameter to a Sensitive value, it just works. No need to explicitly unwrap anything. It gets written out to the file just fine.

@ekohl
Copy link
Member

ekohl commented Jan 27, 2020

I like this approach. One reason to do this regardless of the use of Sensitive is that it doesn't show the password in the process list (ps aux).

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Is this still WIP?

@alexjfisher alexjfisher force-pushed the issue_397 branch 3 times, most recently from 98ad0c8 to 7fd3572 Compare April 2, 2020 12:06
This stops credentials appearing in the process list and failed
downloads won't result in the credentials being logged in puppet reports
and agent log output.

Fixes voxpupuli#397
@alexjfisher alexjfisher changed the title WIP: Use curl netrc file instead of --user Use curl netrc file instead of --user Apr 2, 2020
@alexjfisher alexjfisher added enhancement New feature or request and removed needs-tests tests-fail labels Apr 2, 2020
@ekohl ekohl merged commit 5f77125 into voxpupuli:master Apr 2, 2020
@ekohl
Copy link
Member

ekohl commented Apr 2, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: make password sensitive and hide on fail
3 participants