Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x509.certificate_managed, renewed at each run #50680

Closed
Poil opened this issue Nov 29, 2018 · 6 comments
Closed

x509.certificate_managed, renewed at each run #50680

Poil opened this issue Nov 29, 2018 · 6 comments
Labels
Bug broken, incorrect, or confusing behavior P2 Priority 2 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone
Approved

Comments

@Poil
Copy link

Poil commented Nov 29, 2018

Hi,

I've this code, at each runs, Salt change the "Not After" and "Not Before" and the Serial Number/Finger Print of my ca.crt.

Note : I use the develop branch to have the latest fix for Python 3

/etc/salt/minion.d/signing_policies.conf:
  file.managed:
    - source: salt://application/certificate/files/signing_policies.conf
    - watch_in: salt-minion

/etc/pki:
  file.directory

/etc/pki/issued_certs:
  file.directory

/etc/pki/ca.key:
  x509.private_key_managed:
    - bits: 4096
    - backup: True
    - require:
      - file: /etc/pki

/etc/pki/ca.crt:
  x509.certificate_managed:
    - signing_private_key: /etc/pki/ca.key
    - CN: salt.engsec
    - C: FR
    - ST: Bretagne
    - L: Rennes
    - basicConstraints: "critical CA:true"
    - keyUsage: "critical cRLSign, keyCertSign"
    - subjectKeyIdentifier: hash
    - authorityKeyIdentifier: keyid,issuer:always
    - days_valid: 3650
    - days_remaining: 30
    - backup: True
    - require:
      - file: /etc/pki
      - x509: /etc/pki/ca.key

mine.send:
  module.run:
    - func: x509.get_pem_entries
    - kwargs:
        glob_path: /etc/pki/ca.crt
    - onchanges:
      - x509: /etc/pki/ca.crt

              Certificate:
                  ----------
                  New:
                      Not After:
                          2028-11-26 13:18:49
                      Not Before:
                          2018-11-29 13:18:49

                  Old:
                      ----------
                      Not After:
                          2028-11-26 13:03:06
                      Not Before:
                          2018-11-29 13:03:06
@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 29, 2018

looks like i'm able to replicate this using your exact state but its showing even more differences then just not after and not before for me. i can only replicate this on python3 not python2.

          ID: /etc/pki/ca.crt
    Function: x509.certificate_managed
      Result: True
     Comment: File /etc/pki/ca.crt updated
     Started: 16:57:51.749140
    Duration: 39.999999999054126 ms
     Changes:   
              ----------
              Certificate:
                  ----------
                  New:
                      ----------
                      Issuer:
                          ----------
                          C:
                              FR
                          CN:
                              salt.engsec
                          L:
                              Rennes
                          SP:
                              Bretagne
                      Issuer Hash:
                          5B:07:79:4A
                      Key Size:
                          4096
                      MD5 Finger Print:
                          46:46:60:A4:DF:73:BF:7C:D5:92:2B:7E:3A:9B:14:9F
                      Not After:
                          2028-11-26 16:57:51
                      Not Before:
                          2018-11-29 16:57:51
                      Public Key:
                          -----BEGIN PUBLIC KEY-----
                          MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAu//Q2BYwTv1U+SUYQ+dm
                          JbGAOhV6JFq1HLrdTyhcJLrXSyvVpoCR1Mtl7QVU46zHKYUVb+ooWLc7Nc/WoIX1
                          Lb3NAtAf1mfpfPGA7thKd3RdAh+/GkrYO0tT2l96jNaFM4q7wxB0Py8RKW7yvWkB
                          wYtkHq53aPe2MuxjTDeoTjUHTtkoQAz5uUguIFfcLh/J1YfXh+GqjtHSJQ4D0dHJ
                          d1v7geCKPdPWsuqOp/VGfWu1f3gDWXnrMYBCt65olNNJPVRBVdd0VfeH4P7TXZ3g
                          3VihWwR/6iATY5SpBaGmmQvlvM0QJbIFSseGpQfPPWLYBgY4/Kbk0WZlhmmazScE
                          kmPeTqssq8Orl7SNab78PC79aFe2/YkvCbNHj9LYm/MPWA675O4htmmBtUwfLa7q
                          pQYxP00qHxjoj2DwXqZl7b2g6T8lwqGeI/FsKvfMaXD20sPA27yqg/q9O0abuyeQ
                          4YgS/rNX6pgIhfOtQ1khD9K+fG7rHrnVrAk9wjiOrlsTQoUOM/s5cRaBBLTnzQZI
                          /367mUvaawvJxDGeFoIw86Tv60+3hpkeL6qDjA4GwznkfQogM+F3w/yeazPS6SMX
                          iKO6FBiCzFW6py+Zk5bNFodtWvZ8tFwQ1YgmU4p7yKMbHfHUWo7Z1tk0cGUxV3+V
                          RizCLd+m9nK2DWQ5uaEAom8CAwEAAQ==
                          -----END PUBLIC KEY-----
                      SHA-256 Finger Print:
                          C7:58:B2:83:17:3C:D8:0F:41:56:E4:E4:46:08:03:A5:36:18:C2:DA:AB:07:07:FA:AB:00:F1:40:FC:A2:91:AA
                      SHA1 Finger Print:
                          EB:5A:0A:44:A3:E5:76:B3:0E:45:C0:65:8B:20:E9:74:12:58:44:7D
                      Serial Number:
                          13:FF:A8:40:53:75:D0:0B
                      Subject:
                          ----------
                          C:
                              FR
                          CN:
                              salt.engsec
                          L:
                              Rennes
                          SP:
                              Bretagne
                      Subject Hash:
                          5B:07:79:4A
                      Version:
                          3
                      X509v3 Extensions:
                          ----------
                          basicConstraints:
                              critical CA:TRUE
                          keyUsage:
                              critical Certificate Sign, CRL Sign
                          subjectKeyIdentifier:
                              B2:FC:16:A0:6B:F9:3B:D2:EF:56:D2:C4:7B:14:6A:88:67:67:C9:E5
                          authorityKeyIdentifier:
                              keyid:B2:FC:16:A0:6B:F9:3B:D2:EF:56:D2:C4:7B:14:6A:88:67:67:C9:E5
                              DirName:/CN=salt.engsec/C=FR/ST=Bretagne/L=Rennes
                              serial:13:FF:A8:40:53:75:D0:0B
                  Old:
                      ----------
                      Issuer:
                          ----------
                          C:
                              FR
                          CN:
                              salt.engsec
                          L:
                              Rennes
                          SP:
                              Bretagne
                      Issuer Hash:
                          B1:DF:F1:4B
                      Key Size:
                          4096
                      MD5 Finger Print:
                          64:92:81:3A:49:41:0B:78:BF:29:67:50:BF:E4:91:AE
                      Not After:
                          2028-11-26 16:57:45
                      Not Before:
                          2018-11-29 16:57:45
                      Public Key:
                          -----BEGIN PUBLIC KEY-----
                          MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAu//Q2BYwTv1U+SUYQ+dm
                          JbGAOhV6JFq1HLrdTyhcJLrXSyvVpoCR1Mtl7QVU46zHKYUVb+ooWLc7Nc/WoIX1
                          Lb3NAtAf1mfpfPGA7thKd3RdAh+/GkrYO0tT2l96jNaFM4q7wxB0Py8RKW7yvWkB
                          wYtkHq53aPe2MuxjTDeoTjUHTtkoQAz5uUguIFfcLh/J1YfXh+GqjtHSJQ4D0dHJ
                          d1v7geCKPdPWsuqOp/VGfWu1f3gDWXnrMYBCt65olNNJPVRBVdd0VfeH4P7TXZ3g
                          3VihWwR/6iATY5SpBaGmmQvlvM0QJbIFSseGpQfPPWLYBgY4/Kbk0WZlhmmazScE
                          kmPeTqssq8Orl7SNab78PC79aFe2/YkvCbNHj9LYm/MPWA675O4htmmBtUwfLa7q
                          pQYxP00qHxjoj2DwXqZl7b2g6T8lwqGeI/FsKvfMaXD20sPA27yqg/q9O0abuyeQ
                          4YgS/rNX6pgIhfOtQ1khD9K+fG7rHrnVrAk9wjiOrlsTQoUOM/s5cRaBBLTnzQZI
                          /367mUvaawvJxDGeFoIw86Tv60+3hpkeL6qDjA4GwznkfQogM+F3w/yeazPS6SMX
                          iKO6FBiCzFW6py+Zk5bNFodtWvZ8tFwQ1YgmU4p7yKMbHfHUWo7Z1tk0cGUxV3+V
                          RizCLd+m9nK2DWQ5uaEAom8CAwEAAQ==
                          -----END PUBLIC KEY-----
                      SHA-256 Finger Print:
                          EF:1B:4A:FC:44:0A:09:9A:94:27:A1:55:1B:AD:6C:62:7D:FC:D9:A3:BE:1B:F9:CF:85:E2:E6:3C:EE:53:0A:07
                      SHA1 Finger Print:
                          F7:B6:FB:E2:3F:B6:EE:87:69:02:96:1C:B1:9E:8B:83:71:AB:C4:D0
                      Serial Number:
                          10:42:C8:B9:1D:E7:1A:5E
                      Subject:
                          ----------
                          C:
                              FR
                          CN:
                              salt.engsec
                          L:
                              Rennes
                          SP:
                              Bretagne
                      Subject Hash:
                          B1:DF:F1:4B
                      Version:
                          3
                      X509v3 Extensions:
                          ----------
                          basicConstraints:
                              critical CA:TRUE
                          keyUsage:
                              critical Certificate Sign, CRL Sign
                          subjectKeyIdentifier:
                              B2:FC:16:A0:6B:F9:3B:D2:EF:56:D2:C4:7B:14:6A:88:67:67:C9:E5
                          authorityKeyIdentifier:
                              keyid:B2:FC:16:A0:6B:F9:3B:D2:EF:56:D2:C4:7B:14:6A:88:67:67:C9:E5
                              DirName:/ST=Bretagne/CN=salt.engsec/C=FR/L=Rennes
                              serial:10:42:C8:B9:1D:E7:1A:5E
----------

But when on python2:

----------
          ID: /etc/pki/ca.crt
    Function: x509.certificate_managed
      Result: True
     Comment: File /etc/pki/ca.crt is in the correct state
     Started: 16:58:44.334883
    Duration: 19.9999999986 ms
     Changes:   
----------

Heres my versions report:

[root@1b586d20eebf testing]# salt --versions-report
Salt Version:
           Salt: 2018.11.0-4285-ged5a85f
 
Dependency Versions:
           cffi: 1.11.5
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: 3.5.0
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: 0.31.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.19
       pycrypto: 3.6.6
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.4.9 (default, Aug 14 2018, 21:28:57)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.0.0
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.6
 
System Versions:
           dist: centos 7.4.1708 Core
         locale: UTF-8
        machine: x86_64
        release: 4.18.7-rt5-MANJARO
         system: Linux
        version: CentOS Linux 7.4.1708 Core

i also found this is broken on the fluorine branch. As you stated i cannot test if this is broken in 2018.3 because this state does not currently work in that branch with python3.

@Ch3LL Ch3LL added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P2 Priority 2 labels Nov 29, 2018
@Ch3LL Ch3LL added this to the Approved milestone Nov 29, 2018
@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 29, 2018

ping @clinta looks like you have done some work around this module. mind taking a look here?

@Poil
Copy link
Author

Poil commented Nov 29, 2018

Oh good news, I wasn't sure it was me or a bug before first time I use it.
I will try to investigate tomorrow, I've started to check the code today before I'd seen @clinta made a fix for Python 3

@Poil
Copy link
Author

Poil commented Dec 4, 2018 •
edited
Loading

I've add some debug information in the code

    if (current_comp == new_comp and
            current_days_remaining > days_remaining and
            __salt__['x509.verify_signature'](name, new_issuer_public_key)):
        certificate = __salt__['x509.get_pem_entry'](name, pem_type='CERTIFICATE')
    else:
        if rotate_private_key and not new_private_key:
            new_private_key = True
            private_key = __salt__['x509.create_private_key'](
                text=True, bits=private_key_args['bits'], verbose=private_key_args['verbose'])
            kwargs['public_key'] = private_key
        new_certificate = True
        certificate = __salt__['x509.create_certificate'](text=True, **kwargs)

If I dump the value of the if condition

current_comp == new_comp is false
current_days_remaining > days_remaining is true
x509.verify_signature is true

So If I do a dict diff on new_comp and current_comp

2018-12-04 11:34:53,278 [salt.loaded.int.states.x509:538 ][ERROR   ][7978] Issuer is different - 
    new: {'C': 'FR', 'commonName': 'salt.engsec', 'localityName': 'Rennes', 'ST': 'Bretagne'}, 
    current: {'C': 'FR', 'SP': 'Bretagne', 'CN': 'salt.engsec', 'L': 'Rennes'}
2018-12-04 11:34:53,279 [salt.loaded.int.states.x509:540 ][ERROR   ][7978] key Issuer Hash is not present in new_comp
2018-12-04 11:34:53,279 [salt.loaded.int.states.x509:538 ][ERROR   ][7978] Subject is different - 
    new: {'C': 'FR', 'commonName': 'grafana.ssllab.com', 'localityName': 'Rennes', 'ST': 'Bretagne'}, 
    current: {'C': 'FR', 'SP': 'Bretagne', 'CN': 'grafana.ssllab.com', 'L': 'Rennes'}

I think it is because we call x509.create_certificate with testrun=true, this is not giving a certificate but only a dict of what it should be so I think we can't compare new_comp to current_comp, or we should create a real temporary certificate, read it witch read_certificate, and after we can compare them

@Poil
Copy link
Author

Poil commented Dec 4, 2018 •
edited
Loading

I've fixed, not very clean but I can't find how to do better (I call x509.create_certificate 2 times) :

+++ /usr/lib/python2.7/site-packages/salt/states/x509.py	2018-12-04 15:05:15.470327443 +0100
@@ -499,11 +499,12 @@
         raise salt.exceptions.SaltInvocationError(
             'signing_policy must be specified if ca_server is.')
 
-    new = __salt__['x509.create_certificate'](testrun=True, **kwargs)
+    new = __salt__['x509.create_certificate'](testrun=False, text=True, **kwargs)
+    new = __salt__['x509.read_certificate'](certificate=new)
+    newcert = __salt__['x509.create_certificate'](testrun=True, **kwargs)
 
     if isinstance(new, dict):
         new_comp = copy.deepcopy(new)
-        new.pop('Issuer Public Key')
         if 'serial_number' not in kwargs:
             new_comp.pop('Serial Number')
             if 'signing_cert' not in kwargs:
@@ -518,7 +519,7 @@
         new_comp.pop('MD5 Finger Print')
         new_comp.pop('SHA1 Finger Print')
         new_comp.pop('SHA-256 Finger Print')
-        new_issuer_public_key = new_comp.pop('Issuer Public Key')
+        new_issuer_public_key = newcert.pop('Issuer Public Key')
     else:
         new_comp = new
 
@@ -526,8 +527,7 @@
     if (current_comp == new_comp and
             current_days_remaining > days_remaining and
             __salt__['x509.verify_signature'](name, new_issuer_public_key)):
-        certificate = __salt__['x509.get_pem_entry'](
-            name, pem_type='CERTIFICATE')
+        certificate = __salt__['x509.get_pem_entry'](name, pem_type='CERTIFICATE')
     else:
         if rotate_private_key and not new_private_key:
             new_private_key = True

@Poil Poil mentioned this issue Dec 4, 2018
Merged
cachedout pushed a commit that referenced this issue Dec 13, 2018
Merge pull request #50734 from Poil/fix_x509
4e54f21 
Fix #50680 X509 -  renewal check - short subject title not match
@OrangeDog OrangeDog mentioned this issue Mar 14, 2019
Closed
@OrangeDog
Copy link
Contributor

This probably needs a change at the module level, so you can create a certificate and return both the PEM and the Issuer Public Key (or whole descriptive dict), or just skip the check that the current certificate validates with the the new issuer.

Then you only need one call to create_certificate and possibly one to read_certificate, vs. the current three and two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug broken, incorrect, or confusing behavior P2 Priority 2 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Milestone
Approved
Development

No branches or pull requests
3 participants
@Poil @OrangeDog @Ch3LL