Add openssl_signature module #63

MarkusTeufelberger · 2020-06-06T13:53:03Z

SUMMARY

Adds a new module to create or verify a Base64 encoded signature of a file.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

openssl_signature

ADDITIONAL INFORMATION

Originally written by @aveexy, his version is the initial commit in this PR.

Changes + tests (+ bugs/errors) are by me.

Currently still working a bit on making tests a bit more exhaustive, maybe also adding a few more features (choosing padding and hashing algorithms, allowing private keys to be used as content not only paths, allowing to verify with public keys only and not certificates, signing strings instead of files...) if they seem useful. The current state is the one that we used internally at https://mgit.at/ as part of a larger project and it seemed useful and general enough to release in this format.

Main focus of reviews should probably be also how it adheres to in-collection "standards" (parameter names, code structure), functionality wise it is not that complex fortunately.

MarkusTeufelberger · 2020-06-06T19:00:01Z

And so it begins...

FreeBSD apparently uses LibreSSL which doesn't support ed25519 or ed448.

OpenSSL in their eternal wisdom decided that openssl dgst shouldn't work for ed25519 or ed448, so I now have to hope that openssl pkeyutl works on all platforms... which of course it doesn't.

felixfontein · 2020-06-06T19:33:48Z

Out of curiosity: why add a pyOpenSSL backend at all, if you plan to remove it "soon"?

MarkusTeufelberger · 2020-06-06T20:10:24Z

It would be the only one without pyopenssl and it wouldn't run at all on Ubuntu Xenial for example (https://packages.ubuntu.com/de/xenial-updates/python-cryptography). The integration tests only pass there, because they don't actually do anything.

Since this is part of the way we create certificates and we also manage some Xenial hosts, there are just a few options, and all of them are ugly. Running pyOpenSSL is maybe the least bad solution in that case.

MarkusTeufelberger · 2020-06-06T20:26:15Z

Urgh, openssl pkeyutl -help seems to return 1 on some platforms (and provide its help text on stderr for good measure)

MarkusTeufelberger · 2020-06-06T20:33:27Z

Otoh OpenSSL in OSX seems to return 0, even if you ask for help for a command it doesn't understand and where it also prints out "openssl:Error: 'pkeyutl' is an invalid command."...

felixfontein · 2020-06-06T20:57:00Z

The x509_crl and x509_crl_info modules only have a cryptography backend, so it definitely wouldn't be the only one. But if pyOpenSSL is the way to get it to work on a platform you need to support, feel free to support it :)

MarkusTeufelberger · 2020-06-06T22:06:02Z

The bigger pain point at the moment is anyways OpenSSL itself... I'm close to writing some feature detection for that whole mess, but I'm afraid that that would again mean a lot of edge cases. There aren't even that many distros tested in shippable! :-/

MarkusTeufelberger · 2020-06-16T20:39:29Z

Hm, pyopenssl (and OpenSSL too) seems to have issues in general with Ed25519 and Ed448 signatures.

MarkusTeufelberger · 2020-06-16T21:50:36Z

FreeBSD 11 manages to die quite spectacularly when signing with secp256r1 keys... I'll just disable these tests for now on FreeBSD and move on, there needs to be some rework in 1-2 years when we remove support for pyopenssl anyways.

MarkusTeufelberger · 2020-06-16T22:15:27Z

Alright, this is now ready for review. I'll squash down my commits before merging.

felixfontein · 2020-06-21T20:53:42Z

plugins/modules/openssl_signature.py

+DOCUMENTATION = r'''
+---
+module: openssl_signature
+short_description: Sign and verify data with openssl


Suggested change

short_description: Sign and verify data with openssl

short_description: Sign and verify data with openssl

version_added: 1.1.0

Should be 1.1.0 now :)

felixfontein · 2020-06-21T20:53:56Z

plugins/modules/openssl_signature.py

+    - The module can use the cryptography Python library, or the pyOpenSSL Python
+      library. By default, it tries to detect which one is available. This can be
+      overridden with the I(select_crypto_backend) option. Please note that the PyOpenSSL backend
+      was deprecated in Ansible 2.9 and will be removed in Ansible 2.13.


Suggested change

was deprecated in Ansible 2.9 and will be removed in Ansible 2.13.

was deprecated in Ansible 2.9 and will be removed in community.crypto 2.0.0.

felixfontein · 2020-06-21T21:05:52Z

plugins/modules/openssl_signature.py

+    - Markus Teufelberger (@MarkusTeufelberger)
+options:
+    action:
+        description: Action to be executed


Suggested change

description: Action to be executed

description: Action to be executed.

felixfontein · 2020-06-21T21:06:00Z

plugins/modules/openssl_signature.py

+module: openssl_signature
+short_description: Sign and verify data with openssl
+description:
+    - This module allows one to sign and verify data via a certificate and a private key


Suggested change

- This module allows one to sign and verify data via a certificate and a private key

- This module allows one to sign and verify data via a certificate and a private key.

felixfontein · 2020-06-21T21:06:08Z

plugins/modules/openssl_signature.py

+        choices: [ sign, verify ]
+    privatekey_path:
+        description:
+            - The path to the private key to use when signing


Suggested change

- The path to the private key to use when signing

- The path to the private key to use when signing.

felixfontein · 2020-06-21T21:07:24Z

plugins/modules/openssl_signature.py

+RETURN = r'''
+signature:
+    description: Base64 encoded signature
+    returned: changed or success


Suggested change

returned: changed or success

returned: success

Changed is part of success :)

felixfontein · 2020-06-21T21:11:32Z

plugins/modules/openssl_signature.py

+                module.fail_json(msg=missing_required_lib('pyOpenSSL >= {0}'.format(MINIMAL_PYOPENSSL_VERSION)),
+                                 exception=PYOPENSSL_IMP_ERR)
+            module.deprecate('The module is using the PyOpenSSL backend. This backend has been deprecated',
+                             version='2.13')


Suggested change

version='2.13')

version='2.0.0', collection_name='community.crypto')

felixfontein · 2020-06-21T21:12:57Z

plugins/modules/openssl_signature.py

+
+EXAMPLES = r'''
+- name: Sign example file
+  openssl_signature:


Suggested change

openssl_signature:

community.crypto.openssl_signature:

felixfontein · 2020-06-21T21:13:04Z

plugins/modules/openssl_signature.py

+  register: sig
+
+- name: Verify signature of example file
+  openssl_signature:


Suggested change

openssl_signature:

community.crypto.openssl_signature:

felixfontein · 2020-06-21T21:13:10Z

plugins/modules/openssl_signature.py

+        description: Action to be executed
+        type: str
+        required: true
+        choices: [ sign, verify ]


Conceptually, I think it would be better to have two modules, one for signing, the other for verifying. I think the idea of having modules which do different actions depending on a module option is nowadays discouraged in Ansible.

How about having something like openssl_signature_sign and openssl_signature_verify?

I'm not opposed, though the content to boilerplate/docs ratio there would be even higher.

Instead of openssl_signature_verify there could also be an openssl_signature_info module that tells you if a signature is valid/verified? That would follow the current pattern here maybe even more...

Yep, it definitely would increase the content/boilerplate ratio.

_info is probably better. I first thought about that too, and was wondering whether it is a good fit and first decided against it, but now (after thinking some more about it) I guess it's better.

Alright, I'll go the openssl_signature/openssl_signature_info route. Time to train those Ctrl + c / Ctrl + v skills! :-D

MarkusTeufelberger · 2020-07-28T19:59:47Z

rebased + rewrote to 2 different modules. The tests are identical for now, not sure if there's much point in separating them?

Things that would be nice, but could be implemented later as additional features:

openssl_signature - choose hashing algorithm and padding algorithm used when signing (big can of worms when it comes to which key type allows which settings though)
openssl_signature_info - more return values than just changed: false and valid: true/false, e.g. hashing algo and passing used (not sure how to reliably detect that though)
openssl_signature_info - also allow public keys rather than parsing them out of a certificate (should be fairly straightforward)

felixfontein · 2020-07-28T20:08:31Z

The tests are identical for now, not sure if there's much point in separating them?

No; I would only keep one copy, and add the other module in aliases, like https://github.com/ansible-collections/community.crypto/blob/main/tests/integration/targets/x509_crl/aliases#L2 (the x509_crl tests also test x509_crl_info).

plugins/modules/openssl_signature.py

plugins/modules/openssl_signature_info.py

plugins/modules/openssl_signature.py

plugins/modules/openssl_signature_info.py

felixfontein · 2020-07-28T20:33:38Z

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

+
+- name: Create statement to be signed
+  copy:
+    content: "Erst wenn der Subwoofer die Katze inhaliert, fickt der Bass richtig übel. -- W.A. Mozart"


Suggested change

content: "Erst wenn der Subwoofer die Katze inhaliert, fickt der Bass richtig übel. -- W.A. Mozart"

content: "Erst wenn der Subwoofer die Katze inhaliert, fickt der Bass richtig übel. -- W.A. Mozart"

:D

felixfontein

Looks good. Feel free to merge if you don't want to make further changes!

MarkusTeufelberger · 2020-08-09T21:15:49Z

Still needs a squash, hoped to do it this weekend, but alas... I'm still on it though and will merge then.

felixfontein · 2020-08-10T05:10:57Z

@MarkusTeufelberger the GitHub merge configured for this repo will already squash all commits :)

MarkusTeufelberger · 2020-08-10T08:47:57Z

Yeah, but it will likely preserve all these ugly commit messages...

felixfontein · 2020-08-10T08:50:18Z

True :)

* Use module_utils from collection, clean up code a bit * add DSA keys, because why not... * sign/verify was added in pyOpenSSL 0.11 apparently * Add signing capability detection to module_utils.crypto.basic * Rework feature detection of signature types. * Rename parameters to match other modules * Add initial version of integration tests * fix whitespace in tests * More whitespace fixes * small fixes for issues in testing * Organize integration tests as test matrix * another indentation fix to make pep8 happy * use openssl pkeyutl when possible, otherwise fall back to openssl dgst * More linter fixes * openssl pkeyutl -help can apparently return 1 * ignore errors on openssl call and another try at formatting * Remove the OpenSSL calls in tests * Add collection name to deprecation notice and deprecate at version 2.0.0 * Exclude Ed448/25519 tests on pyopenssl * revert the collection name in the deprecation notice (breaks 2.9) * limit test platforms even more * disable FreeBSD DSA and ECC tests * Add module name to README * rewrite and split into 2 modules instead * add module to README and fix whitespace issue * remove duplicated tests * address review remarks * resolve another comment

felixfontein · 2020-08-18T11:24:25Z

I've squashed the commits into two commits (one per author). I'll rebase-merge once the tests pass.

felixfontein · 2020-08-18T13:13:44Z

Gna. Why does GitHub always changes the merge button selection when reloading?! This is exactly what I wanted to prevent...

* Use module_utils from collection, clean up code a bit * add DSA keys, because why not... * sign/verify was added in pyOpenSSL 0.11 apparently * Add signing capability detection to module_utils.crypto.basic * Rework feature detection of signature types. * Rename parameters to match other modules * Add initial version of integration tests * fix whitespace in tests * More whitespace fixes * small fixes for issues in testing * Organize integration tests as test matrix * another indentation fix to make pep8 happy * use openssl pkeyutl when possible, otherwise fall back to openssl dgst * More linter fixes * openssl pkeyutl -help can apparently return 1 * ignore errors on openssl call and another try at formatting * Remove the OpenSSL calls in tests * Add collection name to deprecation notice and deprecate at version 2.0.0 * Exclude Ed448/25519 tests on pyopenssl * revert the collection name in the deprecation notice (breaks 2.9) * limit test platforms even more * disable FreeBSD DSA and ECC tests * Add module name to README * rewrite and split into 2 modules instead * add module to README and fix whitespace issue * remove duplicated tests * address review remarks * resolve another comment

felixfontein · 2020-08-18T13:19:58Z

With @gundalow's help I purged the commit from main and then manually pushed the two commits from this PR to main. So now we have two commits for this PR, one per author, which both mention this PR in their commit message.

felixfontein · 2020-08-18T13:21:13Z

@MarkusTeufelberger and Patrick Pichler, thanks a lot for contributing these new modules! :)

MarkusTeufelberger force-pushed the openssl_signature branch from df9977c to fd1618f Compare June 16, 2020 21:07

MarkusTeufelberger changed the title ~~WIP: openssl_signature module~~ Add openssl_signature module Jun 16, 2020

felixfontein reviewed Jun 21, 2020

View reviewed changes

felixfontein mentioned this pull request Jun 27, 2020

Release plan #74

Closed

felixfontein changed the base branch from master to main July 2, 2020 12:13

MarkusTeufelberger force-pushed the openssl_signature branch from c2294d7 to d2f60d2 Compare July 28, 2020 19:14

felixfontein reviewed Jul 28, 2020

View reviewed changes

felixfontein approved these changes Jul 29, 2020

View reviewed changes

Patrick Pichler and others added 2 commits August 18, 2020 13:22

Add openssl_signature module (ansible-collections#63)

fedd43d

felixfontein force-pushed the openssl_signature branch from fd7f12f to 0153eab Compare August 18, 2020 11:23

felixfontein approved these changes Aug 18, 2020

View reviewed changes

felixfontein merged commit 5b1b5d7 into ansible-collections:main Aug 18, 2020

felixfontein pushed a commit that referenced this pull request Aug 18, 2020

Add openssl_signature module (#63)

128991c

	short_description: Sign and verify data with openssl
	short_description: Sign and verify data with openssl
	version_added: 1.1.0

	was deprecated in Ansible 2.9 and will be removed in Ansible 2.13.
	was deprecated in Ansible 2.9 and will be removed in community.crypto 2.0.0.

	description: Action to be executed
	description: Action to be executed.

	- This module allows one to sign and verify data via a certificate and a private key
	- This module allows one to sign and verify data via a certificate and a private key.

	- The path to the private key to use when signing
	- The path to the private key to use when signing.

	version='2.13')
	version='2.0.0', collection_name='community.crypto')

	content: "Erst wenn der Subwoofer die Katze inhaliert, fickt der Bass richtig übel. -- W.A. Mozart"
	content: "Erst wenn der Subwoofer die Katze inhaliert, fickt der Bass richtig übel. -- W.A. Mozart"

Add openssl_signature module #63

Add openssl_signature module #63

Conversation

MarkusTeufelberger commented Jun 6, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

MarkusTeufelberger commented Jun 6, 2020

felixfontein commented Jun 6, 2020

MarkusTeufelberger commented Jun 6, 2020 • edited Loading

MarkusTeufelberger commented Jun 6, 2020 • edited Loading

MarkusTeufelberger commented Jun 6, 2020

felixfontein commented Jun 6, 2020

MarkusTeufelberger commented Jun 6, 2020

MarkusTeufelberger commented Jun 16, 2020

MarkusTeufelberger commented Jun 16, 2020

MarkusTeufelberger commented Jun 16, 2020

felixfontein Jun 21, 2020 • edited Loading

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

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

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

MarkusTeufelberger commented Jul 28, 2020

felixfontein commented Jul 28, 2020

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

MarkusTeufelberger commented Aug 9, 2020

felixfontein commented Aug 10, 2020

MarkusTeufelberger commented Aug 10, 2020

felixfontein commented Aug 10, 2020

felixfontein commented Aug 18, 2020

felixfontein commented Aug 18, 2020

felixfontein commented Aug 18, 2020

felixfontein commented Aug 18, 2020

MarkusTeufelberger commented Jun 6, 2020 •

edited

Loading

MarkusTeufelberger commented Jun 6, 2020 •

edited

Loading

felixfontein Jun 21, 2020 •

edited

Loading