-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore: Bump cryptography #24657
chore: Bump cryptography #24657
Conversation
updating cryptography package to fix vulnerability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to update the cryptography version in setup.py and then use the following commands to update all other requirement files. pip install -r requirements/integration.txt |
updating cryptography in setup.py
yeah sorry forget to change it in setup.py. have done that now |
I'm not sure if you've run the application with this version of Cryptography, but we've found that it has some prohibitive performance issues. @betodealmeida do you have any suggestions here about how we can keep this versioning flexible so that people can continue to use the current version if they choose? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safety first!
setup.py
Outdated
@@ -78,7 +78,7 @@ def get_git_sha() -> str: | |||
"colorama", | |||
"croniter>=0.3.28", | |||
"cron-descriptor", | |||
"cryptography>=39.0.1, <40", | |||
"cryptography>=41.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put up a warning in the UPDATING.md
file about this, since there are some backwards incompatible changes from 39.x
to 41.x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@betodealmeida good catch! I saw the Python 3.6 version which shouldn't affect Superset since we're already running on a minimum of 3.9, but do you think that the versions for OpenSSL
and LibreSSL
could be an issue? I'm wondering if this change would be considered a potential breaking change for Superset, and if so, if we need to think about cherrying it into 3.0 or wait until 4.0. I checked my local for example, and it seems likely that we could be running LibreSSL < 3.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @michael-s-molina or @rusackas
The big performance hit was from |
Codecov Report
@@ Coverage Diff @@
## master #24657 +/- ##
==========================================
+ Coverage 67.14% 69.00% +1.86%
==========================================
Files 1904 1904
Lines 74106 74106
Branches 8194 8194
==========================================
+ Hits 49755 51140 +1385
+ Misses 22232 20847 -1385
Partials 2119 2119
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 101 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
setup.py
Outdated
@@ -78,7 +78,7 @@ def get_git_sha() -> str: | |||
"colorama", | |||
"croniter>=0.3.28", | |||
"cron-descriptor", | |||
"cryptography>=39.0.1, <40", | |||
"cryptography>=41.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suryadev99 can we reintroduce the lower bound (which could be increased) otherwise one could install a version older than 39.0.1
and thus (in theory) could be exposed to the issue @betodealmeida mentioned in #24657 (comment).
@suryadev99 are you able to address this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking merging for the moment based on #24657 (comment)
There was also a less-blocking comment above about making an addition in UPGRADING.md that would be a nice addition.
Holler if we can help address these issues further.
@rusackas I have made the changes, please do look into it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @suryadev99 for making the changes. There's a couple of minor formatting suggestions I recommended. Once accepted I'll go ahead and merge the PR.
Co-authored-by: John Bodley <[email protected]>
Co-authored-by: John Bodley <[email protected]>
Hey @john-bodley I have incorporated the suggestions |
@rusackas would you mind stamping this as I believe your blocking request has been resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Y
Fixing security vulnerability: updating cryptography package to fix vulnerability
SUMMARY
Updating cryptography package to 41.0.0
TESTING INSTRUCTIONS
just changing the package versions
ADDITIONAL INFORMATION