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 [email protected] 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 https://github.com/saltstack/salt/issues/52167#issuecomment-474039282, or mute the thread https://github.com/notifications/unsubscribe-auth/ADB73QcKhhujsLBWE1yfTcukyRIrNqgtks5vX9eUgaJpZM4btvcs .

[th31nitiate]

I believe that this issue is not only specific to remote signing but it occurs when local singing also. This means that even when you generate a certificate locally it still identifies it as needing a new certificate. I have attached an image of the diff identified when running the state. The differences seem to be key size and version number. When working with the generation of certificate it does not through duplicate the CA certificates, it just duplicates certificates generated by a CA.

It is strange because this issue was not present within previous version as far as I am concerned.

Workaround:

• Make sure you use the UNLESS requisite command with some script that performs the diff check on your be halve.
• you can use with openssl to check the value you need or python script, the only requirement is that it returns False on failure/0 or True of success/1

[Ch3LL]

thanks for that follow up information @th31nitiate

@clinta any luck with the test suite? anything i can help with?

[OrangeDog]

@th31nitiate that's a different problem, possibly #52180

This issue is about it generating new certificates (which are all saved to disk) that are never used because the current certificate doesn't need changing.

[glynnforrest]

Could this be related to https://github.com/saltstack/salt/pull/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 (https://github.com/saltstack/salt/issues/39608, https://github.com/saltstack/salt/issues/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: https://github.com/saltstack/salt/pull/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!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[OrangeDog]

Still not resolved. PR is in progress.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[iavael]

Up

[sagetherage]

ok this shouldn't get hit by the stale bot again and there are changes requested on the PR, but I rebased it against master @glynnforrest when you get time can you take a look at the changes, sometimes we don't hit resolved so I may be off, but attempting to move this along.

[glynnforrest]

Thanks for the nudge @sagetherage. I did some more work on #52935 today and rebased onto master. Unfortunately that PR will not fix this issue, but if it gets merged I'd like to turn my attention to fixing this one afterwards.

[OrangeDog]

Generating new certificates you don't need, whether they stay in memory or are saved to disk, can be considered a security issue. In a strict environment, every one would need to be audited and added to the CRL before being discarded.

An alternate perspective, and my original complaint, sees the audit system (i.e. copypath) quickly fill up with irrelevant certificates that were never used.

The whole approach of the state needs to change - check the current certificate details first, not create a new one and compare the fields.

---

@sagetherage can we hide the various comments above that were talking about separate issues (e.g. comparisons not working)? That should avoid confusion of which PR solves what in future.

[glynnforrest]

The whole approach of the state needs to change - check the current certificate details first, not create a new one and compare the fields.

Agree 100% @OrangeDog, the current approach really isn't great. However, I feel it's too much to change at once to include in #52935. Let me be clear though - if it gets merged, I will fix this issue shortly afterwards.

[glynnforrest]

Can this be reopened please? #52935 wasn't intended to fix this.

[sagetherage]

looks like we have a separate issue so assigning the same Open Core Team member for this as well

[sagetherage]

this is much bigger and needs to be done in major release so leaving it in Magnesium for now and adjusting up the severity

[tonyplovich]

It was suggested in one of the above comments to use openssl. I pieced together the following bash hackery from a few stack exchange posts and figured I'd share it.

/SOME/PATH/cert.pem:
  x509.certificate_managed:
    # https://github.com/saltstack/salt/issues/52167
    - unless:
      # Will trigger 5 days (432000 sec) from cert expiration
      - 'enddate=$(date -d "$(openssl x509 -in /SOME/PATH/CERT.PEM -enddate -noout | cut -d= -f2)" +%s) ; now=$(date +%s) ; expire_date=$(( now + 432000)); [ $enddate -gt $expire_date ]'

[sagetherage]

We discussed this today and decided this module needs to be rewritten. That is too big to get into Aluminium but we will start planning this work now for Silicon thank you @OrangeDog and I apologize we didn't get to this sooner.

[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.

[dgengtek]

I used the following patch to fix the issue for me in the past. Still using this patch. Patch is not on HEAD

---
 salt/modules/x509.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/salt/modules/x509.py b/salt/modules/x509.py
index 0909bace48..d73ce491fa 100644
--- a/salt/modules/x509.py
+++ b/salt/modules/x509.py
@@ -1988,7 +1988,11 @@ def will_expire(certificate, days):
             _check_time = datetime.datetime.utcnow() + datetime.timedelta(days=days)
             _expiration_date = cert.get_not_after().get_datetime()

-            ret["cn"] = _parse_subject(cert.get_subject())["CN"]
+            subject = _parse_subject(cert.get_subject())
+            if "CN" in subject:
+                ret["cn"] = subject["CN"]
+            else:
+                ret["cn"] = subject["commonName"]

             if _expiration_date.strftime("%Y-%m-%d %H:%M:%S") <= _check_time.strftime(
                 "%Y-%m-%d %H:%M:%S"
--