x509.certificate_managed creates new certificate every run

[OrangeDog]

The state always creates a new certificate before it's confirmed whether one is needed.
If using a signing_policy with copypath, this can quickly fill the directory with thousands of certificates that were never used.

https://github.com/saltstack/salt/blob/v2018.3.4/salt/states/x509.py#L502
https://github.com/saltstack/salt/blob/v2018.2/salt/states/x509.py#L493

I notice on the develop branch, it's now creating two certificates every time.
https://github.com/saltstack/salt/blob/develop/salt/states/x509.py#L533

[Ch3LL]

can you share your state where you are seeing this issue? thanks

[OrangeDog]

Any x509.certificate_managed state with ca_server, such as the documentation examples.

[Ch3LL]

ping @clinta any chance you can take a look here? Are you seeing this same behavior?

[clinta]

I am exploring this. I'm seeing an issue related to order of items in the subject. Right now I'm trying to get the test suite running locally so I can write some tests to expose this.

…

On Mon, Mar 18, 2019, 14:14 Megan Wilhite ***@***.***> wrote: ping @clinta <https://github.com/clinta> any chance you can take a look here? Are you seeing this same behavior? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52167 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADB73QcKhhujsLBWE1yfTcukyRIrNqgtks5vX9eUgaJpZM4btvcs> .

[glynnforrest]

Could this be related to #50734 (short tags vs long tags, e.g. CN vs commonName)?

[OrangeDog]

@glynnforrest no, this issue is about it generating new certificates before it does any checks at all.

[glynnforrest]

Right you are @OrangeDog, I should have looked at the code you linked before commenting.

I've ran into too many bugs with x509.certificate_managed (#39608, #41858) and need to keep moving before 2019.2.1 is released. I'm going to rewrite it as a custom state function.

@Ch3LL would you be interested in a PR that would significantly change the function code if it kept API compatibility and fixed the outstanding issues?

Either way, I will paste the function here when I've got it working.

[glynnforrest]

Update: I've written a replacement with better error messages that's actually working quite well. I'm hoping to deploy it this week, confirm it solves the issues I've been facing, then submit a PR.

[OrangeDog]

I just stopped using copypath to avoid the major issues with this.
If submitting a complete rewrite @glynnforrest then you may also want to look at my solution to #52180

[glynnforrest]

PR is in: #52935

@OrangeDog the rewrite changes how certificates are checked for updates so #52180 may not apply any more. I'd be grateful if you could try out the rewritten function!

[OrangeDog]

Still not resolved. PR is in progress.

[OrangeDog]

@sagetherage that would also be a good time to look at deduplicating between the tls and x509 states and modules.

[s0undt3ch]

@OrangeDog TCP and x509?
Sorry, on the phone so it's hard to look at the relationship on those.

[OrangeDog]

@s0undt3ch whoops, sorry, TLS not TCP.

[OrangeDog]

Just noting the additional irony of this comment

# We don't want generate a new certificate, even in memory,
# for security reasons.

given that it generates an additional cert every run, even in test mode (so twice if pre-requried x509.private_key_managed),