openssh_keypair and openssl_privatekey: make idempotence configurable

[felixfontein]

SUMMARY

We had several discussions in the past about the behavior modules like openssh_keypair and openssl_privatekey should have when they encounter something not meeting the specified module options. (References: #53535, #32038, #65638 resp. the meeting log.)

The current state (until #65638) was that modules should make sure that the object in question does conform to its options, i.e. if a private key is found which is invalid, (not) passphrase protected (or with the wrong one), has the wrong size, type, ..., it will be regenerated.

There are also other approaches:

• openssh_keypair - do fail instead of regenerate for password protected and invalid keys #65638 will now make the openssh_keypair module fail if it encounters keys it cannot read (passphrase protected or invalid);
• the docker_volume module will not recreate volumes (even if they don't fit the options) by default, but only if recreate is set to always (also known as force=yes) or options-changed (also known as classic Ansible behavior).

Let's use this issue for discussing on how this could be configured for these modules.

CC @MaxBab @samdoran @nitzmahone

ISSUE TYPE

• Feature Idea

COMPONENT NAME

openssh_keypair
openssl_privatekey

[felixfontein]

I would prefer if it is possible to configure the behavior of openssh_keypair and openssl_privatekey similarly to docker_volume. (Currently independent of how the default of the new option will look like - I always prefer backwards compatibility, but I can live with changes as long as I can set this option to whatever value I prefer.)

I can think of the following cases (which would correspond to values for that option):

• if a file of the correct name is there, don't do anything;
• if a readable key of the correct name exists, don't do anything; fail if the key cannot be read (i.e. invalid or wrong passphrase);
• if a readable key of the correct name exists, make sure it adheres to the module options (i.e. regenerate if required); fail if the key cannot be read (i.e. invalid or wrong passphrase);
• make sure the key adheres to the module options (i.e. regenerate if required), in particluar if the key is invalid or the wrong passphrase is specified;
• always regenerate (i.e. what force=yes currently does).

I think this covers the most important choices people would want. To make this proposal more concrete, how about naming the option recreate_behavior (or simply recreate) with options ignore-existing, never, config-changes-for-valid-keys, config-changes, always (corresponding to the items above). (I don't care about the names, these were the first to cross my mind.)

[ansibot]

cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @lolcube @puiterwijk @resmo
click here for bot help

[Shaps]

Why would we need all those behaviors? I can only see 2 behaviors:

• Fail if the key is invalid ( unreadable/wrong passphrase etc );
• Regenerate if it does not adhere to the module options

[MarkusTeufelberger]

A trivial 3rd behavior would be to just return "ok" if everything is ok. ;-)

[ffdybuster]

So we have:

1. don't modify key if it exists (just fail if it is broken / cannot be checked),
2. fail if key exists but doesn't satisfy conditions (no matter which condition),
3. fail if key is invalid / password protected, fail; otherwise regenerate if neccessary,
4. always regenerate if neccessary.

So current default is 4), and either 2) or 3) would be a potential new default. 1) is for people who want to have a key, but don't want errors / regenerations if they ever adjust the task/playbook parameters (except for wrong password / broken key).

What do you think?

[nodiscc]

1. would solve the problem of this simple task not being idempotent

- name: generate openssl private key
  openssl_privatekey:
    path: "/opt/netdata/etc/netdata/ssl/key.pem"
    owner: root
    group: netdata
    mode: "0640"

(this tasks returns changed every time and effectively renews the key which is unexpected - in addition ansible-playbook --check does not warn of these changes, should I open another issue for this?)

TASK [generate openssl private key] 
ok: [my.host] => {
    "changed": false,
    "filename": "/opt/netdata/etc/netdata/ssl/key.pem",
    "fingerprint": {},
    "size": 4096,
    "type": "RSA"
}

In practice it would be good to see something like a regenerate parameter for this module with these possible values:

• never: never regenerate an existing key file
• fail: fail if key file exists and is different from module parameters
• fail_on_password: fail if the key is password protected
• always: always regenerate an existing key file if different from module parameters

[felixfontein]

1. would solve the problem of this simple task not being idempotent

- name: generate openssl private key
  openssl_privatekey:
    path: "/opt/netdata/etc/netdata/ssl/key.pem"
    owner: root
    group: netdata
    mode: "0640"

(this tasks returns changed every time and effectively renews the key which is unexpected - in addition ansible-playbook --check does not warn of these changes, should I open another issue for this?)

This is definitely not expected - calling the module twice with the same parameters should not result in a change (assuming you're not specifying force: yes or something similar). Could you create an issue for this, with more information how this can be reproduced (and which Ansible version you are using)?

In practice it would be good to see something like a regenerate parameter for this module with these possible values:

* `never`: never regenerate an existing key file

* `fail`: fail if key file exists and is different from module parameters

* `fail_on_password`: fail if the key is password protected

* `always`: always regenerate an existing key file if different from module parameters

For regenerate: always I would expect the same as force: yes, i.e. also regenerate if module parameters match. How about on_change or changed as the value instead of always?

[felixfontein]

Ok, here's an updated (and expanded) proposal:

regenerate:
  description:
    - Allows to configure in which situations the module is allowed to regenerate private keys.
      The module will always generate a new key if the destination file does not exist.
    - By default, the key will be regenerated when it doesn't match the module's options,
      except when the key cannot be read or the passphrase does not match. Please note that
      this B(changed) for Ansible 2.10. For Ansible 2.9, the behavior was as if C(full_idempotence)
      is specified.
    - If set to C(never), the module will fail if the key cannot be read or the passphrase
      isn't matching, and will never regenerate an existing key.
    - If set to C(fail), the module will fail if the key does not correspond to the module's
      options.
    - If set to C(partial_idempotence), the key will be regenerated if it does not conform to
      the module's options. The key is B(not) regenerated if it cannot be read (broken file),
      the key is protected by an unknown passphrase, or when they key is not protected by a
      passphrase, but a passphrase is specified.
    - If set to C(full_idempotence), the key will be regenerated if it does not conform to the
      module's options. This is also the case if the key cannot be read (broken file), the key
      is protected by an unknown passphrase, or when they key is not protected by a passphrase,
      but a passphrase is specified. Make sure you have a B(backup) when using this option!
    - If set to C(always), the module will always regenerate the key. This is equivalent to
      setting I(force) to C(yes).
  type: str
  choices:
    - never
    - fail
    - partial_idempotence
    - full_idempotence
    - always
  default: partial_idempotence

Any naming suggestions, or other suggestions?

[felixfontein]

I've started a PR implementing such an option: #67038

[nodiscc]

This is definitely not expected - calling the module twice with the same parameters should not result in a change

Sorry, false alarm, another task in the playbook was changing the key file.