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.

Sign up for GitHub

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

Jump to bottom

SNOW-843716: Cryptography library cleanup #1605

Closed
geofft opened this issue Jun 18, 2023 · 9 comments · May be fixed by #1752
Closed

SNOW-843716: Cryptography library cleanup #1605

geofft opened this issue Jun 18, 2023 · 9 comments · May be fixed by #1752
Labels
feature needs triage

Comments

@geofft
Copy link

geofft commented Jun 18, 2023

What is the current behavior?

Hi! It looks like the connector depends on five cryptography-related packages, asn1crypto, cryptography, oscrypto, pyOpenSSL, and pycryptodome.

Would there be interest in consolidating these as far as possible? I think most of this can be consolidated onto just the cryptography library. I'd be happy to contribute a pull request, just wanted to check with you all first. The one I'm least sure about is asn1crypto, but it looks like that one's being used for OCSP support, which cryptography has high-level functions for. From a quick look, everything else seems like stuff I'm pretty sure cryptography can handle.

What is the desired behavior?

Ideally, the connector should depend on a single, well-maintained cryptography library.

How would this improve snowflake-connector-python?

This would reduce maintenance overhead and security risk.

References and other background

No response
@github-actions github-actions bot changed the title Cryptography library cleanup SNOW-843716: Cryptography library cleanup Jun 18, 2023
@sfc-gh-spanaite
Copy link

Thanks for this @geofft.
I think it should fine to submit a PR and we can have a look at it after.
Any thoughts @sfc-gh-achandrasekaran and @sfc-gh-mkeller?

@waltervos
Copy link

If you can get rid of urllib3.contrib.pyopenssl this way, that should also solve #1586

geofft added a commit to geofft/snowflake-connector-python that referenced this issue Jun 23, 2023
@geofft
Remove dependencies on Cryptodome and oscrypto
6910dc5 
The code to use cryptography (which uses OpenSSL) already existed, but
it just wasn't being used by default. Since cryptography is currently a
mandatory dependency, we may as well use it all the time.

Partially resolves snowflakedb#1605/SNOW-843716.
@geofft geofft mentioned this issue Jun 23, 2023
Closed
@kpark-hrp
Copy link

Getting rid of oscrypto with #1616 will be super nice because from oscrypto import asymmetric attempts to import OpenSSL shared libraries which breaks simple Python containers without OpenSSL/LibreSSL.

Wondering if #1616 can be expedited as it will resolve our pain point? @sfc-gh-spanaite

geofft added a commit to geofft/snowflake-connector-python that referenced this issue Oct 4, 2023
@geofft
Remove dependencies on Cryptodome and oscrypto
9f30872 
The code to use cryptography (which uses OpenSSL) already existed, but
it just wasn't being used by default. Since cryptography is currently a
mandatory dependency, we may as well use it all the time.

Partially resolves snowflakedb#1605/SNOW-843716.
@sfc-gh-sfan sfc-gh-sfan mentioned this issue Oct 4, 2023
Draft
7 tasks
@DustinMoriarty
Copy link

DustinMoriarty commented Oct 9, 2023 •
edited

oscrypto has had this very serious bug since August.

wbond/oscrypto#76

The fix was merged. However, the fix was never released to pypi. An issue was raised to request a release. However, there has been no response.

wbond/oscrypto#78

The repo has had almost no commits since then. The repo does not appear to be well supported. It could be that oscrypto is a case of an open source library that just does not have enough maintainers to make it reliable.

@munir0b0t
Copy link

This is also affecting snowsql client. It doesn't start up on Debian 12 Bookworm because of the issues with oscrypto.

@jfo8001
Copy link

jfo8001 commented Oct 12, 2023 •
edited

Debian 12 Bookworm just busted for us too - very hard to work around this issue in the various libraries we use. Would be great to remove dependency on oscrypto.

I have mentioned this in a separate bug, which feels more urgent
#1767

@munir0b0t
Copy link

I managed to get my Debian 12 docker containers working by adding apt-mark hold openssl libssl3 before installing openssl or any package that depends on openssl like git. Not ideal, but might be good enough until the issue is fixed properly. I also created a case with Snowflake Support for this, so hopefully there's some more eyes on this.

@sfc-gh-aling sfc-gh-aling mentioned this issue Oct 20, 2023
Closed
@sfc-gh-aling
Copy link
Collaborator

thanks for all the feedback, we're going to remove the dependency on oscrpyto and pycryptodomex in our next connector release and we will do a release for snowsql accordingly.

I have created an announcement issue: #1781

@sfc-gh-aling
Copy link
Collaborator

we have released v3.4.0 removing oscrypto and pycryptodomex, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature needs triage
Projects
None yet
Milestone
No milestone
8 participants
@waltervos @geofft @DustinMoriarty @sfc-gh-spanaite @sfc-gh-aling @jfo8001 @munir0b0t @kpark-hrp