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

Add openssl_certificate_info module #54709

Merged
merged 12 commits into from
Apr 5, 2019

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Apr 2, 2019

SUMMARY

Adds a new module openssl_certificate_info. Useful for doing most checks from #54635 / openssl_certificate's assertonly provider directly with in Ansible with register/assert.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

openssl_certificate_info

@ansibot
Copy link
Contributor

ansibot commented Apr 2, 2019

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Apr 2, 2019
@MarkusTeufelberger
Copy link
Contributor

I'd still offer the validation parameters in here eventually, assert is not really nice to use once you have moderately complex structures and its outputs are both too large and too small (An assertion failed is too little and calling assert with a loop prints EVERY item in full).

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 2, 2019
@felixfontein
Copy link
Contributor Author

BTW, assert got a quiet parameter for Ansible 2.8, which reduces the noise a lot :) (docs) Also, when an assertion fails, the first condition which failed is printed (I think that's not new for 2.8), so there's usually enough information available.

I'm not really sure about the validation parameters; they feel wrong in an _info module for me. I have to think some more about this...

@MarkusTeufelberger
Copy link
Contributor

"A slightly complex subject object I give to the module is identical to the subject of an x509 certificate" is information that I would consider valuable knowing about a certificate.

A bigger question in case of a difference is if the module result should be failed (probably not, got vetoed in the core meeting), changed (similar to running modules in check_mode?) or OK (but it was different and that's NOT ok?).

@felixfontein
Copy link
Contributor Author

Instead of passing the subject into the _info module, you could also check it in an assert elegantly:

- name: Check certificate subject
  assert:
    that:
      - result.subject == expected_subject
  vars:
    expected_subject:
      commonName: www.ansible.com
      country: us

About the return value: _info modules should pass unchanged in general. (If they can't receive the information they need, they can fail, for example if they can't read or parse the certificate.) So if you want to return something like that, you need to add it as a return variable.

But for most conditions of current assertonly, they can be checked rather elegantly similar to the above assert. So I'm not really convinced that the _info module should have such checks.

(I added the valid_at option mainly because there is currently no way in Ansible to handle time comparisons in a good way.)

@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 3, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 3, 2019
@MarkusTeufelberger
Copy link
Contributor

#29951 would likely also need a special option here, right? As well as checking if a given private key fits to a certain certificate.

@felixfontein
Copy link
Contributor Author

#29951 would likely also need a special option here, right?

If we decide that this module should be able to do that, yes. But that won't happen before stable-2.8 is branched.

As well as checking if a given private key fits to a certain certificate.

That would already be possible if there is a corresponding openssl_privatekey_info module. Just get infos on both private key and certificate, and compare public keys.

point_1: "+1d"
point_2: "+3w"
register: result
- assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an empty line above this one and give it a name, it seems like the assert task is just a parameter of the openssl_certificate_info one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've extended the examples a bit. Can you check again?

@MarkusTeufelberger
Copy link
Contributor

(you might want to change/fix the title of this PR by the way)

I mostly went through the docs, not deeply into the code so far. Great work as always, I'm starting to warm up more and more to the idea of rewriting my role(s) to use a second assert task in the future. Should this module still get merged into 2.8 (+ a deprecation notice in openssl_certificate for at least assertonly and maybe even pyOpenSSL) or are you targeting 2.9?

@felixfontein felixfontein changed the title Add openssl_certifacte_info module Add openssl_certificate_info module Apr 5, 2019
@felixfontein
Copy link
Contributor Author

(you might want to change/fix the title of this PR by the way)

Thanks! Fixed it (also in the description). I also noticed I forgot to check in the tests... I hope they pass as they do on my machine ;-)

I mostly went through the docs, not deeply into the code so far. Great work as always, I'm starting to warm up more and more to the idea of rewriting my role(s) to use a second assert task in the future. Should this module still get merged into 2.8 (+ a deprecation notice in openssl_certificate for at least assertonly and maybe even pyOpenSSL) or are you targeting 2.9?

I'm currently aiming at 2.8. I'm not sure whether I want to add the deprecation notice for assertonly already, I think it's better to spend some more time thinking on whether there's a better way. In particular, having at least some of the _info modules around already for 2.8 is definitely a good thing IMO, independently of assertonly.

@felixfontein
Copy link
Contributor Author

(Also, core freeze finally happened and feature freeze will be on April 11th, so there isn't that much time left :) )

@MarkusTeufelberger
Copy link
Contributor

Yeah, that's why I was asking. ;-)

@MarkusTeufelberger
Copy link
Contributor

Probably needs a changelog entry?

@felixfontein
Copy link
Contributor Author

New modules don't need one, changelog entries are autogenerated for new modules. (source)

debug:
var: cryptography_info_results
- name: Compare results
assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't that fail CSR/Cert number 2 later, since it was issued for the private key with a password?

Choose a reason for hiding this comment

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

The private key isn't used/needed for retrieving information on certificates.

(What's interesting though is that creating the self-signed certificate works with the wrong key... But that's unrelated to this PR...)

Copy link
Contributor

Choose a reason for hiding this comment

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

So this would only be discovered if there's also an openssl_privatekey_info task and then the public keys of the subject, issuer and private key need to match up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passphrases are only discovered when private keys protected with passphrases are loaded, it doesn't matter by whom.

Does the cert actually contain the public key of the issuer? I thougt there's only the signature, which can be validated given the public key of the issuer (which has to be taken fom the issuer's cert, or from another source).

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.alvestrand.no/objectid/2.5.29.35.html exists, but that's not set in many cases. We'd need #29951 for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also a feature request for that: #50782

But that doesn't store the public key, only some kind of hash, so you still need to get the public key from somewhere else :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Urgh, sometimes x509 really feels like they want to make it as hard as possible to varify/validate information... :-/

So in this case (selfsigned) only Issuer == Subject can be checked currently and the matching public key check once the openssl_privatekey_info module is 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.

The aim is also to make the cert small, so you can't insert everything ;) If the objectID extension is there, you can also check if the cert's public key matches it.

Anyway, it might make sense to have another module which does some more validation, or have some more parameters to allow more complex information retrieval. But I wouldn't do that for 2.8, since we have to look at it in some more detail to find a really good solution ;)

@MarkusTeufelberger
Copy link
Contributor

One feature that's now missing a bit (but not really critical, since it is likely that this module will be used in tandem with openssl_certificate) is the file permissions stuff (user/group/mode...). I wouldn't see that as a blocker though and this can be fixed later, if it is really seen as necessary.

Not much more from my side on this, having an info module that exposes everything in a cert seems generally useful.

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 5, 2019
@felixfontein
Copy link
Contributor Author

One could argue that people should use the stat module if they want to be sure that file permissions are correct ;) A case where it is more useful is probably the openssl_privatekey_info module (because certs are usually not sensitive, while private keys are), though I'd also leave that out for now.

@MarkusTeufelberger
Copy link
Contributor

(stat really should be aliased to file_info btw.)

@gundalow gundalow merged commit 65d7f0d into ansible:devel Apr 5, 2019
@felixfontein felixfontein deleted the openssl_certificate_info branch April 5, 2019 14:49
@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger you're right! Also, thanks for reviewing!
@gundalow thanks for merging!

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants