Salt: 2019.2/py3/x509 some comparison py3 types issues

[arsiesys]

Description of Issue/Question

ISSUE 1
While running a state x509.certificate_managed on the current x509 of the branches 2019.2 (not the one from the .deb which already fail before that). I got the following error:

          ID: mongodb_generate_cert_mongodb
    Function: x509.certificate_managed
        Name: /etc/ssl/mongodb.pem
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 1933, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1939, in wrapper
                  return f(*args, **kwargs)
                File "/var/cache/salt/minion/extmods/states/x509.py", line 556, in certificate_managed
                  file_args['contents'] += salt.utils.stringutils.to_str(certificate)
              TypeError: can't concat str to bytes
     Started: 15:25:42.075799
    Duration: 697.928 ms
     Changes:   
----------

The state is including a "managed_private_key" on the same file which is the use case at the origin of the previous error cause it will try to concatenate both certificates in a same file.

I was able to fix it by adding a .decode() on the following line:
https://github.com/saltstack/salt/pull/52014/commits/32ed6d438f51d741d74b81d0717c8c3b623fa977#diff-5499a295a50d60a761c34f4080e4014bR459

The certificate was then generated now.. but it's regenerated and replaced at each execution due to an other issue.

ISSUE 2
To decide if x509 is going to replace the local certificate or not, it will compare some elements of the certificate including the Issuer public key.. The local one, when read with the module will return a bytes type while the new generated one will be in str... Then the comparison fail and the certificate replaced...

I proposed the following fix:
https://github.com/saltstack/salt/pull/52014/commits/32ed6d438f51d741d74b81d0717c8c3b623fa977#diff-4dc56ad912db8d1856c286cd18fc6cdeR729

Versions Report

Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.32.0
           Mako: Not Installed
   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: 3.6.7 (default, Oct 22 2018, 11:32:17)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-1027-gcp
         system: Linux

[garethgreenaway]

@arsiesys Thanks for the report. Can you confirm that your proposed fix works consistently in both Py2 and Py3?

[arsiesys]

python 2.7
Hard to say... I just tried to install a 2019.2 python 2.7 minions/salt-master/ca-server without the fix and I get this error :(.

Maybe I made a wrong install or mistake somewhere... Will try to do further test an other day :/

    Function: x509.certificate_managed
        Name: /etc/ssl/mongodb.pem
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python2.7/dist-packages/salt/state.py", line 1933, in call
                  **cdata['kwargs'])
                File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 1939, in wrapper
                  return f(*args, **kwargs)
                File "/var/cache/salt/minion/extmods/states/x509.py", line 502, in certificate_managed
                  new = __salt__['x509.create_certificate'](testrun=True, **kwargs)
                File "/var/cache/salt/minion/extmods/modules/x509.py", line 1409, in create_certificate
                  if not any(certs):
              TypeError: 'NoneType' object is not iterable
     Started: 23:13:37.105092
    Duration: 2131.86 ms
     Changes:

Regarding the debug logs, it seems that the publish command reach the master but my ca "minion server" do not seems to react in debug logs (receive nothing ?)

[DEBUG   ] Sent payload to publish daemon.
[DEBUG   ] Closing AsyncZeroMQReqChannel instance
[DEBUG   ] salt.crypt.get_rsa_pub_key: Loading public key
[DEBUG   ] Initializing new AsyncZeroMQReqChannel for (u'/etc/salt/pki/master', u'salt-master-11089058.xxxxxxxx_master', u'tcp://0.0.0.0:4506', u'clear')
[DEBUG   ] Connecting the Minion to the Master URI (for the return server): tcp://0.0.0.0:4506
[DEBUG   ] Trying to connect to: tcp://0.0.0.0:4506
[DEBUG   ] Sending event: tag = 20190307231339188680; data = {u'_stamp': '2019-03-07T23:13:39.189059', u'minions': []}
[DEBUG   ] Gathering reactors for tag 20190307231339188680
[DEBUG   ] Sending event: tag = salt/job/20190307231339188680/new; data = {u'tgt_type': 'glob', u'jid': u'20190307231339188680', u'user': 'salt', u'tgt': "u'ca-10642002.xxxx'", u'arg': [{"u'CN'": "u'elasticsearch-master-0058ea70f7c3fe00a.xxxxx'", "u'signing_policy'": "u'core_graylog_mongodb'", "u'public_key'": "u'-----BEGIN PUBLIC KEY-----xxxxxxxx-----END PUBLIC KEY-----'", 'testrun': True, "u'public_key_passphrase'": 'None'}], u'fun': 'x509.sign_remote_certificate', u'missing': [], u'_stamp': '2019-03-07T23:13:39.191576', u'minions': []}
[DEBUG   ] Gathering reactors for tag salt/job/20190307231339188680/new
[DEBUG   ] Adding minions for job 20190307231339188680: []
[INFO    ] User salt Published command x509.sign_remote_certificate with jid 20190307231339188680
[DEBUG   ] Published command details {u'tgt_type': 'glob', u'jid': u'20190307231339188680', u'tgt': "u'ca-10642002.xxxxxxx'", u'ret': '', u'to': 5, u'user': 'salt', u'arg': [{"u'CN'": "u'elasticsearch-master-0058ea70f7c3fe00a.xxxxxx'", "u'signing_policy'": "u'core_graylog_mongodb'", "u'public_key'": "u'-----BEGIN PUBLIC KEY-----xxxxxxxxxxx-----END PUBLIC KEY-----'", 'testrun': True, "u'public_key_passphrase'": 'None'}], u'fun': 'x509.sign_remote_certificate', u'master_id': u'salt-master-11089058.xxxxxxx', u'id': 'elasticsearch-master-0058ea70f7c3fe00a.xxxxxxxx'}
[DEBUG   ] Signing data packet
[DEBUG   ] salt.crypt.get_rsa_key: Loading private key
[DEBUG   ] salt.crypt.sign_message: Signing message.
[DEBUG   ] Publish Side Match: []

[t0fik]

I made quick test on python 2.7 and poposed fix worked. My salt env:

```Salt Version:
Salt: 2019.2.0

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: Not Installed
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.7.2
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: 0.31.0
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: 3.7.3
pygit2: Not Installed
Python: 2.7.5 (default, Oct 30 2018, 23:45:53)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 15.3.0
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.2.1
ZMQ: 4.1.4

System Versions:
dist: centos 7.6.1810 Core
locale: UTF-8
machine: x86_64
release: 3.10.0-957.5.1.el7.x86_64
system: Linux
version: CentOS Linux 7.6.1810 Core
```

[alxwr]

I found a similar bug in modules.x509.read_crl, so I added the patch to this PR.
Tested on FreeBSD 11.2.
Looking forward to writing some tests, but haven't got the time now. Sry.

[stale[bot]]

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.

[stale[bot]]

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

[waynew]

I'm looking into this at the moment and I was thinking perhaps the issue could have been in m2crypto, since we were getting different subject hashes on Python2 and Python 3, causing https://github.com/saltstack/salt/blob/9536d322acf35931baaf0e780528c56334d718cd/salt/states/x509.py#L801 to not be equal.

But I've verified that both m2crypto on py2 and py3 return the same subject hash, so when we're generating our new_comp we've introduced a bug. I'm pretty sure the bug is in Python3, because under Python3 reading the cert file directly with m2crypto gives me the same hash that Python2 generates originally.

[s0undt3ch]

Please state otherwise, but I believe this is no longer an issue, at least on v3001rc1 onward.

[sagetherage]

@s0undt3ch can you verify this is corrected with a sha or PR link?

[s0undt3ch]

The types comparison is no longer an issue under Py3, just because it's py3, so this one is fixed.
There's another issue which is also linked on this one, https://github.com/saltstack/salt/issues/56556 which was only partially addressed. The module needs some rewriting.

[sagetherage]

Got it so it may only be partially fixed, so I updated the labels and let's test this again once that is re-written. The reference ticket is for Magnesium release.

[sagetherage]

Ok, the module still needs to be re-written, and the label is confusing since what is in place is more of a work-around, so I am updating this and will get it into planning soon.

[sagetherage]

This can also have a PR written against it for the module re-write

[hatifnatt]

Last comparison issue must be fixed with #58296

So, after all "recent" changes:

• #52935 which fix a lot of things and make x509 usable
• #56788 which is cherry-picked into #57688 which itself hopefully fix #56556
• and above mentioned #58296 which fix last noticeable comparison issue

I suppose x509 will reach some kind of working state in 3002.2.

[OrangeDog]

The two problems at the top of this issue are now fixed.
Others remain, but this could probably be closed now.