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 elliptic curve keys in from_cryptography_key(). #636

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

theno
Copy link

@theno theno commented Jun 10, 2017

With this change an (Certificate Transparency) SCT can be verified
against the public key of a CT-Log. CT-Logs usually are signed with an
elliptic curve digest. The argument cert of the function
verify(cert, signature, data, digest) is just a wrapper for its pkey
attribute. This attribute now can contain an ec-pubkey. Here is an example
of this usage:
https://github.com/theno/ctutlz/blob/60cd6b9aeee7a7c6f0f90b0262a306a292808985/ctutlz/sct/verification.py#L29

Elliptic curve support in PKey objects still needs to be implemented.

Elliptic curve support in PKey objects still needs to be implemented.

With this change an (Certificate Transparency) SCT can be verified
against the public key of a CT-Log. CT-Logs usually are signed with an
elliptic curve digest.  The argument `cert` of the function
`verify(cert, signature, data, digest)` is just a wrapper for its `pkey`
attribute.  This attribute now can contain an ec-pubkey.  Here is an example
of this usage:
https://github.com/theno/ctutlz/blob/master/ctutlz/sct/verification.py#L29
@Lukasa
Copy link
Member

Lukasa commented Jun 10, 2017

Thanks for this! However, I think you're approaching this backwards. Without support for EC keys in PKey, I don't think we should allow them to be loaded: I'd much rather see EC PKey support added first.

I'd even more rather see SCR verification land in cryptography, as this seems right up @alex's street.

`from_cryptography()` now accepts EC Pkeys and does not raise an TypeError.
@codecov
Copy link

codecov bot commented Jun 10, 2017

Codecov Report

Merging #636 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   96.78%   96.69%   -0.09%     
==========================================
  Files          18       18              
  Lines        5625     5625              
  Branches      390      390              
==========================================
- Hits         5444     5439       -5     
- Misses        121      125       +4     
- Partials       60       61       +1
Impacted Files Coverage Δ
tests/test_crypto.py 98.38% <100%> (-0.2%) ⬇️
src/OpenSSL/crypto.py 96.21% <100%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9280c5...6e7c49d. Read the comment docs.

@theno
Copy link
Author

theno commented Jun 10, 2017

For convenience I marked the failing test to be skipped when running pytest to make the unit tests passing.

This even makes the arguments of Lukasa more obvious.

theno added a commit to theno/ctutlz that referenced this pull request Jun 11, 2017
It is a modified version of `OpenSSL.crypto.PKey.from_cryptography_key()`
which also accepts EC Keys.

Using this function, a patched PyOpenSSL is no longer required, which
makes it possiblel to remove the `--process-dependency-links` argument
at the pip-install.

There is a pull request for `OpenSSL.crypto.PKey.from_cryptography_key()` of
PyOpenSSL to also accept EC Keys, but there are comprehensible reasons not to
accept this request in the near future -- or at all:
pyca/pyopenssl#636
@ronf
Copy link

ronf commented Jul 11, 2017

I was just about to submit a pull request very similar to this. I have a need for this functionality in my AsyncSSH package. I'm in the middle of adding support for X.509 certificates in SSH and I have things working for RSA certificates. However, I ran into this issue when attempting to use EC certificates.

As it turns out, EC certificates work just fine when you feed them to OpenSSL.crypto.load_certificate(). However, to support generating certificates, I need to be able to convert a cryptography EC key to a PKey, and it looks like these explicit checks are the only thing stopping that from working.

While it's true that PyOpenSSL doesn't support creating new EC PKeys directly yet, I don't need that in my application and supporting conversion from already constructed PyCA keys would be a good first step.

I might be able to work around this by generating a certificate using the native PyCA X.509 support and then using OpenSSL.crypto.load_certificate() to get a PyOpenSSL certificate object out of that. However, I'd prefer not to have to do that.

Long term, I'd love to convert everything over to PyCA and not depend on PyOpenSSL at all, but I can't do that until PyCA adds support for X.509 certificate chain validation.

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

Successfully merging this pull request may close these issues.

3 participants