Rewrite x509.certificate_managed to be easier to use

[glynnforrest]

What does this PR do?

See #52167

After running into show-stopping bugs with x509.certificate_managed I decided to rewrite it. I have been running this function for the past 2 days with positive results.

What issues does this PR fix or reference?

#52180, #39608, #41858 and others:

• Error messages from the x509 module calls are written directly to the certificate file - fixed, the certificate file is only created when the x509 module calls succeed.
• Certificates are created when no changes are required - fixed, the comparison logic has been updated.

EDIT: this does not attempt to correct #52167 - using a signing policy with copypath enabled. That is an issue with calling x509.create_certificate with testrun set to True. I will not be modifying the x509 execution module in this PR, just the state module.

Other improvements:

• Support for test=True
• Detailed messages explaining why the certificate needs to be changed (change in certificate information, cert has expired, not signed by the correct issuer, etc)
• The files specified in append_certs are checked to be valid certificates before appending them
• Better error messages when something goes wrong

I decided to remove the managed_private_key option after reading this comment. Its removal significantly reduces the complexity of the function, and it can easily be replaced by an extra x509.private_key_managed call.

Screenshots! As well as showing the changes to the certificate information, it shows the reason the cert was updated.

test=True:

Tests written?

No

This is my first time writing code for salt and can't get the testsuite running (starting the salt-minion hangs). If I can get it working I'd be keen to add more tests covering this module.

Commits signed with GPG?

Yes

[glynnforrest]

One thing not fixed in this rewrite, the requirement of a dummy subjectAltName: #51869 (comment)

That seems to be an issue with the x509 execution module.

[Ch3LL]

@glynnforrest thanks for this :) . I'm more than willing to help you get the tests running. If you haven't seen it already we have some test documentation here: https://docs.saltstack.com/en/latest/topics/tutorials/writing_tests.html Can you explain what you are attempting to run when you run into the issue when the salt-minion hangs? Also feel free to ping me in the community slack, my name is also ch3ll there and I can walk you through it.

also @clinta can we get your review here?

[alxwr]

@Ch3LL I too fail to run the test suite: #52456 (comment)

[Ch3LL]

@alxwr i responded in the other issue

[clinta]

@Ch3LL My first look at this and it looks good.

I'll add another voice to the frustrations in running tests. I wanted to add a lot more x509 tests so that some of these bugs could be nailed down, but I've not been able to run them either. I think that posted documentation is out of date. See #51136.

I also agree with the idea of deprecating managed_private_key, that complicated things far more than was necessary.

[Ch3LL]

@clinta thanks for the review and for bringing that issue to my attention. I went ahead and assigned myself, although I might not be able to get to it till next week. I do agree that we should definitely add some details about kitchen-salt as it makes things much easier, but a user can still run the test runner without using kitchen-salt. Can you elaborate on what your particular issues were when attempting to run the tests? In the meantime I'll try to find some time next week to push out more details about kitchen-salt so its easier to setup the tests and run them.

[glynnforrest]

Apologies for going off the grid. I'm still keen to get this tested well.

@Ch3LL My approach so far is to try and get runtests.py --name=integration.states.test_x509 working. After installing the various requirements with pip I'm running into Python 2/3 issues. I haven't figured out a way to get m2crypto installed on python 3, and the test bootstrap doesn't seem to work on python 2.

Run with python3 - test suite bootstraps OK, but both x509 tests are skipped because m2crypto isn't available. I can't figure out how to get it installed with pip3. It looks like python3 compatibility has been an issue in the past, here's an issue @thatch45 commented on: https://gitlab.com/m2crypto/m2crypto/issues/114

Run with python2 - test suite has errors at the bootstrap stage:

Process MWorker-2:
Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/home/glynn/Desktop/salt/salt/utils/process.py", line 765, in _run
    return self._original_run()
  File "/home/glynn/Desktop/salt/salt/master.py", line 1141, in run
    self.__bind()
  File "/home/glynn/Desktop/salt/salt/master.py", line 1033, in __bind
    req_channel.post_fork(self._handle_payload, io_loop=self.io_loop)  # TODO: cleaner? Maybe lazily?
  File "/home/glynn/Desktop/salt/salt/transport/zeromq.py", line 704, in post_fork
    self.stream = zmq.eventloop.zmqstream.ZMQStream(self._socket, io_loop=self.io_loop)
  File "/usr/lib/python2.7/dist-packages/zmq/eventloop/zmqstream.py", line 114, in __init__
    self._init_io_state()
  File "/usr/lib/python2.7/dist-packages/zmq/eventloop/zmqstream.py", line 535, in _init_io_state
    self.io_loop.add_handler(self.socket, self._handle_events, self._state)
  File "/home/glynn/.local/lib/python2.7/site-packages/tornado/ioloop.py", line 910, in add_handler
    self._impl.register(fd, events | self.ERROR)
TypeError: argument must be an int, or have a fileno() method.

this repeats with various worker numbers (Process MWorker-0, Process MWorker-1, etc) until I kill the process.

Have you got a preferred way to update stale pull requests? Just merge develop?

@clinta I appreciate you taking a look over this. Glad to hear you support the removal of managed_private_key.

[dmurphy18]

