Conversation
norakoiralamsft
requested review from
a team,
D1v38om83r and
nkuchta
as code owners
| pass | ||
|
|
||
| if cryptImported == False: | ||
| if sys.version_info[0] == 3 and sys.version_info[1] >= 13 or sys.version_info[0] > 3: |
There was a problem hiding this comment.
would be good to explain what these numbers mean or rather what this expression means
|
|
||
| if cryptImported: | ||
| return crypt(password, salt) | ||
| elif passLibImported: |
There was a problem hiding this comment.
This shouldn't ignore crypt_id and salt. This could result in a bug depending on n distro configuration?
There was a problem hiding this comment.
+1, I think it would be a good idea to get the hashes for all scenarios/libraries first and confirming they all match prior to merging. (i.e. assuming they are using sha512_crypt and passing in salt).
There was a problem hiding this comment.
The crypt_id defines the hashing algorithm to use and is included as part of the salt parameter in the call to crypt.crypt (notice that it is prepended to the salt). For some reason, the WALinuxAgent allowed users to set the crypt_id in waagent.conf; I am assuming that VmAccess is using legacy code that reads this value from waagent.conf.
@norakoiralamsft - For WALinuxAgent, we decided to make this change (see the README in the develop branch, marked with ******* below):
Provisioning.PasswordCryptId
Type: String
Default: 6
Algorithm used by crypt when generating password hash.
******This parameter, as well as PasswordCryptSaltLength, is used only on Python versions previous to 3.13, and SHA-512 is the default for most distros. From Python 3.13 onwards, SHA-512 is always used, regardless of the value of this parameter.*******
1 - MD5
2a - Blowfish
5 - SHA-256
6 - SHA-512
I think that would be appropriate for VmAccess as well, but pointing it out in case you want to follow a different approach.
There was a problem hiding this comment.
The call to passlib.hash() is not given a salt. One will be generated automatically and that is the recommended usage.
| pass | ||
|
|
||
| if cryptImported == False: | ||
| if sys.version_info[0] == 3 and sys.version_info[1] >= 13 or sys.version_info[0] > 3: |
There was a problem hiding this comment.
add braces for sys.version_info[0] == 3 and sys.version_info[1] >= 13 ? (for easier readability)
| from passlib.hash import sha512_crypt | ||
| passLibImported = True | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
If all 3 imports fail, do we need to throw an error here + log ?
There was a problem hiding this comment.
This condition (all 3 imports failing) needs to be handled, but raising an exception here would require all modules depending on this module, directly or indirectly, to handle that exception, or the extension would crash.
@norakoiralamsft - in the original PR, an exception is raised on usage of these functions if no module was imported. I see that you are handling this at line 192 by returning None. Do verify that this error handling is appropriate for your usage.
8 participants
This PR is aimed to add fallbacks for importing the cypt library with was deprecated in python 3.11, takes a similar approach as to Linux Agent, first trying to import crypt (which includes crypt + crypt_r), legacycrypt, and finally passlib.
This was chosen over the crypt_fallback that was introduced in this PR because we want to continue supporting older distros that may not have cyptes available, and Canocial raised concerns about the crypt_r lib as well, and have ensured that passlib will be available on Ubuntu.