x509.certificate_managed: always call file.managed even when cert contents haven't changed

[glynnforrest]

What does this PR do?

Fixes a bug I introduced when rewriting x509.certificate.managed.

The state now always calls file.managed, even if the certificate contents don't need to change. This allows the user to change the user / group / mode of an existing certificate.

Both file and certificate changes are properly shown in the state output:

Ping @Ch3LL and @waynew, since you worked on #52935.

What issues does this PR fix or reference?

See #52935 (comment)

Merge requirements satisfied?

• Tests written/updated

Commits signed with GPG?

Yes

[hatifnatt]

First of all I must notice that I'm running Salt 2019.2.3 (Fluorine) on Debian 9, Python 2.7 but this PR is based on master branch.

Salt Version:
           Salt: 2019.2.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.5.0
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.24.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: 1.3.7
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Sep 26 2018, 18:42:22)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1

System Versions:
           dist: debian 9.12
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-12-amd64
         system: Linux
        version: debian 9.12

I run few tests:

• change key file parameters - ok, changes generated with test=True, file parameters updated after execution
• change cert file parameters - ok, changes generated with test=True, file parameters updated after execution
• change cert file parameters and change certificate parameters like subjectAltName - mostly ok, changes generated with test=True but changes in file parameters are not shown only changes in certificate are shown. I.e. file mode changed from 640 to 600 and subjectAltName changed from DNS:salt.domain.tld to DNS:salt.domain.tld, DNS:master.domain.tld ony changes in certificate are shown

          ID: salt_api_cert
    Function: x509.certificate_managed
      Result: None
     Comment: Certificate /etc/pki/api/salt.crt will be created
     Started: 18:19:29.181241
    Duration: 334.043 ms
     Changes:   
              ----------
              Status:
                  ----------
                  New:
                      Certificate will be valid and up to date
                  Old:
                      Certificate properties are different: X509v3 Extensions

but after execution all changes are displayed correctly

Duration: 664.762 ms
 Changes:
          ----------
          Certificate:
              ----------
              New:
              ... snip ...
                      subjectAltName:
                          DNS:salt.domain.tld, DNS:master.domain.tld
                  Old:
              ... snip ...
                      subjectAltName:
                          DNS:salt.domain.tld
              File:
                  ----------
                  diff:
                      ---
                      +++
                      @@ -1,40 +1,40 @@
                       -----BEGIN CERTIFICATE-----
                       ... snip ...
                       -----END CERTIFICATE-----
                  mode:
                      0600
              Status:
                  ----------
                  New:
                      Certificate is valid and up to date
                  Old:
                      Certificate properties are different: X509v3 Extensions

May be it's possible to show File: changes during test run too?

[hatifnatt]

@glynnforrest what OS, Salt and Python versions you are running? I'm trying to upgrade to Deb 10, and therefore forced to switch to Py3 and it looks like I'm faced with #56556

[sjorge]

@hatifnatt this got merged into master, I tested the copy of x509 module and state and sadly still have the same behavior as in #56556

[hatifnatt]

@sjorge this is not merged to master for now, #52935 - is merged. But both of these PRs are not supposed to fix issue from #56556.
That's why I'm trying to clarify what versions @glynnforrest is using.

[glynnforrest]

Thanks @hatifnatt for trying it out. I've added File: changes to the test output:

Note that the diff in test mode won't include the new certificate in it, for security and performance reasons. See my comment here: https://github.com/saltstack/salt/pull/56788/files#diff-5499a295a50d60a761c34f4080e4014bR647

@garethgreenaway @dwoz this is ready now.

[sjorge]

Should it include the old one though?

[glynnforrest]

Should it include the old one though?

Not 100% sure what you're asking here. If you mean the diff in the file.managed output - that's how the state has behaved up until now (and the new cert is also shown when test mode is off). We could discuss removing the cert from the output in a different PR but I imagine the SaltStack folks will have something to say about that regarding backwards compatibility.

[glynnforrest]

Ping @sagetherage, any chance you could get someone to look at this?

[s0undt3ch]

This is getting merged as part of #57688 (cherry-picked)
Closing.

[s0undt3ch]

Please state if closing was the wrong move, I can re-open.