From b0c7efcc6b8dc26847f00c3fdbc95c5b4e8f3366 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 2 Dec 2018 18:40:14 +0100 Subject: [PATCH] ACME: add diff to acme_account, account_public_key to acme_account_facts, and general refactoring (#49410) * Only one exit point. * Refactoring account handling. * Add diff support for acme_account. * Insert public_account_key into acme_account_facts result and into acme_account diff. * Add changelog. --- changelogs/fragments/49410-acme-diff.yml | 3 + lib/ansible/module_utils/acme.py | 113 ++++++++++-------- .../modules/crypto/acme/acme_account.py | 92 ++++++++------ .../modules/crypto/acme/acme_account_facts.py | 32 +++-- .../modules/crypto/acme/acme_certificate.py | 11 +- .../crypto/acme/acme_certificate_revoke.py | 12 +- .../targets/acme_account/tasks/impl.yml | 86 +++++++++++++ .../targets/acme_account/tests/validate.yml | 58 +++++++++ .../acme_account_facts/tests/validate.yml | 2 + 9 files changed, 305 insertions(+), 104 deletions(-) create mode 100644 changelogs/fragments/49410-acme-diff.yml diff --git a/changelogs/fragments/49410-acme-diff.yml b/changelogs/fragments/49410-acme-diff.yml new file mode 100644 index 0000000000..ee2d514859 --- /dev/null +++ b/changelogs/fragments/49410-acme-diff.yml @@ -0,0 +1,3 @@ +minor_changes: +- "acme_account: add support for diff mode." +- "acme_account_facts: also return ``public_account_key`` in JWK format." diff --git a/lib/ansible/module_utils/acme.py b/lib/ansible/module_utils/acme.py index cc7054b86d..7a60a681d6 100644 --- a/lib/ansible/module_utils/acme.py +++ b/lib/ansible/module_utils/acme.py @@ -646,12 +646,14 @@ class ACMEAccount(object): def _new_reg(self, contact=None, agreement=None, terms_agreed=False, allow_creation=True): ''' - Registers a new ACME account. Returns True if the account was - created and False if it already existed (e.g. it was not newly - created). + Registers a new ACME account. Returns a pair ``(created, data)``. + Here, ``created`` is ``True`` if the account was created and + ``False`` if it already existed (e.g. it was not newly created), + or does not exist. In case the account was created or exists, + ``data`` contains the account data; otherwise, it is ``None``. https://tools.ietf.org/html/draft-ietf-acme-acme-14#section-7.3 ''' - contact = [] if contact is None else contact + contact = contact or [] if self.version == 1: new_reg = { @@ -668,6 +670,7 @@ class ACMEAccount(object): 'contact': contact } if not allow_creation: + # https://tools.ietf.org/html/draft-ietf-acme-acme-14#section-7.3.1 new_reg['onlyReturnExisting'] = True if terms_agreed: new_reg['termsOfServiceAgreed'] = True @@ -679,7 +682,7 @@ class ACMEAccount(object): # Account did not exist if 'location' in info: self.set_account_uri(info['location']) - return True + return True, result elif info['status'] == (409 if self.version == 1 else 200): # Account did exist if result.get('status') == 'deactivated': @@ -689,22 +692,22 @@ class ACMEAccount(object): # "Once an account is deactivated, the server MUST NOT accept further # requests authorized by that account's key." if not allow_creation: - return False + return False, None else: raise ModuleFailException("Account is deactivated") if 'location' in info: self.set_account_uri(info['location']) - return False + return False, result elif info['status'] == 400 and result['type'] == 'urn:ietf:params:acme:error:accountDoesNotExist' and not allow_creation: # Account does not exist (and we didn't try to create it) - return False + return False, None else: raise ModuleFailException("Error registering: {0} {1}".format(info['status'], result)) def get_account_data(self): ''' Retrieve account information. Can only be called when the account - URI is already known (such as after calling init_account). + URI is already known (such as after calling setup_account). Return None if the account was deactivated, or a dict otherwise. ''' if self.uri is None: @@ -732,66 +735,82 @@ class ACMEAccount(object): raise ModuleFailException("Error getting account data from {2}: {0} {1}".format(info['status'], result, self.uri)) return result - def init_account(self, contact, agreement=None, terms_agreed=False, allow_creation=True, update_contact=True, remove_account_uri_if_not_exists=False): + def setup_account(self, contact=None, agreement=None, terms_agreed=False, allow_creation=True, remove_account_uri_if_not_exists=False): ''' - Create or update an account on the ACME server. For ACME v1, + Detect or create an account on the ACME server. For ACME v1, as the only way (without knowing an account URI) to test if an account exists is to try and create one with the provided account key, this method will always result in an account being present (except on error situations). For ACME v2, a new account will - only be created if allow_creation is set to True. + only be created if ``allow_creation`` is set to True. - For ACME v2, check_mode is fully respected. For ACME v1, the account - might be created if it does not yet exist. + For ACME v2, ``check_mode`` is fully respected. For ACME v1, the + account might be created if it does not yet exist. - If the account already exists and if update_contact is set to - True, this method will update the contact information. + Return a pair ``(created, account_data)``. Here, ``created`` will + be ``True`` in case the account was created or would be created + (check mode). ``account_data`` will be the current account data, + or ``None`` if the account does not exist. - Return True in case something changed (account was created, contact - info updated) or would be changed (check_mode). The account URI - will be stored in self.uri; if it is None, the account does not - exist. + The account URI will be stored in ``self.uri``; if it is ``None``, + the account does not exist. https://tools.ietf.org/html/draft-ietf-acme-acme-14#section-7.3 ''' - new_account = True - changed = False if self.uri is not None: - new_account = False - if not update_contact: - # Verify that the account key belongs to the URI. - # (If update_contact is True, this will be done below.) - if self.get_account_data() is None: - if remove_account_uri_if_not_exists and not allow_creation: - self.uri = None - return False + created = False + # Verify that the account key belongs to the URI. + # (If update_contact is True, this will be done below.) + account_data = self.get_account_data() + if account_data is None: + if remove_account_uri_if_not_exists and not allow_creation: + self.uri = None + else: raise ModuleFailException("Account is deactivated or does not exist!") else: - new_account = self._new_reg( + created, account_data = self._new_reg( contact, agreement=agreement, terms_agreed=terms_agreed, allow_creation=allow_creation and not self.module.check_mode ) if self.module.check_mode and self.uri is None and allow_creation: - return True - if not new_account and self.uri and update_contact: - result = self.get_account_data() - if result is None: - if not allow_creation: - self.uri = None - return False - raise ModuleFailException("Account is deactivated or does not exist!") + created = True + account_data = { + 'contact': contact or [] + } + return created, account_data - # ...and check if update is necessary - if result.get('contact', []) != contact: - if not self.module.check_mode: - upd_reg = result - upd_reg['contact'] = contact - result, dummy = self.send_signed_request(self.uri, upd_reg) - changed = True - return new_account or changed + def update_account(self, account_data, contact=None): + ''' + Update an account on the ACME server. Check mode is fully respected. + + The current account data must be provided as ``account_data``. + + Return a pair ``(updated, account_data)``, where ``updated`` is + ``True`` in case something changed (contact info updated) or + would be changed (check mode), and ``account_data`` the updated + account data. + + https://tools.ietf.org/html/draft-ietf-acme-acme-14#section-7.3.2 + ''' + # Create request + update_request = {} + if contact is not None and account_data.get('contact', []) != contact: + update_request['contact'] = list(contact) + + # No change? + if not update_request: + return False, dict(account_data) + + # Apply change + if self.module.check_mode: + account_data = dict(account_data) + account_data.update(update_request) + else: + account_data, dummy = self.send_signed_request(self.uri, update_request) + return True, account_data def cryptography_get_csr_domains(module, csr_filename): diff --git a/lib/ansible/modules/crypto/acme/acme_account.py b/lib/ansible/modules/crypto/acme/acme_account.py index 3d149e98d1..b49eec02e2 100644 --- a/lib/ansible/modules/crypto/acme/acme_account.py +++ b/lib/ansible/modules/crypto/acme/acme_account.py @@ -166,43 +166,51 @@ def main(): try: account = ACMEAccount(module) + changed = False state = module.params.get('state') + diff_before = {} + diff_after = {} if state == 'absent': - changed = account.init_account( - [], - allow_creation=False, - update_contact=False, - ) - if changed: - raise AssertionError('Unwanted account change') - if account.uri is not None: - # Account does exist - account_data = account.get_account_data() - if account_data is not None: - # Account is not yet deactivated - if not module.check_mode: - # Deactivate it - payload = { - 'status': 'deactivated' - } - result, info = account.send_signed_request(account.uri, payload) - if info['status'] != 200: - raise ModuleFailException('Error deactivating account: {0} {1}'.format(info['status'], result)) - module.exit_json(changed=True, account_uri=account.uri) - module.exit_json(changed=False, account_uri=account.uri) + created, account_data = account.setup_account(allow_creation=False) + if account_data: + diff_before = dict(account_data) + diff_before['public_account_key'] = account.key_data['jwk'] + if created: + raise AssertionError('Unwanted account creation') + if account_data is not None: + # Account is not yet deactivated + if not module.check_mode: + # Deactivate it + payload = { + 'status': 'deactivated' + } + result, info = account.send_signed_request(account.uri, payload) + if info['status'] != 200: + raise ModuleFailException('Error deactivating account: {0} {1}'.format(info['status'], result)) + changed = True elif state == 'present': allow_creation = module.params.get('allow_creation') # Make sure contact is a list of strings (unfortunately, Ansible doesn't do that for us) contact = [str(v) for v in module.params.get('contact')] terms_agreed = module.params.get('terms_agreed') - changed = account.init_account( + created, account_data = account.setup_account( contact, terms_agreed=terms_agreed, allow_creation=allow_creation, ) - if account.uri is None: + if account_data is None: raise ModuleFailException(msg='Account does not exist or is deactivated.') - module.exit_json(changed=changed, account_uri=account.uri) + if created: + diff_before = {} + else: + diff_before = dict(account_data) + diff_before['public_account_key'] = account.key_data['jwk'] + updated = False + if not created: + updated, account_data = account.update_account(account_data, contact) + changed = created or updated + diff_after = dict(account_data) + diff_after['public_account_key'] = account.key_data['jwk'] elif state == 'changed_key': # Parse new account key error, new_key_data = account.parse_key( @@ -212,15 +220,13 @@ def main(): if error: raise ModuleFailException("error while parsing account key: %s" % error) # Verify that the account exists and has not been deactivated - changed = account.init_account( - [], - allow_creation=False, - update_contact=False, - ) - if changed: - raise AssertionError('Unwanted account change') - if account.uri is None or account.get_account_data() is None: + created, account_data = account.setup_account(allow_creation=False) + if created: + raise AssertionError('Unwanted account creation') + if account_data is None: raise ModuleFailException(msg='Account does not exist or is deactivated.') + diff_before = dict(account_data) + diff_before['public_account_key'] = account.key_data['jwk'] # Now we can start the account key rollover if not module.check_mode: # Compose inner signed message @@ -241,7 +247,25 @@ def main(): result, info = account.send_signed_request(url, data) if info['status'] != 200: raise ModuleFailException('Error account key rollover: {0} {1}'.format(info['status'], result)) - module.exit_json(changed=True, account_uri=account.uri) + if module._diff: + account.key_data = new_key_data + account.jws_header['alg'] = new_key_data['alg'] + diff_after = account.get_account_data() + elif module._diff: + # Kind of fake diff_after + diff_after = dict(diff_before) + diff_after['public_account_key'] = new_key_data['jwk'] + changed = True + result = { + 'changed': changed, + 'account_uri': account.uri, + } + if module._diff: + result['diff'] = { + 'before': diff_before, + 'after': diff_after, + } + module.exit_json(**result) except ModuleFailException as e: e.do_fail(module) diff --git a/lib/ansible/modules/crypto/acme/acme_account_facts.py b/lib/ansible/modules/crypto/acme/acme_account_facts.py index 12c77fb0b7..0e68483a8d 100644 --- a/lib/ansible/modules/crypto/acme/acme_account_facts.py +++ b/lib/ansible/modules/crypto/acme/acme_account_facts.py @@ -89,6 +89,11 @@ account: returned: always type: str sample: https://example.ca/account/1/orders + public_account_key: + description: the public account key as a L(JSON Web Key,https://tools.ietf.org/html/rfc7517). + returned: always + type: str + sample: https://example.ca/account/1/orders ''' from ansible.module_utils.acme import ( @@ -129,24 +134,25 @@ def main(): try: account = ACMEAccount(module) # Check whether account exists - changed = account.init_account( + created, account_data = account.setup_account( [], allow_creation=False, - update_contact=False, remove_account_uri_if_not_exists=True, ) - if changed: - raise AssertionError('Unwanted account change') - if account.uri is None: - # Account does exist - module.exit_json(changed=False, exists=False, account_uri=None) - else: - # Account exists: retrieve account information - data = account.get_account_data() + if created: + raise AssertionError('Unwanted account creation') + result = { + 'changed': False, + 'exists': account.uri is not None, + 'account_uri': account.uri, + } + if account.uri is not None: # Make sure promised data is there - if 'contact' not in data: - data['contact'] = [] - module.exit_json(changed=False, exists=True, account_uri=account.uri, account=data) + if 'contact' not in account_data: + account_data['contact'] = [] + account_data['public_account_key'] = account.key_data['jwk'] + result['account'] = account_data + module.exit_json(**result) except ModuleFailException as e: e.do_fail(module) diff --git a/lib/ansible/modules/crypto/acme/acme_certificate.py b/lib/ansible/modules/crypto/acme/acme_certificate.py index 0bec2b8ef0..8719b3b0cb 100644 --- a/lib/ansible/modules/crypto/acme/acme_certificate.py +++ b/lib/ansible/modules/crypto/acme/acme_certificate.py @@ -406,16 +406,21 @@ class ACMEClient(object): contact = [] if module.params['account_email']: contact.append('mailto:' + module.params['account_email']) - self.changed = self.account.init_account( + created, account_data = self.account.setup_account( contact, agreement=module.params.get('agreement'), terms_agreed=module.params.get('terms_agreed'), allow_creation=modify_account, - update_contact=modify_account ) + if account_data is None: + raise ModuleFailException(msg='Account does not exist or is deactivated.') + updated = False + if not created and account_data and modify_account: + updated, account_data = self.account.update_account(account_data, contact) + self.changed = created or updated else: # This happens if modify_account is False and the ACME v1 - # protocol is used. In this case, we do not call init_account() + # protocol is used. In this case, we do not call setup_account() # to avoid accidental creation of an account. This is OK # since for ACME v1, the account URI is not needed to send a # signed ACME request. diff --git a/lib/ansible/modules/crypto/acme/acme_certificate_revoke.py b/lib/ansible/modules/crypto/acme/acme_certificate_revoke.py index dfc8c63bfa..c1386f38ea 100644 --- a/lib/ansible/modules/crypto/acme/acme_certificate_revoke.py +++ b/lib/ansible/modules/crypto/acme/acme_certificate_revoke.py @@ -177,13 +177,11 @@ def main(): result, info = account.send_signed_request(endpoint, payload, key_data=private_key_data, jws_header=jws_header) else: # Step 1: get hold of account URI - changed = account.init_account( - [], - allow_creation=False, - update_contact=False, - ) - if changed: - raise AssertionError('Unwanted account change') + created, account_data = account.setup_account(allow_creation=False) + if created: + raise AssertionError('Unwanted account creation') + if account_data is None: + raise ModuleFailException(msg='Account does not exist or is deactivated.') # Step 2: sign revokation request with account key result, info = account.send_signed_request(endpoint, payload) if info['status'] != 200: diff --git a/test/integration/targets/acme_account/tasks/impl.yml b/test/integration/targets/acme_account/tasks/impl.yml index 42104d930a..3cd10c479e 100644 --- a/test/integration/targets/acme_account/tasks/impl.yml +++ b/test/integration/targets/acme_account/tasks/impl.yml @@ -16,6 +16,22 @@ ignore_errors: yes register: account_not_created +- name: Create it now (check mode, diff) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_src: "{{ output_dir }}/accountkey.pem" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + state: present + allow_creation: yes + terms_agreed: yes + contact: + - mailto:example@example.org + check_mode: yes + diff: yes + register: account_created_check + - name: Create it now acme_account: select_crypto_backend: "{{ select_crypto_backend }}" @@ -30,6 +46,35 @@ - mailto:example@example.org register: account_created +- name: Create it now (idempotent) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_src: "{{ output_dir }}/accountkey.pem" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + state: present + allow_creation: yes + terms_agreed: yes + contact: + - mailto:example@example.org + register: account_created_idempotent + +- name: Change email address (check mode, diff) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_content: "{{ lookup('file', output_dir ~ '/accountkey.pem') }}" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + state: present + # allow_creation: no + contact: + - mailto:example@example.com + check_mode: yes + diff: yes + register: account_modified_check + - name: Change email address acme_account: select_crypto_backend: "{{ select_crypto_backend }}" @@ -70,6 +115,20 @@ ignore_errors: yes register: account_modified_wrong_uri +- name: Clear contact email addresses (check mode, diff) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_src: "{{ output_dir }}/accountkey.pem" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + state: present + # allow_creation: no + contact: [] + check_mode: yes + diff: yes + register: account_modified_2_check + - name: Clear contact email addresses acme_account: select_crypto_backend: "{{ select_crypto_backend }}" @@ -100,6 +159,21 @@ - name: Parse account key (to ease debugging some test failures) command: openssl ec -in {{ output_dir }}/accountkey2.pem -noout -text +- name: Change account key (check mode, diff) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_src: "{{ output_dir }}/accountkey.pem" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + new_account_key_src: "{{ output_dir }}/accountkey2.pem" + state: changed_key + contact: + - mailto:example@example.com + check_mode: yes + diff: yes + register: account_change_key_check + - name: Change account key acme_account: select_crypto_backend: "{{ select_crypto_backend }}" @@ -113,6 +187,18 @@ - mailto:example@example.com register: account_change_key +- name: Deactivate account (check mode, diff) + acme_account: + select_crypto_backend: "{{ select_crypto_backend }}" + account_key_src: "{{ output_dir }}/accountkey2.pem" + acme_version: 2 + acme_directory: https://{{ acme_host }}:14000/dir + validate_certs: no + state: absent + check_mode: yes + diff: yes + register: account_deactivate_check + - name: Deactivate account acme_account: select_crypto_backend: "{{ select_crypto_backend }}" diff --git a/test/integration/targets/acme_account/tests/validate.yml b/test/integration/targets/acme_account/tests/validate.yml index 5d4cbcf1f1..1fcfda65f7 100644 --- a/test/integration/targets/acme_account/tests/validate.yml +++ b/test/integration/targets/acme_account/tests/validate.yml @@ -3,6 +3,18 @@ assert: that: - account_not_created is failed + - account_not_created.msg == 'Account does not exist or is deactivated.' + +- name: Validate that account was created in the second step (check mode) + assert: + that: + - account_created_check is changed + - account_created_check.account_uri is none + - "'diff' in account_created_check" + - "account_created_check.diff.before == {}" + - "'after' in account_created_check.diff" + - account_created_check.diff.after.contact | length == 1 + - account_created_check.diff.after.contact[0] == 'mailto:example@example.org' - name: Validate that account was created in the second step assert: @@ -10,6 +22,23 @@ - account_created is changed - account_created.account_uri is not none +- name: Validate that account was created in the second step (idempotency) + assert: + that: + - account_created_idempotent is not changed + - account_created_idempotent.account_uri is not none + +- name: Validate that email address was changed (check mode) + assert: + that: + - account_modified_check is changed + - account_modified_check.account_uri is not none + - "'diff' in account_modified_check" + - account_modified_check.diff.before.contact | length == 1 + - account_modified_check.diff.before.contact[0] == 'mailto:example@example.org' + - account_modified_check.diff.after.contact | length == 1 + - account_modified_check.diff.after.contact[0] == 'mailto:example@example.com' + - name: Validate that email address was changed assert: that: @@ -27,6 +56,16 @@ that: - account_modified_wrong_uri is failed +- name: Validate that email address was cleared (check mode) + assert: + that: + - account_modified_2_check is changed + - account_modified_2_check.account_uri is not none + - "'diff' in account_modified_2_check" + - account_modified_2_check.diff.before.contact | length == 1 + - account_modified_2_check.diff.before.contact[0] == 'mailto:example@example.com' + - account_modified_2_check.diff.after.contact | length == 0 + - name: Validate that email address was cleared assert: that: @@ -39,12 +78,29 @@ - account_modified_2_idempotent is not changed - account_modified_2_idempotent.account_uri is not none +- name: Validate that the account key was changed (check mode) + assert: + that: + - account_change_key_check is changed + - account_change_key_check.account_uri is not none + - "'diff' in account_change_key_check" + - account_change_key_check.diff.before.public_account_key != account_change_key_check.diff.after.public_account_key + - name: Validate that the account key was changed assert: that: - account_change_key is changed - account_change_key.account_uri is not none +- name: Validate that the account was deactivated (check mode) + assert: + that: + - account_deactivate_check is changed + - account_deactivate_check.account_uri is not none + - "'diff' in account_deactivate_check" + - "account_deactivate_check.diff.before != {}" + - "account_deactivate_check.diff.after == {}" + - name: Validate that the account was deactivated assert: that: @@ -61,8 +117,10 @@ assert: that: - account_not_created_2 is failed + - account_not_created_2.msg == 'Account does not exist or is deactivated.' - name: Validate that the account is gone (old account key) assert: that: - account_not_created_3 is failed + - account_not_created_3.msg == 'Account does not exist or is deactivated.' diff --git a/test/integration/targets/acme_account_facts/tests/validate.yml b/test/integration/targets/acme_account_facts/tests/validate.yml index f44008c546..5863a8c361 100644 --- a/test/integration/targets/acme_account_facts/tests/validate.yml +++ b/test/integration/targets/acme_account_facts/tests/validate.yml @@ -13,6 +13,7 @@ - account_created.account_uri is not none - "'account' in account_created" - "'contact' in account_created.account" + - "'public_account_key' in account_created.account" - account_created.account.contact | length == 1 - "account_created.account.contact[0] == 'mailto:example@example.org'" @@ -23,6 +24,7 @@ - account_modified.account_uri is not none - "'account' in account_modified" - "'contact' in account_modified.account" + - "'public_account_key' in account_modified.account" - account_modified.account.contact | length == 0 - name: Validate that account does not exist with wrong account URI