[BUG] x509.certificate_managed TypeError when using append_certs

[tqre]

Description
Creating certificate chains, ie. making one file of multiple certificates fails with x509 module.

Setup
Basic localized master/minion setup with the following state:
cert.sls

key1:
  x509.private_key_managed:
    - name: /home/salt/venv/certs/key.pem

cert1:
  x509.certificate_managed:
    - name: /home/salt/venv/certs/cert.pem
    - signing_private_key: /home/salt/venv/certs/key.pem
    - require:
      - x509: key1

key2:
  x509.private_key_managed:
    - name: /home/salt/venv/certs/key2.pem
  
cert2:
  x509.certificate_managed:
    - name: /home/salt/venv/certs/cert2.pem
    - signing_private_key: /home/salt/venv/certs/key2.pem
    - append_certs: 
      - /home/salt/venv/certs/cert.pem 
    - require:
      - x509: key2

Steps to Reproduce the behavior

$ salt '*' state.apply cert
localminion-dev:
----------
          ID: key1
    Function: x509.private_key_managed
        Name: /home/salt/venv/certs/key.pem
      Result: True
     Comment: File /home/salt/venv/certs/key.pem is in the correct state
     Started: 16:10:58.605159
    Duration: 13.318 ms
     Changes:   
----------
          ID: cert1
    Function: x509.certificate_managed
        Name: /home/salt/venv/certs/cert.pem
      Result: True
     Comment: Certificate /home/salt/venv/certs/cert.pem is valid and up to date
     Started: 16:10:58.618687
    Duration: 5.903 ms
     Changes:   
----------
          ID: key2
    Function: x509.private_key_managed
        Name: /home/salt/venv/certs/key2.pem
      Result: True
     Comment: File /home/salt/venv/certs/key2.pem is in the correct state
     Started: 16:10:58.624656
    Duration: 0.966 ms
     Changes:   
----------
          ID: cert2
    Function: x509.certificate_managed
        Name: /home/salt/venv/certs/cert2.pem
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/home/salt/salt/state.py", line 2157, in call
                  ret = self.states[cdata["full"]](
                File "/home/salt/salt/loader.py", line 2097, in wrapper
                  return f(*args, **kwargs)
                File "/home/salt/salt/states/x509.py", line 718, in certificate_managed
                  contents += append_file_contents
              TypeError: can only concatenate str (not "bytes") to str
     Started: 16:10:58.625773
    Duration: 3.961 ms
     Changes:   

Summary for localminion-dev
------------
Succeeded: 3
Failed:    1
------------
Total states run:     4
Total run time:  24.148 ms
ERROR: Minions returned with non-zero exit code

Expected behavior
The certificates are appended correctly.

Versions Report

salt --versions-report

``` Salt Version: Salt: 3001.1-481-gf534d4de9a

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: Not Installed
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
Jinja2: 2.11.2
libgit2: Not Installed
M2Crypto: 0.36.0
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 1.0.0
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: 3.9.8
pygit2: Not Installed
Python: 3.8.5 (default, Sep 5 2020, 10:50:12)
python-gnupg: Not Installed
PyYAML: 5.3.1
PyZMQ: 19.0.2
smmap: Not Installed
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.3.2

System Versions:
dist: arch rolling n/a
locale: utf-8
machine: x86_64
release: 5.8.9-arch2-1
system: Linux
version: Arch Linux rolling n/a

Additional context
A simple type conversion fixes this, and shouldn't break anything. I'll submit a pull request soon.

[sagetherage]

Thank you for the PR! I will try to get this triaged quickly!

[joachimtingvold]

Dirty quickfix until patch arrives;

bugfix_salt_states_x509:
  file.replace:
    - name: /usr/lib/python3/dist-packages/salt/states/x509.py
    - pattern: 'contents \+\= append_file_contents'
    - repl: |
        contents += str(append_file_contents.decode('utf-8'))
    - backup: false
    - ignore_if_missing: true

[7meis]

I'm not sure what "Needs Testcase" mean, but:
We had this bug as we played around with this tutorial:
https://backbeat.tech/blog/secure-servers-with-saltstack-and-vault-part-4/

salt/ca/init.sls (this state was working).

when we tried this state, we had the bug.
salt/vault/install.sls

the workaround was very helpful, thanks a lot! :-)

hope this helps?
Simon

[sagetherage]

the label is applied to the PR and am simply copying it here - the PR will need a test case to be merged into the code base. We started requiring unit/integration tests on all PRs in 2018. We hold Test Clinics on Tuesdays and Thursday via Twitch the schedule is in that link as well. I hope that helps @7meis

[hatifnatt]

Faced this issue too, any chance that core team fix this?

[jc-rob]

Something changed recently, we're now hitting this issue with 3000.9 where it's been working as expected for months

            Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3.6/site-packages/salt/state.py", line 1981, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3.6/site-packages/salt/loader.py", line 1977, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3.6/site-packages/salt/states/x509.py", line 560, in certificate_managed
                  'contents'] += __salt__['x509.get_pem_entry'](append_cert, pem_type='CERTIFICATE')
              TypeError: must be str, not bytes

[zeddD1abl0]

The source of this problem comes from specifically this line which was updated in 2018, probably just after a lot of these tutorials were written.

The problem is because it returns bytes, not a string, and this isn't handled in the places where "get_pem_entry" is called from. I haven't written a PR for this yet, just started investigating it and tracked back down through the processes.

[StefanTT]

This is still an issue.
The fix of joachimtingvold works.
Would be great if this one line change could make it into the source.

[clee-level]

Please fix! Still an issue:

----------
          ID: os-pki-clients-localhost-crt
    Function: x509.certificate_managed
        Name: /etc/pki/tls/certs/localhost.crt
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3.10/site-packages/salt/state.py", line 2179, in call
                  ret = self.states[cdata["full"]](
                File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 149, in __call__
                  return self.loader.run(run_func, *args, **kwargs)
                File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 1201, in run
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 1216, in _run_as
                  return _func_or_method(*args, **kwargs)
                File "/usr/lib/python3.10/site-packages/salt/loader/lazy.py", line 1249, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3.10/site-packages/salt/states/x509.py", line 706, in certificate_managed
                  contents += append_file_contents
              TypeError: can only concatenate str (not "bytes") to str
     Started: 03:50:52.999871
    Duration: 341.852 ms
     Changes:

[svenseeberg]

There is a PR for this: #56159 However, I have not much time for writing test cases. If anyone is willing to contribute, feel free.

[lkubb]

Fwiw, the current x509 modules will be deprecated in the next release because of several issues with them. It will feature completely rewritten x509 modules (https://github.com/saltstack/salt/blob/master/salt/modules/x509_v2.py) where this should not be an issue. The new ones will need to be explicitly enabled though because they are not 100% backwards-compatible (but mostly). I forgot to link this issue in my PR #63099, sorry.

[svenseeberg]

Awesome news. I think I'll close my PR then and wait for the new module and then migrate.

[Ch3LL]

Closing this issue as this is fixed in the new module. Please open an issue with details if this is not the case.