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

Allow x509.certificate_managed to use a CSR #58282

Merged
merged 5 commits into from Sep 9, 2020

Conversation

alxwr
Copy link
Contributor

@alxwr alxwr commented Aug 25, 2020

This capability was present prior to e9e49d1.

What does this PR do?

Bring back signing of CSRs.

What issues does this PR fix or reference?

I don't know of any reported issue.

Previous Behavior

          ID: /path/to/my.crt                                                                                       
    Function: x509.certificate_managed                                                                                                                                          
      Result: False                                                                                                                                                             
     Comment: Attempt 1: Returned a result of "False", with the following comment: "An exception occurred in this state: Traceback (most recent call last):                     
                File "/usr/local/lib/python3.7/site-packages/salt/state.py", line 2154, in call                                                                                 
                  *cdata["args"], **cdata["kwargs"]                                                                                                                             
                File "/usr/local/lib/python3.7/site-packages/salt/loader.py", line 2085, in wrapper                                                                             
                  return f(*args, **kwargs)                                                                                                                                     
                File "/usr/local/lib/python3.7/site-packages/salt/states/x509.py", line 659, in certificate_managed                                                             
                  "public_key or signing_private_key must be specified."                                                                                                        
              salt.exceptions.SaltInvocationError: public_key or signing_private_key must be specified.

New Behavior

CSR is signed. Certificate is created.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No


@glynnforrest This may interest you.

@alxwr alxwr requested a review from a team as a code owner August 25, 2020 22:04
@ghost ghost requested review from krionbsd and removed request for a team August 25, 2020 22:05
@alxwr alxwr force-pushed the allow-csr-in-x509_certificate_managed branch from e9bcf70 to 737bbf6 Compare August 25, 2020 22:26
Copy link
Contributor

@glynnforrest glynnforrest left a comment

Choose a reason for hiding this comment

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

Thanks for the ping @alxwr, I'm sorry that e9e49d1 broke things for you. The fix looks good, though I'd like to see the error message updated please - one of the goals of #52935 was to make the error messages clear and unambiguous. Cheers for adding a test.

I can't comment on the other formatting changes you've made, e.g. {0} -> {}.

salt/states/x509.py Outdated Show resolved Hide resolved
Co-authored-by: Glynn Forrest <glynn@backbeat.tech>
@alxwr
Copy link
Contributor Author

alxwr commented Aug 25, 2020

Thanks for the ping @alxwr, I'm sorry that e9e49d1 broke things for you.

np :-) Thanks for the refactoring and making things easier!

The fix looks good, though I'd like to see the error message updated please - one of the goals of #52935 was to make the error messages clear and unambiguous.

I thought about that and then forgot it. :-) Thanks for the suggestion! I applied it right away.

Cheers for adding a test.

Just recently learned about nox. Great tool!

I can't comment on the other formatting changes you've made, e.g. {0} -> {}.

pre-commit run (as used in CI) made me do it. :-D
(see also #58272 (comment))

@alxwr
Copy link
Contributor Author

alxwr commented Sep 9, 2020

@krionbsd @sagetherage Is there a reason this is not merged? :-) How long do you expect me to keep this PR up-to-date with current developments in master?

@krionbsd
Copy link
Contributor

krionbsd commented Sep 9, 2020

@krionbsd @sagetherage Is there a reason this is not merged? :-) How long do you expect me to keep this PR up-to-date with current developments in master?

sorry for delay, it's in the queue to be merged

@sagetherage
Copy link
Contributor

thank you @krionbsd and yes, queued up for merging, we removed the label Merge Ready as it didn't mean exactly as it is worded so not to confuse, we are using more dynamic indicators for merge status and looking to be explicit and automatic instead. Apologies for the confusion and thank you for the mention! @alxwr

@dwoz dwoz merged commit 881b389 into saltstack:master Sep 9, 2020
28 checks passed
@alxwr alxwr deleted the allow-csr-in-x509_certificate_managed branch September 11, 2020 15:24
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants