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

X509 fixes #36898

Merged
merged 5 commits into from
Oct 19, 2016
Merged

X509 fixes #36898

merged 5 commits into from
Oct 19, 2016

Conversation

clinta
Copy link
Contributor

@clinta clinta commented Oct 10, 2016

What does this PR do?

Fixes #36814 as well as a bug reading CRLs and allows customizing CRL digest.

What issues does this PR fix or reference?

#36814

Previous Behavior

Generating CSR's fails on newer versions of OpenSSL. Comparing CRL's fails. And digest for CRLs is only MD5.

New Behavior

CRL digest can be specified if M2Crypto is new enough. CSRs and CRLs work as expected.

Tests written?

No

Please review Salt's Contributing Guide for best practices.

@@ -19,6 +19,7 @@
import re
import datetime
import ast
import pkg_resources
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this is a part of setuptools, which is not a dependency of Salt.

@@ -670,7 +674,7 @@ def create_private_key(path=None, text=False, bits=2048):

def create_crl(path=None, text=False, signing_private_key=None,
signing_cert=None, revoked=None, include_expired=False,
days_valid=100):
days_valid=100, digest=""):
Copy link
Contributor

Choose a reason for hiding this comment

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

Single quotes here please

@@ -774,7 +780,10 @@ def create_crl(path=None, text=False, signing_private_key=None,
key = OpenSSL.crypto.load_privatekey(OpenSSL.crypto.FILETYPE_PEM,
get_pem_entry(signing_private_key))

crltext = crl.export(cert, key, OpenSSL.crypto.FILETYPE_PEM, days=days_valid)
if digest and pkg_resources.get_distribution("pyopenssl").version >= '0.15':
Copy link
Contributor

Choose a reason for hiding this comment

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

Single-quotes please

kwargs['private_key'] = kwargs['public_key']
log.warning("OpenSSL no longer allows working with non-signed CSRs. A private_key must be specified. Attempting to use public_key as private_key")

if 'private_key' not in kwargs not in kwargs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clarify the intention here? I'm having a little trouble parsing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous versions of the OpenSSL API would allow working with CSRs that were not signed. I now suspect that this was not intended behavior, but because it worked I did not require the private key to generate a CSR, only the public key. Now because the CSR needs to be signed the private key must be included, which is why the private_key kwarg was added to the module.

Throughout this module anywhere that a public key is specified, a private key can be passed in in. This works because the public key can always be derived easily from the private key, and allowing this means that users of the module have fewer files to manage on disk and keep track of.

Because of that it is quite possible that the user actually provided a private key via the public_key kwarg. Instead of just erroring out and requiring the user to rewrite their states, I'll first try and see if the public_key kwarg actually does contain a private key, and issue a warning so that they know they need to update their states.

@cachedout cachedout merged commit 6b94153 into saltstack:2016.3 Oct 19, 2016
@cachedout
Copy link
Contributor

Many thanks as always @clinta. We're grateful for all of your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants