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

[V2V] Filter certain options in the ConversionHost#run_conversion method #18852

Merged
merged 1 commit into from
Jun 14, 2019
Merged

[V2V] Filter certain options in the ConversionHost#run_conversion method #18852

merged 1 commit into from
Jun 14, 2019

Conversation

djberg96
Copy link
Contributor

At the moment the ConversionHost#run_conversion method potentially exposes some sensitive data on failure. This PR filters anything that ends with "password", "key" or "fingerprint" from the logs and/or UI.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1716415

@djberg96
Copy link
Contributor Author

@miq-bot add_label transformation, hammer/yes, changelog/yes

@djberg96
Copy link
Contributor Author

@miq-bot add_reviewer @agrare

@miq-bot miq-bot requested a review from agrare June 13, 2019 13:06
@ghost
Copy link

ghost commented Jun 13, 2019

@miq-bot add-label security
@miq-bot add-reviewer @Fryguy

app/models/conversion_host.rb Outdated Show resolved Hide resolved
app/models/conversion_host.rb Outdated Show resolved Hide resolved
def run_conversion(conversion_options)
ignore = %w[password fingerprint key]
filtered_options = conversion_options.clone.tap { |h| h.each { |k, _v| h[k] = "__FILTERED__" if ignore.any? { |i| k.to_s.end_with?(i) } } }
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this?

filtered_options = conversion_options.each_with_object({}) { |(k, v), h| h[k] = (k.to_s =~ /(fingerprint|key|password)\z/) ? "__FILTERED__" : v }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks more difficult than mine. I'm sure there are multiple ways to skin a cat here, but I'd like to keep mine unless you have strong objections.

Replace sensitive values with FILTERED, and remove unnecessary variable assignment.
@miq-bot
Copy link
Member

miq-bot commented Jun 14, 2019

Checked commit https://github.com/djberg96/manageiq/commit/a2948baf526df31fbdc4c360e000fb43d06fe938 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@bdunne bdunne merged commit 11c18ab into ManageIQ:master Jun 14, 2019
@bdunne bdunne self-assigned this Jun 14, 2019
@bdunne bdunne added this to the Sprint 114 Ending Jun 24, 2019 milestone Jun 14, 2019
simaishi pushed a commit that referenced this pull request Jun 14, 2019
…ering

[V2V] Filter certain options in the ConversionHost#run_conversion method

(cherry picked from commit 11c18ab)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1720756
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit be181d476a05385cd27b62dc3701ec003d0ba28c
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Fri Jun 14 14:42:13 2019 -0400

    Merge pull request #18852 from djberg96/conversion_host_password_filtering
    
    [V2V] Filter certain options in the ConversionHost#run_conversion method
    
    (cherry picked from commit 11c18abf79e0b9a14633f2fc83fcfd3e583e7375)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1720756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants