Allow custom validity dates over new X509 certificates

[jeduardo]

What does this PR do?

This PR makes it possible to issue certificates with custom dates using the Salt X509 support by specifying the not_before and not_after attributes to new certificates.

What issues does this PR fix or reference?

#53148

Previous Behavior

not_before and not_after attributes are ignored by the Salt x509 module and valid dates for a X509 certificate are always calculated from a dynamic calculation.

New Behavior

If not_before and not_after are specified, they are used as validity dates when issuing the new X509 certificate.

Tests written?

Yes

Commits signed with GPG?

No

[twangboy]

Also, can we get some tests for this please?

[Ch3LL]

Can you add some tests for these new kwargs. Thanks :)

[jeduardo]

@twangboy @Ch3LL sure, will look into adding documentation and tests. Thanks for the initial review!

[jeduardo]

@twangboy @Ch3LL I've added comments and tests where both were missing. Could you please give it another look?

[Ch3LL]

ping @clinta can you give this PR a review as well? thanks

[Ch3LL]

FYI I migrated this PR from develop to neon to ensure it is included in the upcoming neon release. Let me know if this caused any issues. Thanks

[jeduardo]

@Ch3LL Works On My Machine(tm) so I think it's all fine!

[Ch3LL]

ping @twangboy just waiting on your re-review

and if we could also get a review from @clinta that would be ++

[jeduardo]

@twangboy ping :)

[waynew]

Found a couple of more spots that need some attention.

[jeduardo]

Hey @waynew, it took me a while to find time to look into this but I've applied all of your suggestions to the merge request. Could you please give it another look?

[jeduardo]

Ping... @waynew @Ch3LL

[Ch3LL]

hey @jeduardo sorry for the late reply. We just recently changed our branch and releasing strategy as detailed in this SEP: saltstack/salt-enhancement-proposals#20

Any chance you would be willing to port this PR over to the master branch, since we will be releasing from this branch going forward. If not I can help you port this over there.

[jeduardo]

Hey @Ch3LL. Not a problem, the fixes are not that many. I've ported them all to the master branch and changed the PR to point to master instead of Neon.

Whilst tests are Working On My Machine(tm), I've noticed that the CSR tests that were present in the Neon branch are no longer present in master. They have nothing to do with this fix but I thought it would be useful to mention it.

I would also ask for you to check if the versionadded comment for the new properties still applies and, if so, if it's correct or not.

Finally, Github seems to be complaining about many conflicts that must be resolved that seem to be caused by all the branch-changing and porting.

Could you please give it a pass and tell me if any changes are needed?

[jeduardo]

Hey @Ch3LL, no idea what the black tool is and which pre-commit hooks you are using now, I've been unfortunately a bit away from playing more with the Salt build process since I switched jobs. Happy to learn more about these though if you have information to share. :)

In the meanwhile I've managed to perform the merge to master locally on my machine and all local test/lint runs completed fine, so I think this is resolved. Let's see what the automated tests will say after they're done.

[Ch3LL]

thanks for doing that. it looks like the pre-commit is still failing.

There are some docs here: https://docs.saltstack.com/en/latest/topics/development/contributing.html#quickstart on how to get it setup. But essentially you just need to install/setup pre-commiit as per those docs. And then when you do a git commit it should automatically run the pre-commit hooks. Looks like in your instance isort and black are failing. The black tool is a python code formatter and isort sorts imports in the same way across all files. If you take a look at the jenkins output here: https://jenkinsci.saltstack.com/job/pr-pre-commit/job/PR-53149/4/console it actually provides you the diff if you want to just take that approach as well.

[jeduardo]

@Ch3LL thanks for the helpful information! I was able to get pre-commit running here at my workstation and got some promising results:

isort....................................................................Passed
black....................................................................Passed
Lint Salt................................................................Passed
Lint Tests...............................................................Passed

Luckily this time everything will work fine. Fingers crossed here.

[Ch3LL]

thanks for doing that. Looks like there is a merge conflict. I dont believe those doc test failures are related. When i rebased your PR on the head of master they started passing.

[jeduardo]

Hey @Ch3LL, I was able, again, to merge the code on my machine but found that unfortunately some of the logic I needed to add to salt/states/x509.py causes a syntax error, as it depends on variable called current_days_remaining that no longer exists in the current version of the code in the certificate_managed function. It looks to me that some of it was recently rewritten.

I will need to have a deeper look at the current version of the code and most likely run it in a test instance to see if the changes are still required. I am not confident that just the merge will result in a working state and won't create a bigger bug.

[Ch3LL]

let me know if i can help at all. thanks for diving in

[jeduardo]

Hey @Ch3LL, I've added some integration tests covering the situations when someone specifies not_before, not_after, or both of them, exercising the state in addition to the unit tests covering the module.

I've executed these tests before and after the merge and got the same results in both of them, so I think the logic I had in the state can be safely discarded. Waiting for the automatic checks to run now and crossing fingers again.

[jeduardo]

Autochecks look fine!

[dwoz]

@waynew Are you good with this now?

[Ch3LL]

just one more small change

[Ch3LL]

managed_private_key was removed on purpose here: https://github.com/saltstack/salt/pull/52935/files#diff-5499a295a50d60a761c34f4080e4014bL80 in favor of using the separate x509.private_key_managed state. Can you remove the reference here and on line 175.

[jeduardo]

Hey @Ch3LL, good catch. I remember seeing the change in the code but forgot to look for it in the docs. Just removed it, thanks!

[Ch3LL]

ping @waynew just need your review and we can get this one in :)

[waynew]

Excellent! Thanks for addressing those changes!

[jeduardo]

Thanks all for merging it :)