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

[WIP] move openssl_certificate assertonly provider to new module #54635

Open
wants to merge 6 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@felixfontein
Copy link
Contributor

felixfontein commented Mar 30, 2019

SUMMARY

As discussed in #29951 (and also discussed with @Shaps on IRC), we want to move the assertonly provider of openssl_certificate into a new module.

This PR is an experiment/WIP/testbed on how this could be done.

My suggestion is:

  • add a new module openssl_certificate_validate
  • move assertonly functionality there
  • (probably not add anything new, since 2.8 isn't that far away, but in the future being able to validate chains etc. would be useful)
  • deprecate assertonly provider in openssl_certificate
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

openssl_certificate_validate

@felixfontein felixfontein changed the title [WIP]: move openssl_certificate assertonly provider to new module [WIP] move openssl_certificate assertonly provider to new module Mar 30, 2019

@felixfontein felixfontein force-pushed the felixfontein:openssl_certificate-assertonly branch Mar 30, 2019

@ansibot

This comment has been minimized.

@ansibot

This comment was marked as outdated.

Copy link
Contributor

ansibot commented Mar 30, 2019

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

lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'attr' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'attributes' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'group' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'mode' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'owner' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'selevel' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'serole' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'setype' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'seuser' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec
lib/ansible/modules/crypto/openssl_certificate_validate.py:0:0: E323 Argument 'unsafe_writes' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec

click here for bot help

@ansibot ansibot added the ci_verified label Mar 30, 2019

@felixfontein felixfontein force-pushed the felixfontein:openssl_certificate-assertonly branch to d8d0fb8 Mar 30, 2019

@ansibot ansibot removed the ci_verified label Mar 30, 2019

@felixfontein felixfontein force-pushed the felixfontein:openssl_certificate-assertonly branch to e060795 Mar 30, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Mar 30, 2019

Ok, I think this is now a good preview of what I was thinking about. @Spredzy @MarkusTeufelberger @Shaps @Xyon @puiterwijk opinions, comments, ...?

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

MarkusTeufelberger commented Mar 31, 2019

The reason I didn't do this before was that I couldn't find good examples of Ansible modules that are only capable of asserting properties (well, aside from assert). Roughly speaking, most modules either just record current state (*_facts, *_info) or are used to converge idempotently to a defined state (most other modules). This module is a third category that gets a defined state, but can only pass or fail.

I'm still not sure if this is an anti-pattern, maybe this should have the word "assert" in its name to be more clear or be downgraded to openssl_certificate_info with an assert task following that checks various results instead to be more idiomatic. I just find the assert module to be a bit too cumbersome and limiting to do the things the assertonly provider currently can do.

@ansibot ansibot removed the needs_triage label Mar 31, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Mar 31, 2019

I'm also not aware of any other assert-type module in Ansible. In most cases, this probably won't make sense, since _facts/_info + assert can do exactly the same things, and an _assert/_validate module will be a lot less flexible.

I'm still not sure if this is an anti-pattern, maybe this should have the word "assert" in its name to be more clear or be downgraded to openssl_certificate_info with an assert task following that checks various results instead to be more idiomatic.

I've thought a bit about different names (mostly of type openssl_certificate_validat*, or openssl_certificate_assert), and picked openssl_certificate_validate yesterday, though I'm wondering now whether openssl_certificate_assert wouldn't be better, especially beause the implication that one could also combine _info with assert, as you mentioned.

I just find the assert module to be a bit too cumbersome and limiting to do the things the assertonly provider currently can do.

Definitely. Otherwise, I would rather add a _info module and suggest to convert assertonly usages to that :)

I'll ask around in #ansible-devel whether people have opinions on this.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Mar 31, 2019

I went through all validation features the module currently offers (and one it will offer in the future), to see what could be done easily with certificate_info + assert, what needs additional support, and what can't be done in a good way.

What can be done with certificate_info module + assert:

  • CSR subject matches certificate subject (assuming csr_info module exists as well)
  • certificate signature algorithms
  • certificate subject
  • certificate issuer
  • certificate has (not yet) expired
  • certificate version
  • certificate key usage
  • certificate subject alternative names
  • certificate not before
  • certificate not after

What can be done with certificate_info module + assert + utility filters for time manipulation:

  • certificate valid at
  • certificate invalid at
  • certificate valid in

What needs special support, i.e. cannot simply be done with certificate_info + assert:

  • private key matches public key of certificate
  • CSR public key matches certificate public key
    • both would be possible if there is a compare_public_keys() filter/comparison function/test which allows to compare public keys (given as PEM, DER, or in form of PEM/DER private key);
  • CSR extensions match certificate extensions
    • could be done directly if both certificate_info and csr_info provide all extensions in a useful way;
  • certificate chain is valid (not yet supported by the module; see also #29951)
    • cannot be done directly, definitely needs a module or a lookup (though that only works on the current machine).

@felixfontein felixfontein referenced this pull request Mar 31, 2019

Open

Crypto issues #444

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Mar 31, 2019

There has been a proposal for such a kind of modules before: ansible/proposals#148

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Apr 1, 2019

I've read through the discussion about that proposal (and PR #47980): https://meetbot.fedoraproject.org/ansible-meeting/2018-11-06/ansible_core_public_irc_meeting.2018-11-06-19.04.log.html

From how I understand it, it is OK as long as we don't fail the module when the conditions are not met. So if we provide a combined result result.condition_failed (for at least one condition failed), the user can add failed_when: result is failed or result.condition_failed to achieve the same result as currently. I guess the module could be named openssl_certificate_validate_info then, or something similar?

@MarkusTeufelberger

This comment has been minimized.

Copy link
Contributor

MarkusTeufelberger commented Apr 1, 2019

Sounds to me as if that would be ok too. Having an openssl_certificate_info module might be useful as well, so maybe this could/should be combined into a module that returns all possible fields/info about a certificate AND validation results on all (optional) assertion/validation parameters that are supplied (as well as a "global" one like result.condition_failed)?

openssl_certificate_validate_info is certainly a mouthful by the way... I would argue that assertion/validation results are just optional/derived information too, so openssl_certificate_info should be enough if it also returns the actual information/content of a certificate.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

felixfontein commented Apr 2, 2019

I created a WIP for openssl_certificate_info (#54709), which allows to do most of the checks directly in Ansible (with register/assert).

The "special support" section of above cannot yet be done, though public key and subject comparisons are prepared (they need corresponding openssl_privatekey_info and openssl_csr_info modules -- will try that later). Checking whether extensions of CSR and certificate match is the only thing which can't be done this way.

gundalow added a commit that referenced this pull request Apr 8, 2019

@ansibot ansibot added the stale_ci label Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.