Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of ECS_Certificate Module #60883

Merged
merged 11 commits into from Aug 28, 2019

Conversation

@ctrufan
Copy link
Contributor

commented Aug 19, 2019

SUMMARY

Addition of a module to submit certificate requests to the Entrust Certificate Services API.

Supports various entrust specific parameters, such as specifying client ID or tracking information such as additional emails or custom fields.

If the certificate in 'path' is present but not an entrust certificate, it will be treated as not valid - expectation is that this allows for transitioning from self-signed/etc to Entrust issued certificates.

If the certificate present in path but is an Entrust certificate, whether or not a new one is requested is based on the days remaining and the value of 'force', similarly to how both openssl_certificate and acme_certificate behave.

The "tracking_id" parameter is the value to uniquely identify certificates in the ECS system, and is something customers would be familiar with if they needed to use it.

The ECS API supports two operations on existing certificates, 'RENEW' and 'REISSUE'. If the tracking_id parameter is used, this module can be used to 'adopt' an existing ECS certificate for management under Ansible - specifying a tracking_id means that it will download that certificate to path if it is a valid certificate, and if it is within remaining_days of expiration or the 'force' command is used, it can be reissued or renewed. This design decision was made to make it easier to migrate existing infrastructure to ansible, as many customers have internal setups that already keep track of the tracking_id of existing certificates that they could pass to an ansible playbook.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

crypto
ecs_certificate
entrust/api

ADDITIONAL INFORMATION

Integration Tests

Tests run against VMs set up with Python 2.7 and Python 3.6. The reason the attached playbook doesn't run as 'localhost' was specifically to test behavior on different endpoint environments.

Attached are the results of the following integration tests running against our QA environment. Once merged these tests will run nightly against both ansible/devel and any baseline releases that include this module, against both our "next release" of ECS and the "current release". If needed I can try to adapt them (e.g. remove any sensitive values) for inclusion in ansible code base, though I'll note that customers and other ansible developers cannot access our internal QA environment, they can only submit against our actual CA, so testing would need to involve owned domains and actual ECS accounts, and unless the account was configured otherwise, would issue genuine publicly trusted certificates.

I am going to clean up the integration tests additionally (migrate the 'fail' calls into distinct pytest methods that check data against our database as well).

Integration Tests:

  • Have ECS generate a signed certificate for us using a local file path for the specification
  • Verify that ECS will not mark 'changed' if calling a still valid file
  • Verify that ECS will force a new request even if current is still valid, and REISSUE works with no CSR.
  • Test that ECS will download an existing older cert if tracking_id specified, but will not perform action if still valid.
  • Perform an actual renew based on tracking ID.
  • Request a certificate with all various parameters
  • Request a certificate with client ID other than 1
    ecs_certificate_playbook.txt
    ecs_certificate_output.txt

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@ctrufan this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

The test ansible-test sanity --test botmeta [explain] failed with 1 error:

.github/BOTMETA.yml:0:0: expected str for dictionary value @ data['files']['$module_utils/ecs/']. Got None

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:0:0: missing: __metaclass__ = type

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:0:0: should not have a shebang

The test ansible-test sanity --test validate-modules [explain] failed with 5 errors:

lib/ansible/modules/crypto/entrust/ecs_certificate.py:0:0: E325 Argument 'number1' in argument_spec found in custom_fields defines type as 'int' but documentation defines type as 'float'
lib/ansible/modules/crypto/entrust/ecs_certificate.py:0:0: E325 Argument 'number2' in argument_spec found in custom_fields defines type as 'int' but documentation defines type as 'float'
lib/ansible/modules/crypto/entrust/ecs_certificate.py:0:0: E325 Argument 'number3' in argument_spec found in custom_fields defines type as 'int' but documentation defines type as 'float'
lib/ansible/modules/crypto/entrust/ecs_certificate.py:0:0: E325 Argument 'number4' in argument_spec found in custom_fields defines type as 'int' but documentation defines type as 'float'
lib/ansible/modules/crypto/entrust/ecs_certificate.py:0:0: E325 Argument 'number5' in argument_spec found in custom_fields defines type as 'int' but documentation defines type as 'float'

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Aug 19, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:0:0: missing: __metaclass__ = type

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Aug 20, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/ecs_credential.py:9:1: E302 expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Aug 20, 2019

@sivel sivel removed the needs_triage label Aug 20, 2019

@ctrufan ctrufan force-pushed the EntrustDatacard:ecs_certificate_module branch from 68da153 to 2691e49 Aug 21, 2019

@felixfontein
Copy link
Contributor

left a comment

I didn't look at the implementation, just at the documentation so far.

BTW, all new modules should have tests of some kind (either unit tests, or integration tests, even if they are marked as unsupported, i.e. they are not run via CI).

.github/BOTMETA.yml Outdated Show resolved Hide resolved
lib/ansible/plugins/doc_fragments/ecs_credential.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/entrust/ecs_certificate.py Outdated Show resolved Hide resolved
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/crypto/entrust/ecs_certificate.py:532:0: anomalous-backslash-in-string Anomalous backslash in string: '\Z'. String constant might be missing an r prefix.
lib/ansible/modules/crypto/entrust/ecs_certificate.py:534:0: anomalous-backslash-in-string Anomalous backslash in string: '\Z'. String constant might be missing an r prefix.

The test ansible-test sanity --test import --python 3.8 [explain] failed with 2 errors:

lib/ansible/modules/crypto/entrust/ecs_certificate.py:532:0: SyntaxWarning: invalid escape sequence \Z
lib/ansible/modules/crypto/entrust/ecs_certificate.py:534:0: SyntaxWarning: invalid escape sequence \Z

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/crypto/entrust/ecs_certificate.py:532:91: W605 invalid escape sequence '\Z'
lib/ansible/modules/crypto/entrust/ecs_certificate.py:534:108: W605 invalid escape sequence '\Z'

click here for bot help

@ansibot ansibot removed the ci_verified label Aug 27, 2019

@ansibot ansibot added core_review and removed needs_revision labels Aug 27, 2019

@ctrufan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@felixfontein
I've been using the "mark resolved" to internally mark when I fixed something, but it occurred to me maybe that's typically something the reviewer does to indicate that their comments are addressed? My apologies if I've been a bit backwards on the process :).

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@felixfontein
I've been using the "mark resolved" to internally mark when I fixed something, but it occurred to me maybe that's typically something the reviewer does to indicate that their comments are addressed? My apologies if I've been a bit backwards on the process :).

No worries, that's totally fine! (Well, at least as long as you're very sure the comment is really addressed. If you're not sure, don't resolve it.)

@felixfontein
Copy link
Contributor

left a comment

Found a few nits, besides that, I think everything is fine!

The only thing that is missing are tests (integration tests marked as unsupported are also fine).

If you can add them today, we should be able to get this new module in for Ansible 2.9.

ctrufan and others added 3 commits Aug 28, 2019
Update lib/ansible/modules/crypto/entrust/ecs_certificate.py
Co-Authored-By: Felix Fontein <felix@fontein.de>
@ctrufan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

For running the integration tests, I couldn't for the life of me get it to pick up on an integration_config.yml file - since I figured it was likely a config issue and not something that would require code changes, I ran tests with:

cat integration_config.yml >> test/integration/targets/ecs_certificate/vars/main.yml
ansible-test integration unsupported/ecs_certificate | tee ecs_certificate_test_output.txt

Contents of integration_config are:

entrust_api_user: CENSORED
entrust_api_key: CENSORED
entrust_api_ip_address: CENSORED
entrust_cloud_ip_address: CENSORED
entrust_api_client_cert_path: /var/integration-testing/publicCert.pem
entrust_api_client_cert_key_path: /var/integration-testing/privateKey.pem
cacerts_bundle_path_local: /var/integration-testing/cacerts

Attached is the file output:
ecs_certificate_test_output.txt

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

In case the tests modify the system, you want to add --docker ubuntu1804 or something similar to the ansible-test integration command line (https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html#container-images). Then it won't screw up your system, but only a ephemeral container :)

@felixfontein
Copy link
Contributor

left a comment

Looks good. Just a few minor optional points. Also, tests for check mode would be nice. But I'm totally fine with adding them in a later PR.

@ctrufan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

In case the tests modify the system, you want to add --docker ubuntu1804 or something similar to the ansible-test integration command line (https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html#container-images). Then it won't screw up your system, but only a ephemeral container :)

I was running in a VM already, so there were no worries there. This does bring up an issue with the integration tests though - these ones can't run with --docker, because they need access to the cert/key files, and since the entire playbook runs from within the docker container, there's no way to get them to it (our internal molecule tests solve this by copying from the host to the docker VMs, as well as doing API calls delegated to localhost).

Unless I'm missing something in the docker behavior, there's no way to push data from the host to the container, so I might have to refactor the variable to take in the full PEM string of cert/key, then write them to disk.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Ah, in that case, don't ;-)

@felixfontein felixfontein merged commit 44c18b5 into ansible:devel Aug 28, 2019

1 check passed

Shippable Run 140693 status is SUCCESS.
Details
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@ctrufan thanks for contributing this module!

@ctrufan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@felixfontein

Thank you for the reviews and input! Very appreciated, especially getting it in before 2.8.

adharshsrivatsr added a commit to adharshsrivatsr/ansible that referenced this pull request Sep 3, 2019
Addition of ECS_Certificate Module (ansible#60883)
* Addition of ecs_certificate module.

* Documentation and code fixes

* Updates per code review

* Doc fixes, rename of chain_path to full_chain_path, add regex for cert_Expiry check

* Fixes to pep8 check to make regexp string 'raw'.

* Mistakes with find/replace of caseing.

* Added integration tests and some doc cleanup

* Some additional assertions and test typo cleanup

* Update lib/ansible/modules/crypto/entrust/ecs_certificate.py

Co-Authored-By: Felix Fontein <felix@fontein.de>

* Responses to code review comments

* Remove fake passwords from aliases file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.