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.

Sign up for GitHub

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

Jump to bottom

Clean up the password field and value in automate and evm.log #228

Merged
merged 2 commits into from
Sep 24, 2018
Merged

Clean up the password field and value in automate and evm.log #228

merged 2 commits into from
Sep 24, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Sep 13, 2018 •
edited
Loading

Hide password values in automate.log and evm.log.
Add "_id" attribute only for VMDB objects.

Depends on ManageIQ/manageiq-gems-pending#373.
Includes ManageIQ/manageiq#17986.

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

@miq-bot add_label bug, gaprindashvili/yes

@lfu lfu mentioned this pull request Sep 13, 2018
Merged
@lfu lfu changed the title Password log 1619385 Clean up the password field and value in automate and evm.log Sep 13, 2018
gmcculloug
gmcculloug reviewed Sep 13, 2018
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb Outdated

def attribute_for_vmdb_object?(klass, value)
Kernel.const_defined?(klass) && value.to_i.nonzero?
rescue NameError
Copy link
Member

Choose a reason for hiding this comment

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

Should use klass.safe_constantize here which will return nil if the class does not exist.

lib/miq_automation_engine/engine/miq_ae_engine.rb Outdated
@@ -310,4 +310,10 @@ def self.ae_user_object(options = {})
$miq_ae_logger.info("User [#{obj.userid}] with current group ID [#{obj.current_group.id}] name [#{obj.current_group.description}]")
end
end

V2_PASSWORD_REGEXP = /v([0-9]+)%3A%7B(.*?)%7D/
Copy link
Member

Choose a reason for hiding this comment

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

Push this to the top of the file to group with the other constant. Also, I would recommend adding a comment clarifying why this one is different then the one in MiqPassword.

lib/miq_automation_engine/engine/miq_ae_engine.rb Outdated

V2_PASSWORD_REGEXP = /v([0-9]+)%3A%7B(.*?)%7D/

def self.sanitize_string(str)
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend changing this name to be more specific since it is really handling the special use-case of an encoded string.

Maybe: sanitize_encoded_string

@bdunne Do you think this should live in MiqPassword? A bit of a special case.

@lfu lfu force-pushed the password_log_1619385 branch 2 times, most recently from 2b29f8f to 643fce9 Compare September 13, 2018 21:13
@JPrause
Copy link
Member

JPrause commented Sep 14, 2018

@miq-bot add_label blocker

mkanoor
mkanoor reviewed Sep 14, 2018
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine.rb Outdated

def self.instantiate(uri, user)
$miq_ae_logger.info("MiqAeEngine: Instantiating Workspace for URI=#{uri}")
$miq_ae_logger.info("MiqAeEngine: Instantiating Workspace for URI=#{sanitize_encoded_string(uri)}")
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu Why can't we use MiqPassword.sanitize_string here?

Copy link
Member

Choose a reason for hiding this comment

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

The string is URL encoded so it no longer matches the regex in MiqPassword.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu @gmcculloug
Are we getting passwords as URL's?
Can the function sanitize_encoded_string to MiqPassword so its all in one place and we can control if it should be asterisk or [FILTERED] or something else

Copy link
Contributor

Choose a reason for hiding this comment

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

Its the password coming in from Dialogs any dialog_field that is marked as encrypted comes in with the v2 string.

@tinaafitz
Copy link
Member

@lfu @gmcculloug @mkanoor I know we've always used "********" in our logs for password fields.
The production.log below shows "[FILTERED]" for a password value.
I'm wondering if there's a standard for passwords and logging. I just started searching and havent found anything yet. Thoughts?

production.log:[----] I, [2018-08-30T10:12:03.800666 #2303:1170470]
INFO -- : Parameters: {"user_name"=>"admin",
"user_password"=>"[FILTERED]", "browser_name"=>"Chrome",
"browser_version"=>"68", "browser_os"=>"Mac", "user_TZO"=>"-4"}

@lfu
Copy link
Member Author

lfu commented Sep 14, 2018

[FILTERED] is used by log_hashes. ******** is used by MiqPassword.sanitize_string.

@gmcculloug
Copy link
Member

The [FILTERED] string also comes from rails https://guides.rubyonrails.org/action_controller_overview.html#parameters-filtering.

I would suggest we look into including the support for encoded password strings into MiqPassword as @mkanoor and I mentioned (#228 (comment) and #228 (comment)) and the discussion about what replacement string to use should be a separate PR/issue.

@lfu
The "_id" attribute should be added only for VMDB objects.
c9a6e98 
The "_id" attribute is not meant for fields like password::dialog_password_field.

https://bugzilla.redhat.com/show_bug.cgi?id=1619385
@lfu lfu force-pushed the password_log_1619385 branch from 643fce9 to b25ce0b Compare September 14, 2018 17:33
@lfu lfu mentioned this pull request Sep 14, 2018
Merged
@lfu lfu force-pushed the password_log_1619385 branch from b25ce0b to 4a3cf53 Compare September 17, 2018 13:37
@miq-bot
Copy link
Member

miq-bot commented Sep 17, 2018

Checked commits lfu/manageiq-automation_engine@c9a6e98~...4a3cf53 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍪

@mkanoor mkanoor merged commit f294b56 into ManageIQ:master Sep 24, 2018
@mkanoor mkanoor added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 24, 2018
simaishi pushed a commit that referenced this pull request Oct 1, 2018
@mkanoor @simaishi
Merge pull request #228 from lfu/password_log_1619385
2452d0e 
Clean up the password field and value in automate and evm.log

(cherry picked from commit f294b56)

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

simaishi commented Oct 1, 2018

Gaprindashvili backport details:

$ git log -1
commit 2452d0e82324067fb18782805ff5ad2872744fdf
Author: Madhu Kanoor <[email protected]>
Date:   Mon Sep 24 16:44:14 2018 -0400

    Merge pull request #228 from lfu/password_log_1619385
    
    Clean up the password field and value in automate and evm.log
    
    (cherry picked from commit f294b5636db07acfea58391230a7e8e41be73b1b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1634808

@lfu lfu deleted the password_log_1619385 branch November 4, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@mkanoor mkanoor mkanoor approved these changes

@bdunne bdunne bdunne approved these changes

@tinaafitz tinaafitz Awaiting requested review from tinaafitz

Assignees

@mkanoor mkanoor

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lfu @JPrause @tinaafitz @gmcculloug @miq-bot @simaishi @bdunne @mkanoor