[BUG]. keystore state/module unable to list ASN1 - DER certs in keystore

cesan3 commented 3 years ago

Description of Issue

This seems a python3 issue.

When a new keystore is created and a new cert added (python-36 at the moment), keystore.list is unable to determine the cert type (ASN1)

Setup

formula.app.saml.keystore: keystore.managed:

name: '{{ app.saml.keystore.url }}'
passphrase: '{{ app.saml.keystore.pass }}'
force_remove: True
entries:
- alias: '{{ app.saml.keystore.key }}' certificate: '{{ app.saml.cert }}' private_key: '{{ app.saml.key }}'

Steps to Reproduce Issue

Re-run your state in debug and test mode

salt-call -l debug formula.app test=True
          ID: formula.app.saml.keystore
    Function: keystore.managed
        Name: /opt/app/conf/saml.jks
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/opt/behavox/salt/venv/lib64/python3.6/site-packages/salt/state.py", line 2154, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/opt/behavox/salt/venv/lib64/python3.6/site-packages/salt/loader.py", line 2087, in wrapper
                  return f(*args, **kwargs)
                File "/opt/behavox/salt/library/extmods/states/keystore.py", line 90, in managed
                  existing_entries = __salt__["keystore.list"](name, passphrase)
                File "/opt/behavox/salt/library/extmods/modules/keystore.py", line 120, in list
                  if "\x30" in cert_result[0]:
              TypeError: argument of type 'int' is not iterable
     Started: 15:45:19.325124
    Duration: 5.537 ms
     Changes:

I also tried to run the keystore.list module directly:

    Passed invalid arguments to keystore.list: argument of type 'int' is not iterable

        Lists certificates in a keytool managed keystore.

        :param keystore: The path to the keystore file to query
        :param passphrase: The passphrase to use to decode the keystore
        :param alias: (Optional) If found, displays details on only this key
        :param return_certs: (Optional) Also return certificate PEM.

        .. warning::

            There are security implications for using return_cert to return decrypted certificates.

        CLI Example:

        .. code-block:: bash

            salt '*' keystore.list /usr/lib/jvm/java-8/jre/lib/security/cacerts changeit
            salt '*' keystore.list /usr/lib/jvm/java-8/jre/lib/security/cacerts changeit debian:verisign_-_g5.pem

Versions Report

Salt Version: Salt: 3001.3

Dependency Versions: cffi: 1.14.4 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.37.1 Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 1.0.0 mysql-python: Not Installed pycparser: 2.20 pycrypto: Not Installed pycryptodome: 3.9.7 pygit2: Not Installed Python: 3.6.8 (default, Nov 16 2020, 16:55:22) 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: centos 7 Core locale: UTF-8 machine: x86_64 release: 3.10.0-1062.18.1.el7.x86_64 system: Linux version: CentOS Linux 7 Core

welcome[bot] commented 3 years ago

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at core@saltstack.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

zeroschism commented 3 years ago

I've seen this problem too on python38, and I think the entire check is unnecessary -- at least presuming the comments in the source pyjks library are accurate: https://github.com/kurtbrose/pyjks/blob/master/jks/jks.py#L63

"""A byte string containing the actual certificate data. In the case of X.509 certificates, this is the DER-encoded X.509 representation of the certificate."""

If there is some scenario I'm not thinking of and that check really is needed, then I think the correct modification would be to change it to be:

if b"\x30" in cert_result

After fixing this locally, I found another couple of bugs -- some of them are encoding issues, and some of them relate to the fact that jks forces the alias names to be lowercase, but you can define mixed-case aliases in salt. It looks like there is already a PR for some of this (https://github.com/saltstack/salt/pull/59507). I'll start from there.

saltstack / salt