[BUG] Broken key fingerprinting

[johnnybubonic]

Description
This is essentially a re-open of #14593 but with a proposed cross-platform solution (that is, it should be noted, a more "proper" way of fingerprinting a key).

Setup
1.) Set hash_type to sha512 on both a Linux master and Windows minion (this is probably unnecessary, as the same code should be used on both.)
2.) Set master_finger in the Windows minion's config file to the output of salt-key -f master.pub on the Linux master.
3.) Get the following (critical) error:

The specified fingerprint in the master configuration file:
<output of `salt-key -f master.pub` on master>
Does not match the authenticating master's key:
<completely incorrect fingerprint>
Verify that the configured fingerprint matches the fingerprint of the correct master and that this minion is not subject to a man-in-the-middle attack.

• on-prem machine
• [Hyper-V, Libvirt w/KVM] VM (Virtualbox, KVM, etc. please specify)
• VM running on a cloud service, please be explicit and add details
• container (Kubernetes, Docker, containerd, etc. please specify)
• or a combination, please be explicit
• jails if it is FreeBSD
• classic packaging
• onedir packaging
• used bootstrap to install

Steps to Reproduce the behavior
See above,

Expected behavior
The Windows minion should properly fingerprint the Master's key.

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Master:

Salt Version:
          Salt: 3005.1

Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.0
       libgit2: 1.5.0
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: 1.11.1
        Python: 3.9.16 (main, Jan  6 2023, 22:52:19)
  python-gnupg: 0.4.8
        PyYAML: 5.4.1
         PyZMQ: 23.2.0
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist: almalinux 8.7 Stone Smilodon
        locale: utf-8
       machine: x86_64
       release: 4.18.0-425.10.1.el8_7.x86_64
        system: Linux
       version: AlmaLinux 8.7 Stone Smilodon

Minion:

Salt Version:
          Salt: 3005.1

Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: 4.0.7
     gitpython: Not Installed
        Jinja2: 3.1.0
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: 1.1.4
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.10.1
        pygit2: Not Installed
        Python: 3.8.16 (tags/v3.8.16:1e3d2d5, Dec  9 2022, 17:48:54) [MSC v.1916 64 bit (AMD64)]
  python-gnupg: 0.4.8
        PyYAML: 5.4.1
         PyZMQ: 22.0.3
         smmap: 4.0.0
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist:
        locale: cp1252
       machine: AMD64
       release: 2022Server
        system: Windows
       version: 2022Server 10.0.20348 SP0 Multiprocessor Free

Additional context
ROOT CAUSE:
This is due to twofold problems:

1.) Window performing the hashing in pem_finger (salt/utils/crypt.py) against a CRLF-line-terminated (\r\n) file (master_minion.pem), whereas Linux master (and Linux minions) performing hashing against the (much more sane, of course) LF-terminated (\n) file (either master.pub or minion_master.pub depending on context).

2.) A very silly way of calculating a key's fingerprint.

1 is inexcusable. There is no reason a minion's platform should need a different checksum for its master's finger. This is bad design.

THE PROPER SOLUTION:

But thankfully, a more sane approach to 2 also simultaneously completely fixes 1, and with no code porting across platforms or checks thereof - it's completely cross-platform.

So, let's take an example Master pubkey. (This is generated explicitly for the purpose of demonstration.)

