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

Entrust Datacard - Support for "entrust" provider in openssl_certificate module #59272

Merged
merged 18 commits into from Aug 17, 2019
Merged

Entrust Datacard - Support for "entrust" provider in openssl_certificate module #59272

merged 18 commits into from Aug 17, 2019

Conversation

ctrufan
Copy link
Contributor

@ctrufan ctrufan commented Jul 18, 2019

SUMMARY

Addition of support for a new 'provider' value for the openssl_certificate module, to connect to Entrust Datacards "ECS" API for submission of CSRs.

Currently returns (e.g. as the 'return value') all the data that is returned by the ECS API when placing a certificate request. While returned, this data is not documented as part of the shared return data of the module, as much of it is essentially metadata. Some customers may want to register or log it for auditing and tracking purposes, so there is value in it's availability, but it isn't intended to be actioned upon by other tasks in a playbook. I couldn't find any instructions in documentation about whether documenting return values was mandatory, so this seemed in line with existing behavior (e.g. some providers return various pieces of data that aren't a part of the module doc currently). Regardless, the releases of the actual ECS API are intended to be backwards compatible - there are no values in this metadata that will not be available on future playbook runs.

The connection to the ECS API requires both client certificate and HTTP basic auth, and in addition to the CSR there are requester_name, requester_phone, and requester_email fields. The "ecs" module_util creates a client based on the OpenAPI 2.0 specification file defined for our API. We have integration tests on our end to verify that we never break forwards/backwards compatibility with future specification releases. The specification file defaults to a downloadable location, but can also be manually specified. This location is currently not available, but will be as of the next release of the API.

I left the "ocsp" variables without an entrust_ prefix as the functionality could easily be expanded for general cert validation logic, but for now documented them as entrust only. The actual method is not entrust specific, but I didn't test with a variety of other CAs and OCSP responders, so wasn't comfortable making it supported for other providers myself.

Both the module utility and the OCSP check use the ansible 'urls' lib for network connections, and to_text/to_native and six for conversions of input and output.

Details about the integration tests that were run included below, with an attached text file (with sensitive information removed) showing the results of test run.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ansible/modules/crypto/openssl_certificate.py
ansible/module_utils/ecs/init.py

ADDITIONAL INFORMATION

We (will) run integration tests on a weekly basis against the "devel" branch of ansible, against QA test systems representing both the current "live" release of the ECS API, and the in-development next release, with the goal of catching any breaking changes early.
entrust_openssl_playbook_partial.txt
entrust_openssl_playbookoutput.txt

For integration tests, we test via a molecule playbook set up to run against a number of docker images:

  • CentOS7 with python 2.7, 'cryptography' installed
  • CentOS7 with python 2.7, 'pyopenssl' installed
  • CentOS7 with python 3.6, 'cryptography' installed
  • CentOS7 with python 3.6, 'pyopenssl' installed
    (while I tested with Python 3.5, I had some issues with the docker images and ansible run unrelated to these module changes, so the automation runs on 3.6)

The actual operations tested in the test playbook include, in order:

  • openssl_certificate with a local copy of API specification (for customers that want to minimize network traffic)
  • openssl_certificate with a specified entrust_notafter.
  • openssl_certificate with no api specification path specified (uses default).
  • openssl_certificate explicitly specifying the path of the spec for an HTTP endpoint
  • openssl_certificate pointing to an existing entrust-issued cert, that is still valid and with a far expiration data
  • openssl_certificate pointing to an existing entrust-issued certificate that is close to expiration - this performs a 'RENEW' operation
  • openssl_certificate pointing to an existing cert, but with a request for a different certificate type or pointing to a different private key than used to issue cert - this requests a new certificate

The python tests run by testinfra at the end of the integration tests test, in general:

  • New certificates were issued for the commands expected to issue new certificates
  • Expiration date was correct for the test which included an entrust_notafter
  • The 'renew and succeed' certificate resulted in a new certificate issuance
  • name: Generate an Entrust certificate via the Entrust ECS API
    openssl_certificate:
    path: /etc/ssl/crt/ansible.com.crt
    csr_path: /etc/ssl/csr/ansible.com.csr
    provider: entrust
    entrust_requester_name: Jo Doe
    entrust_requester_email: jdoe@ansible.com
    entrust_requester_phone: 555-555-5555
    entrust_cert_type: STANDARD_SSL
    entrust_api_user: api_username
    entrust_api_key: api_password
    entrust_api_client_cert_path: /etc/ssl/entrust/ecs-client.crt
    entrust_api_client_cert_key_path: /etc/ssl/entrust/ecs-key.crt
    entrust_api_specification_path: /etc/ssl/entrust/api-docs/cms-api-2.1.0.yaml

@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2019

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. python3 support:community This issue/PR relates to code supported by the Ansible community. labels Jul 18, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2019

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/module_utils/ecs/__init__.py:126:72: SyntaxError: x.get('name'): kwargs.get(x.get('name', None), None) for x in self.parameters if x.get('in') == 'path' and kwargs.get(x.get('name', None), None)

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

lib/ansible/module_utils/ecs/__init__.py:0:0: empty __init__.py required

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

lib/ansible/module_utils/ecs/__init__.py:126:72: SyntaxError: invalid syntax
lib/ansible/modules/crypto/openssl_certificate.py:704:0: SyntaxError: invalid syntax (__init__.py, line 126)

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 18, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 19, 2019
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
lib/ansible/module_utils/ecs/api.py Outdated Show resolved Hide resolved
lib/ansible/modules/crypto/openssl_certificate.py Outdated Show resolved Hide resolved
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jul 19, 2019
@ctrufan
Copy link
Contributor Author

ctrufan commented Jul 19, 2019

Ah, whoops. Meant to squash before pushing, sorry about the commit flood. Anyone can feel free to let me know if you'd like me to squash, or if ansible policy is to handle squash during hypothetical PR merge.

Thanks,
Chris

@felixfontein
Copy link
Contributor

There's no need to squash now (as long as the commit count isn't getting too huge); it will be squashed during merge anyway.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 28, 2019
@ctrufan ctrufan closed this Jul 31, 2019
@ctrufan
Copy link
Contributor Author

ctrufan commented Aug 5, 2019

Closing. Ran into something with my integration tests - the behavior of the CSR/issued certificate is inconsistent with the openssl_certificate module's other providers.

Namely, the "Organization" field in the subject DN. For OV/EV certificate types we default to the organization that is tied to the requester details, which is a customers validated organization. We do support other organizations, but only if explicitly specified as a parameter.

I could simply document that, but I think it would be better to change the behavior so the organization in the CSR is explicitly called out when we make the API call, so that we will get a clean "Unapproved organization of x provided" if the CSR is one that we can't issue a certificate against as-is.

@ctrufan ctrufan closed this Aug 5, 2019
@ctrufan ctrufan reopened this Aug 6, 2019
@ctrufan ctrufan closed this Aug 6, 2019
@ctrufan ctrufan reopened this Aug 6, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 6, 2019
@felixfontein
Copy link
Contributor

I guess you can later also add a entrust_certificate (or more) modules which allow to access the API more flexibly. The openssl_certificate module should offer the basic options which are easy to implement (depending on the backend). I don't know how the Entrust API works, but your change sounds good :)

@ctrufan
Copy link
Contributor Author

ctrufan commented Aug 7, 2019

I guess you can later also add a entrust_certificate (or more) modules which allow to access the API more flexibly. The openssl_certificate module should offer the basic options which are easy to implement (depending on the backend). I don't know how the Entrust API works, but your change sounds good :)

That was the goal - basic options (and to be more or less plug and play with the other providers). Actually working on another module that will support a greater breadth of our options/functionality now.

@felixfontein thanks for all your input, really appreciate your comments.

…nssl_certificate_module.yaml

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

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @MarkusTeufelberger @Spredzy do you want to take a look as well?

@felixfontein
Copy link
Contributor

@MarkusTeufelberger @Spredzy if you need more time to look at this, please tell me. Otherwise I'll probably merge it this weekend. @ctrufan is working on another module which (I think) needs the new module_utils added here, and also there are too many openssl_certificate PRs open right now ;)

@felixfontein felixfontein merged commit 8636653 into ansible:devel Aug 17, 2019
@felixfontein
Copy link
Contributor

@ctrufan thank you for implementing this!


body['certType'] = module.params['entrust_cert_type']

# Handle expiration (30 days if not specified)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean 365 days here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, holdover from before I changed 30->365 as the code default.

gmt_now = datetime.datetime.fromtimestamp(time.mktime(time.gmtime()))
expiry = gmt_now + datetime.timedelta(days=365)

expiry_iso3339 = expiry.strftime("%Y-%m-%dT%H:%M:%S.00Z")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be RFC 3339, ISO standard 3339 doesn't look timestamp related. ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're right! Good catch.

@MarkusTeufelberger
Copy link
Contributor

Thanks for adding the provider and for reviewing the code!

@ctrufan
Copy link
Contributor Author

ctrufan commented Aug 18, 2019

@MarkusTeufelberger @felixfontein
Thanks for the reviews and merge!

@felixfontein
Copy link
Contributor

@MarkusTeufelberger thank for reviewing as well! Sorry I already merged this... Wanted to remove (potential) merge conflicts... (looks like I forgot to update the sanity PR though)
@ctrufan can you address @MarkusTeufelberger's review comments in a new PR?

@MarkusTeufelberger
Copy link
Contributor

No problem, I was not very available in the last few weeks and didn't tell y'all.

Better merge and fix than being stuck in rebase purgatory.

@ansible ansible locked and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 botmeta This PR modifies the BOTMETA.yml and this requires special attention! core_review In order to be merged, this PR must follow the core review workflow. crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. python3 support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants