openssh_cert - Adding regenerate option (#256)

* Initial commit

* Fixing unit tests

* More unit fixes

* Adding changelog fragment

* Minor refactor in Certificate.generate()

* Addressing option case-sensitivity and directive overrides

* Renaming idempotency to regenerate

* updating changelog

* Minor refactoring of default options

* Cleaning up with inline functions

* Fixing false failures when regenerate=fail and improving clarity

* Applying second round of review suggestions

* adding helper for safe atomic moves

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

@@ -22,6 +22,12 @@
     - name: Import key_idempotency tests
       import_tasks: ../tests/key_idempotency.yml
 
+    - name: Import options tests
+      import_tasks: ../tests/options_idempotency.yml
+
+    - name: Import regenerate tests
+      import_tasks: ../tests/regenerate.yml
+
     - name: Import remove tests
       import_tasks: ../tests/remove.yml
   when: not (ansible_facts['distribution'] == "CentOS" and ansible_facts['distribution_major_version'] == "6")

tests/integration/targets/openssh_cert/tests/idempotency.yml

@@ -177,12 +177,17 @@
         valid_from: "2001-01-21"
         valid_to: "2019-01-21"
         changed: false
-      # Options are currently not checked for idempotency purposes
       - test_name: Generate an OpenSSH user Certificate with no options (idempotent)
         type: user
         valid_from: "2001-01-21"
         valid_to: "2019-01-21"
         changed: false
+      - test_name: Generate an OpenSSH user Certificate with no options - full idempotency (idempotent)
+        type: user
+        valid_from: "2001-01-21"
+        valid_to: "2019-01-21"
+        regenerate: full_idempotence
+        changed: true
       - test_name: Generate cert without serial
         type: user
         valid_from: always
@@ -228,13 +233,19 @@
         valid_from: always
         valid_to: forever
         changed: false
-      # Identifiers are not included in idempotency checks so a new cert will not be generated
       - test_name: Generate cert with identifier
         type: user
         identifier: foo
         valid_from: always
         valid_to: forever
         changed: false
+      - test_name: Generate cert with identifier - full idempotency
+        type: user
+        identifier: foo
+        valid_from: always
+        valid_to: forever
+        regenerate: full_idempotence
+        changed: true
 
 - name: Execute idempotency tests
   openssh_cert:
@@ -251,6 +262,7 @@
     valid_at: "{{ test_case.valid_at | default(omit) }}"
     valid_from: "{{ test_case.valid_from | default(omit) }}"
     valid_to: "{{ test_case.valid_to | default(omit) }}"
+    regenerate: "{{ test_case.regenerate | default(omit) }}"
   check_mode: "{{ test_case.check_mode | default(omit) }}"
   register: idempotency_test_output
   loop: "{{ test_cases }}"

tests/integration/targets/openssh_cert/tests/key_idempotency.yml

@@ -40,12 +40,35 @@
     valid_to: forever
   register: new_public_key_output
 
-# Signing key and public key are not considered during idempotency checks
-- name: Assert changes to public key or signing key results in no change
+- name: Generate cert with new signing key - full idempotency
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ new_signing_key }}"
+    valid_from: always
+    valid_to: forever
+    regenerate: full_idempotence
+  register: new_signing_key_full_idempotency_output
+
+- name: Generate cert with new pubic key - full idempotency
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ new_public_key }}"
+    signing_key: "{{ new_signing_key }}"
+    valid_from: always
+    valid_to: forever
+    regenerate: full_idempotence
+  register: new_public_key_full_idempotency_output
+
+- name: Assert changes to public key or signing key results in no change unless idempotency=full
   assert:
     that:
       - new_signing_key_output is not changed
       - new_public_key_output is not changed
+      - new_signing_key_full_idempotency_output is changed
+      - new_public_key_full_idempotency_output is changed
 
 - name: Remove certificate
   openssh_cert:

tests/integration/targets/openssh_cert/tests/options_idempotency.yml

