Salt master fails to authenticate minion key with mismatched line terminators

[cruscio]

Description of Issue/Question

The salt master will fail to authenticate a minion when the minion's minion.pem has CRLF line terminators and the master's /etc/salt/pki/master/minions/minion_id file has LF line terminators. This happened when pre-seeding a Windows minion.

The minion and/or master should normalize line terminators before attempting to authenticate.

salt@f89d9528b188:/etc/salt/pki/master$ tail /var/log/salt/master -n 1
2019-03-22 13:50:44,040 [salt.transport.mixins.auth:256 ][ERROR   ][22] Authentication attempt from my_minion_id failed, the public keys did not match. This may be an attempt to compromise the Salt cluster.

salt@f89d9528b188:/etc/salt/pki/master$ diff minions/my_minion_id minions_denied/my_minion_id
1,9c1,9
< -----BEGIN PUBLIC KEY-----
< MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoe5QSDYRWKyknbVyRrIj
< rm1ht5HgKzAVUber0x54+b/UgxTd1cqI6I+eDlx53LqZSH3G8Rd5cUh8LHoGedSa
< E62vEiLAjgXa+RdgcGiQpYS8+Z2RvQJ8oIcZgO+2AzgBRHboNWHTYRRmJXCd3dKs
< 9tcwK6wxChR06HzGqaOTixAuQlegWbOTU+X4dXIbW7AnuQBt9MCib7SxHlscrqcS
< cBrRvq51YP6cxPm/rZJdBqZhVrlghBvIpa45NApP5PherGi4AbEGYte4l+gC+fOA
< osEBis1V27djPpIyQS4qk3XAPQg6CYQMDltHqA4Fdo0Nt7SMScxJhfH0r6zmBFAe
< BQIDAQAB
< -----END PUBLIC KEY-----
\ No newline at end of file
---
> -----BEGIN PUBLIC KEY-----
> MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoe5QSDYRWKyknbVyRrIj
> rm1ht5HgKzAVUber0x54+b/UgxTd1cqI6I+eDlx53LqZSH3G8Rd5cUh8LHoGedSa
> E62vEiLAjgXa+RdgcGiQpYS8+Z2RvQJ8oIcZgO+2AzgBRHboNWHTYRRmJXCd3dKs
> 9tcwK6wxChR06HzGqaOTixAuQlegWbOTU+X4dXIbW7AnuQBt9MCib7SxHlscrqcS
> cBrRvq51YP6cxPm/rZJdBqZhVrlghBvIpa45NApP5PherGi4AbEGYte4l+gC+fOA
> osEBis1V27djPpIyQS4qk3XAPQg6CYQMDltHqA4Fdo0Nt7SMScxJhfH0r6zmBFAe
> BQIDAQAB
> -----END PUBLIC KEY-----

salt@f89d9528b188:/etc/salt/pki/master$ diff -w minions/my_minion_id minions_denied/my_minion_id

salt@f89d9528b188:/etc/salt/pki/master$ file minions/my_minion_id 
minions/my_minion_id: ASCII text

salt@f89d9528b188:/etc/salt/pki/master$ file minions_denied/my_minion_id 
minions_denied/my_minion_id: ASCII text, with CRLF line terminators

Setup

Preseed a Windows minion such that the C:\salt\conf\pki\minion\minion.pem file has CRLF line terminators.

In theory, it'd also fail if you convert an existing minion.pem to CRLF terminators on any Windows or Linux minion

Steps to Reproduce Issue

Start the salt minion
See authentication errors in both the minion and master logs
See a new denied key on the master

Versions Report

Salt Master

Salt Version:
           Salt: 2018.3.3
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.7
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.15rc1 (default, Nov 12 2018, 14:31:15)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5
 
System Versions:
           dist: Ubuntu 18.04 bionic
         locale: ANSI_X3.4-1968
        machine: x86_64
        release: 4.15.0-1037-azure
         system: Linux
        version: Ubuntu 18.04 bionic

Salt Minion

Salt Version:
           Salt: 2018.3.2
 
Dependency Versions:
           cffi: 1.10.0
       cherrypy: 10.2.1
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.3
          ioflo: Not Installed
         Jinja2: 2.9.6
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.6
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: 2.17
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.14 (v2.7.14:84471935ed, Sep 16 2017, 20:25:58) [MSC v.1500 64 bit (AMD64)]
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.3
           RAET: Not Installed
          smmap: 2.0.3
        timelib: 0.2.4
        Tornado: 4.5.1
            ZMQ: 4.1.6
 
System Versions:
           dist:   
         locale: cp1252
        machine: AMD64
        release: 2012ServerR2
         system: Windows
        version: 2012ServerR2 6.3.9600  Multiprocessor Free

[Ch3LL]

Do you know if this is a regression?

[cruscio]

I do not know for sure. It's my first time preseeding windows minions.

There doesn't seem to be any recent history of change to how salt.transport.mixins.auth opens the minion's public key on the master.

And while there have been some changes to salt.crypt.get_rsa_pub_key related to M2Crypto, that's not installed on my minion.

There may be other places a change could have broken this, but it's such a niche issue that it wouldn't be too unlikely to have always existed.

[twangboy]

That comparison is: pubfn_handle.read().strip() != load['pub'].strip():
What if we did something like this:

pubfn_handle.read().strip().replace('\r', '').replace('\n', '') != load['pub'].strip().replace('\r', '').replace('\n', ''):

Maybe even throw a .lower() in there

EDIT: The .lower() idea is stupid... sorry.

[erwindon]

That comparison is: pubfn_handle.read().strip() != load['pub'].strip():
What if we did something like this:

pubfn_handle.read().strip().replace('\r', '').replace('\n', '') != load['pub'].strip().replace('\r', '').replace('\n', ''):

Maybe even throw a .lower() in there

strip already removes CR and LF characters
print(len(" test \r\n".strip())) prints 4, the length of the word test
(just passing by... sorry to add to the confusion)

[cruscio]

.strip() (py2/py3) only removes leading and trailing white space. Newline terminators in the middle of a multi-line string aren't removed. Try print(len(" test \r\n string \r\n".strip()))

Re .lower(), these RSA keys are case sensitive, aren't they?

[erwindon]

@cruscio You are correct! I should have dived deeper into it before reacting...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[cruscio]

@Ch3LL , you tagged this Info Needed shortly after it was opened - Is that label still relevant? I'm almost certain this is not a regression, and is just an overlooked edge case that's always been there. Can't say for sure though.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[Ch3LL]

@cruscio thanks for the bump. will label as bug thanks.

ping @twangboy seems you had an idea on a fix here?

[defanator]

Just stepped into this (likely). We are spinning up a bunch of different minions as AWS EC2 VMs, and the only problematic one is Debian 10 arm64 from this AMI: ami-0d224ff05feac2300 (us-west-2).

We are using standard bootstrap-salt.sh (2023.04.26 as of now) everywhere across the stack for setting up salt on fresh VMs.

The scenario with denied keys between the master (3006.5 running on Debian 11 "bullseye") and minion on Debian 10 arm64 is currently 100% reproducible in our env. Collecting more information at the moment.

UPDATE: it seems like a bootstrap issue, preconfigured repository shows amd64 arch instead of arm64 one:

admin@ip-10-212-32-87:/etc/apt/sources.list.d$ cat salt.list 
deb [signed-by=/usr/share/keyrings/salt-archive-keyring.gpg arch=amd64] https://repo.saltproject.io/salt/py3/debian/10/amd64/3006/ buster main

So bootstrap eventually installs salt-minion from Debian's own repos where it's quite ancient:

admin@ip-10-212-32-87:/etc/apt/sources.list.d$ dpkg -s salt-minion
Package: salt-minion
Status: install ok installed
Priority: optional
Section: admin
Installed-Size: 111
Maintainer: Debian Salt Team <pkg-salt-team@lists.alioth.debian.org>
Architecture: all
Source: salt
Version: 2018.3.4+dfsg1-6+deb10u3
Depends: bsdmainutils, dctrl-tools, lsb-base (>= 3.0-6), python3-crypto (>= 2.6), python3-systemd | sysvinit-core, python3-zmq (>= 13.1.0), salt-common (= 2018.3.4+dfsg1-6+deb10u3), python3:any
Recommends: debconf-utils, dmidecode, e2fsprogs, sfdisk
Suggests: python3-augeas
Conffiles:
 /etc/init.d/salt-minion e9f07842654aa259520859354cdd77ec
 /etc/salt/minion a46af5c6fa7298b48989c734073c4e4f
