mask_password is overzealous

[Matthew Edmonds]

I created a new cinder volume that included "password" in the name, and this caused a number of problems that were traced back to mask_password. The r'(%(key)s\s*--?[A-z]+\s*)\S+(\s*)' pattern matched the backend volume id ("volume-wmevol_san_password-fa002fa2-79d6") causing the strutils.mask_password(stdout) call in oslo_concurrency.processutils.ssh_execute() to take the correct output of a "svcinfo lsvdisk -bytes -delim :" command:

593:volume-wmevol_san_password-fa002fa2-79d6:0:io_grp0:online:0:npiv_pool:1073741824:striped:::::6005076802820A9DA80000000000AAD8:0:1:empty:1:no:0

and change it to:

593:volume-wmevol_san_password-fa****

It appears that this pattern was first added under [1] and then later modified with [2].

[1] https://github.com/openstack/oslo-incubator/commit/cdcc19c1d78a4a88daabfb64b27abd4924aa442d
[2] https://github.com/openstack/oslo-incubator/commit/e9bb0b596650540f336afb070a1f8c7de099721f

[Doug Hellmann (doug-hellmann)]

Was there a problem creating the volume, or just with the logging of the operation?

[Matthew Edmonds (edmondsw)]

Neither... the volume was created, but I was unable to delete it. I suspect this is the cause, and may cause other issues as well.

[Matthew Edmonds (edmondsw)]

I don't think volume attach would work on this volume, since the lsvdisk call here [1] should hit this problem:

def get_vdisk_attributes(self, vdisk):
    attrs = self.ssh.lsvdisk(vdisk)
    return attrs

Etc.

[1] https://github.com/openstack/cinder/blob/master/cinder/volume/drivers/ibm/storwize_svc/helpers.py#L633

[ChangBo Guo(gcb) (glongwave)]

Is this still a valid issue, from the bug description, it likes cinder use mask_password. We usuall use this method to log something. Can you point out how to use it in Cinder ?

[Sean McGinnis (sean-mcginnis)]

No response for more info. Please reopen if this is still seen.

[Launchpad Janitor (janitor)]

[Expired for oslo.utils because there has been no activity for 60 days.]

[ChangBo Guo(gcb) (glongwave)]

any update ?

[prashkre (prashkre)]

Same issue is seen in my environment i.e. volume details are masked by stdout = strutils.mask_password(stdout) at [0] when "password" string exists in its name. In cinder, oslo_concurrency/processutils.ssh_execute() at [1] is being invoked at many places to run a command over ssh session and parse the returned output from it to get required information but due to above masking behavior before output is parsed(outside of processutils.ssh_execute()) causing an issue.

[0] https://github.com/openstack/oslo.concurrency/blob/master/oslo_concurrency/processutils.py#L540
[1] https://github.com/openstack/oslo.concurrency/blob/master/oslo_concurrency/processutils.py#L504

[Matthew Edmonds (edmondsw)]

mask_password is just a mess. See comments in https://review.openstack.org/#/c/385197/ . At this point I'm convinced that we should have more specific methods for different cases, e.g. ssh_execute here, instead of a general method that tries to cover too many cases and thus can't handle any of them very well, which is what mask_password is today.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/537823

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/537823
Committed: https://git.openstack.org/cgit/openstack/oslo.concurrency/commit/?id=21ae27e66d77f276c23a7ca2eaa8869438fb4d31
Submitter: Zuul
Branch: master

commit 21ae27e66d77f276c23a7ca2eaa8869438fb4d31
Author: prashkre <email address hidden>
Date: Thu Jan 25 13:41:42 2018 +0530

    Mask passwords only when command execution fails

    At many places, processutils.ssh_execute() is being invoked to run
    a command over ssh and output returned is parsed to get appropriate
    information. In this flow, unsanitized output is being expected
    where processutils.ssh_execute() was invoked but found that
    output like volume details(containing "password" string in its name)
    is being masked away with strutils.mask_password(stdout) even though
    no error occured during command execution.

    This is regression issue from patch[0]. In this fix, stdout and stderr
    in processutils.ssh_execute() will be masked only when
    ProcessExecutionError exception is thrown i.e. command execution failed
    due to some reasons.

    [0] https://github.com/openstack/oslo.concurrency/commit/
    ae9e05bfc3d7ec867bd6ec78c301f11c2bdd0d5f

    Change-Id: I2ce344330905eef437ef3f89a2a01169a30df8ab
    Closes-Bug: #1482382

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/queens
Review: https://review.openstack.org/547388

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/547388
Committed: https://git.openstack.org/cgit/openstack/oslo.concurrency/commit/?id=0c4718fcb77e9f4e3a22ae458869b7294b7bc91f
Submitter: Zuul
Branch: stable/queens

commit 0c4718fcb77e9f4e3a22ae458869b7294b7bc91f
Author: prashkre <email address hidden>
Date: Thu Jan 25 13:41:42 2018 +0530

    Mask passwords only when command execution fails

    At many places, processutils.ssh_execute() is being invoked to run
    a command over ssh and output returned is parsed to get appropriate
    information. In this flow, unsanitized output is being expected
    where processutils.ssh_execute() was invoked but found that
    output like volume details(containing "password" string in its name)
    is being masked away with strutils.mask_password(stdout) even though
    no error occured during command execution.

    This is regression issue from patch[0]. In this fix, stdout and stderr
    in processutils.ssh_execute() will be masked only when
    ProcessExecutionError exception is thrown i.e. command execution failed
    due to some reasons.

    [0] https://github.com/openstack/oslo.concurrency/commit/
    ae9e05bfc3d7ec867bd6ec78c301f11c2bdd0d5f

    Change-Id: I2ce344330905eef437ef3f89a2a01169a30df8ab
    Closes-Bug: #1482382
    (cherry picked from commit 21ae27e66d77f276c23a7ca2eaa8869438fb4d31)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/549323

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.concurrency 3.26.0 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.concurrency 3.25.1 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/cinder 14.0.0.0rc1 release candidate.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements rocky-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements stein-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements yoga-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements train-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements ussuri-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements victoria-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements wallaby-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements xena-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/requirements zed-eom release.