@@ -0,0 +1,102 @@
+####################################################################
+# WARNING: These are designed specifically for Ansible tests       #
+# and should not be used as examples of how to write Ansible roles #
+####################################################################
+
+- name: Generate cert with no options
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    options:
+      - clear
+    regenerate: full_idempotence
+  register: no_options
+
+- name: Generate cert with no options with explicit directives
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    options:
+      - no-user-rc
+      - no-x11-forwarding
+      - no-agent-forwarding
+      - no-port-forwarding
+      - no-pty
+    regenerate: full_idempotence
+  register: no_options_explicit_directives
+
+- name: Generate cert with explicit extension
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    options:
+      - clear
+      - permit-pty
+    regenerate: full_idempotence
+  register: explicit_extension_before
+
+- name: Generate cert with explicit extension (idempotency)
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    options:
+      - clear
+      - permit-pty
+    regenerate: full_idempotence
+  register: explicit_extension_after
+
+- name: Generate cert with explicit extension and corresponding directive
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    options:
+      - no-pty
+      - permit-pty
+    regenerate: full_idempotence
+  register: explicit_extension_and_directive
+
+- name: Generate cert with default options
+  openssh_cert:
+    type: user
+    path: "{{ certificate_path }}"
+    public_key: "{{ public_key }}"
+    signing_key: "{{ signing_key }}"
+    valid_from: always
+    valid_to: forever
+    regenerate: full_idempotence
+  register: default_options
+
+- name: Assert options results
+  assert:
+    that:
+      - no_options is changed
+      - no_options_explicit_directives is not changed
+      - explicit_extension_before is changed
+      - explicit_extension_after is not changed
+      - explicit_extension_and_directive is changed
+      - default_options is not changed
+
+- name: Remove certificate
+  openssh_cert:
+    path: "{{ certificate_path }}"
+    state: absent

tests/integration/targets/openssh_cert/tests/regenerate.yml

@@ -0,0 +1,135 @@
+####################################################################
+# WARNING: These are designed specifically for Ansible tests       #
+# and should not be used as examples of how to write Ansible roles #
+####################################################################
+
+- set_fact:
+    test_cases:
+      - test_name: Generate certificate
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: never
+        changed: true
+      - test_name: Regenerate never - same options
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: never
+        changed: false
+      - test_name: Regenerate never - different options
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        options:
+          - clear
+        regenerate: never
+        changed: false
+      - test_name: Regenerate never with force
+        force: true
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: never
+        changed: true
+      - test_name: Remove certificate
+        path: "{{ certificate_path }}"
+        state: absent
+        changed: true
+      - test_name: Regenerate fail - new certificate
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: fail
+        changed: true
+      - test_name: Regenerate fail - same options
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: fail
+        changed: false
+      - test_name: Regenerate fail - different options
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        options:
+          - clear
+        regenerate: fail
+        changed: false
+        ignore_errors: true
+      - test_name: Regenerate fail with force
+        force: true
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: fail
+        changed: true
+      - test_name: Regenerate always
+        type: user
+        signing_key: "{{ signing_key }}"
+        public_key: "{{ public_key }}"
+        path: "{{ certificate_path }}"
+        valid_from: always
+        valid_to: forever
+        regenerate: always
+        changed: true
+
+- name: Execute regenerate tests
+  openssh_cert:
+    force: "{{ test_case.force | default(omit) }}"
+    options: "{{ test_case.options | default(omit) }}"
+    path: "{{ test_case.path | default(omit) }}"
+    public_key: "{{ test_case.public_key | default(omit) }}"
+    principals: "{{ test_case.principals | default(omit) }}"
+    regenerate: "{{ test_case.regenerate | default(omit) }}"
+    serial_number: "{{ test_case.serial_number | default(omit) }}"
+    signing_key: "{{ test_case.signing_key | default(omit) }}"
+    state: "{{ test_case.state | default(omit) }}"
+    type: "{{ test_case.type | default(omit) }}"
+    valid_at: "{{ test_case.valid_at | default(omit) }}"
+    valid_from: "{{ test_case.valid_from | default(omit) }}"
+    valid_to: "{{ test_case.valid_to | default(omit) }}"
+  check_mode: "{{ test_case.check_mode | default(omit) }}"
+  ignore_errors: "{{ test_case.ignore_errors | default(omit) }}"
+  register: regenerate_tests_output
+  loop: "{{ test_cases }}"
+  loop_control:
+    loop_var: test_case
+
+- name: Assert task statuses
+  assert:
+    that:
+      - result.changed == test_cases[index].changed
+  loop: "{{ regenerate_tests_output.results }}"
+  loop_control:
+    index_var: index
+    loop_var: result
+
+- name: Remove certificate
+  openssh_cert:
+    path: "{{ certificate_path }}"
+    state: absent

tests/unit/plugins/module_utils/openssh/test_certificate.py

@@ -9,7 +9,9 @@ import pytest
 
 from ansible_collections.community.crypto.plugins.module_utils.openssh.certificate import (
     OpensshCertificate,
-    OpensshCertificateTimeParameters
+    OpensshCertificateOption,
+    OpensshCertificateTimeParameters,
+    parse_option_list
 )
 
 # Type: ssh-rsa-cert-v01@openssh.com user certificate
@@ -42,7 +44,7 @@ RSA_CERT_SIGNED_BY_DSA = (
     b'Q7c8c/tNDaL7uqV46QQAAADcAAAAHc3NoLWRzcwAAAChaQ94wqca+KhkHtbkLpjvGsfu0Gy03SAb0+o11Shk/BXnK7N/cwEVD ' +
     b'ansible@ansible-host'
 )
-RSA_FINGERPRINT = b'SHA256:SvUwwUer4AwsdePYseJR3LcZS8lnKi6BqiL51Dop030'
+RSA_FINGERPRINT = 'SHA256:SvUwwUer4AwsdePYseJR3LcZS8lnKi6BqiL51Dop030'
 # Type: ssh-dss-cert-v01@openssh.com user certificate
 # Public key: DSA-CERT SHA256:YCdJ2lYU+FSkWUud7zg1SJszprXoRGNU/GVcqXUjgC8
 # Signing CA: ECDSA SHA256:w9lp4zGRJShhm4DzO3ulVm0BEcR0PMjrM6VanQo4C0w
@@ -64,7 +66,7 @@ DSA_CERT_SIGNED_BY_ECDSA_NO_OPTS = (
     b'wvanQKM01uU73swNIt+ZFra9kRSi21xjzgMPn7U0AAABkAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAABJAAAAIGmlKa/riG7+EpoW6dTJY6' +
     b'0N8BrEcniKgOxdRM1EPJ2DAAAAIQDnK4stvbvS+Bn0/42Was7uOfJtnLYXs5EuB2L3uejPcQ== ansible@ansible-host'
 )
-DSA_FINGERPRINT = b'SHA256:YCdJ2lYU+FSkWUud7zg1SJszprXoRGNU/GVcqXUjgC8'
+DSA_FINGERPRINT = 'SHA256:YCdJ2lYU+FSkWUud7zg1SJszprXoRGNU/GVcqXUjgC8'
 # Type: ecdsa-sha2-nistp256-cert-v01@openssh.com user certificate
 # Public key: ECDSA-CERT SHA256:w9lp4zGRJShhm4DzO3ulVm0BEcR0PMjrM6VanQo4C0w
 # Signing CA: ED25519 SHA256:NP4JdfkCopbjwMepq0aPrpMz13cNmEd+uDOxC/j9N40
@@ -90,7 +92,7 @@ ECDSA_CERT_SIGNED_BY_ED25519_VALID_OPTS = (
     b'TUxOQAAAEAdp3eOLRN5t2wW29TBWbz604uuXg88jH4RA4HDhbRupa/x2rN3j6iZQ4VXPLA4JtdfIslHFkH6HUlxU8XsoJwP ' +
     b'ansible@ansible-host'
 )
-ECDSA_FINGERPRINT = b'SHA256:w9lp4zGRJShhm4DzO3ulVm0BEcR0PMjrM6VanQo4C0w'
+ECDSA_FINGERPRINT = 'SHA256:w9lp4zGRJShhm4DzO3ulVm0BEcR0PMjrM6VanQo4C0w'
 # Type: ssh-ed25519-cert-v01@openssh.com user certificate
 # Public key: ED25519-CERT SHA256:NP4JdfkCopbjwMepq0aPrpMz13cNmEd+uDOxC/j9N40
 # Signing CA: RSA SHA256:SvUwwUer4AwsdePYseJR3LcZS8lnKi6BqiL51Dop030
