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 password property is sensitive for mount resource #6442

Merged
merged 4 commits into from
Sep 27, 2017
Merged

fix password property is sensitive for mount resource #6442

merged 4 commits into from
Sep 27, 2017

Conversation

dimsh99
Copy link
Contributor

@dimsh99 dimsh99 commented Sep 26, 2017

Signed-off-by: dmitrys dmitrys@northernlight.com

Description

Password properties become sensitive by default

Issues Resolved

#4850

Check List

Signed-off-by: dmitrys <dmitrys@northernlight.com>
@dimsh99 dimsh99 requested a review from a team September 26, 2017 16:22
@coderanger
Copy link
Contributor

Not a fan of this approach. It's cute but too much magic for me to be in Chef core. 4850 should be fixed in the mount resource itself IMO.

@coderanger
Copy link
Contributor

To expand, keying this off the name is not super robust. If a property was named user_password, for example, or if the code isn't written in English. It's not a bad assumption but it's overly narrow and we can't possibly parse all human-readable names into secret and not-secret. Better to use the existing explicit API of property(:password, sensitive: true).

@lamont-granquist
Copy link
Contributor

pretty sure we're following that pattern of marking the password property as sensitive on an individual resource now somewhere as well...

it looks like i was the one that mentioned doing this globally and that was probably not one of my best ideas...

@lamont-granquist
Copy link
Contributor

execute.rb: property :password, String, sensitive: true

@lamont-granquist
Copy link
Contributor

remote_file.rb: property :remote_password, String, sensitive: true

This reverts commit 8242ac9.

Signed-off-by: Dmitry Shestoperov <dmitry@shestoperov.info>
Signed-off-by: Dmitry Shestoperov <dmitry@shestoperov.info>
@dimsh99 dimsh99 changed the title fix sensitive? to return true by default for password property fix password property is sensitive for mount resource Sep 27, 2017
@coderanger
Copy link
Contributor

This also makes the username property be sensitive, is that needed?

@dimsh99
Copy link
Contributor Author

dimsh99 commented Sep 27, 2017

It doesn't make it sensitive, but it's my mistake made while removing old :password definition. I'm really sorry for that.

Signed-off-by: Dmitry Shestoperov <dmitry@shestoperov.info>
Copy link
Contributor

@coderanger coderanger left a comment

Choose a reason for hiding this comment

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

lgtm thanks :)

@thommay thommay merged commit 0c6d82f into chef:master Sep 27, 2017
@thommay
Copy link
Contributor

thommay commented Sep 27, 2017

This is a great addition, thank you!

@dimsh99 dimsh99 deleted the fix_for_4850 branch September 27, 2017 13:26
@chef chef locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants