java_cert: import certificate+key bundle from pkcs12 (#3080) (#3085)

* import certificate+key bundle from pkcs12

* fix typo/syntax

* fix variable name

* fix passwords order and improve error handling

* add changelog fragment

* enter keystore pass only once if keystore already exists, and twice at creation

* nomalize tests

- Replace `command` tasks by dedicated (community.crypto) modules.
- Add spaces around jinja2 variable names.
- Call modules by their FQCNs.

* Add tests to check keystore has a private key

fix tests for RedHat/CentOS < 8 (run openssl command as an alternative to
`openssl_pkcs12` module)

(cherry picked from commit 21d5668c97)

Co-authored-by: quidame <quidame@poivron.org>

changelogs/fragments/3080-java_cert-2460-import_private_key.yml

@@ -0,0 +1,4 @@
+---
+bugfixes:
+  - java_cert - import private key as well as public certificate from PKCS#12
+    (https://github.com/ansible-collections/community.general/issues/2460).

plugins/modules/system/java_cert.py

@@ -11,15 +11,15 @@ DOCUMENTATION = r'''
 ---
 module: java_cert
 
-short_description: Uses keytool to import/remove key from java keystore (cacerts)
+short_description: Uses keytool to import/remove certificate to/from java keystore (cacerts)
 description:
-  - This is a wrapper module around keytool, which can be used to import/remove
+  - This is a wrapper module around keytool, which can be used to import certificates
-    certificates from a given java keystore.
+    and optionally private keys to a given java keystore, or remove them from it.
 options:
   cert_url:
     description:
       - Basic URL to fetch SSL certificate from.
-      - One of C(cert_url) or C(cert_path) is required to load certificate.
+      - Exactly one of C(cert_url), C(cert_path) or C(pkcs12_path) is required to load certificate.
     type: str
   cert_port:
     description:
@@ -30,7 +30,7 @@ options:
   cert_path:
     description:
       - Local path to load certificate from.
-      - One of C(cert_url) or C(cert_path) is required to load certificate.
+      - Exactly one of C(cert_url), C(cert_path) or C(pkcs12_path) is required to load certificate.
     type: path
   cert_alias:
     description:
@@ -46,6 +46,10 @@ options:
   pkcs12_path:
     description:
       - Local path to load PKCS12 keystore from.
+      - Unlike C(cert_url) and C(cert_path), the PKCS12 keystore embeds the private key matching
+        the certificate, and is used to import both the certificate and its private key into the
+        java keystore.
+      - Exactly one of C(cert_url), C(cert_path) or C(pkcs12_path) is required to load certificate.
     type: path
   pkcs12_password:
     description:
@@ -267,6 +271,7 @@ def _export_public_cert_from_pkcs12(module, executable, pkcs_file, alias, passwo
     export_cmd = [
         executable,
         "-list",
+        "-noprompt",
         "-keystore",
         pkcs_file,
         "-alias",
@@ -336,6 +341,44 @@ def _download_cert_url(module, executable, url, port):
     return fetch_out
 
 
+def import_pkcs12_path(module, executable, pkcs12_path, pkcs12_pass, pkcs12_alias,
+                       keystore_path, keystore_pass, keystore_alias, keystore_type):
+    ''' Import pkcs12 from path into keystore located on
+        keystore_path as alias '''
+    import_cmd = [
+        executable,
+        "-importkeystore",
+        "-noprompt",
+        "-srcstoretype",
+        "pkcs12",
+        "-srckeystore",
+        pkcs12_path,
+        "-srcalias",
+        pkcs12_alias,
+        "-destkeystore",
+        keystore_path,
+        "-destalias",
+        keystore_alias
+    ]
+    import_cmd += _get_keystore_type_keytool_parameters(keystore_type)
+
+    secret_data = "%s\n%s" % (keystore_pass, pkcs12_pass)
+    # Password of a new keystore must be entered twice, for confirmation
+    if not os.path.exists(keystore_path):
+        secret_data = "%s\n%s" % (keystore_pass, secret_data)
+
+    # Use local certificate from local path and import it to a java keystore
+    (import_rc, import_out, import_err) = module.run_command(import_cmd, data=secret_data, check_rc=False)
+
+    diff = {'before': '\n', 'after': '%s\n' % keystore_alias}
+    if import_rc == 0 and os.path.exists(keystore_path):
+        module.exit_json(changed=True, msg=import_out,
+                         rc=import_rc, cmd=import_cmd, stdout=import_out,
+                         error=import_err, diff=diff)
+    else:
+        module.fail_json(msg=import_out, rc=import_rc, cmd=import_cmd, error=import_err)
+
+
 def import_cert_path(module, executable, path, keystore_path, keystore_pass, alias, keystore_type, trust_cacert):
     ''' Import certificate from path into keystore located on
         keystore_path as alias '''
@@ -522,6 +565,10 @@ def main():
                 # The existing certificate must first be deleted before we insert the correct one
                 delete_cert(module, executable, keystore_path, keystore_pass, cert_alias, keystore_type, exit_after=False)
 
+            if pkcs12_path:
+                import_pkcs12_path(module, executable, pkcs12_path, pkcs12_pass, pkcs12_alias,
+                                   keystore_path, keystore_pass, cert_alias, keystore_type)
+            else:
                 import_cert_path(module, executable, new_certificate, keystore_path,
                                  keystore_pass, cert_alias, keystore_type, trust_cacert)
 

tests/integration/targets/java_cert/defaults/main.yml

@@ -5,8 +5,10 @@ test_keystore2_path: "{{ output_dir }}/keystore2.jks"
 test_keystore2_password: changeit
 test_cert_path: "{{ output_dir }}/cert.pem"
 test_key_path: "{{ output_dir }}/key.pem"
+test_csr_path: "{{ output_dir }}/req.csr"
 test_cert2_path: "{{ output_dir }}/cert2.pem"
 test_key2_path: "{{ output_dir }}/key2.pem"
+test_csr2_path: "{{ output_dir }}/req2.csr"
 test_pkcs_path: "{{ output_dir }}/cert.p12"
 test_pkcs2_path: "{{ output_dir }}/cert2.p12"
 test_ssl: setupSSLServer.py

tests/integration/targets/java_cert/tasks/main.yml

@@ -7,32 +7,34 @@
   block:
 
     - name: prep pkcs12 file
-      copy: src="{{ test_pkcs12_path }}" dest="{{output_dir}}/{{ test_pkcs12_path }}"
+      ansible.builtin.copy:
+        src: "{{ test_pkcs12_path }}"
+        dest: "{{ output_dir }}/{{ test_pkcs12_path }}"
 
     - name: import pkcs12
-      java_cert:
+      community.general.java_cert:
-        pkcs12_path: "{{output_dir}}/{{ test_pkcs12_path }}"
+        pkcs12_path: "{{ output_dir }}/{{ test_pkcs12_path }}"
         pkcs12_password: changeit
         pkcs12_alias: default
         cert_alias: default
-        keystore_path: "{{output_dir}}/{{ test_keystore_path }}"
+        keystore_path: "{{ output_dir }}/{{ test_keystore_path }}"
         keystore_pass: changeme_keystore
         keystore_create: yes
         state: present
       register: result_success
 
     - name: verify success
-      assert:
+      ansible.builtin.assert:
         that:
         - result_success is successful
 
     - name: import pkcs12 with wrong password
-      java_cert:
+      community.general.java_cert:
-        pkcs12_path: "{{output_dir}}/{{ test_pkcs12_path }}"
+        pkcs12_path: "{{ output_dir }}/{{ test_pkcs12_path }}"
         pkcs12_password: wrong_pass
         pkcs12_alias: default
         cert_alias: default_new
-        keystore_path: "{{output_dir}}/{{ test_keystore_path }}"
+        keystore_path: "{{ output_dir }}/{{ test_keystore_path }}"
         keystore_pass: changeme_keystore
         keystore_create: yes
         state: present
@@ -40,16 +42,16 @@
       register: result_wrong_pass
 
     - name: verify fail with wrong import password
-      assert:
+      ansible.builtin.assert:
         that:
         - result_wrong_pass is failed
 
     - name: test fail on mutually exclusive params
-      java_cert:
+      community.general.java_cert:
         cert_path: ca.crt
-        pkcs12_path: "{{output_dir}}/{{ test_pkcs12_path }}"
+        pkcs12_path: "{{ output_dir }}/{{ test_pkcs12_path }}"
         cert_alias: default
-        keystore_path: "{{output_dir}}/{{ test_keystore_path }}"
+        keystore_path: "{{ output_dir }}/{{ test_keystore_path }}"
         keystore_pass: changeme_keystore
         keystore_create: yes
         state: present
@@ -57,26 +59,26 @@
       register: result_excl_params
 
     - name: verify failed exclusive params
-      assert:
+      ansible.builtin.assert:
         that:
         - result_excl_params is failed
 
     - name: test fail on missing required params
-      java_cert:
+      community.general.java_cert:
-        keystore_path: "{{output_dir}}/{{ test_keystore_path }}"
+        keystore_path: "{{ output_dir }}/{{ test_keystore_path }}"
         keystore_pass: changeme_keystore
         state: absent
       ignore_errors: true
       register: result_missing_required_param
 
     - name: verify failed missing required params
-      assert:
+      ansible.builtin.assert:
         that:
         - result_missing_required_param is failed
 
     - name: delete object based on cert_alias parameter
-      java_cert:
+      community.general.java_cert:
-        keystore_path: "{{output_dir}}/{{ test_keystore_path }}"
+        keystore_path: "{{ output_dir }}/{{ test_keystore_path }}"
         keystore_pass: changeme_keystore
         cert_alias: default
         state: absent
@@ -84,7 +86,7 @@
       register: result_alias_deleted
 
     - name: verify object successfully deleted
-      assert:
+      ansible.builtin.assert:
         that:
         - result_alias_deleted is successful
 
@@ -92,7 +94,7 @@
       import_tasks: state_change.yml
 
     - name: cleanup environment
-      file:
+      ansible.builtin.file:
         path: "{{ item }}"
         state: absent
       loop:
@@ -101,7 +103,9 @@
         - "{{ test_keystore2_path }}"
         - "{{ test_cert_path }}"
         - "{{ test_key_path }}"
+        - "{{ test_csr_path }}"
         - "{{ test_cert2_path }}"
         - "{{ test_key2_path }}"
+        - "{{ test_csr2_path }}"
         - "{{ test_pkcs_path }}"
         - "{{ test_pkcs2_path }}"

tests/integration/targets/java_cert/tasks/state_change.yml

@@ -1,36 +1,96 @@
 ---
-- name: Generate the self signed cert used as a place holder to create the java keystore
+#
-  command: openssl req -x509 -newkey rsa:4096 -keyout {{ test_key_path }} -out {{ test_cert_path }} -days 365 -nodes -subj '/CN=localhost'
+# Prepare X509 and PKCS#12 materials
-  args:
+#
-    creates: "{{ test_key_path }}"
+
+- name: Create private keys
+  community.crypto.openssl_privatekey:
+    path: "{{ item }}"
+    mode: "u=rw,go="
+  loop:
+    - "{{ test_key_path }}"
+    - "{{ test_key2_path }}"
+
+- name: Generate CSR for self-signed certificate used as a placeholder to create the java keystore
+  community.crypto.openssl_csr:
+    path: "{{ test_csr_path }}"
+    privatekey_path: "{{ test_key_path }}"
+    commonName: "localhost"
+
+- name: Generate CSR for self-signed certificate used for testing
+  community.crypto.openssl_csr:
+    path: "{{ test_csr2_path }}"
+    privatekey_path: "{{ test_key2_path }}"
+    commonName: "localhost"
+
+- name: Generate the self-signed cert used as a placeholder to create the java keystore
+  community.crypto.x509_certificate:
+    path: "{{ test_cert_path }}"
+    csr_path: "{{ test_csr_path }}"
+    privatekey_path: "{{ test_key_path }}"
+    provider: selfsigned
 
 - name: Generate the self signed cert we will use for testing
-  command: openssl req -x509 -newkey rsa:4096 -keyout '{{ test_key2_path }}' -out '{{ test_cert2_path }}' -days 365 -nodes -subj '/CN=localhost'
+  community.crypto.x509_certificate:
-  args:
+    path: "{{ test_cert2_path }}"
-    creates: "{{ test_key2_path }}"
+    csr_path: "{{ test_csr2_path }}"
+    privatekey_path: "{{ test_key2_path }}"
+    provider: selfsigned
 
 - name: Create the pkcs12 archive from the test x509 cert
-  command: >
+  community.crypto.openssl_pkcs12:
-    openssl pkcs12
+    name: "test_pkcs12_cert"
+    path: "{{ test_pkcs_path }}"
+    passphrase: "{{ test_keystore2_password }}"
+    certificate_path: "{{ test_cert_path }}"
+    privatekey_path: "{{ test_key_path }}"
+  when:
+    - "not (ansible_os_family == 'RedHat' and ansible_distribution_version is version('8.0', '<'))"
+
+- name: Create the pkcs12 archive from the test x509 cert (command)
+  ansible.builtin.command:
+    cmd: >
+      openssl pkcs12 -export
       -in {{ test_cert_path }}
       -inkey {{ test_key_path }}
-    -export
       -name test_pkcs12_cert
       -out {{ test_pkcs_path }}
-    -passout pass:"{{ test_keystore2_password }}"
+      -passout stdin
+    stdin: "{{ test_keystore2_password }}"
+  when:
+    - "ansible_os_family == 'RedHat'"
+    - "ansible_distribution_version is version('8.0', '<')"
 
 - name: Create the pkcs12 archive from the certificate we will be trying to add to the keystore
-  command: >
+  community.crypto.openssl_pkcs12:
-    openssl pkcs12
+    name: "test_pkcs12_cert"
+    path: "{{ test_pkcs2_path }}"
+    passphrase: "{{ test_keystore2_password }}"
+    certificate_path: "{{ test_cert2_path }}"
+    privatekey_path: "{{ test_key2_path }}"
+  when:
+    - "not (ansible_os_family == 'RedHat' and ansible_distribution_version is version('8.0', '<'))"
+
+- name: Create the pkcs12 archive from the certificate we will be trying to add to the keystore (command)
+  ansible.builtin.command:
+    cmd: >
+      openssl pkcs12 -export
       -in {{ test_cert2_path }}
       -inkey {{ test_key2_path }}
-    -export
       -name test_pkcs12_cert
       -out {{ test_pkcs2_path }}
-    -passout pass:"{{ test_keystore2_password }}"
+      -passout stdin
+    stdin: "{{ test_keystore2_password }}"
+  when:
+    - "ansible_os_family == 'RedHat'"
+    - "ansible_distribution_version is version('8.0', '<')"
+
+#
+# Run tests
+#
 
 - name: try to create the test keystore based on the just created pkcs12, keystore_create flag not enabled
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: test_pkcs12_cert
     pkcs12_path: "{{ test_pkcs_path }}"
@@ -41,12 +101,12 @@
   register: result_x509_changed
 
 - name: Verify the x509 status is failed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_changed is failed
 
 - name: Create the test keystore based on the just created pkcs12
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: test_pkcs12_cert
     pkcs12_path: "{{ test_pkcs_path }}"
@@ -55,8 +115,19 @@
     keystore_pass: "{{ test_keystore2_password }}"
     keystore_create: yes
 
+- name: List newly created keystore content
+  ansible.builtin.command:
+    cmd: "keytool -list -keystore {{ test_keystore2_path }}"
+    stdin: "{{ test_keystore2_password }}"
+  register: keytool_list_keystore
+
+- name: Assert that the keystore has a private key entry
+  ansible.builtin.assert:
+    that:
+      - "keytool_list_keystore.stdout_lines[5] is match('test_pkcs12_cert,.*, PrivateKeyEntry, $')"
+
 - name: try to import from pkcs12 a non existing alias
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: non_existing_alias
     pkcs12_path: "{{ test_pkcs_path }}"
@@ -68,12 +139,12 @@
   register: result_x509_changed
 
 - name: Verify the x509 status is failed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_changed is failed
 
 - name:  import initial test certificate from file path
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_cert
     cert_path: "{{ test_cert_path }}"
     keystore_path: "{{ test_keystore2_path }}"
@@ -83,7 +154,7 @@
   register: result_x509_changed
 
 - name: Verify the x509 status is changed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_changed is changed
 
@@ -92,7 +163,7 @@
     If the java_cert has been updated properly, then this task will report changed each time
     since the module will be comparing the hash of the certificate instead of validating that the alias
     simply exists
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_cert
     cert_path: "{{ test_cert2_path }}"
     keystore_path: "{{ test_keystore2_path }}"
@@ -101,13 +172,13 @@
   register: result_x509_changed
 
 - name: Verify the x509 status is changed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_changed is changed
 
 - name: |
     We also want to make sure that the status doesnt change if we import the same cert
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_cert
     cert_path: "{{ test_cert2_path }}"
     keystore_path: "{{ test_keystore2_path }}"
@@ -116,13 +187,13 @@
   register: result_x509_succeeded
 
 - name: Verify the x509 status is ok
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_succeeded is succeeded
 
 - name: >
     Ensure the original pkcs12 cert is in the keystore
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: test_pkcs12_cert
     pkcs12_path: "{{ test_pkcs_path }}"
@@ -134,7 +205,7 @@
 - name: |
     Perform the same test, but we will now be testing the pkcs12 functionality
     If we add a different pkcs12 cert with the same alias, we should have a changed result, NOT the same
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: test_pkcs12_cert
     pkcs12_path: "{{ test_pkcs2_path }}"
@@ -145,13 +216,13 @@
   register: result_pkcs12_changed
 
 - name: Verify the pkcs12 status is changed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_pkcs12_changed is changed
 
 - name: |
     We are requesting the same cert now, so the status should show OK
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     pkcs12_alias: test_pkcs12_cert
     pkcs12_path: "{{ test_pkcs2_path }}"
@@ -161,7 +232,7 @@
   register: result_pkcs12_succeeded
 
 - name: Verify the pkcs12 status is ok
-  assert:
+  ansible.builtin.assert:
     that:
       - result_pkcs12_succeeded is succeeded
 
@@ -178,7 +249,7 @@
 - name: |
     Download the original cert.pem from our temporary server. The current cert should contain
      cert2.pem. Importing this cert should return a status of changed
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_cert_localhost
     cert_url: localhost
     cert_port: "{{ test_ssl_port }}"
@@ -188,12 +259,12 @@
   register: result_url_changed
 
 - name: Verify that the url status is changed
-  assert:
+  ansible.builtin.assert:
     that:
       - result_url_changed is changed
 
 - name: Ensure we can remove the x509 cert
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_cert
     keystore_path: "{{ test_keystore2_path }}"
     keystore_pass: "{{ test_keystore2_password }}"
@@ -201,12 +272,12 @@
   register: result_x509_absent
 
 - name: Verify the x509 cert is absent
-  assert:
+  ansible.builtin.assert:
     that:
       - result_x509_absent is changed
 
 - name: Ensure we can remove the certificate imported from pkcs12 archive
-  java_cert:
+  community.general.java_cert:
     cert_alias: test_pkcs12_cert
     keystore_path: "{{ test_keystore2_path }}"
     keystore_pass: "{{ test_keystore2_password }}"
@@ -214,6 +285,6 @@
   register: result_pkcs12_absent
 
 - name: Verify the pkcs12 archive is absent
-  assert:
+  ansible.builtin.assert:
     that:
       - result_pkcs12_absent is changed