Refactor x509_certificate module, add x509_certificate_pipe module #135

felixfontein · 2020-11-01T16:48:46Z

SUMMARY

Similar refactoring / addition than in #119 and #123: refactor x509_certificate module to move large parts of documentation to doc fragments and large pieces of code to module backends, in this case split up per provider, and add a new x509_certificate_pipe module which does not operate on certificate files (and supports only a subset of the providers, maybe just selfsigned and ownca in the beginning).

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

x509_certificate
x509_certificate_pipe

felixfontein · 2020-11-02T18:45:58Z

ready_for_review

@ctrufan could you test whether with this PR the entrust provider still works for x509_certificate?

ctrufan · 2020-11-02T20:27:27Z

Probably not until later this week or this weekend unfortunately, need to rebuild my testing VM.

felixfontein · 2020-11-03T12:10:56Z

Tests should run better now that #137 is done and merged. The remaining failed run is because RHEL8 is currently broken in CI (unrelated to any PR).

felixfontein · 2020-11-08T10:03:10Z

@ctrufan did you got around to test this already?

felixfontein · 2020-11-08T16:33:34Z

(I rebased to be able to work with this module with the bugfix from #141.)

ctrufan · 2020-11-09T18:44:40Z

@felixfontein
Sorry, ran into some issues getting my environment set up to test with 2.10 and the new collections paradigm.

Do I need to run the tests from the primary ansible subdirectory? I tried running from the checked out branch for openssl_certificate_pipe and got an error. The documentation doesn't quite make it clear how to run tests within a collection-from-code.

The current working directory must be at or below:

-  The ansible source: /ansible/ansible
- An Ansible collection {...}/ansible_collections/${namespace}/${collection}

ctrufan · 2020-11-09T18:51:34Z

@felixfontein
Sorry, ran into some issues getting my environment set up to test with 2.10 and the new collections paradigm.

Do I need to run the tests from the primary ansible subdirectory? I tried running from the checked out branch for openssl_certificate_pipe and got an error. The documentation doesn't quite make it clear how to run tests within a collection-from-code.
The current working directory must be at or below:

-  The ansible source: /ansible/ansible
- An Ansible collection {...}/ansible_collections/${namespace}/${collection}

Nevermind, I didn't have quite the right folder structure. Working on testing now/today. Sorry about the unnecessary ping!

ctrufan · 2020-11-09T21:48:54Z

It's not working even pre-pull request because entrust_certificate was broken when the additions were made to support providing the full cert instead of path for 2.10/community migration.

 if self.csr_content is None **or** not os.path.exists(self.csr_path):
            raise CertificateError(
                'The certificate signing request file {0} does not exist'.format(self.csr_path)
            )

vs, for example, ownca:

if self.csr_content is None **and** not os.path.exists(self.csr_path):
            raise CertificateError(
                'The certificate signing request file {0} does not exist'.format(self.csr_path)
            )

I forgot you can't bold within a code block, but it's a difference between and and or.

[Edit: I'll keep testing tomorrow after manually fixing ]

plugins/module_utils/crypto/module_backends/certificate_entrust.py

felixfontein · 2020-11-10T05:25:45Z

Hmm, that bad condition comes from ansible/ansible#66384. Looks like we totally overlooked it back then...

felixfontein · 2020-11-10T05:55:21Z

I guess we should sooner or later add some kind of tests for the ECS modules. Integration tests will need some kind of API simulator. Alternatively, this could be done as a unit test which patches the Request.open() method (similar to the framework in https://github.com/ansible-collections/community.internal_test_tools/blob/main/tests/unit/utils/fetch_url_module_framework.py; see https://github.com/ansible-collections/community.general/blob/main/tests/unit/plugins/modules/net_tools/test_hetzner_firewall.py#L69-L111 for a concrete test case of a module with that framework).

ctrufan · 2020-11-10T06:50:37Z

I like the idea of returning a mocked response, but it would get a bit tricky, because there's a call to download the API spec unless it's provided locally (the download is unauthenticated, but I don't think it would be great to have a unit test download a large file hah, nor would it be good to include large file in the repo).

So the easier route to go might be to mock the module_util return somehow.

We do have integration tests on 'our' side, but I neglected to update them for the new collections paradigm, which requires quite a few tweaks to the molecule framework.

felixfontein · 2020-11-10T07:56:18Z

I've run the new acme provider tests from #142 with this PR and they passed. So at least the acme provider still seems to work with this PR ;)

felixfontein · 2020-11-10T08:02:55Z

@ctrufan with large file, do you mean https://cloud.entrust.net/EntrustCloud/documentation/cms-api-2.1.0.yaml ? It's "only" 110 kb, so downloading it during the tests should be ok. (Assuming your server doesn't mind.) It's probably best to download it once during test setup, and then keep the file locally.

In any case, mocking the module_utils is definitely easier. And it still allows us to do some modifications to the caller and being half-way confident that it still works ;)

ctrufan · 2020-11-10T19:34:04Z

Previously, if the certificate file pointed to by path was an empty or invalid file, the module still worked, and replaced it with a valid certificate. That's no longer the case, but I think that's valid behavior, it's only "not backwards compatible" in a "this probably shouldn't have worked anyways" sense.

Running into some issues getting the test CA recognized properly after updating Python, because using custom CA certs is an OS/python version dependent nightmare, so might not have the valid test case through until tomorrow - module is working correctly up through the cert request stage though.

ctrufan · 2020-11-10T19:37:19Z

Yeah our server has no problem with multiple downloads of the spec.

Something that's been on my to-do list for ages is to update the module so that it can 'cache' the API spec, but it's not something (last I looked) that Ansible has elegant solutions for.

felixfontein · 2020-11-10T21:01:44Z

Previously, if the certificate file pointed to by path was an empty or invalid file, the module still worked, and replaced it with a valid certificate. That's no longer the case, but I think that's valid behavior, it's only "not backwards compatible" in a "this probably shouldn't have worked anyways" sense.

That wasn't an intended change - I've restored the old behavior in dce4f76.

Running into some issues getting the test CA recognized properly after updating Python, because using custom CA certs is an OS/python version dependent nightmare,

Indeed!

so might not have the valid test case through until tomorrow - module is working correctly up through the cert request stage though.

BTW, I recently learned of https://trustme.readthedocs.io/en/latest/ (ansible-community/antsibull#133). Haven't gotten around to really look into it yet, but it might help with such things. Or at least its internals might be useful :)

felixfontein · 2020-11-10T21:03:54Z

Something that's been on my to-do list for ages is to update the module so that it can 'cache' the API spec, but it's not something (last I looked) that Ansible has elegant solutions for.

Since modules can run anywhere, it's kind of hard to cache something reliably. (And most users don't want modules to create files in random positions they didn't explicitly ask for.) Maybe there could be a module entrust_cache_apispec to handle only that? Not sure how practical / useful that is though...

ctrufan · 2020-11-10T21:49:27Z

BTW, I recently learned of https://trustme.readthedocs.io/en/latest/ (ansible-community/antsibull#133). Haven't gotten around to really look into it yet, but it might help with such things. Or at least its internals might be useful :)

That's a neat project, I'll have to look at it more indepth sometime, but I suspect it solves a different problem - I've got a robust test CA infrastructure set up, CRLs/OCSP included, but the underlying HTTP calls made by the python url libraries like to insist there's no such thing as an operating system or editable trust store, especially if certifi is installed.

ctrufan · 2020-11-10T21:52:39Z

ince modules can run anywhere, it's kind of hard to cache something reliably. (And most users don't want modules to create files in random positions they didn't explicitly ask for.) Maybe there could be a module entrust_cache_apispec to handle only that? Not sure how practical / useful that is though...

That was my thought process when I was first developing the module and wanted to cache - don't want to create a file the user isn't expecting, can't easily store it in memory because of how modules run anywhere.

ctrufan · 2020-11-11T19:14:08Z

@felixfontein

Tested the module, and it looks good, worked with entrust provider after the fixes. (I'm assuming from PR description entrust doesn't leverage the new "pipe" module, so there's no new input/output paradigms I need to test?)

Though now I definitely need to carve time out to add unit tests and update our integration tests to work properly with collections, newer python min version, etc... and probably update x509 w/ entrust provider, and ecs_certificate to take advantage of similar new functionality...

felixfontein · 2020-11-11T21:52:47Z

@ctrufan thanks for testing!!! I noticed that I wrote the PR description some time ago when I didn't actually implement the _pipe module; right now the _pipe module also supports the entrust provider, but from the way the unified provider interface works, it should probably just work. But then, it's still better to check it I guess ;) I guess there are two choices: a) remove the entrust provider - that's a very simple change. b) you actually test it.

I'd like to get this merged soon, so I guess if you don't have time now to test it, it's better to remove it for now. Re-adding it later is very easy, so we can do that for the next release without much work involved. (Well, at least not for me - it's mostly work for you, thanks to testing :) ) What do you think?

plugins/modules/x509_certificate_pipe.py

ctrufan · 2020-11-11T23:36:25Z

It looks like if I use x509_certificate_pipe, requests fail when I pass file contents in with lookup(), because the value of "csr" that gets populated in the API request that goes out to the server is bytes rather than a string.

It works if I throw an arbitrary to_text in the line of entrust_certificate around line 100 (along with an import of to_text at the top)
body['csr'] = to_text(self.csr_content)
But that was just a sort of makeshift "see if this fixes it" approach.

I think a better answer might be that I should be converting all the values in the array that gets passed to the ECS api.py file to normalize them to text. Either that or doing something on first parameter load. I'll have to revisit the ansible guidelines on this stuff and Python 2/3, all the string/byte stuff is not something I recall without looking it up again!

I don't think I'll have time to investigate any farther until tomorrow, so feel free to merge it with the provider removed for now, I don't want to delay your merge.

felixfontein · 2020-11-12T06:50:31Z

It looks like if I use x509_certificate_pipe, requests fail when I pass file contents in with lookup(), because the value of "csr" that gets populated in the API request that goes out to the server is bytes rather than a string.

Actually x509_certificate would fail the same way if you use csr_content. So it's something to fix.

It works if I throw an arbitrary to_text in the line of entrust_certificate around line 100 (along with an import of to_text at the top)
body['csr'] = to_text(self.csr_content)
But that was just a sort of makeshift "see if this fixes it" approach.

This is the correct fix (except that using to_native is even better), since csr_content is converted to bytes because that's the format it is usually needed in (for loading the CSR, or writing it to disk for the acme provider).

I think a better answer might be that I should be converting all the values in the array that gets passed to the ECS api.py file to normalize them to text. Either that or doing something on first parameter load. I'll have to revisit the ansible guidelines on this stuff and Python 2/3, all the string/byte stuff is not something I recall without looking it up again!

For everything that is taken from module.params that should not be necessary, they should already be in the correct format. It's only that csr_content is encoded in the CertificateBackend base class since the providers (and the base class) usually need it as bytes. Well, except the entrust provider in this one case :)

I don't think I'll have time to investigate any farther until tomorrow, so feel free to merge it with the provider removed for now, I don't want to delay your merge.

I'll still wait a few days, in the hope that @MarkusTeufelberger also finds some time to look at this :)

Co-authored-by: Chris Trufan <[email protected]>

This reverts commit 6ee5d7d.

felixfontein · 2020-11-15T08:19:14Z

I rebased to main and added an empty ci_complete commit to make sure that all tests are run.

felixfontein · 2020-11-17T20:41:42Z

@Xyon @Shaps @MarkusTeufelberger @puiterwijk would be happy if one of you could take a look at this PR!

MarkusTeufelberger · 2020-11-18T07:54:13Z

Sorry for the radio silence, bit busy times right now... :-/

I'll take a look at it in the evening today.

MarkusTeufelberger

I mostly looked at documentation and kinda hope that immediate failures would be caught by the integration tests. Hope it still helps, thanks for the great work as always!

plugins/doc_fragments/module_certificate.py

plugins/modules/x509_certificate_pipe.py

MarkusTeufelberger · 2020-11-18T23:17:31Z

plugins/modules/x509_certificate_pipe.py

+- debug:
+    var: result.certificate
+
+- name: Generate an OpenSSL Certificate Signing Request with an inline certificate


Create a CSR with an inline certificate? I'm not sure I understand what this should do...

Probably this task should also be delegated to localhost in a more realistic scenario, right?

Might be worth having an example where a certificate is slurped to the Ansible host, checked/modified by community.crypto.x509_certificate_pipe there and then copied back with copy to the target host again.

Heh, it should have been Generate certificate with inline CSR :) I've fixed that. I also added more comments and a delegate_to to the copy task. This example should show that while the OwnCA data (private key, certificate) is on the remote machine, we don't have to copy CSR and certificate to/from there.

I reformatted the example and added another one as you suggested.

Co-authored-by: MarkusTeufelberger <[email protected]>

felixfontein · 2020-11-19T20:07:39Z

recheck

felixfontein · 2020-11-20T06:02:35Z

@MarkusTeufelberger if you're happy with the changes, I'll merge (and release 1.3.0 I guess, if nobody objects).

plugins/module_utils/crypto/module_backends/certificate.py

resmo · 2020-11-24T15:56:20Z

plugins/module_utils/crypto/module_backends/certificate.py

+def get_certificate_argument_spec():
+    return ArgumentSpec(
+        argument_spec=dict(
+            provider=dict(type='str', choices=[]),


ehm is this as it should?

Yes: the choices list is updated for example here: https://github.com/ansible-collections/community.crypto/pull/135/files#diff-ccf70653d5d26e8f8b623afd6fccc608043ef7ac3b6481e4a9851a0c74338409R113

(Maybe a comment would help, though :) )

Added 14a0ca7

resmo · 2020-11-24T15:57:09Z

plugins/module_utils/crypto/module_backends/certificate.py

+            # General properties of a certificate
+            privatekey_path=dict(type='path'),
+            privatekey_content=dict(type='str', no_log=True),
+            privatekey_passphrase=dict(type='str', no_log=True),


👌 no_log=True ;)

felixfontein · 2020-11-24T16:23:09Z

@ctrufan @MarkusTeufelberger @resmo thanks a lot for all the testing and reviewing!!!

felixfontein changed the title ~~[WIP] Refactor x509_certificate module, add x509_certificate_pipe module~~ Refactor x509_certificate module, add x509_certificate_pipe module Nov 2, 2020

felixfontein mentioned this pull request Nov 3, 2020

Next release: 1.3.0 #126

Closed

10 tasks

felixfontein force-pushed the openssl_certificate_pipe branch 2 times, most recently from 4a5d6d1 to 369e92d Compare November 3, 2020 11:40

felixfontein force-pushed the openssl_certificate_pipe branch from 369e92d to 480ea30 Compare November 8, 2020 16:29

ctrufan reviewed Nov 9, 2020

View reviewed changes

plugins/module_utils/crypto/module_backends/certificate_entrust.py Outdated Show resolved Hide resolved

ctrufan reviewed Nov 11, 2020

View reviewed changes

plugins/modules/x509_certificate_pipe.py Outdated Show resolved Hide resolved

felixfontein and others added 8 commits November 15, 2020 09:18

Update plugins/modules/x509_certificate_pipe.py

44a14a5

Co-authored-by: Chris Trufan <[email protected]>

Remove Entrust provider from x509_certificate_pipe for now.

89e1d91

Add own CA tests.

3b54398

One more fix for Entrust provider, when csr_content is used.

7ecbaa7

Update plugins/modules/x509_certificate_pipe.py

4f73eb8

Co-authored-by: Chris Trufan <[email protected]>

Fix another broken example.

14decd2

Revert "Remove Entrust provider from x509_certificate_pipe for now."

371d3d0

This reverts commit 6ee5d7d.

ci_complete

a5f5424

felixfontein force-pushed the openssl_certificate_pipe branch from 01bbf93 to a5f5424 Compare November 15, 2020 08:18

MarkusTeufelberger reviewed Nov 18, 2020

View reviewed changes

felixfontein and others added 4 commits November 19, 2020 08:06

Apply suggestions from code review

eea5a0d

Co-authored-by: MarkusTeufelberger <[email protected]>

Improve example.

95b9c46

Improve readability of example, add another one.

670b01d

Extend descriptions of csr_* for selfsigned.

815ca1b

felixfontein requested a review from MarkusTeufelberger November 22, 2020 12:11

Improve documentation.

70e3900

resmo reviewed Nov 24, 2020

View reviewed changes

plugins/module_utils/crypto/module_backends/certificate.py Outdated Show resolved Hide resolved

resmo reviewed Nov 24, 2020

View reviewed changes

felixfontein added 2 commits November 24, 2020 17:00

Move deprecation message up.

1b74d24

Explain empty choices.

14a0ca7

resmo approved these changes Nov 24, 2020

View reviewed changes

felixfontein merged commit 69335a8 into ansible-collections:main Nov 24, 2020

felixfontein deleted the openssl_certificate_pipe branch November 24, 2020 16:22

Refactor x509_certificate module, add x509_certificate_pipe module #135

Refactor x509_certificate module, add x509_certificate_pipe module #135

Conversation

felixfontein commented Nov 1, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

felixfontein commented Nov 2, 2020

ctrufan commented Nov 2, 2020

felixfontein commented Nov 3, 2020

felixfontein commented Nov 8, 2020

felixfontein commented Nov 8, 2020

ctrufan commented Nov 9, 2020 • edited Loading

ctrufan commented Nov 9, 2020

ctrufan commented Nov 9, 2020 • edited Loading

felixfontein commented Nov 10, 2020

felixfontein commented Nov 10, 2020

ctrufan commented Nov 10, 2020 • edited Loading

felixfontein commented Nov 10, 2020

felixfontein commented Nov 10, 2020

ctrufan commented Nov 10, 2020 • edited Loading

ctrufan commented Nov 10, 2020

felixfontein commented Nov 10, 2020

felixfontein commented Nov 10, 2020

ctrufan commented Nov 10, 2020

ctrufan commented Nov 10, 2020

ctrufan commented Nov 11, 2020

felixfontein commented Nov 11, 2020

ctrufan commented Nov 11, 2020 • edited Loading

felixfontein commented Nov 12, 2020

felixfontein commented Nov 15, 2020

felixfontein commented Nov 17, 2020

MarkusTeufelberger commented Nov 18, 2020

MarkusTeufelberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Nov 19, 2020

felixfontein commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Nov 24, 2020

ctrufan commented Nov 9, 2020 •

edited

Loading

ctrufan commented Nov 9, 2020 •

edited

Loading

ctrufan commented Nov 10, 2020 •

edited

Loading

ctrufan commented Nov 10, 2020 •

edited

Loading

ctrufan commented Nov 11, 2020 •

edited

Loading