Repository: ansible/ansible
Pull request: openssl_privatekey: add backup option
##### SUMMARY
Adds a `backup` option (similarly to the `copy` or `template` modules) and mention it as an option for paranoid/careless users. (Default is `off`.)

Fixes #32038.

##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
openssl_privatekey


Problem statement:
openssl_privatekey: Do not recreate already existing private key
##### ISSUE TYPE
 - Feature Idea

##### COMPONENT NAME
openssl_privatekey

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
ansible 2.4.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/bott/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]
```

##### CONFIGURATION
Default

##### OS / ENVIRONMENT
Debian Stretch

##### SUMMARY
The openssl_privatekey module recreates an already existing private key if it does not match all criteria - e.g. size is specified with 4096 bits whereas the existing key only has 2048 bits length. While this makes sense in some scenarios, there is also one where it is not very helpful: if you already have e.g. signed (external) certs depending on that key, you would have to get new certificates asap. While we would like to create new keys with 4096 bits, we would like to keep existing ones.
We would like to see some sort of modified/altered "force" parameter which softens the check on existing keys.

##### STEPS TO REPRODUCE
* Create a key with size:2048
* Change the task to create a key with size: 4096
* Re-run and the key will be overwritten with a new one

Hints from issue discussion:
cc @Spredzy
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
I strongly disagree. If a specified criterion is not matched, some action has to be taken or the module should fail. If you are worried that keys could be regenerated, run the play in check mode.
I tend to agree with @MarkusTeufelberger. Configuration Management tool compare expected state with current state and if different make it so exected state == current state.
cc @felixfontein
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @resmo @spredzy
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @Shaps @Xyon @puiterwijk
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Perhaps this can be solved by being able to provide a list of parameters which are ignored for idempotence checking (including some special parameter `*`, which means "put everything on this list", i.e. only create if the key isn't there)?
I disagree that this is a bug at all, it is expected behavior to create a key with the specified length if it doesn't exist yet. At most there could/should(?) be a `backup: true/false` parameter that works like the one in the `copy` module for example to prevent someone from removing a key that is actually still in use and has some expensive metadata (like a certificate) attached. The key should still be generated in the scenario given in the description above.
It's not a bug, it's a feature request :) Having a backup option is definitely a good idea.

One question would be whether backup should be enabled by default (to prevent people to shoot themselves in the foot) or not (fully backwards compatible, but easier to destroy something important). Considering that the loss of an existing private key can be really expensive, I would enable it by default.
I don't want old keys staying around or files being created that I don't expect or have defined initially. There are other files created with the "copy" module for example that can be really expensive/hard to recover as well. If a key is really that expensive, it probably should be backed up with an actual backup program. The thing people want isn't backup anyways, its restore... ;-)

Commit message: Add tests for backup.

Patch to review:
diff --git a/changelogs/fragments/53593-openssl_privatekey-backup.yml b/changelogs/fragments/53593-openssl_privatekey-backup.yml
new file mode 100644
index 00000000000000..55a9a4ddab795a
--- /dev/null
+++ b/changelogs/fragments/53593-openssl_privatekey-backup.yml
@@ -0,0 +1,2 @@
+minor_changes:
+- "openssl_privatekey - add ``backup`` option."
diff --git a/lib/ansible/module_utils/crypto.py b/lib/ansible/module_utils/crypto.py
index 629cc0a0e8c191..6bee5d7c06c431 100644
--- a/lib/ansible/module_utils/crypto.py
+++ b/lib/ansible/module_utils/crypto.py
@@ -230,7 +230,7 @@ def generate(self):
 
         pass
 
-    def remove(self):
+    def remove(self, module):
         """Remove the resource from the filesystem."""
 
         try:
diff --git a/lib/ansible/modules/crypto/openssl_certificate.py b/lib/ansible/modules/crypto/openssl_certificate.py
index 56135963b7541c..ba91773176e14b 100644
--- a/lib/ansible/modules/crypto/openssl_certificate.py
+++ b/lib/ansible/modules/crypto/openssl_certificate.py
@@ -1220,7 +1220,7 @@ def main():
             module.exit_json(**result)
 
         try:
-            certificate.remove()
+            certificate.remove(module)
         except CertificateError as exc:
             module.fail_json(msg=to_native(exc))
 
diff --git a/lib/ansible/modules/crypto/openssl_csr.py b/lib/ansible/modules/crypto/openssl_csr.py
index 48cb4483e40e3a..ffc5204e9a4e6d 100644
--- a/lib/ansible/modules/crypto/openssl_csr.py
+++ b/lib/ansible/modules/crypto/openssl_csr.py
@@ -1044,7 +1044,7 @@ def main():
             module.exit_json(**result)
 
         try:
-            csr.remove()
+            csr.remove(module)
         except (CertificateSigningRequestError, crypto_utils.OpenSSLObjectError) as exc:
             module.fail_json(msg=to_native(exc))
 
diff --git a/lib/ansible/modules/crypto/openssl_pkcs12.py b/lib/ansible/modules/crypto/openssl_pkcs12.py
index 1d39d616951073..0cf2ab1abf8a77 100644
--- a/lib/ansible/modules/crypto/openssl_pkcs12.py
+++ b/lib/ansible/modules/crypto/openssl_pkcs12.py
@@ -241,7 +241,7 @@ def generate(self, module):
         self.pkcs12 = crypto.PKCS12()
 
         try:
-            self.remove()
+            self.remove(module)
         except PkcsError as exc:
             module.fail_json(msg=to_native(exc))
 
@@ -274,14 +274,14 @@ def generate(self, module):
                                                      self.iter_size, self.maciter_size))
             os.close(pkcs12_file)
         except (IOError, OSError) as exc:
-            self.remove()
+            self.remove(module)
             raise PkcsError(exc)
 
     def parse(self, module):
         """Read PKCS#12 file."""
 
         try:
-            self.remove()
+            self.remove(module)
             with open(self.src, 'rb') as pkcs12_fh:
                 pkcs12_content = pkcs12_fh.read()
             p12 = crypto.load_pkcs12(pkcs12_content,
@@ -298,7 +298,7 @@ def parse(self, module):
             os.close(pkcs12_file)
 
         except IOError as exc:
-            self.remove()
+            self.remove(module)
             raise PkcsError(exc)
 
 
@@ -378,7 +378,7 @@ def main():
 
         if os.path.exists(module.params['path']):
             try:
-                pkcs12.remove()
+                pkcs12.remove(module)
                 changed = True
             except PkcsError as exc:
                 module.fail_json(msg=to_native(exc))
diff --git a/lib/ansible/modules/crypto/openssl_privatekey.py b/lib/ansible/modules/crypto/openssl_privatekey.py
index 91f4866f9ee0b3..871d72ad62f247 100644
--- a/lib/ansible/modules/crypto/openssl_privatekey.py
+++ b/lib/ansible/modules/crypto/openssl_privatekey.py
@@ -23,6 +23,11 @@
       L(ECC,https://en.wikipedia.org/wiki/Elliptic-curve_cryptography)
       private keys.
     - Keys are generated in PEM format.
+    - "Please note that the module regenerates private keys if they don't match
+      the module's options. In particular, if you provide another passphrase
+      (or specify none), change the keysize, etc., the private key will be
+      regenerated. If you are concerned of this overwriting your private key,
+      please consider using the I(backup) option."
     - The module can use the cryptography Python library, or the pyOpenSSL Python
       library. By default, it tries to detect which one is available. This can be
       overridden with the I(select_crypto_backend) option."
@@ -111,6 +116,13 @@
         default: auto
         choices: [ auto, cryptography, pyopenssl ]
         version_added: "2.8"
+    backup:
+        description:
+            - Create a backup file including the timestamp information so you can get
+              the original private key back if you somehow clobbered it incorrectly.
+        type: bool
+        default: no
+        version_added: "2.8"
 extends_documentation_fragment:
 - files
 seealso:
@@ -182,6 +194,11 @@
       sha256: "41:ab:c7:cb:d5:5f:30:60:46:99:ac:d4:00:70:cf:a1:76:4f:24:5d:10:24:57:5d:51:6e:09:97:df:2f:de:c7"
       sha384: "85:39:50:4e:de:d9:19:33:40:70:ae:10:ab:59:24:19:51:c3:a2:e4:0b:1c:b1:6e:dd:b3:0c:d9:9e:6a:46:af:da:18:f8:ef:ae:2e:c0:9a:75:2c:9b:b3:0f:3a:5f:3d"
       sha512: "fd:ed:5e:39:48:5f:9f:fe:7f:25:06:3f:79:08:cd:ee:a5:e7:b3:3d:13:82:87:1f:84:e1:f5:c7:28:77:53:94:86:56:38:69:f0:d9:35:22:01:1e:a6:60:...:0f:9b"
+backup_file:
+    description: Name of backup file created.
+    returned: changed and if I(backup) is C(yes)
+    type: str
+    sample: /path/to/privatekey.pem.2019-03-09@11:22~
 '''
 
 import abc
@@ -255,6 +272,9 @@ def __init__(self, module):
         self.privatekey = None
         self.fingerprint = {}
 
+        self.backup = module.params['backup']
+        self.backup_path = None
+
         self.mode = module.params.get('mode', None)
         if self.mode is None:
             self.mode = 0o600
@@ -271,6 +291,8 @@ def generate(self, module):
         """Generate a keypair."""
 
         if not self.check(module, perms_required=False) or self.force:
+            if self.backup:
+                self.backup_file = module.backup_local(self.path)
             privatekey_data = self._generate_private_key_data()
             try:
                 privatekey_file = os.open(self.path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC)
@@ -298,6 +320,11 @@ def generate(self, module):
         if module.set_fs_attributes_if_different(file_args, False):
             self.changed = True
 
+    def remove(self, module):
+        if self.backup:
+            self.backup_file = module.backup_local(self.path)
+        super(PrivateKeyBase, self).remove(module)
+
     @abc.abstractmethod
     def _check_passphrase(self):
         pass
@@ -325,6 +352,8 @@ def dump(self):
             'changed': self.changed,
             'fingerprint': self.fingerprint,
         }
+        if self.backup_path:
+            result['backup_path'] = self.backup_path
 
         return result
 
@@ -583,6 +612,7 @@ def main():
             path=dict(type='path', required=True),
             passphrase=dict(type='str', no_log=True),
             cipher=dict(type='str'),
+            backup=dict(type='bool', default=False),
             select_crypto_backend=dict(type='str', choices=['auto', 'pyopenssl', 'cryptography'], default='auto'),
         ),
         supports_check_mode=True,
@@ -656,7 +686,7 @@ def main():
             module.exit_json(**result)
 
         try:
-            private_key.remove()
+            private_key.remove(module)
         except PrivateKeyError as exc:
             module.fail_json(msg=to_native(exc))
 
diff --git a/lib/ansible/modules/crypto/openssl_publickey.py b/lib/ansible/modules/crypto/openssl_publickey.py
index be0f4bd50f450e..8d8292a5d60b14 100644
--- a/lib/ansible/modules/crypto/openssl_publickey.py
+++ b/lib/ansible/modules/crypto/openssl_publickey.py
@@ -205,7 +205,7 @@ def generate(self, module):
             except (IOError, OSError) as exc:
                 raise PublicKeyError(exc)
             except AttributeError as exc:
-                self.remove()
+                self.remove(module)
                 raise PublicKeyError('You need to have PyOpenSSL>=16.0.0 to generate public keys')
 
         self.fingerprint = crypto_utils.get_fingerprint(
@@ -315,7 +315,7 @@ def main():
             module.exit_json(**result)
 
         try:
-            public_key.remove()
+            public_key.remove(module)
         except PublicKeyError as exc:
             module.fail_json(msg=to_native(exc))
 
diff --git a/test/integration/targets/openssl_privatekey/tasks/impl.yml b/test/integration/targets/openssl_privatekey/tasks/impl.yml
index c948349c473759..d26e29eb4c9e0e 100644
--- a/test/integration/targets/openssl_privatekey/tasks/impl.yml
+++ b/test/integration/targets/openssl_privatekey/tasks/impl.yml
@@ -149,6 +149,7 @@
     passphrase: hunter2
     cipher: "{{ 'aes256' if select_crypto_backend == 'pyopenssl' else 'auto' }}"
     select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
   register: passphrase_1
 
 - name: Generate privatekey with passphrase (idempotent)
@@ -157,18 +158,21 @@
     passphrase: hunter2
     cipher: "{{ 'aes256' if select_crypto_backend == 'pyopenssl' else 'auto' }}"
     select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
   register: passphrase_2
 
 - name: Regenerate privatekey without passphrase
   openssl_privatekey:
     path: '{{ output_dir }}/privatekeypw.pem'
     select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
   register: passphrase_3
 
 - name: Regenerate privatekey without passphrase (idempotent)
   openssl_privatekey:
     path: '{{ output_dir }}/privatekeypw.pem'
     select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
   register: passphrase_4
 
 - name: Regenerate privatekey with passphrase
@@ -177,4 +181,25 @@
     passphrase: hunter2
     cipher: "{{ 'aes256' if select_crypto_backend == 'pyopenssl' else 'auto' }}"
     select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
   register: passphrase_5
+
+- name: Remove module
+  openssl_privatekey:
+    path: '{{ output_dir }}/privatekeypw.pem'
+    passphrase: hunter2
+    cipher: "{{ 'aes256' if select_crypto_backend == 'pyopenssl' else 'auto' }}"
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
+    state: absent
+  register: remove_1
+
+- name: Remove module (idempotent)
+  openssl_privatekey:
+    path: '{{ output_dir }}/privatekeypw.pem'
+    passphrase: hunter2
+    cipher: "{{ 'aes256' if select_crypto_backend == 'pyopenssl' else 'auto' }}"
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    backup: yes
+    state: absent
+  register: remove_2
diff --git a/test/integration/targets/openssl_privatekey/tests/validate.yml b/test/integration/targets/openssl_privatekey/tests/validate.yml
index da5e2ecb571738..c98fb35649d7c3 100644
--- a/test/integration/targets/openssl_privatekey/tests/validate.yml
+++ b/test/integration/targets/openssl_privatekey/tests/validate.yml
@@ -113,3 +113,16 @@
       - passphrase_3 is changed
       - passphrase_4 is not changed
       - passphrase_5 is changed
+      - passphrase_1.backup_file is undefined
+      - passphrase_2.backup_file is undefined
+      - passphrase_3.backup_file is not none
+      - passphrase_4.backup_file is undefined
+      - passphrase_5.backup_file is not none
+
+- name: Validate remove
+  assert:
+    that:
+      - remove_1 is changed
+      - remove_2 is not changed
+      - remove_1.backup_file is not none
+      - remove_2.backup_file is undefined


Code review comments:
diff_hunk: @@ -23,6 +23,11 @@
       L(ECC,https://en.wikipedia.org/wiki/Elliptic-curve_cryptography)
       private keys.
     - Keys are generated in PEM format.
+    - "Please note that the module regenerates private keys if they don't match
+      the module's options. In particular, if you provide another passphrase
+      (or specify none), change the keysize, etc., the private key will be
+      regenerated. If you are concerned of this overwriting your private key,
path: lib/ansible/modules/crypto/openssl_privatekey.py
text: @user1:
```suggestion
      regenerated. If you are concerned that this could overwrite your private key,
```

diff_hunk: @@ -111,6 +116,13 @@
         default: auto
         choices: [ auto, cryptography, pyopenssl ]
         version_added: "2.8"
+    backup:
+        description:
+            - Create a backup file including the timestamp information so you can get
path: lib/ansible/modules/crypto/openssl_privatekey.py
text: @user1:
```suggestion
            - Create a backup file including a timestamp so you can get
```