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

shrink bindings more #5356

Merged
merged 3 commits into from Jul 27, 2020
Merged

shrink bindings more #5356

merged 3 commits into from Jul 27, 2020

Conversation

reaperhulk
Copy link
Member

This attempts to remove many of the bindings we do not use in cryptography or pyopenssl. Newer bindings (added by external consumers or for upcoming pyopenssl PRs) were generally not touched. However, supporting consumers we're unaware of with these huge bindings is becoming an untenable burden. If you see this PR in the future and are one of the people affected: we cannot support you if we don't know you exist!

@reaperhulk
Copy link
Member Author

I got tired about 2/3 of the way through ssl.py so there's still some more in there + the X509 bindings to go through.

@alex alex merged commit 84a15eb into pyca:master Jul 27, 2020
@reaperhulk reaperhulk deleted the bindings-less branch July 28, 2020 03:50
RSA *, int);
int RSA_private_encrypt(int, const unsigned char *, unsigned char *,
RSA *, int);
int RSA_public_decrypt(int, const unsigned char *, unsigned char *,
Copy link

Choose a reason for hiding this comment

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

This breaks our application when upgrading to newer cryptography version, we are using this function to interoperate with some odd Java services.

Copy link
Member Author

Choose a reason for hiding this comment

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

Directly using our bindings like this is not really a supported behavior unfortunately. It's not sustainable for us to maintain all possible bindings like this, so we need to move people to supported interfaces. If you're using RSA_public_decrypt it sounds like you, for whatever reason, need to recover the original digest from the RSA signature? #5457 is interested in exposing similar functionality, but no API has been proposed for it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

An API was proposed in issue #5495. Hopefully it solves your needs as long as you are relying on PKCS #1-padding. If you need to support a non-standard padding, additional API extensions are needed (such as adding a RSA_RAW padding type, supported by all RSA functions).

@misterzed88
Copy link
Contributor

Just a crazy thought which goes in the reverse direction of what you are doing here: what if you instead made the OpenSSL bindings through CFFI a supported, low-level interface of the cryptography library, but without the full high-level documentation? You already seem to have exposed this interface in the "Bindings" section of the documentation.

You could simply keep all the current functions (before you started shrinking them). The interface would be stable by never removing anything, only extending. You could even add a special OpenSSL interface version which is updated as more functions are exposed.

Why would you want to do this? To make it possible to use this library for more exotic cases, without cluttering your nice, clean, high-level API.

@reaperhulk
Copy link
Member Author

@misterzed88 A significant part of our pain is testing against a million different OpenSSLs (and its forks), with varying amounts of weird backported patches from distros, with even weirder custom configurations. Maintaining those bindings as public API surface is exceedingly painful and to actually be able to ship functioning software we've discovered that we need to know the consumers of those APIs (so we can figure out what we can remove and what we need to go to extreme lengths to preserve).

This is a long-winded way of saying we're not going to produce a separate bindings package because it's frankly too hard to keep it working to the standard we expect of software we distribute.

The intention of our bindings shrink is also in prep for trying to more directly support BoringSSL, an OpenSSL fork both @alex and myself actually like. We won't be dropping OpenSSL support, but it would be nice to support Boring.

@mhils mhils mentioned this pull request Nov 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 23, 2021
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.

None yet

4 participants