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?