IPA displays IPMI credentials in DEBUG logs during cleaning

[Tony Breeds]

While looking at an unrelated bug I noticed that IPA when run at debug level will display in the clear IPMI (and probably other drivers) details. If the logs are fed into logstash or similar this disclosure can result in a user being able to disrupt baremetal nodes.

See: http://logs.openstack.org/99/501799/24/check/ipa-tempest-dsvm-partition-bios-agent_ipmitool-coreos-src/6039bde/logs/ironic-bm-logs/node-0_console_2018-01-23-01:15:21.txt.gz

This is similar to https://bugs.launchpad.net/ironic/+bug/1572796

[Dmitry Tantsur (divius)]

The draft master patch attached, please review. I did not test it yet, as I don't have an environment ready for cleaning. Tony, do you think you could test it for me?

[Ruby Loo (rloo)]

Thanks for the patch Dmitry. I didn't test it, but I think it works. My comments:

wrt ironic/db/sqlalchemy/models.py:
+ if secure:
+ d['driver_info'] = strutils.mask_dict_password(
+ d.get('driver_info', {}), "******")

It looks to me that d['driver_info'] could initially be None; this will change it to {}. Which is probably fine, but I'm not sure. Was wondering about changing it to:
    if secure and d['driver_info']:
        d['driver_info'] = strutils.mask_dict_password(d['driver_info'], "******")

My other thought is whether we also want to do this for d['instance_info']. It isn't an issue here, but since we add that 'secure' parameter, users might assume that we have also handled instance_info.

-----------------------------

wrt ironic/tests/unit/drivers/modules/test_agent_client.py:
Even though nothing is done in as_dict(), maybe we should add secure=True to the method calls (L248 & L264)

[Dmitry Tantsur (divius)]

IIRC driver_info, being a JSON field, cannot come as None from the database. As to instance_info, I agree, we seem to hide it in the API, so we should hide it here. I'll update the patch as soon as Julia report her testing results.

[Julia Kreger (juliaashleykreger)]

The proposed patch does not work and causes cleaning to fail in an unexpected way:

| maintenance | True |
| maintenance_reason | Asynchronous exception for node 4e41df61-84b1-5856-bfb6-6b5f2cd3dd11: |
| | Node failed to start the first cleaning step. Exception: as_dict() got |
| | an unexpected keyword argument 'secure' |

Verified my patched deployment passes unit/py35 tests.

For what is is worth, we don't need to update instance_info. instance_info is wiped in the teardown sequence and if I remember correctly the virt driver code also removes the entries when it tears the node down, so realistically nothing should be present.

[Ruby Loo (rloo)]

Julia, thanks for testing. I should have looked at it in more detail. That is a Node object, not a db object. I think that the as_dict() that got invoked was from objects.base.IronicObject [1].

wrt the node's instance_info. In this case, we don't need to update it. However, since we're adding a 'secure' to the Node, in the future, someone might use it for something else and assume that the node dict they get back is secure.

The alternative is to figure out what information the agent needs, and only pass it that info. But that'd take us longer to figure out I think.

[1] https://github.com/openstack/ironic/blob/7f64a501c3dd95d20c25e4872226b10dbec207fc/ironic/objects/base.py#L66

[Julia Kreger (juliaashleykreger)]

Rloo, I agree with that logic for instance_info. The alternative is problematic, although really all information, at least in most cases should be in the driver_internal_info field... well beyond properties.

Driver and instance info should be good to blank out when requesting secure. If we have anything that is not using driver_internal_info, then we should treat that as an unrelated bug.

[Dmitry Tantsur (divius)]

I cannot understand how we end up using RPC objects without RPC.. But fine, I can update the patch.

[Tony Breeds (o-tony)]

Sorry I can happily check any patches that need checking. I trust y'all to do the analysis but my impression was that this happened in more cases than just cleaning.

[Dmitry Tantsur (divius)]

New version of the patch for review. Changed to modify the Node RPC object.

[Julia Kreger (juliaashleykreger)]

Attached is the resulting output from IPA upon boot where the password was being leaked. We can see that the password is masked in this case. Performing additional testing at the moment.

[Ruby Loo (rloo)]

Thanks for the patch Dmitry. It looks fine to me, but I didn't test it. Julia is going to do that. It occurs to me that the best way to prevent this from happening in the future, is to set the default as secure=True. However, that may mean a lot of code changes :-(

[Dmitry Tantsur (divius)]

I'm ++ to secure=True in master, but it may be too risky for stable branches. So maybe as a follow-up?

[Ruby Loo (rloo)]

Yup, follow up (or not at all, I'm not sure how involved it would be). I didn't mean for it to be done *now* :)

Also, wanted to point out that the code in agent_client.py has been around since kilo, when we implemented cleaning. We're only going to patch Ocata & Pike, right?

[Dmitry Tantsur (divius)]

Upstream - yes. We cannot patch non-existing branches :) I will prepare and paste here at least a Newton patch, as I need it downstream.

[Dmitry Tantsur (divius)]

Hi all,

I talked to Summer and Pedro (cc'ed) from Red Hat security team about this issue. They tend to think this is not a CVE, mostly because it only happens in the DEBUG mode.

I suggest we proceed with reviewing the patch here. At some point next week we just land it and all backports. Then we request releases from all branches, including the final one for master.

I guess this day will be next Wednesday then. What do you think?

P.S.
/me updates the patch to include a "security" release note.

[Dmitry Tantsur (divius)]

Good news: the patch applies cleanly to all branches Pike - Newton!

[Dmitry Tantsur (divius)]

Tested the patch on a master devstack. During cleaning I could no longer see plain-text ipmi_password anywhere in the machine logs. I did the same exercise with deployment and could not find any mentions of ipmi_password.

I'd like to publish the bugs and all patches tomorrow, EU afternoon. Please review the code.

[Ruby Loo (rloo)]

The patch looks good to me. Thanks!

If you do another revision, I have a few nits:

ironic/tests/unit/objects/test_node.py:
- we could also test self.node.instance_info['configdrive'] (or 'rescue_password')

releasenotes/notes/node-credentials-cleaning-b1903f49ffeba029.yaml:
- s/from node's/from a node's/
- s/on cleaning/during cleaning/

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/pike
Review: https://review.openstack.org/541703

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ocata
Review: https://review.openstack.org/541704

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/541683
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=c2185469c4a9246fa62ceae76676a08ed84e686d
Submitter: Zuul
Branch: master

commit c2185469c4a9246fa62ceae76676a08ed84e686d
Author: Dmitry Tantsur <email address hidden>
Date: Tue Jan 23 16:13:29 2018 +0100

    Do not pass credentials to the ramdisk on cleaning

    Currently the driver_info is passed as is to the ramdisk when calling
    get_clean_steps or execute_clean_step. This may lead to their exposure,
    as ironic<->ramdisk communication is currently not secure.

    This change applies the same logic we use in the API to filter
    the fields.

    Change-Id: I4fd44786fea6c7092d2b0029cea6d680d31babde
    Closes-Bug: #1744836

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/541704
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=87a298ff706bd81dfa37de789da8ecfaef3c569e
Submitter: Zuul
Branch: stable/ocata

commit 87a298ff706bd81dfa37de789da8ecfaef3c569e
Author: Dmitry Tantsur <email address hidden>
Date: Tue Jan 23 16:13:29 2018 +0100

    Do not pass credentials to the ramdisk on cleaning

    Currently the driver_info is passed as is to the ramdisk when calling
    get_clean_steps or execute_clean_step. This may lead to their exposure,
    as ironic<->ramdisk communication is currently not secure.

    This change applies the same logic we use in the API to filter
    the fields.

    Change-Id: I4fd44786fea6c7092d2b0029cea6d680d31babde
    Closes-Bug: #1744836
    (cherry picked from commit c2185469c4a9246fa62ceae76676a08ed84e686d)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/541703
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=ec80191e815196262bd865695ba6f6f9b56f707a
Submitter: Zuul
Branch: stable/pike

commit ec80191e815196262bd865695ba6f6f9b56f707a
Author: Dmitry Tantsur <email address hidden>
Date: Tue Jan 23 16:13:29 2018 +0100

    Do not pass credentials to the ramdisk on cleaning

    Currently the driver_info is passed as is to the ramdisk when calling
    get_clean_steps or execute_clean_step. This may lead to their exposure,
    as ironic<->ramdisk communication is currently not secure.

    This change applies the same logic we use in the API to filter
    the fields.

    Change-Id: I4fd44786fea6c7092d2b0029cea6d680d31babde
    Closes-Bug: #1744836
    (cherry picked from commit c2185469c4a9246fa62ceae76676a08ed84e686d)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/ironic 10.1.0 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/ironic 7.0.4 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/ironic 9.1.3 release.