@gyroplast Which version of m2crypto are you having issue with on Python 3 ?, have v0.31. and v0.33.on Python 3 running fine (RHEL 7). What is the the OS the problem is occurring on ?

[glynnforrest]

Which version of m2crypto are you having issue with on Python 3 ?, have v0.31. and v0.33.on Python 3 running fine (RHEL 7). What is the the OS the problem is occurring on ?

@dmurphy18 I don't really understand python packaging, so hopefully doing something wrong that can be fixed easily.

Debian Stretch, python3 --version is 3.5.3. I'm trying to install m2crypto with pip3 install m2crypto. At first it complained about missing openssl headers, so I installed Debian's libssl-dev package. Now it fails, something about 'swig'?

I'm happy to get a VM or vagrant box going if that'll make things easier.

[dmurphy18]

@glynnforrest I cannot find openssl/err.h on Debian 9 either, not even with backports enabled, however it is there on my Ubutnu 18.04.2
root@david-lap2:/home/david# l /usr/include/openssl/err.h
-rw-r--r-- 1 root root 11K Dec 5 2018 /usr/include/openssl/err.h

openssl:
Installed: 1.1.0g-2ubuntu4.3
Candidate: 1.1.0g-2ubuntu4.3
Version table:
*** 1.1.0g-2ubuntu4.3 500
500 http://us.archive.ubuntu.com/ubuntu bionic-updates/main amd64 Packages
500 http://security.ubuntu.com/ubuntu bionic-security/main amd64 Packages
100 /var/lib/dpkg/status
1.1.0g-2ubuntu4 500
500 http://us.archive.ubuntu.com/ubuntu bionic/main amd64 Packages

Sorry I can only suggest Ubuntu instead of Debian for this test at the moment. Will look further in the morning

[OrangeDog]

With all the test work for 2019.2.1 it's probably worth trying to redo this on that branch? I'm also happy to test something manually.

[glynnforrest]

OK, I'd be happy to rebase on the 2019.2.1 branch.

With all the test work for 2019.2.1

Can you elaborate, are the tests going to be easier to run? Is there an issue I could take a look at?

I did manage to get the suite running in an Ubuntu VM. Not giving up on this!

[OrangeDog]

@glynnforrest
I don't know any specifics, but that is the reason given for why it took seven months to get the first bugfix release out.

[Ch3LL]

@glynnforrest due to this new salt enhancement proposal saltstack/salt-enhancement-proposals#20 we are merging everything into master. Do you mind migrating this PR over to the master branch?

Also, we recently migrated to nox which should handle the requirements for you.

To run the tests with nox, should be as easy as

1. pip install nox-py2 (even if your on py3)
2. nox -e 'runtests-zeromq-2.7(coverage=False)' -- -n integration.states.test_x509

[glynnforrest]

Do you mind migrating this PR over to the master branch?

Thanks @Ch3LL, I've been working on rebasing this branch onto master but it's a bit messy. There's some work on develop that's not on master, notably the ext_mapping option added by #49102:

• develop (ext_mapping option included)
• master (ext_mapping option not included)

Looks like #49102 is ported to master in #54584. What would you like me to do?

EDIT: sorry, ignore me, that option is for csr_managed. I've rebased and squashed everything to one commit branching from master. I'll try to get some tests written this week.

[Ch3LL]

thanks for doing that i really appreciate it. let me know when its ready for review or if you have any other test questions.

[TOoSmOotH]

Any idea if this will make it in any time soon?

[Ch3LL]

looks like we are just waiting on some tests getting written. anything i can help with @glynnforrest

[glynnforrest]

looks like we are just waiting on some tests getting written. anything i can help with @glynnforrest

Thanks for waiting, it's been hard to find the time to figure out how the tests work. I've written a couple of tests for a self signed cert, can you take a quick look @Ch3LL before I write any more? I'd like to test this pretty thoroughly but would like some initial feedback.

[Ch3LL]

yeah let me give it a review

[Ch3LL]

thanks for rebasing and posting that stacktrace. I created this issue #56603 so we can try to troubleshoot that there instead of adding noise to this PR.

[Ch3LL]

ping @waynew can we get your re-review here and this will be ready to go

[waynew]

🚀 Thanks for working through that, looks great!

[glynnforrest]

Thanks @waynew and @Ch3LL, great news! Want me to do anything with the outdated branch? Rebase or merge master?

[waynew]

@glynnforrest Should be OK, I think we've been merging when there aren't any merge conflicts, just due to the length of time the test runs take.

We have some plans in place to reduce that here in the near future, but for now I don't think there's anything you need to do.

[hatifnatt]

I found a problem with this updated state. It does not check is any file.managed options that are changed. I.e. if I change key / cert file properties like user, group, mode etc. no changes will be generated.

[glynnforrest]

Thanks @hatifnatt, I'll get a fix for that written soon.

[glynnforrest]

Can you try #56788 @hatifnatt?

[hatifnatt]

@glynnforrest ok, I will try tomorrow.

[hatifnatt]

I need to correct my previous statement, changing key file paramaters are correctly generate changes after this PR, in my case state was not executed because I have prereq requisite defined. Sorry for this.
But issue with certificate file parameters is still the case.