Skip to content

Commit 9c48a8a

Browse files
norakoiralamsftnorakoiralamsft
committed
PR comments
1 parent 22ebf30 commit 9c48a8a

2 files changed

Lines changed: 183 additions & 5 deletions

File tree

‎Utils/distroutils.py‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@
2424
pass
2525

2626
if cryptImported == False:
27-
if sys.version_info[0] == 3 and sys.version_info[1] >= 13 or sys.version_info[0] > 3:
27+
# checking for python version
28+
if (sys.version_info[0] == 3 and sys.version_info[1] >= 13) or (sys.version_info[0] > 3):
2829
try:
2930
from legacycrypt import crypt
3031
cryptImported = True
@@ -171,9 +172,6 @@ def change_password(self, user, password):
171172

172173
def chpasswd(self, username, password, crypt_id=6, salt_len=10):
173174
passwd_hash = self.gen_password_hash(password, crypt_id, salt_len)
174-
if passwd_hash is None:
175-
return "This feature requires one of the 'crypt', 'legacycrypt', 'crypt-r', or 'passlib' Python packages to be installed."
176-
177175
cmd = ['usermod', '-p', passwd_hash, username]
178176
ret, output = ext_utils.run_command_get_output(cmd, log_cmd=False)
179177
if ret != 0:
@@ -185,11 +183,18 @@ def gen_password_hash(self, password, crypt_id, salt_len):
185183
salt = "${0}${1}".format(crypt_id, salt)
186184

187185
if cryptImported:
186+
# salt is randomly generated above
187+
# default crypt_id is 6 (SHA-512), see change_password() for details
188188
return crypt(password, salt)
189189
elif passLibImported:
190+
# passlib auto-generates a cryptographically random salt
191+
# no crypt id as this uses SHA-512, so passed in crypt_id will be ignored
190192
return sha512_crypt.hash(password)
191193
else:
192-
return None
194+
raise ImportError(
195+
"Password hashing is unavailable. Install one of: 'crypt' (Python < 3.13), "
196+
"'legacycrypt', or 'passlib'."
197+
)
193198

194199
def create_account(self, user, password, expiration, thumbprint, enable_nopasswd):
195200
"""

‎Utils/test/test_distroutils_password_hash.py‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
#!/usr/bin/env python
2+
#
3+
# Copyright 2026 Microsoft Corporation
4+
#
5+
# Licensed under the Apache License, Version 2.0 (the "License");
6+
# you may not use this file except in compliance with the License.
7+
# You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing, software
12+
# distributed under the License is distributed on an "AS IS" BASIS,
13+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
# See the License for the specific language governing permissions and
15+
# limitations under the License.
16+
#
17+
# Tests for gen_password_hash in GenericDistro (distroutils.py).
18+
#
19+
# Strategy:
20+
# - We cannot compare hashes across libraries because each generates a fresh
21+
# random salt, so hashes will always differ.
22+
# - Instead, we verify each library's hash *verifies* against the original
23+
# password using that same library.
24+
# - We also test that gen_password_hash raises ImportError when nothing is available.
25+
26+
import unittest
27+
import unittest.mock as mock
28+
29+
# ---------------------------------------------------------------------------
30+
# Minimal stub config so GenericDistro can be instantiated without the full
31+
# extension environment.
32+
# ---------------------------------------------------------------------------
33+
class _StubConfig:
34+
def get(self, key):
35+
return None
36+
37+
38+
def _make_distro():
39+
# Import after path manipulation if needed; distroutils expects Utils.*
40+
# to be importable, so run tests from the repo root:
41+
# python -m pytest Utils/test/test_distroutils_password_hash.py
42+
import Utils.distroutils as du
43+
return du.GenericDistro(_StubConfig())
44+
45+
46+
class TestGenPasswordHashWithCrypt(unittest.TestCase):
47+
"""Hash produced by the built-in 'crypt' (or 'legacycrypt') module."""
48+
49+
def setUp(self):
50+
# Skip if crypt / legacycrypt is not available in this environment.
51+
try:
52+
import crypt as _crypt
53+
self._crypt = _crypt.crypt
54+
except ImportError:
55+
try:
56+
from legacycrypt import crypt as _crypt
57+
self._crypt = _crypt
58+
except ImportError:
59+
self.skipTest("Neither 'crypt' nor 'legacycrypt' is available")
60+
61+
def test_hash_verifies_with_crypt(self):
62+
distro = _make_distro()
63+
password = "TestP@ssw0rd!"
64+
hash_val = distro.gen_password_hash(password, crypt_id=6, salt_len=10)
65+
66+
# A valid crypt hash starts with the salt prefix
67+
self.assertTrue(hash_val.startswith("$6$"), "Expected SHA-512 hash prefix '$6$'")
68+
# Verify: re-hashing with the produced hash as the salt must equal the hash
69+
self.assertEqual(self._crypt(password, hash_val), hash_val,
70+
"Hash does not verify against the original password")
71+
72+
def test_different_passwords_produce_different_hashes(self):
73+
distro = _make_distro()
74+
hash1 = distro.gen_password_hash("PasswordOne1!", crypt_id=6, salt_len=10)
75+
hash2 = distro.gen_password_hash("PasswordTwo2!", crypt_id=6, salt_len=10)
76+
self.assertNotEqual(hash1, hash2)
77+
78+
def test_crypt_id_5_produces_sha256_hash(self):
79+
distro = _make_distro()
80+
hash_val = distro.gen_password_hash("TestP@ssw0rd!", crypt_id=5, salt_len=10)
81+
self.assertTrue(hash_val.startswith("$5$"), "Expected SHA-256 hash prefix '$5$'")
82+
83+
84+
class TestGenPasswordHashWithPasslib(unittest.TestCase):
85+
"""Hash produced by passlib (fallback when crypt/legacycrypt unavailable)."""
86+
87+
def setUp(self):
88+
try:
89+
from passlib.hash import sha512_crypt as _sha512
90+
self._sha512 = _sha512
91+
except ImportError:
92+
self.skipTest("'passlib' is not available")
93+
94+
def test_hash_verifies_with_passlib(self):
95+
import Utils.distroutils as du
96+
97+
# Force the passlib path by temporarily patching the module-level flags.
98+
with mock.patch.object(du, 'cryptImported', False), \
99+
mock.patch.object(du, 'passLibImported', True):
100+
distro = du.GenericDistro(_StubConfig())
101+
password = "TestP@ssw0rd!"
102+
hash_val = distro.gen_password_hash(password, crypt_id=6, salt_len=10)
103+
104+
self.assertTrue(hash_val.startswith("$6$"),
105+
"Expected passlib SHA-512 hash prefix '$6$'")
106+
self.assertTrue(self._sha512.verify(password, hash_val),
107+
"Hash does not verify against the original password")
108+
109+
def test_passlib_different_passwords_produce_different_hashes(self):
110+
import Utils.distroutils as du
111+
112+
with mock.patch.object(du, 'cryptImported', False), \
113+
mock.patch.object(du, 'passLibImported', True):
114+
distro = du.GenericDistro(_StubConfig())
115+
hash1 = distro.gen_password_hash("PasswordOne1!", crypt_id=6, salt_len=10)
116+
hash2 = distro.gen_password_hash("PasswordTwo2!", crypt_id=6, salt_len=10)
117+
118+
self.assertNotEqual(hash1, hash2)
119+
120+
class TestCreateAccountPasswordHashFailure(unittest.TestCase):
121+
"""
122+
Verify behavior of create_account and change_password when no hashing
123+
library is available.
124+
125+
gen_password_hash raises ImportError, which propagates through
126+
chpasswd -> change_password -> create_account, causing vmaccess.py to
127+
fail the extension operation via its general except block.
128+
"""
129+
130+
def test_create_account_raises_when_password_hash_unavailable(self):
131+
import Utils.distroutils as du
132+
133+
with mock.patch.object(du, 'cryptImported', False), \
134+
mock.patch.object(du, 'passLibImported', False), \
135+
mock.patch('pwd.getpwnam', side_effect=KeyError), \
136+
mock.patch('Utils.extensionutils.run', return_value=0), \
137+
mock.patch('os.path.isdir', return_value=True), \
138+
mock.patch('Utils.extensionutils.set_file_contents'), \
139+
mock.patch('os.chmod'):
140+
distro = du.GenericDistro(_StubConfig())
141+
with self.assertRaises(ImportError):
142+
distro.create_account(
143+
user="testuser",
144+
password="SomePassword1!",
145+
expiration=None,
146+
thumbprint=None,
147+
enable_nopasswd=False
148+
)
149+
150+
class TestGenPasswordHashNoLibraryAvailable(unittest.TestCase):
151+
"""When no hashing library is importable, gen_password_hash raises ImportError."""
152+
153+
def test_gen_password_hash_raises_when_nothing_importable(self):
154+
import Utils.distroutils as du
155+
156+
with mock.patch.object(du, 'cryptImported', False), \
157+
mock.patch.object(du, 'passLibImported', False):
158+
distro = du.GenericDistro(_StubConfig())
159+
with self.assertRaises(ImportError):
160+
distro.gen_password_hash("SomePassword1!", crypt_id=6, salt_len=10)
161+
162+
def test_chpasswd_raises_when_nothing_importable(self):
163+
import Utils.distroutils as du
164+
165+
with mock.patch.object(du, 'cryptImported', False), \
166+
mock.patch.object(du, 'passLibImported', False):
167+
distro = du.GenericDistro(_StubConfig())
168+
with self.assertRaises(ImportError):
169+
distro.chpasswd("someuser", "SomePassword1!")
170+
171+
172+
if __name__ == '__main__':
173+
unittest.main()

0 commit comments

Comments
 (0)