openssl_csr: fix idempotency problems (#55142)

* Add test for generating a CSR with everything, and testing idempotency. * Proper SAN normalization before comparison. * Fix check in cryptography backend. * Convert SANs to text. Update comments. * Add changelog.
6 years ago · cb5c57bcd5
parent 91e808eed2
commit cb5c57bcd5
5 changed files with 209 additions and 7 deletions
--- a/changelogs/fragments/55142-openssl_csr-crypto-backend-san.yml
+++ b/changelogs/fragments/55142-openssl_csr-crypto-backend-san.yml
@ -0,0 +1,3 @@
+bugfixes:
+- "openssl_csr - the cryptography backend's idempotency checking for basic constraints was broken."
+- "openssl_csr - SAN normalization for IP addresses for the pyOpenSSL backend was broken."
--- a/lib/ansible/modules/crypto/openssl_csr.py
+++ b/lib/ansible/modules/crypto/openssl_csr.py
@ -554,6 +554,16 @@ class CertificateSigningRequestPyOpenSSL(CertificateSigningRequestBase):
        except crypto_utils.OpenSSLBadPassphraseError as exc:
            raise CertificateSigningRequestError(exc)

+    def _normalize_san(self, san):
+        # apperently openssl returns 'IP address' not 'IP' as specifier when converting the subjectAltName to string
+        # although it won't accept this specifier when generating the CSR. (https://github.com/openssl/openssl/issues/4004)
+        if san.startswith('IP Address:'):
+            san = 'IP:' + san[len('IP Address:'):]
+        if san.startswith('IP:'):
+            ip = ipaddress.ip_address(san[3:])
+            san = 'IP:{0}'.format(ip.compressed)
+        return san
+
    def _check_csr(self):
        def _check_subject(csr):
            subject = [(OpenSSL._util.lib.OBJ_txt2nid(to_bytes(sub[0])), to_bytes(sub[1])) for sub in self.subject]
@ -565,12 +575,11 @@ class CertificateSigningRequestPyOpenSSL(CertificateSigningRequestBase):

        def _check_subjectAltName(extensions):
            altnames_ext = next((ext for ext in extensions if ext.get_short_name() == b'subjectAltName'), '')
-            altnames = [altname.strip() for altname in str(altnames_ext).split(',') if altname.strip()]
-            # apperently openssl returns 'IP address' not 'IP' as specifier when converting the subjectAltName to string
-            # although it won't accept this specifier when generating the CSR. (https://github.com/openssl/openssl/issues/4004)
-            altnames = [name if not name.startswith('IP Address:') else "IP:" + name.split(':', 1)[1] for name in altnames]
+            altnames = [self._normalize_san(altname.strip()) for altname in
+                        to_text(altnames_ext, errors='surrogate_or_strict').split(',') if altname.strip()]
            if self.subjectAltName:
-                if set(altnames) != set(self.subjectAltName) or altnames_ext.get_critical() != self.subjectAltName_critical:
+                if (set(altnames) != set([self._normalize_san(to_text(name)) for name in self.subjectAltName]) or
+                        altnames_ext.get_critical() != self.subjectAltName_critical):
                    return False
            else:
                if altnames:
@ -761,8 +770,8 @@ class CertificateSigningRequestCryptography(CertificateSigningRequestBase):

        def _check_basicConstraints(extensions):
            bc_ext = _find_extension(extensions, cryptography.x509.BasicConstraints)
-            current_ca = bc_ext.ca if bc_ext else False
-            current_path_length = bc_ext.path_length if bc_ext else None
+            current_ca = bc_ext.value.ca if bc_ext else False
+            current_path_length = bc_ext.value.path_length if bc_ext else None
            ca, path_length = crypto_utils.cryptography_get_basic_constraints(self.basicConstraints)
            # Check CA flag
            if ca != current_ca:
--- a/lib/ansible/modules/crypto/openssl_csr_info.py
+++ b/lib/ansible/modules/crypto/openssl_csr_info.py
@ -439,6 +439,8 @@ class CertificateSigningRequestInfoPyOpenSSL(CertificateSigningRequestInfo):
            return None, False

    def _normalize_san(self, san):
+        # apperently openssl returns 'IP address' not 'IP' as specifier when converting the subjectAltName to string
+        # although it won't accept this specifier when generating the CSR. (https://github.com/openssl/openssl/issues/4004)
        if san.startswith('IP Address:'):
            san = 'IP:' + san[len('IP Address:'):]
        if san.startswith('IP:'):
--- a/test/integration/targets/openssl_csr/tasks/impl.yml
+++ b/test/integration/targets/openssl_csr/tasks/impl.yml
@ -330,3 +330,184 @@
    backup: yes
    select_crypto_backend: '{{ select_crypto_backend }}'
  register: csr_backup_5
+
+- name: Generate CSR with everything
+  openssl_csr:
+    path: '{{ output_dir }}/csr_everything.csr'
+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    subject:
+      commonName: www.example.com
+      C: de
+      L: Somewhere
+      ST: Zurich
+      streetAddress: Welcome Street
+      O: Ansible
+      organizationalUnitName: Crypto Department
+      serialNumber: "1234"
+      SN: Last Name
+      GN: First Name
+      title: Chief
+      pseudonym: test
+      UID: asdf
+      emailAddress: test@example.com
+      postalAddress: 1234 Somewhere
+      postalCode: "1234"
+    useCommonNameForSAN: no
+    key_usage:
+      - digitalSignature
+      - keyAgreement
+      - Non Repudiation
+      - Key Encipherment
+      - dataEncipherment
+      - Certificate Sign
+      - cRLSign
+      - Encipher Only
+      - decipherOnly
+    key_usage_critical: yes
+    extended_key_usage:
+      - serverAuth  # the same as "TLS Web Server Authentication"
+      - TLS Web Server Authentication
+      - TLS Web Client Authentication
+      - Code Signing
+      - E-mail Protection
+      - timeStamping
+      - OCSPSigning
+      - Any Extended Key Usage
+      - qcStatements
+      - DVCS
+      - IPSec User
+      - biometricInfo
+    subject_alt_name:
+      - "DNS:www.ansible.com"
+      - "IP:1.2.3.4"
+      - "IP:::1"
+      - "email:test@example.org"
+      - "URI:https://example.org/test/index.html"
+    basic_constraints:
+      - "CA:TRUE"
+      - "pathlen:23"
+    basic_constraints_critical: yes
+    ocsp_must_staple: yes
+    select_crypto_backend: '{{ select_crypto_backend }}'
+  register: everything_1
+
+- name: Generate CSR with everything (idempotent, check mode)
+  openssl_csr:
+    path: '{{ output_dir }}/csr_everything.csr'
+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    subject:
+      commonName: www.example.com
+      C: de
+      L: Somewhere
+      ST: Zurich
+      streetAddress: Welcome Street
+      O: Ansible
+      organizationalUnitName: Crypto Department
+      serialNumber: "1234"
+      SN: Last Name
+      GN: First Name
+      title: Chief
+      pseudonym: test
+      UID: asdf
+      emailAddress: test@example.com
+      postalAddress: 1234 Somewhere
+      postalCode: "1234"
+    useCommonNameForSAN: no
+    key_usage:
+      - digitalSignature
+      - keyAgreement
+      - Non Repudiation
+      - Key Encipherment
+      - dataEncipherment
+      - Certificate Sign
+      - cRLSign
+      - Encipher Only
+      - decipherOnly
+    key_usage_critical: yes
+    extended_key_usage:
+      - serverAuth  # the same as "TLS Web Server Authentication"
+      - TLS Web Server Authentication
+      - TLS Web Client Authentication
+      - Code Signing
+      - E-mail Protection
+      - timeStamping
+      - OCSPSigning
+      - Any Extended Key Usage
+      - qcStatements
+      - DVCS
+      - IPSec User
+      - biometricInfo
+    subject_alt_name:
+      - "DNS:www.ansible.com"
+      - "IP:1.2.3.4"
+      - "IP:::1"
+      - "email:test@example.org"
+      - "URI:https://example.org/test/index.html"
+    basic_constraints:
+      - "CA:TRUE"
+      - "pathlen:23"
+    basic_constraints_critical: yes
+    ocsp_must_staple: yes
+    select_crypto_backend: '{{ select_crypto_backend }}'
+  check_mode: yes
+  register: everything_2
+
+- name: Generate CSR with everything (idempotent)
+  openssl_csr:
+    path: '{{ output_dir }}/csr_everything.csr'
+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    subject:
+      commonName: www.example.com
+      C: de
+      L: Somewhere
+      ST: Zurich
+      streetAddress: Welcome Street
+      O: Ansible
+      organizationalUnitName: Crypto Department
+      serialNumber: "1234"
+      SN: Last Name
+      GN: First Name
+      title: Chief
+      pseudonym: test
+      UID: asdf
+      emailAddress: test@example.com
+      postalAddress: 1234 Somewhere
+      postalCode: "1234"
+    useCommonNameForSAN: no
+    key_usage:
+      - digitalSignature
+      - keyAgreement
+      - Non Repudiation
+      - Key Encipherment
+      - dataEncipherment
+      - Certificate Sign
+      - cRLSign
+      - Encipher Only
+      - decipherOnly
+    key_usage_critical: yes
+    extended_key_usage:
+      - serverAuth  # the same as "TLS Web Server Authentication"
+      - TLS Web Server Authentication
+      - TLS Web Client Authentication
+      - Code Signing
+      - E-mail Protection
+      - timeStamping
+      - OCSPSigning
+      - Any Extended Key Usage
+      - qcStatements
+      - DVCS
+      - IPSec User
+      - biometricInfo
+    subject_alt_name:
+      - "DNS:www.ansible.com"
+      - "IP:1.2.3.4"
+      - "IP:::1"
+      - "email:test@example.org"
+      - "URI:https://example.org/test/index.html"
+    basic_constraints:
+      - "CA:TRUE"
+      - "pathlen:23"
+    basic_constraints_critical: yes
+    ocsp_must_staple: yes
+    select_crypto_backend: '{{ select_crypto_backend }}'
+  register: everything_3
--- a/test/integration/targets/openssl_csr/tests/validate.yml
+++ b/test/integration/targets/openssl_csr/tests/validate.yml
@ -138,3 +138,10 @@
      - csr_backup_4.backup_file is string
      - csr_backup_5 is not changed
      - csr_backup_5.backup_file is undefined
+
+- name: Check CSR with everything
+  assert:
+    that:
+      - everything_1 is changed
+      - everything_2 is not changed
+      - everything_3 is not changed