search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.16k stars 5.48k forks source link

[BUG] Broken key fingerprinting #63742

Closed johnnybubonic closed 1 year ago

johnnybubonic commented 1 year ago

Description This is essentially a re-open of https://github.com/saltstack/salt/issues/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.

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: ```yaml 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: ```yaml 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 commented 1 year ago

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

johnnybubonic commented 1 year ago

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

twangboy commented 1 year ago

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

nf-brentsaner commented 1 year ago

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 commented 1 year ago

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