java_keystore: overwrite instead of fail when password or alias does not match (#2262)

* Overwrite instead of fail when password does not match.

* Update documentation.

* Fix tests.

* Update plugins/modules/system/java_keystore.py

Co-authored-by: Amin Vakil <info@aminvakil.com>

* Fix documentation.

* Apply suggestions from code review

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

* Update tests/unit/plugins/modules/system/test_java_keystore.py

* One more.

Co-authored-by: Amin Vakil <info@aminvakil.com>
Co-authored-by: quidame <quidame@poivron.org>

changelogs/fragments/2262-java_keystore-passphrase.yml

@@ -0,0 +1,8 @@
+breaking_changes:
+- "java_keystore - instead of failing, now overwrites keystore if the alias (name) is changed.
+  This was originally the intended behavior, but did not work due to a logic error. Make sure
+  that your playbooks and roles do not depend on the old behavior of failing instead of
+  overwriting (https://github.com/ansible-collections/community.general/issues/1671)."
+- "java_keystore - instead of failing, now overwrites keystore if the passphrase is changed.
+  Make sure that your playbooks and roles do not depend on the old behavior of failing instead
+  of overwriting (https://github.com/ansible-collections/community.general/issues/1671)."

plugins/modules/system/java_keystore.py

@@ -19,8 +19,9 @@ options:
   name:
     description:
       - Name of the certificate in the keystore.
-      - If the provided name does not exist in the keystore, the module fails.
+      - If the provided name does not exist in the keystore, the module
-        This behavior will change in a next release.
+        will re-create the keystore. This behavior changed in community.general 3.0.0,
+        before that the module would fail when the name did not match.
     type: str
     required: true
   certificate:
@@ -60,7 +61,9 @@ options:
     description:
       - Password that should be used to secure the keystore.
       - If the provided password fails to unlock the keystore, the module
-        fails. This behavior will change in a next release.
+        will re-create the keystore with the new passphrase. This behavior
+        changed in community.general 3.0.0, before that the module would fail
+        when the password did not match.
     type: str
     required: true
   dest:
@@ -187,16 +190,11 @@ def read_stored_certificate_fingerprint(module, keytool_bin, alias, keystore_pat
     (rc, stored_certificate_fingerprint_out, stored_certificate_fingerprint_err) = run_commands(
         module, stored_certificate_fingerprint_cmd, environ_update=dict(STOREPASS=keystore_password))
     if rc != 0:
-        # First intention was to not fail, and overwrite the keystore instead,
+        if "keytool error: java.lang.Exception: Alias <%s> does not exist" % alias in stored_certificate_fingerprint_out:
-        # in case of alias mismatch; but an issue in error handling caused the
+            return "alias mismatch"
-        # module to fail anyway.
+        if re.match(r'keytool error: java\.io\.IOException: [Kk]eystore( was tampered with, or)? password was incorrect',
-        # See: https://github.com/ansible-collections/community.general/issues/1671
+                    stored_certificate_fingerprint_out):
-        # And: https://github.com/ansible-collections/community.general/pull/2183
+            return "password mismatch"
-        # if "keytool error: java.lang.Exception: Alias <%s> does not exist" % alias in stored_certificate_fingerprint_out:
-        #     return "alias mismatch"
-        # if re.match(r'keytool error: java\.io\.IOException: [Kk]eystore( was tampered with, or)? password was incorrect',
-        #             stored_certificate_fingerprint_out):
-        #    return "password mismatch"
         return module.fail_json(msg=stored_certificate_fingerprint_out,
                                 err=stored_certificate_fingerprint_err,
                                 cmd=stored_certificate_fingerprint_cmd,

tests/integration/targets/java_keystore/tasks/tests.yml

@@ -64,7 +64,6 @@
   loop: "{{ java_keystore_new_certs }}"
   check_mode: yes
   register: result_alias_change_check
-  when: false  # FIXME: module currently crashes
 
 - name: Create a Java keystore for the given certificates (alias changed)
   community.general.java_keystore:
@@ -72,7 +71,6 @@
     name: foobar
   loop: "{{ java_keystore_new_certs }}"
   register: result_alias_change
-  when: false  # FIXME: module currently crashes
 
 
 - name: Create a Java keystore for the given certificates (password changed, check mode)
@@ -83,7 +81,6 @@
   loop: "{{ java_keystore_new_certs }}"
   check_mode: yes
   register: result_pw_change_check
-  when: false  # FIXME: module currently crashes
 
 - name: Create a Java keystore for the given certificates (password changed)
   community.general.java_keystore:
@@ -92,7 +89,6 @@
     password: hunter2
   loop: "{{ java_keystore_new_certs }}"
   register: result_pw_change
-  when: false  # FIXME: module currently crashes
 
 - name: Check that the remote certificates have not been removed
   ansible.builtin.file:
@@ -117,7 +113,7 @@
       - result_idem_check is not changed
       - result_change is changed
       - result_change_check is changed
-      # - result_alias_change is changed        # FIXME: module currently crashes
+      - result_alias_change is changed
-      # - result_alias_change_check is changed  # FIXME: module currently crashes
+      - result_alias_change_check is changed
-      # - result_pw_change is changed           # FIXME: module currently crashes
+      - result_pw_change is changed
-      # - result_pw_change_check is changed     # FIXME: module currently crashes
+      - result_pw_change_check is changed

tests/unit/plugins/modules/system/test_java_keystore.py

@@ -250,20 +250,34 @@ class TestCertChanged(ModuleTestCase):
             supports_check_mode=self.spec.supports_check_mode
         )
 
-        module.fail_json = Mock()
-
         with patch('os.remove', return_value=True):
             self.create_file.side_effect = ['/tmp/placeholder']
             self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''),
                                              (1, 'keytool error: java.lang.Exception: Alias <foo> does not exist', '')]
-            cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo')
+            result = cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo')
-            module.fail_json.assert_called_once_with(
+            self.assertTrue(result, 'Alias mismatch detected')
-                cmd=["keytool", "-list", "-alias", "foo", "-keystore", "/path/to/keystore.jks", "-storepass:env", "STOREPASS", "-v"],
+
-                msg='keytool error: java.lang.Exception: Alias <foo> does not exist',
+    def test_cert_changed_password_mismatch(self):
-                err='',
+        set_module_args(dict(
-                rc=1
+            certificate='cert-foo',
+            private_key='private-foo',
+            dest='/path/to/keystore.jks',
+            name='foo',
+            password='changeit'
+        ))
+
+        module = AnsibleModule(
+            argument_spec=self.spec.argument_spec,
+            supports_check_mode=self.spec.supports_check_mode
         )
 
+        with patch('os.remove', return_value=True):
+            self.create_file.side_effect = ['/tmp/placeholder']
+            self.run_commands.side_effect = [(0, 'foo=abcd:1234:efgh', ''),
+                                             (1, 'keytool error: java.io.IOException: Keystore password was incorrect', '')]
+            result = cert_changed(module, "openssl", "keytool", "/path/to/keystore.jks", "changeit", 'foo')
+            self.assertTrue(result, 'Password mismatch detected')
+
     def test_cert_changed_fail_read_cert(self):
         set_module_args(dict(
             certificate='cert-foo',