(The key was generated with hash_type: sha512 and key_size: 4096 on the master, configured before first-run. Now would be a good time to mention that specifying a 4096-bit key in the config does not properly generate a 4096-bit master key and instead only generates a 2048-bit key... but that's a bug for another day.)

-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAqrIK3hYb4dHGXWtdpq8b
7QCkSYoT1s4i52AZxPoZ4KBNgQhj+g/f5ikZ2DaEW8nsL/Spc/dTnlftiEpvUOb2
sSe/6NElNcRML+IwCQ+tY5nEZEzQeXsP1Wz7hazsj0+Ahts50xYDkR6P/2TdEb40
O1I/zYIvKbCtaLkMsHa5UBdfox1YVu36JAVSFWF4YDKppXo7NNX4rUoRiMpXBve1
ovPvZoyOuDxAAbG+AzSwpWQUt6ZANHmaB3UvP9ULLtmciK3RN2mW93dPVIun+mnL
tSW9Tn6KmRDpvHCPtpauE7IAD+AZSKcs702/k3v55LWWdSKbwpQSD4vjaUvxekM9
6QIDAQAB
-----END PUBLIC KEY-----

This is a PEM-encoded 2048-bit RSA public key, using PKIX format.

# openssl rsa -pubin -in master.pub -noout -text
Public-Key: (2048 bit)
Modulus:
    (...)
Exponent: 65537 (0x10001)

# openssl rsa -in master.pem -noout -text
Private-Key: (2048 bit, 2 primes)
modulus:
    (...)
publicExponent: 65537 (0x10001)
privateExponent:
    (...)
prime1:
    (...)
prime2:
    (...)
exponent1:
    (...)
exponent2:
    (...)
coefficient:
    (...)

And, as per PEM encoding specifications, the block contained between the start label (and headers, not present) and end label is Base64-Encoded ASN.1.

Which means that a more proper fingerprinting (aside from encoding as an actual ASN.1 field itself, which is a bit of a hassle) would be to base64-DECODE the PEM block, and hash that.

A nice feature of Python's stdlib base64 library is that it is newline-agnostic. It doesn't care if there are newlines in base64 or not. It doesn't care if they're line-terminated with LF or CRLF.

#!/usr/bin/env python3

import base64
import hashlib

key_pem_linux = '-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAvD9IDVty65y/vFFyv9rc\nhVDg6Do4Ealr9RGDRhJ11Tw/NK8iCP+I4IW5MbfNbSUURW39LTq9oiTMVc2w2+zo\nIGJ3dzdLtw9cSCtdzm8R6SAayTYG7+IrtMKWjGqdOFhD9FSH8ma+SBOo2nNgT8UW\nPIda2gajV2ZnXveil8VZj8Y7POKvzLJCQ1cjWAx6wyd5z3X40ibWWhLwFyQinmgU\nUSFIe6hcw5qAL3kpa6v+aE0S/VMECiT6mQMJyCO7w9TTi70QXOPAiXJWVo3CJ7IN\npRkSNEQr6x8+Ab7FK7ZPY1AO3HW02qdAsRoD2B4uiffX3lk/ARkDcfbnC0IhGXMh\n5wIDAQAB\n-----END PUBLIC KEY-----\n'

key_pem_windows = key_pem_linux.replace('\n', '\r\n')


# Parsing and fingerprinting the Linux-formatted keyfile.
key_linux = base64.b64decode('\n'.join(key_pem_linux.splitlines()[1:-1]).encode('utf-8'))
hash_linux = hashlib.sha512()
hash_linux.update(key_linux)
hash_linux = hash_linux.digest()

# Parsing and fingerprinting the Windows-formatted keyfile.
key_windows = base64.b64decode('\r\n'.join(key_pem_windows.splitlines()[1:-1]).encode('utf-8'))
hash_windows = hashlib.sha512()
hash_windows.update(key_windows)
hash_windows = hash_windows.digest()

# They match.
assert hash_linux == hash_windows

[OrangeDog]

So it's not actually broken right now, it could just be better?

[johnnybubonic]

No, it's absolutely broken in cross-platform environments.

[twangboy]

Though your method makes sense, we can't change the way we're creating the fingerprint as it would break existing installations.

[nf-brentsaner]

Shoot, that's gonna bug me; Salt is the only software I know of that fingerprints against PEM instead of DER/BER. But fair enough on the breakage (though it should be noted that by fixing the fingerprint calculation for Windows, those minions will break with the linked PR if they were using master_finger regardless).

Maybe earmark that method to implement if Salt public keygen is overhauled/broken/vuln in the future and installations would require rekeying anyways? That would let external tools properly generate/parse Salt keypairs/fingerprints as well.

[nf-brentsaner]

(Sorry, same guy as OP to clarify, this is my work GH)