[BUG] v3007.0 broken multi-master authentication when connecting with older minions

[rjel2159]

Description

The introduction of clean_key() in lib/python3.9/site-packages/salt/crypt.py (versions >3005.1) breaks signature verification on minions running older versions (<=3005.1)

Setup

Master running 3007.0 Minions running 3005.1 or older

# salt-call --version
salt-call 3005.1

[root@master123 root]# /usr/bin/salt --version
salt 3007.0 (Chlorine)

Steps to Reproduce the behavior

# salt-call --master master123 grains.items
<snip>
[DEBUG   ] salt.crypt.get_rsa_pub_key: Loading public key
[DEBUG   ] salt.crypt.verify_signature: Verifying signature
[DEBUG   ] Signature verification failed: bad signature
[DEBUG   ] Failed to verify signature of public key
[ERROR   ] Received signed public-key from master <XXXX> but signature verification failed!
[CRITICAL] The Salt Master server's public key did not authenticate!
The master may need to be updated if it is a version of Salt lower than 3005.1, or
If you are confident that you are connecting to a valid Salt Master, then remove the master public key and restart the Salt Minion.
The master public key can be found at:
/etc/salt/pki/minion/minion_master.pub

Expected behavior

[DEBUG   ] salt.crypt.get_rsa_pub_key: Loading public key
[DEBUG   ] salt.crypt.verify_signature: Verifying signature
[DEBUG   ] <salt.crypt.AsyncAuth object at 0xXXXXXXXXXX> Got new master aes key.
[DEBUG   ] Closing AsyncReqChannel instance
[DEBUG   ] Connecting the Minion to the Master publish port, using the URI: tcp://<ip>:<port>

Additional context

W/A:

# diff -u /local/data/saltcrystal/3007.0-1/lib/python3.9/site-packages/salt/crypt.py{,.saved}
--- /local/data/saltcrystal/3007.0-1/lib/python3.9/site-packages/salt/crypt.py  2024-03-22 16:02:04.008606926 -0400
+++ /local/data/saltcrystal/3007.0-1/lib/python3.9/site-packages/salt/crypt.py.saved    2024-03-22 15:51:27.907420781 -0400
@@ -87,8 +87,7 @@
     """
     Clean the key so that it only has unix style line endings (\\n)
     """
+    return key
+    #return "\n".join(key.strip().splitlines())
-    return "\n".join(key.strip().splitlines())

 def read_dropfile(cachedir):
#

[welcome[bot]]

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

• Community Wiki
• Salt’s Contributor Guide
• Join our Community Slack
• IRC on LiberaChat
• Salt Project YouTube channel
• Salt Project Twitch channel

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

[whytewolf]

this looks related to https://github.com/saltstack/salt/issues/66126

[rjel2159]

Thanks @whytewolf for the reply. This fix to 3007.x seems promising https://github.com/saltstack/salt/pull/66161/files - I'll test it shortly.

[rjel2159]

Unfortunately, the above fix (https://github.com/saltstack/salt/pull/66161) won't address our case where the clean_key() on the master "messes up" the master's signature verification on older minions which don't have this function in their code base. As far as I can see, the only way to move forward is either to upgrade all the minions to the version that introduced this new code at the same time as upgrading the master(s) (which is not easily feasible for saltenv's with 100k+ nodes), or apply the patch suggested in the description of this bug case.

[ixs]

Aditionally to the situation of the master being on 3007 and the minion on 3005, the opposite is also a problem:

I have a master being on 3005.5 with the minion being upgraded to 3007.0. The same problem is occuring that the key is not recognized anymore, and the recommended solution of deleting it and restarting the minion does not help either. Error is consistently Unable to sign_in to master: Invalid master key.

Having a closer look at the data passing through the function, it seems that the cleaned key for the master has an extra \n linebreak added to the end which gets removed by the clean function. At this point, the comparision fails. The minion key however does not have an added newline at the end of the file and does not suffer from this error.

Again, making the clean_key() function a noop by returning the passsed key variable unchanged resolves this problem but is only a workaround.

[rjel2159]

Aditionally to the situation of the master being on 3007 and the minion on 3005, the opposite is also a problem:

I have a master being on 3005.5 with the minion being upgraded to 3007.0. The same problem is occuring that the key is not recognized anymore, and the recommended solution of deleting it and restarting the minion does not help either. Error is consistently Unable to sign_in to master: Invalid master key.

Having a closer look at the data passing through the function, it seems that the cleaned key for the master has an extra \n linebreak added to the end which gets removed by the clean function. At this point, the comparision fails. The minion key however does not have an added newline at the end of the file and does not suffer from this error.

Again, making the clean_key() function a noop by returning the passsed key variable unchanged resolves this problem but is only a workaround.

Yes, that's exactly what I have found as well. We have no need for use of the clean_key() function in our environment, it only "breaks" the signature validation.

[whytewolf]

Aditionally to the situation of the master being on 3007 and the minion on 3005, the opposite is also a problem:

I have a master being on 3005.5 with the minion being upgraded to 3007.0. The same problem is occuring that the key is not recognized anymore, and the recommended solution of deleting it and restarting the minion does not help either. Error is consistently Unable to sign_in to master: Invalid master key.

Having a closer look at the data passing through the function, it seems that the cleaned key for the master has an extra \n linebreak added to the end which gets removed by the clean function. At this point, the comparision fails. The minion key however does not have an added newline at the end of the file and does not suffer from this error.

Again, making the clean_key() function a noop by returning the passsed key variable unchanged resolves this problem but is only a workaround.

the master being a lower version then the minion has never and will never be a supported setup.

see the first paragraph of https://docs.saltproject.io/salt/install-guide/en/latest/topics/upgrade.html

so while we are working on the fix for this in cases where the master is higher version. we will not do so for in cases where the minion is higher version. if the fix ends up working in such cases. great, but it is not a goal.