[BUG] x509.certificate_managed rolls certificates every now and then

[stefan-as]

Description
Due to a combination of legacy Python2 Salt code and Python3.5 behavior on Debian Stretch, managed certificates are rolled every now and then.

Setup

sensu_ssl_cert:
  x509.certificate_managed:
    - name: /etc/sensu/ssl/sensu.crt
    - public_key: /etc/sensu/ssl/sensu.key
    - ca_server: {{ ca_server }}
    - signing_policy: {{ signing_policy }}
    - commonName: sensu.{{ grains.fqdn }}
    - days_valid: 90
    - days_remaining: 30
    - group: sensu
    - mode: 0644
    - require:
      - x509: sensu_ssl_key

Steps to Reproduce the behavior
salt-call state.apply
salt-call x509.read_certificate /etc/sensu/ssl/sensu.crt

Expected behavior
Idempotent behavior, no changes until the cert is out of date.

Screenshots

• state.apply leads to Certificate properties are different: Subject, Subject Hash.
• x509.read_certificate outputs Subject: commonName and Subject: CN by random.

Versions Report

Salt Version:
          Salt: 3002.2
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: 2.5.3
     docker-py: Not Installed
         gitdb: 2.0.0
     gitpython: 2.1.1
        Jinja2: 2.9.4
       libgit2: Not Installed
      M2Crypto: 0.37.1
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.6.1
        pygit2: Not Installed
        Python: 3.5.3 (default, Nov 18 2020, 21:09:16)
  python-gnupg: Not Installed
        PyYAML: 3.12
         PyZMQ: 17.1.2
         smmap: 2.0.1
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.2.1
 
System Versions:
          dist: debian 9 stretch
        locale: UTF-8
       machine: x86_64
       release: 4.9.0-14-amd64
        system: Linux
       version: Debian GNU/Linux 9 stretch

Additional context

The issue is caused by a combination of bugs. As far as I see, at least the first bug should be solved, since it concerns all versions of Python3.

1. In

   salt/salt/states/x509.py

   Line 436 in ea409f0

   if v != cert_info[k]:

   there is a comparison between unmatching types:

(Pdb) p k
'Subject Hash'
(Pdb) p v
b'9E:B4:30:22'
(Pdb) p cert_info[k]
'9E:B4:30:22'

This way the code path always takes the long way into deep checking, although the involved hashes would be fine. Since there are no Python2 versions for Salt 3002, the compatibility code could be dropped and replaced by a clean check of the hashes.

2. The deep check fails for Python<3.6 every now and then. This is caused by the fact that dictionaries are not stable in Python<3.6. In

   salt/salt/modules/x509.py

   Lines 333 to 335 in ea409f0

   	for nid_name, nid_num in subject.nid.items():
   	if nid_num in nids:
   	continue

   subject nid's are returned in a random manner, while CN and commonName uses the same nid_num. This leads to situations where a subject rendered as CN is compared to a subject containing commonName, they don't match and the cert is rolled over. This is the situation for at least Debian Stretch and could by solved by sorting subject.nid.items() before looping the nids.

[OrangeDog]

Seems like #56556 still.

[stefan-as]

@OrangeDog No, I don't think so. This is about comparing strings to bytes and random ordering because of unstable dicts prior Python 3.6. #56556 is about changed, but stable ordering.

[OrangeDog]

Sorry, the original was #52026. Despite the description of #56556 it was the same thing, and the same as what you describe.
I don't understand how it's still broken after so many PRs.
The whole exercise is also redundant because the other issue (#52167) requires a complete rewrite anyway,

[Ch3LL]

Can you try the latest version 3002.3? Do you still see the same behavior?

[stefan-as]

@Ch3LL Thanks for your response. There is no 3002.3 right now. We are running on 3002.2, what is the latest release (as far as I could figure out).

[sagetherage]

@stefan-as latest releases are here: https://github.com/saltstack/salt/releases or https://repo.saltproject.io/ but guessing you will still see this in 3002.6 or 3003, yes?

[Ch3LL]

can you see if #58296 fixes the issue for you

[OrangeDog]

@Ch3LL that change was in 3002.2 as far as I can tell, which is the version OP is reporting for.