acme: deprecate gen_ss_cert(), introduce replacement make_self_signed_cert()
#9909
Conversation
wgreenberg
force-pushed
the
fix-9828
branch
4 times, most recently
from
5a72ea8
to
615c3d8
Compare
gen_ss_cert includes deprecated pyOpenSSL API in its function signature, so we can't update it without breaking backwards compatibility. A new function, make_self_signed_cert, with nearly identical signature (except for its use of cryptography's extension types) is added for users to migrate to.
Also generally switch to using `cryptography` types internally where possible, and switch to the new make_self_signed_cert function.
|
aaand this is finally ready for review. Unsure if we need to bump the current pinned |
| csr = crypto.X509Req() | ||
| sanlist = [] | ||
| # builder.sign(...) below will error out if this isn't the correct private key type | ||
| private_key: CertificateIssuerPrivateKeyTypes = cast( |
There was a problem hiding this comment.
The documentation recommends checking the returned key type rather than casting. We know it's only going to be one of two types, right? So we can do the isinstance for those and error out with a helpful message if it's not one of those. Who was yelling about the potential error, mypy? Because I don't know how we would have hit it if we were loading actual key files, that would be weird and bad.
| @@ -410,7 +411,7 @@ class TLSALPN01Response(KeyAuthorizationChallengeResponse): | |||
|
|
|||
| """ | |||
|
|
|||
| ID_PE_ACME_IDENTIFIER_V1 = b"1.3.6.1.5.5.7.1.30.1" | |||
| ID_PE_ACME_IDENTIFIER_V1 = "1.3.6.1.5.5.7.1.30.1" | |||
There was a problem hiding this comment.
hm...not sure we can change this before the major version update? I could see someone calling it and now hitting a type error. isn't all of acme in the public stable interface?
| if ext.get_short_name() == b'UNDEF': | ||
| data = ext.get_data() | ||
| return data == self.h | ||
| crypto_cert = cert.to_cryptography() |
There was a problem hiding this comment.
I wonder if it's worth it to fully switch to using cryptography instead of crypto while we're here (making breaking changes)? The note at the top of pyopenssl's crypto docs page makes it seem like it might all go away at some point, even if it isn't deprecated yet. Especially since we immediately convert to cryptography internally in this method. But maybe not worth completely removing it now, especially all at once. I know this would be a lot more work, really just thinking out loud here.
| extensions: Optional[List[x509.Extension]] = None, | ||
| ips: Optional[List[Union[ipaddress.IPv4Address, | ||
| ipaddress.IPv6Address]]] = None | ||
| ) -> crypto.X509: |
There was a problem hiding this comment.
Similar to my comment above...while we're migrating, could it make sense to return a cryptography.X509.certificate instead in our new method? Or would that be very annoying to thread through things?
| if not isinstance(cryptography_priv, (DSAPrivateKey, RSAPrivateKey)): | ||
| raise errors.Error("key must be a private key") | ||
| cryptography_pub = cryptography_priv.public_key() | ||
| builder = builder.public_key(cryptography_pub) |
There was a problem hiding this comment.
Do we need to explicitly set the version like we do in gen_ss_cert?
| else: | ||
| raise ValueError('Invalid key type: {0}'.format(key_type)) | ||
|
|
||
| with open(key_path, 'wb') as file_h: | ||
| file_h.write(crypto.dump_privatekey(crypto.FILETYPE_PEM, key)) | ||
| # TODO ensure this is exactly the same format that pyOpenSSL uses |
| @@ -67,7 +67,7 @@ ndg-httpsclient = "0.3.2" | |||
| parsedatetime = "2.4" | |||
| pbr = "1.8.0" | |||
| ply = "3.4" | |||
| pyOpenSSL = "17.5.0" | |||
| pyOpenSSL = "24.1.0" | |||
There was a problem hiding this comment.
What new functionality of pyOpenSSL/cryptography are we using that requires us to bump this? I thought the point of this PR was that things were being deprecated in newer versions. Will we no longer work with the old versions after this change?
The goal here was to remove any uses of deprecated pyOpenSSL API, but to do that, we're going to also have to deprecate
gen_ss_cert, whose signature includes deprecated types. This also adds a new function,make_self_signed_cert(), with a nearly identical signature and functionality, but acceptscryptographyextension types.Closes #9828