java_keystore: fix keystore type (#2516) (#2893)

* fix keystore type; update unit tests

* add changelog fragment

* document new param 'keystore_type'

* add keystore_type support (backward compatible)

* check JKS format with magic bytes
* update integration tests
* revert first changes in unit tests
* update changelog fragment

* fix magic bytes for python2/python3

* fix integration tests (irrelevant check_mode)

* fix unit test (keystore removed before failure => changed=true)

* fix typo

* fix spelling

* shorten a branch

* mock is_jks_or_pkcs12

* fix function path in unit tests

* Apply suggestions from code review (spelling)

Co-authored-by: Ajpantuso <ajpantuso@gmail.com>

* rename a method (module + unit tests)

* move ArgumentSpec class content to main()

* refactor create() to not loose existing keystore in case of error

* update unit tests

* add integration test (error handling)

* fix keystore backup cleanup

Co-authored-by: Ajpantuso <ajpantuso@gmail.com>
(cherry picked from commit 199ead85d0)

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

plugins/modules/system/java_keystore.py

@@ -1,8 +1,8 @@
 #!/usr/bin/python
 # -*- coding: utf-8 -*-
 
-# Copyright: (c) 2016, Guillaume Grossetie <ggrossetie@yuzutech.fr>
 # Copyright: (c) 2021, quidame <quidame@poivron.org>
+# Copyright: (c) 2016, Guillaume Grossetie <ggrossetie@yuzutech.fr>
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
 from __future__ import (absolute_import, division, print_function)
@@ -97,6 +97,24 @@ options:
       - openssl
       - cryptography
     version_added: 3.1.0
+  keystore_type:
+    description:
+      - Type of the Java keystore.
+      - When this option is omitted and the keystore doesn't already exist, the
+        behavior follows C(keytool)'s default store type which depends on
+        Java version; C(pkcs12) since Java 9 and C(jks) prior (may also
+        be C(pkcs12) if new default has been backported to this version).
+      - When this option is omitted and the keystore already exists, the current
+        type is left untouched, unless another option leads to overwrite the
+        keystore (in that case, this option behaves like for keystore creation).
+      - When I(keystore_type) is set, the keystore is created with this type if
+        it doesn't already exist, or is overwritten to match the given type in
+        case of mismatch.
+    type: str
+    choices:
+      - jks
+      - pkcs12
+    version_added: 3.3.0
 requirements:
   - openssl in PATH (when I(ssl_backend=openssl))
   - keytool in PATH
@@ -107,12 +125,16 @@ author:
 extends_documentation_fragment:
   - files
 seealso:
+  - module: community.crypto.openssl_pkcs12
   - module: community.general.java_cert
 notes:
   - I(certificate) and I(private_key) require that their contents are available
     on the controller (either inline in a playbook, or with the C(file) lookup),
     while I(certificate_path) and I(private_key_path) require that the files are
     available on the target host.
+  - By design, any change of a value of options I(keystore_type), I(name) or
+    I(password), as well as changes of key or certificate materials will cause
+    the existing I(dest) to be overwritten.
 '''
 
 EXAMPLES = '''
@@ -156,6 +178,12 @@ msg:
   type: str
   sample: "Unable to find the current certificate fingerprint in ..."
 
+err:
+  description: Output from stderr of keytool/openssl command after error of given command.
+  returned: failure
+  type: str
+  sample: "Keystore password is too short - must be at least 6 characters\n"
+
 rc:
   description: keytool/openssl command execution return value
   returned: changed and failure
@@ -176,7 +204,7 @@ import tempfile
 
 from ansible.module_utils.six import PY2
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
-from ansible.module_utils.common.text.converters import to_bytes, to_native, to_text
+from ansible.module_utils.common.text.converters import to_bytes, to_native
 
 try:
     from cryptography.hazmat.primitives.serialization.pkcs12 import serialize_key_and_certificates
@@ -201,6 +229,7 @@ except ImportError:
 class JavaKeystore:
     def __init__(self, module):
         self.module = module
+        self.result = dict()
 
         self.keytool_bin = module.get_bin_path('keytool', True)
 
@@ -211,6 +240,7 @@ class JavaKeystore:
         self.password = module.params['password']
         self.private_key = module.params['private_key']
         self.ssl_backend = module.params['ssl_backend']
+        self.keystore_type = module.params['keystore_type']
 
         if self.ssl_backend == 'openssl':
             self.openssl_bin = module.get_bin_path('openssl', True)
@@ -312,6 +342,9 @@ class JavaKeystore:
                 rc=rc
             )
 
+        if self.keystore_type not in (None, self.current_type()):
+            return "keystore type mismatch"
+
         stored_certificate_match = re.search(r"SHA256: ([\w:]+)", stored_certificate_fingerprint_out)
         if not stored_certificate_match:
             return self.module.fail_json(
@@ -322,6 +355,14 @@ class JavaKeystore:
 
         return stored_certificate_match.group(1)
 
+    def current_type(self):
+        magic_bytes = b'\xfe\xed\xfe\xed'
+        with open(self.keystore_path, 'rb') as fd:
+            header = fd.read(4)
+        if header == magic_bytes:
+            return 'jks'
+        return 'pkcs12'
+
     def cert_changed(self):
         current_certificate_fingerprint = self.read_certificate_fingerprint()
         stored_certificate_fingerprint = self.read_stored_certificate_fingerprint()
@@ -389,6 +430,8 @@ class JavaKeystore:
         with open(keystore_p12_path, 'wb') as p12_file:
             p12_file.write(pkcs12_bundle)
 
+        self.result.update(msg="PKCS#12 bundle created by cryptography backend")
+
     def openssl_create_pkcs12_bundle(self, keystore_p12_path):
         export_p12_cmd = [self.openssl_bin, "pkcs12", "-export", "-name", self.name, "-in", self.certificate_path,
                           "-inkey", self.private_key_path, "-out", keystore_p12_path, "-passout", "stdin"]
@@ -401,19 +444,22 @@ class JavaKeystore:
             cmd_stdin = "%s\n" % self.keypass
         cmd_stdin += "%s\n%s" % (self.password, self.password)
 
-        (rc, export_p12_out, dummy) = self.module.run_command(
+        (rc, export_p12_out, export_p12_err) = self.module.run_command(
             export_p12_cmd, data=cmd_stdin, environ_update=None, check_rc=False
         )
 
+        self.result = dict(msg=export_p12_out, cmd=export_p12_cmd, rc=rc)
         if rc != 0:
-            self.module.fail_json(msg=export_p12_out, cmd=export_p12_cmd, rc=rc)
+            self.result['err'] = export_p12_err
+            self.module.fail_json(**self.result)
 
     def create(self):
+        """Create the keystore, or replace it with a rollback in case of
+           keytool failure.
+        """
         if self.module.check_mode:
-            return {'changed': True}
-
-        if os.path.exists(self.keystore_path):
-            os.remove(self.keystore_path)
+            self.result['changed'] = True
+            return self.result
 
         keystore_p12_path = create_path()
         self.module.add_cleanup_file(keystore_p12_path)
@@ -423,6 +469,13 @@ class JavaKeystore:
         else:
             self.openssl_create_pkcs12_bundle(keystore_p12_path)
 
+        if self.keystore_type == 'pkcs12':
+            # Preserve properties of the destination file, if any.
+            self.module.atomic_move(keystore_p12_path, self.keystore_path)
+            self.update_permissions()
+            self.result['changed'] = True
+            return self.result
+
         import_keystore_cmd = [self.keytool_bin, "-importkeystore",
                                "-destkeystore", self.keystore_path,
                                "-srckeystore", keystore_p12_path,
@@ -430,19 +483,38 @@ class JavaKeystore:
                                "-alias", self.name,
                                "-noprompt"]
 
-        (rc, import_keystore_out, dummy) = self.module.run_command(
+        if self.keystore_type == 'jks':
+            keytool_help = self.module.run_command([self.keytool_bin, '-importkeystore', '-help'])
+            if '-deststoretype' in keytool_help[1] + keytool_help[2]:
+                import_keystore_cmd.insert(4, "-deststoretype")
+                import_keystore_cmd.insert(5, self.keystore_type)
+
+        keystore_backup = None
+        if self.exists():
+            keystore_backup = self.keystore_path + '.tmpbak'
+            # Preserve properties of the source file
+            self.module.preserved_copy(self.keystore_path, keystore_backup)
+            os.remove(self.keystore_path)
+
+        (rc, import_keystore_out, import_keystore_err) = self.module.run_command(
             import_keystore_cmd, data='%s\n%s\n%s' % (self.password, self.password, self.password), check_rc=False
         )
-        if rc != 0:
-            return self.module.fail_json(msg=import_keystore_out, cmd=import_keystore_cmd, rc=rc)
+
+        self.result = dict(msg=import_keystore_out, cmd=import_keystore_cmd, rc=rc)
+
+        # keytool may return 0 whereas the keystore has not been created.
+        if rc != 0 or not self.exists():
+            if keystore_backup is not None:
+                self.module.preserved_copy(keystore_backup, self.keystore_path)
+                os.remove(keystore_backup)
+            self.result['err'] = import_keystore_err
+            return self.module.fail_json(**self.result)
 
         self.update_permissions()
-        return {
-            'changed': True,
-            'msg': import_keystore_out,
-            'cmd': import_keystore_cmd,
-            'rc': rc
-        }
+        if keystore_backup is not None:
+            os.remove(keystore_backup)
+        self.result['changed'] = True
+        return self.result
 
     def exists(self):
         return os.path.exists(self.keystore_path)
@@ -465,15 +537,15 @@ def create_file(content):
 def hex_decode(s):
     if PY2:
         return s.decode('hex')
-    else:
-        return s.hex()
+    return s.hex()
 
 
-class ArgumentSpec(object):
-    def __init__(self):
-        self.supports_check_mode = True
-        self.add_file_common_args = True
-        argument_spec = dict(
+def main():
+    choose_between = (['certificate', 'certificate_path'],
+                      ['private_key', 'private_key_path'])
+
+    module = AnsibleModule(
+        argument_spec=dict(
             name=dict(type='str', required=True),
             dest=dict(type='path', required=True),
             certificate=dict(type='str', no_log=True),
@@ -483,25 +555,13 @@ class ArgumentSpec(object):
             private_key_passphrase=dict(type='str', no_log=True),
             password=dict(type='str', required=True, no_log=True),
             ssl_backend=dict(type='str', default='openssl', choices=['openssl', 'cryptography']),
+            keystore_type=dict(type='str', choices=['jks', 'pkcs12']),
             force=dict(type='bool', default=False),
-        )
-        choose_between = (
-            ['certificate', 'certificate_path'],
-            ['private_key', 'private_key_path'],
-        )
-        self.argument_spec = argument_spec
-        self.required_one_of = choose_between
-        self.mutually_exclusive = choose_between
-
-
-def main():
-    spec = ArgumentSpec()
-    module = AnsibleModule(
-        argument_spec=spec.argument_spec,
-        required_one_of=spec.required_one_of,
-        mutually_exclusive=spec.mutually_exclusive,
-        supports_check_mode=spec.supports_check_mode,
-        add_file_common_args=spec.add_file_common_args,
+        ),
+        required_one_of=choose_between,
+        mutually_exclusive=choose_between,
+        supports_check_mode=True,
+        add_file_common_args=True,
     )
     module.run_command_environ_update = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C')