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

Convert between pyOpenSSL and cryptography objects #439

Merged
merged 4 commits into from
Jul 29, 2016

Conversation

reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented Mar 11, 2016

I'm putting this up so people can comment on this, but it is obviously incomplete:

  • tests
  • handle case where a user might send a non-openssl cryptography object.
  • conversion routines for CRLs
  • docs (if someone else wants to take a stab at this I'd really appreciate it)

@hynek do we want this in 16.0?

@hynek
Copy link
Contributor

hynek commented Mar 11, 2016

Let’s bikeshed. :)

I’d be for PKey.from_cryptography_key() instead of repeating the target type name?

@hynek
Copy link
Contributor

hynek commented Mar 11, 2016

as for 16.0: if it’s ready by accident, sure why not. but we’re not gonna wait for it.

and please don’t stall 1.3 just to get this in :D

@hynek
Copy link
Contributor

hynek commented Mar 31, 2016

OK we should start moving on this. I’m reluctant to add a 16.1 milestone to it just yet because there might be a smaller release fixing some little warts.

@codecov-io
Copy link

codecov-io commented Jun 4, 2016

Current coverage is 95.03% (diff: 99.12%)

Merging #439 into master will increase coverage by 4.61%

@@             master       #439   diff @@
==========================================
  Files             7         16      +9   
  Lines          2067       5641   +3574   
  Methods           0          0           
  Messages          0          0           
  Branches        343        412     +69   
==========================================
+ Hits           1869       5361   +3492   
- Misses          107        198     +91   
+ Partials         91         82      -9   

Powered by Codecov. Last update add5b07...da01178

@reaperhulk reaperhulk force-pushed the cryptography-converter branch from 51bd110 to 31573fb Compare June 4, 2016 22:30
@hynek
Copy link
Contributor

hynek commented Jun 5, 2016

Do these docs make sense?

@hynek
Copy link
Contributor

hynek commented Jul 1, 2016

Oh gee, I thought we merged this at the sprints.

@reaperhulk
Copy link
Member Author

Looks like I need to rebase it?

@reaperhulk reaperhulk force-pushed the cryptography-converter branch from cdffb80 to 88d05f4 Compare July 1, 2016 16:39
@hynek
Copy link
Contributor

hynek commented Jul 2, 2016

I think I was waiting for a review of the docs? But I’m not sure anymore. :|

@reaperhulk
Copy link
Member Author

I'm okay with the docs. In cryptography we mostly refer to our keys as instances of a type rather than interfaces, but it's not a big deal.

@hynek
Copy link
Contributor

hynek commented Jul 3, 2016

I’d like to do it right so we can refer people to it that want to add bindings.

The reason I've used “interfaces” is that it says “interfaces” in the docs https://cryptography.io/en/latest/hazmat/primitives/asymmetric/rsa/#key-interfaces ?

Summoning @alex for more opinions. :)

@alex
Copy link
Member

alex commented Jul 4, 2016

RSAPrivateKey is an interface, openssl._RSAPrivateKey() is an instance of RSAPrivateKey. We never say that openssl._RSAPrivateKey() is a provider os RSAPrivateKey though.

hth

@hynek
Copy link
Contributor

hynek commented Jul 4, 2016

Hm I guess when we talk about types (:type: / :rtype:), interfaces seems the better term.


Last open question is the second checkbox: do we want to perform type checks?

@Lukasa
Copy link
Member

Lukasa commented Jul 19, 2016

Is there any intention of extending this to map between PyOpenSSL's X509 object and Cryptography's?

@alex
Copy link
Member

alex commented Jul 19, 2016

Yes, we just needed to start somewhere.

On Tue, Jul 19, 2016 at 8:01 AM, Cory Benfield [email protected]
wrote:

Is there any intention of extending this to map between PyOpenSSL's X509
object and Cryptography's?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#439 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAADBIs60UUGF5hiajJWAYXvR2RlHDfcks5qXLyzgaJpZM4HuvDA
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

@hynek
Copy link
Contributor

hynek commented Jul 19, 2016

I would really like to land this but i’m not sure if it’s ready. It would be nice if someone could have a look and unblock us.


.. versionadded:: 16.1.0
"""
pkey = cls()
Copy link
Member

@Lukasa Lukasa Jul 21, 2016

Choose a reason for hiding this comment

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

Is it worth policing this method to make sure the crypto key from cryptography isn't, say, an ECDSA key, or any other unsupported key type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no opinion here. pyOpenSSL doesn't do much enforcement of this type, but we definitely do it in cryptography. I'll add it if you want, just let me know :)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should do it: there'll be surprises if we don't.

@Lukasa
Copy link
Member

Lukasa commented Jul 21, 2016

I am mostly happy with this, though I have one question about whether we should police the type of keys we allow to be passed to the constructor. Otherwise, I am happy. =)

@reaperhulk
Copy link
Member Author

@Lukasa type check added :)

@Lukasa
Copy link
Member

Lukasa commented Jul 29, 2016

\o/ Yay!

@Lukasa Lukasa merged commit 72d968b into pyca:master Jul 29, 2016
@hynek
Copy link
Contributor

hynek commented Jul 29, 2016

first shots fired 😱

@reaperhulk reaperhulk deleted the cryptography-converter branch August 1, 2016 01:07
akgood pushed a commit to akgood/pyopenssl that referenced this pull request Aug 22, 2016
* convert pkey to cryptography keys and vice versa

* pep8 and such

* Add documentation and changelog

* add a type check and verify that it rejects ECDSA keys from cryptography
jsonn referenced this pull request in jsonn/pkgsrc Jan 28, 2017
Add patch that makes tests on NetBSD progress further.
But then there's a segfault. See
pyca/pyopenssl#596

16.2.0 (2016-10-15)
-------------------

Changes:
^^^^^^^^

- Fixed compatibility errors with OpenSSL 1.1.0.
- Fixed an issue that caused failures with subinterpreters and embedded Pythons.
  `#552 <https://github.com/pyca/pyopenssl/pull/552>`_


16.1.0 (2016-08-26)
-------------------

Deprecations:
^^^^^^^^^^^^^

- Dropped support for OpenSSL 0.9.8.


Changes:
^^^^^^^^

- Fix memory leak in ``OpenSSL.crypto.dump_privatekey()`` with ``FILETYPE_TEXT``.
  `#496 <https://github.com/pyca/pyopenssl/pull/496>`_
- Enable use of CRL (and more) in verify context.
  `#483 <https://github.com/pyca/pyopenssl/pull/483>`_
- ``OpenSSL.crypto.PKey`` can now be constructed from ``cryptography`` objects and also exported as such.
  `#439 <https://github.com/pyca/pyopenssl/pull/439>`_
- Support newer versions of ``cryptography`` which use opaque structs for OpenSSL 1.1.0 compatibility.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants