On 10/12/2024 15:38, Jeremy Stanley wrote:
On 2024-12-10 14:50:39 +0100 (+0100), Thomas Goirand wrote:
I didn't feel it would be controversial, though it seems removing md5 password injection is still up to debate:
https://review.opendev.org/c/openstack/nova/+/935512
Of course, I'd like the TC to agree with me that injecting md5-hashed passwords is, in 2024, to be considered a security problem that should be fixed (and backported) ASAP.
BTW, IMO this patch could be using the new feature from oslo_utils.secretutils that Takashi managed to get in: https://review.opendev.org/c/openstack/oslo.utils/+/931899 https://review.opendev.org/c/openstack/oslo.utils/+/935525
yes it could but that obviously is also not back portable to stable branches since we can rely on those being aviabale. that is fine on master but if you wanted to use this in master and also have somethign backportable we would have to do an opportunistic import and fallback to the existign code or have this be two different patches. a backportable chagne and a master only change. we cant raise our min oslo utils version on stable to require this without breaking stable policy.
While I agree, this will need extensive manual testing with a wide variety of guest operating systems. See the PORTABILITY NOTES section of the crypt(3) manpage, but basically POSIX doesn't guarantee support for any particular hashes and options, so just because the host where libcrypt is being called supports a particular combination, that doesn't mean the guest it's injected into will be able to parse it.
I also agree with comments on the nova change that this mechanism ought to be at least strongly discouraged for use on any platforms where local agents are able to set passwords from metadata (sounds like it already is?), since is neatly sidesteps the portability problem. Deprecation/removal would be great, but it sounds like Windows doesn't have a functional guest agent capable of this?
the only non deprecated way to inject an admin password in nova today is the qemu guest agent. the part of the code that is using md5 is part of the deprecated file injection code path for setting the admin password. it may not be explicitly called out in https://specs.openstack.org/openstack/nova-specs/specs/queens/implemented/de... but it is deprecated for removal since queens.
These, IMO, should also be backported to earlier oslo.utils releases, so we can fix earlier OpenStack releases in a nicer way. I doubt we'll get consensus on this. As you say, it will explicitly drop support for some older guest platforms, which doesn't seem consistent with our usual policy for bug fixes on stable branches. That said, if it's a long-time deprecated feature anyway, maybe loss of some functionality in it is less risky (just in this specific case)?
at the ptg we discussed finally just removing the file base password injection feature https://etherpad.opendev.org/p/r.4f297ee4698e02c16c4007f7ee76b7c1#L541 i would prefer to actually do that instead of modify it to use sha512. with that said we also accepted that we could just use oslo_utils.secretutils to optional support crypt package if installed if there was a strong objection to removing this code. are you raising such an objection? ill also note that file inject is off by default so for md5 code to execute you would have to opt into the file injection feature when deploying your cloud which is consider insecure and an end user woudl have had to request it in the api. file injection in this fashion breaks usecase related to confidential computing as we are actively modifying the content of the root disk, and it does not work with boot from volume guests. i strongly suspect it wont work properly with nova backed by ceph too at least in the case were we are doing the efficient thin cloning of the volumes. so the utility of what little support still exists is questionable.