@@ -114,20 +116,20 @@ ED25519_CERT_SIGNED_BY_RSA_INVALID_OPTS = (
     b'7WJpz3eypBJt4TglwRTJpp54IMN2CyDQm0N97x9ris8jQQHlCF2EgZp1u4aOiZJTSJ5d4hapO0uZwXOI9AIWy/lmx0/6jX07MWrs4iXpfiF' +
     b'5T4s6kEn7YW4SaJ0Z7xGp3V0vDOxh+jwHZGD5GM449Il6QxQwDY5BSJq+iMR467yaIjw2g8Kt4ZiU= ansible@ansible-host'
 )
-ED25519_FINGERPRINT = b'SHA256:NP4JdfkCopbjwMepq0aPrpMz13cNmEd+uDOxC/j9N40'
+ED25519_FINGERPRINT = 'SHA256:NP4JdfkCopbjwMepq0aPrpMz13cNmEd+uDOxC/j9N40'
 # garbage
 INVALID_DATA = b'yDspTN+BJzvIK2Q+CRD3qBDVSi+YqSxwyz432VEaHKlXbuLURirY0QpuBCqgR6tCtWW5vEGkXKZ3'
 
-VALID_OPTS = [(b'force-command', b'/usr/bin/csh')]
-INVALID_OPTS = [(b'test', b'undefined')]
+VALID_OPTS = [OpensshCertificateOption('critical', 'force-command', '/usr/bin/csh')]
+INVALID_OPTS = [OpensshCertificateOption('critical', 'test', 'undefined')]
 VALID_EXTENSIONS = [
-    (b'permit-X11-forwarding', b''),
-    (b'permit-agent-forwarding', b''),
-    (b'permit-port-forwarding', b''),
-    (b'permit-pty', b''),
-    (b'permit-user-rc', b''),
+    OpensshCertificateOption('extension', 'permit-x11-forwarding', ''),
+    OpensshCertificateOption('extension', 'permit-agent-forwarding', ''),
+    OpensshCertificateOption('extension', 'permit-port-forwarding', ''),
+    OpensshCertificateOption('extension', 'permit-pty', ''),
+    OpensshCertificateOption('extension', 'permit-user-rc', ''),
 ]
-INVALID_EXTENSIONS = [(b'test', b'')]
+INVALID_EXTENSIONS = [OpensshCertificateOption('extension', 'test', '')]
 
 VALID_TIME_PARAMETERS = [
     (0, "always", "always", 0,
@@ -177,15 +179,31 @@ INVALID_VALIDITY_TEST = [
     ("2000-01-01 00:00:00", "2000-01-01 00:00:01", "2000-01-01 00:00:02"),
 ]
 
+VALID_OPTIONS = [
+    ("force-command=/usr/bin/csh", OpensshCertificateOption('critical', 'force-command', '/usr/bin/csh')),
+    ("Force-Command=/Usr/Bin/Csh", OpensshCertificateOption('critical', 'force-command', '/Usr/Bin/Csh')),
+    ("permit-x11-forwarding", OpensshCertificateOption('extension', 'permit-x11-forwarding', '')),
+    ("permit-X11-forwarding", OpensshCertificateOption('extension', 'permit-x11-forwarding', '')),
+    ("critical:foo=bar", OpensshCertificateOption('critical', 'foo', 'bar')),
+    ("extension:foo", OpensshCertificateOption('extension', 'foo', '')),
+]
+
+INVALID_OPTIONS = [
+    "foobar",
+    "foo=bar",
+    'foo:bar=baz',
+    [],
+]
+
 
 def test_rsa_certificate(tmpdir):
     cert_file = tmpdir / 'id_rsa-cert.pub'
     cert_file.write(RSA_CERT_SIGNED_BY_DSA, mode='wb')
 
     cert = OpensshCertificate.load(str(cert_file))
-    assert cert.key_id == b'test'
+    assert cert.key_id == 'test'
     assert cert.serial == 0
-    assert cert.type_string == b'ssh-rsa-cert-v01@openssh.com'
+    assert cert.type_string == 'ssh-rsa-cert-v01@openssh.com'
     assert cert.public_key == RSA_FINGERPRINT
     assert cert.signing_key == DSA_FINGERPRINT
 
@@ -196,7 +214,7 @@ def test_dsa_certificate(tmpdir):
 
     cert = OpensshCertificate.load(str(cert_file))
 
-    assert cert.type_string == b'ssh-dss-cert-v01@openssh.com'
+    assert cert.type_string == 'ssh-dss-cert-v01@openssh.com'
     assert cert.public_key == DSA_FINGERPRINT
     assert cert.signing_key == ECDSA_FINGERPRINT
     assert cert.critical_options == []
@@ -208,7 +226,7 @@ def test_ecdsa_certificate(tmpdir):
     cert_file.write(ECDSA_CERT_SIGNED_BY_ED25519_VALID_OPTS)
 
     cert = OpensshCertificate.load(str(cert_file))
-    assert cert.type_string == b'ecdsa-sha2-nistp256-cert-v01@openssh.com'
+    assert cert.type_string == 'ecdsa-sha2-nistp256-cert-v01@openssh.com'
     assert cert.public_key == ECDSA_FINGERPRINT
     assert cert.signing_key == ED25519_FINGERPRINT
     assert cert.critical_options == VALID_OPTS
@@ -220,7 +238,7 @@ def test_ed25519_certificate(tmpdir):
     cert_file.write(ED25519_CERT_SIGNED_BY_RSA_INVALID_OPTS)
 
     cert = OpensshCertificate.load(str(cert_file))
-    assert cert.type_string == b'ssh-ed25519-cert-v01@openssh.com'
+    assert cert.type_string == 'ssh-ed25519-cert-v01@openssh.com'
     assert cert.public_key == ED25519_FINGERPRINT
     assert cert.signing_key == RSA_FINGERPRINT
     assert cert.critical_options == INVALID_OPTS
@@ -275,3 +293,56 @@ def test_valid_validity_test(valid_from, valid_to, valid_at):
 @pytest.mark.parametrize("valid_from,valid_to,valid_at", INVALID_VALIDITY_TEST)
 def test_invalid_validity_test(valid_from, valid_to, valid_at):
     assert not OpensshCertificateTimeParameters(valid_from, valid_to).within_range(valid_at)
+
+
+@pytest.mark.parametrize("option_string,option_object", VALID_OPTIONS)
+def test_valid_options(option_string, option_object):
+    assert OpensshCertificateOption.from_string(option_string) == option_object
+
+
+@pytest.mark.parametrize("option_string", INVALID_OPTIONS)
+def test_invalid_options(option_string):
+    with pytest.raises(ValueError):
+        OpensshCertificateOption.from_string(option_string)
+
+
+def test_parse_option_list():
+    critical_options, extensions = parse_option_list(['force-command=/usr/bin/csh'])
+
+    critical_option_objects = [
+        OpensshCertificateOption.from_string('force-command=/usr/bin/csh'),
+    ]
+
+    extension_objects = [
+        OpensshCertificateOption.from_string('permit-x11-forwarding'),
+        OpensshCertificateOption.from_string('permit-agent-forwarding'),
+        OpensshCertificateOption.from_string('permit-port-forwarding'),
+        OpensshCertificateOption.from_string('permit-user-rc'),
+        OpensshCertificateOption.from_string('permit-pty'),
+    ]
+
+    assert set(critical_options) == set(critical_option_objects)
+    assert set(extensions) == set(extension_objects)
+
+
+def test_parse_option_list_with_directives():
+    critical_options, extensions = parse_option_list(['clear', 'no-pty', 'permit-pty', 'permit-user-rc'])
+
+    extension_objects = [
+        OpensshCertificateOption.from_string('permit-user-rc'),
+        OpensshCertificateOption.from_string('permit-pty'),
+    ]
+
+    assert set(critical_options) == set()
+    assert set(extensions) == set(extension_objects)
+
+
+def test_parse_option_list_case_sensitivity():
+    critical_options, extensions = parse_option_list(['CLEAR', 'no-X11-forwarding', 'permit-X11-forwarding'])
+
+    extension_objects = [
+        OpensshCertificateOption.from_string('permit-x11-forwarding'),
+    ]
+
+    assert set(critical_options) == set()
+    assert set(extensions) == set(extension_objects)