Skip to content

tests: when generating crypted password, generate in target env#1252

Merged
blackboxsw merged 1 commit into
canonical:mainfrom
blackboxsw:tests-crypt-on-target-distro
Feb 14, 2022
Merged

tests: when generating crypted password, generate in target env#1252
blackboxsw merged 1 commit into
canonical:mainfrom
blackboxsw:tests-crypt-on-target-distro

Conversation

@blackboxsw
Copy link
Copy Markdown
Collaborator

This resolves AssertionError: assert None == '$y$j9T$la3Xe...iM0xEOjQwtjxB' failures on our bionic Jenkins test runner when exercising lxd_container jammy-daily jobs

Proposed Commit Message

There are inconsistencies for cryptographic libraries across
major distribution releases.

From a bionic host, attempting run run crypt.crypt locally using
salt and format info from a Jammmy /etc/shadow file will result in
a failure to produce an encrpted password.

To avoid inconsistencies of python cryptographic libs across Linux
releases, perform the password encryption on the system under test.

Additional Context

Test Steps

From a bionic host system

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

There are inconsistencies for cryptographic libraries across
major distribution releases.

From a bionic host, attempting run run crypt.crypt locally using
salt and format info from a Jammmy /etc/shadow file will result in
a failure to produce an encrpted password.

To avoid inconsistencies of python cryptographic libs across Linux
releases, perform the password encryption on the system under test.
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but flake8 failure due to crypt import no longer needed.

Suggested condensed first line of commit:
tests: generate crypted password in target env

Feel free to merge after fixing the flake8.

@holmanb
Copy link
Copy Markdown
Member

holmanb commented Feb 8, 2022

Something doesn't seem right about this resolution to the problem.

Even though the user is added via useradd tom --password "$1$S7$tT1BEDIYrczeryDQJfdPe0" -m (from cloudinit/distros/__init__.py, the hash read from /etc/shadow is $y$j9T$dwk9jSfj/HlK68D989MAa1$tkF6ZjRJWXG9OZ2dS2SzCQCL9nuAogxSd9nRjHC6De6. This shouldn't happen since we're passing a hash.

crypt's default password methods may have changed to yescrypt between distros (hence the new$y format), but that shouldn't affect this since we're passing in a hash. We shouldn't be depending on local crypt settings with this.

What am I missing here?

I don't understand why this fixes the odd behavior I just mentioned.

Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comments

@blackboxsw
Copy link
Copy Markdown
Collaborator Author

blackboxsw commented Feb 11, 2022

Ok @TheRealFalcon and I debugged this further. A couple things to note:

  1. Per your comment

Even though the user is added via useradd tom --password "$1$S7$tT1BEDIYrczeryDQJfdPe0" -m (from cloudinit/distros/init.py, the hash read from /etc/shadow is $y$j9T$dwk9jSfj/HlK68D989MAa1$tkF6ZjRJWXG9OZ2dS2SzCQCL9nuAogxSd9nRjHC6De6. This shouldn't happen since we're passing a hash.

The userdata provided to the instance actually provides additional passwd via chpasswd which effectively overwrites the values provided in "users:" cloud-config.

  1. invoking crypt.crypt from python on jammy reads '/lib/x86_64-linux-gnu/libcrypt.so.1` delivered by libcrypt1:amd64 which is build from source pkg: libxcrypt. There are huge changes for yescrypt support and handling between bionic (where our integration tests run) and Jammy (or test target that is failing). We can walk through some of those changes using git-ubuntu
git-ubuntu clone libxcrypt
cd libxcrypt
# All ubuntu package source uploads are represented under git branches pkg/ubuntu/(bionic|focal|jammy)-(proposed|devel|updates)?
# the defaut clone drops you into pkg/ubuntu/devel (which is current the ubuntu jammy release of libxcrypt)
git diff pkg/ubuntu/bionic-devel

The resulting diff shows us that bionic had no yescrypt support, so running python3 crypt.crypt locally on our bionic system will return None for salt/hash/formats it doesn't understand. The solution to validate hashed passwd via crypt on the target environment will avoid this type of failure due to versionitis.

@blackboxsw blackboxsw force-pushed the tests-crypt-on-target-distro branch from 6c76ab9 to 1f6ebb0 Compare February 13, 2022 16:19
@holmanb
Copy link
Copy Markdown
Member

holmanb commented Feb 14, 2022

Ok @TheRealFalcon and I debugged this further. A couple things to note:

1. Per your comment

Even though the user is added via useradd tom --password "$1$S7$tT1BEDIYrczeryDQJfdPe0" -m (from cloudinit/distros/init.py, the hash read from /etc/shadow is $y$j9T$dwk9jSfj/HlK68D989MAa1$tkF6ZjRJWXG9OZ2dS2SzCQCL9nuAogxSd9nRjHC6De6. This shouldn't happen since we're passing a hash.

The userdata provided to the instance actually provides additional passwd via chpasswd which effectively overwrites the values provided in "users:" cloud-config.

Aha! This is the piece I was missing. Thanks for the explanation. I agree that passing through crypt behavior rather than assuming a specific version of crypt is the way to go. +1 on the approach

Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this change I think we need to add yescrypt support (parsing $y in addition to $1/$5/$6/etc) with changes in the docs and in the parsing code.

While we're at it we might want to add support for the other recently added hashing methods supported by crypt too ($2b/$gy/$7).

That doesn't need to be in this PR, however.

@blackboxsw blackboxsw merged commit 1781854 into canonical:main Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants