Bug fix for webapp ssh #8324
Bug fix for webapp ssh #8324
Conversation
patricklee2
force-pushed
the
fab
branch
23 times, most recently
from
93b78e5 to
f37a94d
Compare
patricklee2
force-pushed
the
fab
branch
3 times, most recently
from
0c56ae3 to
f39a83c
Compare
patricklee2
force-pushed
the
fab
branch
5 times, most recently
from
2c8c6b0 to
a398930
Compare
patricklee2
changed the title
patricklee2
force-pushed
the
fab
branch
3 times, most recently
from
b1eb936 to
28fb93c
Compare
|
@yugangw-msft @panchagnula @yiliaomsft @rramachand21 |
yugangw-msft
left a comment
yugangw-msft
left a comment
There was a problem hiding this comment.
Overall LGTM, the main question is on why pinning the cryptography version.
There was a problem hiding this comment.
I would suggest you log an verbose entry to document the exception details: logger.info(ex). My concern is catching generic exceptions above could swallow non connection related errors which ends up making the diagnosis pretty hard.
There was a problem hiding this comment.
cryptography is a pretty common dependency, why we need to pinning to a specific version?
There was a problem hiding this comment.
paramiko, (one of the dependency) is still using a deprecated method.
as a result, these warnings appear, and I couldn't suppress them
the fix was to pin the version.
paramiko/paramiko#1369
/home/patle/azure-cli/env/lib/python3.7/site-packages/paramiko/kex_ecdh_nist.py:39: CryptographyDeprecationWarning: encode_point has been deprecated on EllipticCurvePublicNumbers and will be removed in a future version. Please use EllipticCurvePublicKey.public_bytes to obtain both compressed and uncompressed point encoding.
m.add_string(self.Q_C.public_numbers().encode_point())
/home/patle/azure-cli/env/lib/python3.7/site-packages/paramiko/kex_ecdh_nist.py:96: CryptographyDeprecationWarning: Support for unsafe construction of public numbers from encoded data will be removed in a future version. Please use EllipticCurvePublicKey.from_encoded_point
self.curve, Q_S_bytes
/home/patle/azure-cli/env/lib/python3.7/site-packages/paramiko/kex_ecdh_nist.py:111: CryptographyDeprecationWarning: encode_point has been deprecated on EllipticCurvePublicNumbers and will be removed in a future version. Please use EllipticCurvePublicKey.public_bytes to obtain both compressed and uncompressed point encoding.
hm.add_string(self.Q_C.public_numbers().encode_point())
There was a problem hiding this comment.
would be reasonable to have "<2.5.0"?
There was a problem hiding this comment.
Also log an issue to track to upgrade paramiko when it rolls out the fix. Please assign that issue to both you and me
There was a problem hiding this comment.
changed it to <2.5.0
There was a problem hiding this comment.
The <2.5.0 now causes a conflict #9629. Please update.
patricklee2
force-pushed
the
fab
branch
4 times, most recently
from
243e9ed to
e18cf7e
Compare
| time.sleep(1) | ||
| try: | ||
| c.run('cat /etc/motd', pty=True) | ||
| c.run('source /etc/profile; /bin/ash', pty=True) |
There was a problem hiding this comment.
this is supposed to be /bin/bash right?
In Azure#8324, a new contraint was create that put a ceiling on the version of cryptography that we can support. However, that constraint never made its way to our requirements.txt file in our Homebrew build. Because the largest version at the time was 2.4.x, there wasn't a problem up to a few days ago when 2.5 was finally available for download in our CI environment. Because there is another library that requires cryptography, and doesn't have this constraint, and because we do not use `pip freeze` or `pipenv` to lock our dependencies ahead of time, pip greedily installs the largest version available when it first sees the cryptography requirement then fails when it gets to appservices.
In #8324, a new contraint was create that put a ceiling on the version of cryptography that we can support. However, that constraint never made its way to our requirements.txt file in our Homebrew build. Because the largest version at the time was 2.4.x, there wasn't a problem up to a few days ago when 2.5 was finally available for download in our CI environment. Because there is another library that requires cryptography, and doesn't have this constraint, and because we do not use `pip freeze` or `pipenv` to lock our dependencies ahead of time, pip greedily installs the largest version available when it first sees the cryptography requirement then fails when it gets to appservices.
This corresponds to Azure#9638, and is related to Azure#8324
4 participants
fixes #8282 #8281, #8280 , #8279
uses fabric ssh library which as a BSD 2-clause license
Added license at src/command_modules/azure-cli-appservice/azure/cli/command_modules/appservice/fabric_license
im not sure if that is the correct place top out it.
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.