Changes to MiqPassword.sanitize_string to support URL encoded password. #373

lfu · 2018-09-14T17:33:12Z

To hide the encoded password value in URL encoded string with*******.

Blocks ManageIQ/manageiq-automation_engine#228.

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

@miq-bot add_label bug, gaprindashvili/yes

Before:

[----] I, [2018-09-04T11:52:44.679450 #12292:c27108]  INFO -- : Q-task_id([service_template_provision_task_1]) Instantiating [/Service/Provisioning/StateMachines/ServiceProvision_Template/CatalogItemInitialization?MiqServer%3A%3Amiq_server=1&Service%3A%3AService=1&ServiceTemplateProvisionTask%3A%3Aservice_template_provision_task=1&User%3A%3Auser=1&ae_state=checkprovisioned&ae_state_previous=---%0A%22%2FManageIQ%2FService%2FProvisioning%2FStateMachines%2FServiceProvision_Template%2FCatalogItemInitialization%22%3A%0A%20%20ae_state%3A%20checkprovisioned%0A%20%20ae_state_retries%3A%201%0A%20%20ae_state_started%3A%202018-09-04%2015%3A51%3A41%20UTC%0A%20%20ae_state_max_retries%3A%20100%0A&ae_state_retries=1&ae_state_started=2018-09-04%2015%3A51%3A41%20UTC&object_name=CatalogItemInitialization&password%3A%3Adialog_password_field=v2%3A%7BSEZAjnaOSeyzt387%2FrAooQ%3D%3D%7D&request=clone_to_service&service_action=Provision&vmdb_object_type=service_template_provision_task]

After:

[----] I, [2018-09-04T11:52:44.679450 #12292:c27108]  INFO -- : Q-task_id([service_template_provision_task_1]) Instantiating [/Service/Provisioning/StateMachines/ServiceProvision_Template/CatalogItemInitialization?MiqServer%3A%3Amiq_server=1&Service%3A%3AService=1&ServiceTemplateProvisionTask%3A%3Aservice_template_provision_task=1&User%3A%3Auser=1&ae_state=checkprovisioned&ae_state_previous=---%0A%22%2FManageIQ%2FService%2FProvisioning%2FStateMachines%2FServiceProvision_Template%2FCatalogItemInitialization%22%3A%0A%20%20ae_state%3A%20checkprovisioned%0A%20%20ae_state_retries%3A%201%0A%20%20ae_state_started%3A%202018-09-04%2015%3A51%3A41%20UTC%0A%20%20ae_state_max_retries%3A%20100%0A&ae_state_retries=1&ae_state_started=2018-09-04%2015%3A51%3A41%20UTC&object_name=CatalogItemInitialization&password%3A%3Adialog_password_field=********&request=clone_to_service&service_action=Provision&vmdb_object_type=service_template_provision_task]

mkanoor · 2018-09-14T18:25:59Z

lib/gems/pending/util/miq-password.rb

@@ -88,6 +89,10 @@ def self.sanitize_string!(s)
    s.gsub!(REGEXP, '********')
  end

+  def self.sanitize_encoded_string(s)


@lfu Since there can be many kinds of encoding (base64, url) Should we call this function sanitize_url_encoded_string?

bdunne · 2018-09-14T19:03:33Z

lib/gems/pending/util/miq-password.rb

@@ -7,6 +7,7 @@ class MiqPasswordError < StandardError; end

  CURRENT_VERSION = "2"
  REGEXP = /v([0-9]+):\{([^}]*)\}/
+  REGEXP_V2_ENCODED = /v([0-9]+)%3A%7B(.*?)%7D/ # for URL encoded string of "v2:{...}"


I think the V2 should be removed from the name since the regex supports v*. Maybe REGEXP_URL_ENCODED?

Why (.*?)?

Also, we probably don't need the capture groups since we would have to URI decode the string before we could do anything with those values

bdunne · 2018-09-14T19:06:55Z

lib/gems/pending/util/miq-password.rb

@@ -88,6 +89,10 @@ def self.sanitize_string!(s)
    s.gsub!(REGEXP, '********')
  end

+  def self.sanitize_encoded_string(s)


Any reason to add a new method rather than add this support to the two existing methods above?

kbrock · 2018-09-17T23:49:32Z

Is there a reason why you are not changing sanitize_string!?
If it simplifies anything, we only support keys of the form v2:{} - at least for the past 4 years.
We've dropped support for legacy and v1 keys. v[3-9] keys have never and will never exist.
Was a little surprised for keeping REGEXP and REGEXP_START_LINE - but I suppose those are needed for one use case of split.

Looking Good.

kbrock

We had plans to have versions of the passwords above v2 - but that is where it stopped and will always stop.
We no longer support anything other than v2, but taking that into account looks like it would change too many specs and be out of scope for this BZ.

I worry about having REGEXP and REGEXP_PASSWORD - but I can see how having a special purpose for cleaning urls may be desired. If not, please have sanitize_string and sanitize_string! use the same regular expression.

All in all, @lfu this is looking good.
Thanks @bdunne for the reviewing here, too.

kbrock · 2018-09-17T23:56:27Z

lib/gems/pending/util/miq-password.rb

@@ -7,6 +7,7 @@ class MiqPasswordError < StandardError; end

  CURRENT_VERSION = "2"
  REGEXP = /v([0-9]+):\{([^}]*)\}/
+  REGEXP_PASSWORD = /v[0-9]+(:\{[^}]*\}|%3A%7B.*?%7D)/ # for "v2:{...}" or its URL encoded string


I'd feel a little more comfortable here with the .* being swapped out for a clause that will be more restrictive.
Since it is base64 encoded the valid characters are [A-Za-z+/=] but a simple [^%] will suffice.

Also the ? in the .*? looks funny to me. Is that for a blank password?

I think the simplest regex would be:

REGEXP_URL_ENCODED = /v[0-2](:\{[^}]*\}|%3A%7B[^%]*%7D)

REGEXP_PASSWORD really doesn't tell you anything. Granted, REGEXP is a bad name, as well.

.*? is for non-greedy match.
+/= become %2B%2F%3D after URL encoded. So [^%]* would not work for +/=.

Naming is always hard. REGEXP_URL_ENCODED was my first try. But actually only the 2nd part of the REGEX is for URL encoded password. The first part is just for an encrypted password.

If REGEXP_URL_ENCODED is preferred, I can certainly rename it.

kbrock · 2018-09-17T23:58:43Z

lib/gems/pending/util/miq-password.rb

@@ -7,6 +7,7 @@ class MiqPasswordError < StandardError; end

  CURRENT_VERSION = "2"
  REGEXP = /v([0-9]+):\{([^}]*)\}/


Not sure if simplifying this regex would simplify your main goal.

REGEXP = `/v([0-2]):\{([^}]*)\}/`

lfu · 2018-09-18T14:19:21Z

Is there a reason why you are not changing sanitize_string!?

I could not think of a use case where the change of sanitize_string! is necessary. I would be glad to make the change if someone can show me one of the use cases.

bdunne · 2018-09-18T15:53:14Z

I could not think of a use case where the change of sanitize_string! is necessary. I would be glad to make the change if someone can show me one of the use cases.

Just for API consistency.

JPrause · 2018-09-18T18:49:46Z

@miq-bot add_label blocker

kbrock

Just the one change with the + and I'm +1

@bdunne you good with this now?

kbrock · 2018-09-19T13:41:36Z

lib/gems/pending/util/miq-password.rb

@@ -6,7 +6,8 @@ class MiqPassword
  class MiqPasswordError < StandardError; end

  CURRENT_VERSION = "2"
-  REGEXP = /v([0-9]+):\{([^}]*)\}/
+  REGEXP = /v([0-2]+):\{([^}]*)\}/
+  REGEXP_PASSWORD = /v[0-2]+(:\{[^}]*\}|%3A%7B.*?%7D)/ # for "v2:{...}" or its URL encoded string


could you remove the + for both of these?

Also, I'd feel more comfortable with a [^%]* over .*? - but keeping with the lazy match and over matching is probably not the end of the world.

I'm punting on the name change. keep it REGEXP_PASSWORD

kbrock · 2018-09-19T13:41:57Z

lib/gems/pending/util/miq-password.rb

  end

  def self.sanitize_string!(s)
-    s.gsub!(REGEXP, '********')
+    s.gsub!(REGEXP_PASSWORD, '********')


kbrock · 2018-09-19T13:42:38Z

spec/util/miq-password_spec.rb

@@ -212,8 +212,10 @@
  end

  it ".sanitize_string" do
-    expect(MiqPassword.sanitize_string!("some :password: v1:{XAWlcAlViNwB} and another :password: v2:{egr+hObB}")).to eq(
+    expect(MiqPassword.sanitize_string("some :password: v1:{XAWlcAlViNwB} and another :password: v2:{egr+hObB}")).to eq(


bdunne · 2018-09-19T15:37:07Z

@lfu Can you take care of the rubocop comment?

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

miq-bot · 2018-09-19T15:47:29Z

Checked commit lfu@744d4c9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

kbrock

I'm good with this - you all set @bdunne ?

Changes to MiqPassword.sanitize_string to support URL encoded password. (cherry picked from commit 2fa61e9) https://bugzilla.redhat.com/show_bug.cgi?id=1619385

simaishi · 2018-09-26T13:20:43Z

Hammer backport details:

$ git log -1
commit 448fc49b99ee8eb97c532450287337eb82978054
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Sep 25 14:52:08 2018 -0400

    Merge pull request #373 from lfu/password_log_1619385
    
    Changes to MiqPassword.sanitize_string to support URL encoded password.
    
    (cherry picked from commit 2fa61e91ce5eeba1dc969e38c76faaee61cb7eb6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1619385

Changes to MiqPassword.sanitize_string to support URL encoded password. (cherry picked from commit 2fa61e9) https://bugzilla.redhat.com/show_bug.cgi?id=1634808

simaishi · 2018-10-01T17:40:31Z

Gaprindashvili backport details:

$ git log -1
commit 3707a9a85a9b93325e566a7dbe1cdcb01f1c55cc
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Sep 25 14:52:08 2018 -0400

    Merge pull request #373 from lfu/password_log_1619385
    
    Changes to MiqPassword.sanitize_string to support URL encoded password.
    
    (cherry picked from commit 2fa61e91ce5eeba1dc969e38c76faaee61cb7eb6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1634808

lfu mentioned this pull request Sep 14, 2018

Clean up the password field and value in automate and evm.log ManageIQ/manageiq-automation_engine#228

Merged

miq-bot added bug gaprindashvili/yes labels Sep 14, 2018

mkanoor reviewed Sep 14, 2018

View reviewed changes

bdunne requested changes Sep 14, 2018

View reviewed changes

lfu force-pushed the password_log_1619385 branch 2 times, most recently from a5b6f38 to 6b4d903 Compare September 17, 2018 13:59

kbrock reviewed Sep 18, 2018

View reviewed changes

lfu force-pushed the password_log_1619385 branch from 6b4d903 to 6721dbe Compare September 18, 2018 16:36

lfu changed the title ~~Add MiqPassword.sanitize_encoded_string.~~ hanges to MiqPassword.sanitize_string to support encoded URL. Sep 18, 2018

lfu changed the title ~~hanges to MiqPassword.sanitize_string to support encoded URL.~~ Changes to MiqPassword.sanitize_string to support encoded URL. Sep 18, 2018

lfu changed the title ~~Changes to MiqPassword.sanitize_string to support encoded URL.~~ Changes to MiqPassword.sanitize_string to support URL encoded password. Sep 18, 2018

miq-bot added the blocker label Sep 18, 2018

kbrock reviewed Sep 19, 2018

View reviewed changes

lfu force-pushed the password_log_1619385 branch from 6721dbe to 8b260c9 Compare September 19, 2018 14:05

Add regex for URL encoded password value.

744d4c9

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

lfu force-pushed the password_log_1619385 branch from 8b260c9 to 744d4c9 Compare September 19, 2018 15:42

kbrock approved these changes Sep 25, 2018

View reviewed changes

bdunne approved these changes Sep 25, 2018

View reviewed changes

bdunne merged commit 2fa61e9 into ManageIQ:master Sep 25, 2018

bdunne added this to the Sprint 96 Ending Oct 8, 2018 milestone Sep 25, 2018

bdunne added the hammer/yes label Sep 25, 2018

simaishi added hammer/backported and removed hammer/yes labels Sep 26, 2018

lfu deleted the password_log_1619385 branch September 29, 2018 14:34

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to MiqPassword.sanitize_string to support URL encoded password. #373

Changes to MiqPassword.sanitize_string to support URL encoded password. #373

lfu commented Sep 14, 2018

mkanoor Sep 14, 2018

bdunne Sep 14, 2018 •

edited

bdunne Sep 14, 2018

bdunne Sep 14, 2018

bdunne Sep 14, 2018

kbrock commented Sep 17, 2018

kbrock left a comment •

edited

kbrock Sep 17, 2018 •

edited

lfu Sep 18, 2018

lfu Sep 18, 2018

kbrock Sep 17, 2018

lfu commented Sep 18, 2018

bdunne commented Sep 18, 2018

JPrause commented Sep 18, 2018

kbrock left a comment

kbrock Sep 19, 2018 •

edited

kbrock Sep 19, 2018

kbrock Sep 19, 2018

bdunne commented Sep 19, 2018

miq-bot commented Sep 19, 2018

kbrock left a comment

simaishi commented Sep 26, 2018

simaishi commented Oct 1, 2018

		@@ -7,6 +7,7 @@ class MiqPasswordError < StandardError; end

		CURRENT_VERSION = "2"
		REGEXP = /v([0-9]+):\{([^}]*)\}/

Changes to MiqPassword.sanitize_string to support URL encoded password. #373

Changes to MiqPassword.sanitize_string to support URL encoded password. #373

Conversation

lfu commented Sep 14, 2018

Choose a reason for hiding this comment

bdunne Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Sep 17, 2018

kbrock left a comment • edited

Choose a reason for hiding this comment

kbrock Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfu commented Sep 18, 2018

bdunne commented Sep 18, 2018

JPrause commented Sep 18, 2018

kbrock left a comment

Choose a reason for hiding this comment

kbrock Sep 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Sep 19, 2018

miq-bot commented Sep 19, 2018

kbrock left a comment

Choose a reason for hiding this comment

simaishi commented Sep 26, 2018

simaishi commented Oct 1, 2018

bdunne Sep 14, 2018 •

edited

kbrock left a comment •

edited

kbrock Sep 17, 2018 •

edited

kbrock Sep 19, 2018 •

edited