Description: client package for salt, the distributed remote execution system
 salt is a powerful remote execution manager that can be used to
 administer servers in a fast and efficient way.
 .
 It allows commands to be executed across large groups of
 servers. This means systems can be easily managed, but data can
 also be easily gathered.  Quick introspection into running
 systems becomes a reality.
 .
 Remote execution is usually used to set up a certain state on a
 remote system. Salt addresses this problem as well, the salt
 state system uses salt state files to define the state a server
 needs to be in.
 .
 Between the remote execution system, and state management Salt
 addresses the backbone of cloud and data center management.
 .
 This particular package provides the worker / agent for salt.
Homepage: http://saltstack.org/

Doing some tests to confirm.

UPDATE: confirmed:

root@ip-10-212-32-87:/tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112# /tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112/deploy.sh -c /tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112 -K -d onedir 3006
 *  INFO: Running version: 2023.11.07
 *  INFO: Executed by: /bin/sh
 *  INFO: Command line: '/tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112/deploy.sh -c /tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112 -K -d onedir 3006'

 *  INFO: System Information:
 *  INFO:   CPU:          
 *  INFO:   CPU Arch:     aarch64
 *  INFO:   OS Name:      Linux
 *  INFO:   OS Version:   4.19.0-25-arm64
 *  INFO:   Distribution: Debian 10

 *  INFO: Installing minion
 *  INFO: Found function install_debian_onedir_deps
 *  INFO: Found function config_salt
 *  INFO: Found function preseed_master
 *  INFO: Found function install_debian_onedir
 *  INFO: Found function install_debian_restart_daemons
 *  INFO: Found function daemons_running_onedir
 *  INFO: Running install_debian_onedir_deps()
Hit:1 http://security.debian.org/debian-security buster/updates InRelease
Hit:2 http://cdn-aws.deb.debian.org/debian buster InRelease
Hit:3 http://cdn-aws.deb.debian.org/debian buster-updates InRelease
Hit:4 http://cdn-aws.deb.debian.org/debian buster-backports InRelease
Reading package lists...
Reading package lists...
Building dependency tree...
Reading state information...
pciutils is already the newest version (1:3.5.2-1).
procps is already the newest version (2:3.3.15-2).
python3-yaml is already the newest version (3.13-2).
The following packages were automatically installed and are no longer required:
  dctrl-tools dmidecode libnorm1 libpgm-5.2-0 libsodium23 libzmq5
  python3-croniter python3-msgpack python3-psutil python3-systemd
  python3-tornado4 python3-tz python3-zmq
Use 'apt autoremove' to remove them.
0 upgraded, 0 newly installed, 0 to remove and 43 not upgraded.
 *  WARN: Support for arm64 packages is experimental and might rely on architecture-independent packages from the amd64 repository.
Reading package lists...
Building dependency tree...
Reading state information...
apt-transport-https is already the newest version (1.8.2.3).
ca-certificates is already the newest version (20200601~deb10u2).
gnupg2 is already the newest version (2.2.12-1+deb10u2).
wget is already the newest version (1.20.1-1.1).
The following packages were automatically installed and are no longer required:
  dctrl-tools dmidecode libnorm1 libpgm-5.2-0 libsodium23 libzmq5
  python3-croniter python3-msgpack python3-psutil python3-systemd
  python3-tornado4 python3-tz python3-zmq
Use 'apt autoremove' to remove them.
0 upgraded, 0 newly installed, 0 to remove and 43 not upgraded.
Hit:1 http://cdn-aws.deb.debian.org/debian buster InRelease
Hit:2 http://cdn-aws.deb.debian.org/debian buster-updates InRelease
Hit:3 http://security.debian.org/debian-security buster/updates InRelease
Hit:4 http://cdn-aws.deb.debian.org/debian buster-backports InRelease
Get:5 https://repo.saltproject.io/salt/py3/debian/10/amd64/3006 buster InRelease [1589 B]
Get:6 https://repo.saltproject.io/salt/py3/debian/10/amd64/3006 buster/main amd64 Packages [6867 B]
Fetched 8456 B in 1s (16.1 kB/s)
Reading package lists...
 *  INFO: Running config_salt()
 *  INFO: Running install_debian_onedir()
Reading package lists...
Building dependency tree...
Reading state information...
The following additional packages will be installed:
  salt-common
Suggested packages:
  python3-mako salt-doc python3-augeas
Recommended packages:
  sfdisk
The following NEW packages will be installed:
  salt-common salt-minion
0 upgraded, 2 newly installed, 0 to remove and 43 not upgraded.
                                                               Need to get 0 B/3304 kB of archives.
                                                                                                   After this operation, 24.4 MB of additional disk space will be used.
                                                                                                                                                                       Selecting previously unselected package salt-common.
(Reading database ... 27421 files and directories currently installed.)
Preparing to unpack .../salt-common_2018.3.4+dfsg1-6+deb10u3_all.deb ...
Unpacking salt-common (2018.3.4+dfsg1-6+deb10u3) ...
Selecting previously unselected package salt-minion.
Preparing to unpack .../salt-minion_2018.3.4+dfsg1-6+deb10u3_all.deb ...
Unpacking salt-minion (2018.3.4+dfsg1-6+deb10u3) ...
Setting up salt-common (2018.3.4+dfsg1-6+deb10u3) ...
Setting up salt-minion (2018.3.4+dfsg1-6+deb10u3) ...

Configuration file '/etc/salt/minion'
 ==> File on system created by you or by a script.
 ==> File also in package provided by package maintainer.
 ==> Using current old file as you requested.
Created symlink /etc/systemd/system/multi-user.target.wants/salt-minion.service → /lib/systemd/system/salt-minion.service.
Processing triggers for man-db (2.8.5-2) ...
Processing triggers for systemd (241-7~deb10u10) ...
 *  INFO: Running install_debian_restart_daemons()
 *  INFO: Running daemons_running_onedir()
 *  INFO: Salt installed!
 * 
root@ip-10-212-32-87:/tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112# cat /etc/apt/sources.list.d/salt.list 
deb [signed-by=/usr/share/keyrings/salt-archive-keyring.gpg arch=amd64] https://repo.saltproject.io/salt/py3/debian/10/amd64/3006/ buster main

UPDATE: this patch fixes the issue for us:

root@ip-10-212-32-87:/tmp/.saltcloud-a444a9ed-3a2a-4e48-870e-c7d3af35c112# diff -u deploy.sh.orig deploy.sh
--- deploy.sh.orig	2023-12-15 08:48:41.712883695 +0000
+++ deploy.sh	2023-12-15 08:59:51.120719109 +0000
@@ -1521,9 +1521,9 @@
             if [ "$_CUSTOM_REPO_URL" != "null" ]; then
                 warn_msg="Support for arm64 is experimental, make sure the custom repository used has the expected structure and contents."
             else
-                # Saltstack official repository has arm64 metadata beginning with Debian 11,
+                # Saltstack official repository has arm64 metadata beginning with Debian 10,
                 # use amd64 repositories on arm64 for anything older, since all pkgs are arch-independent
-                if [ "$DISTRO_NAME_L" = "debian" ] && [ "$DISTRO_MAJOR_VERSION" -lt 11 ]; then
+                if [ "$DISTRO_NAME_L" = "debian" ] && [ "$DISTRO_MAJOR_VERSION" -lt 10 ]; then
                   __REPO_ARCH="amd64"
                 else
                   __REPO_ARCH="arm64"
@@ -9759,4 +9759,4 @@
 
 exit 0
 
-# vim: set sts=4 ts=4 et
\ No newline at end of file
+# vim: set sts=4 ts=4 et

Not sure if it makes sense as is because I don't see salt packages for Debian versions < 10, but still.

[twangboy]

This issue should be fixed by the above PR. If this is still broken, please open a new issue and reference this one.