Skip to content

Use passlib in place of legacycrypt - #396

Merged
glehmann merged 1 commit into
masterfrom
gln/passlib-integration-kuyq
Jul 8, 2026
Merged

Use passlib in place of legacycrypt#396
glehmann merged 1 commit into
masterfrom
gln/passlib-integration-kuyq

Conversation

@glehmann

@glehmann glehmann commented Feb 12, 2026

Copy link
Copy Markdown
Member

legacycrypt requires to install libcrypt.so.1 manually

Keep legacycrypt as a dependency for now, to ease the transition to those who already have a custom data.py.

Move hash_password() to lib.common, to make easier to change the implementation in the future.

This is in draft while I test if it actually works with an install test.

This PR is part of a tree containing 3 PRs:

  1. master
  2. "Use passlib in place of legacycrypt" (this PR) → master
  3. Add Docker-based compatibility test kit for external validation #431Use passlib in place of legacycrypt #396
  4. Introduce TOML-based configuration to replace data.py #491Use passlib in place of legacycrypt #396

Comment thread pyproject.toml
@glehmann
glehmann marked this pull request as ready for review April 23, 2026 19:41
@glehmann
glehmann requested a review from a team as a code owner April 23, 2026 19:41
@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch 3 times, most recently from 4b18836 to 78b593a Compare May 13, 2026 13:58
@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch from 78b593a to 6aa0263 Compare May 18, 2026 16:22
@stormi

stormi commented May 18, 2026

Copy link
Copy Markdown
Member

@xcp-ng/os-platform-release another reviewer wanted here.

@vxgmichel

vxgmichel commented May 19, 2026

Copy link
Copy Markdown
Contributor

Since data.py is a configuration file, wouldn't be better to avoid computing redundant information in it?
Maybe the hash logic can go to lib/installer.py instead, with something like this:

diff --git a/lib/installer.py b/lib/installer.py
index 02fb96d..d384cd5 100644
--- a/lib/installer.py
+++ b/lib/installer.py
@@ -5,7 +5,7 @@ import time
 import xml.etree.ElementTree as ET
 
 from lib.commands import SSHCommandFailed, ssh
-from lib.common import wait_for
+from lib.common import hash_password, wait_for
 
 from typing import Any, Self
 
@@ -36,6 +36,8 @@ class AnswerFile:
     # makes a mutable deep copy of all `contents`
     @staticmethod
     def _normalize_structure(defn: dict[str, Any]) -> dict[str, Any]:
+        from data import HOST_DEFAULT_PASSWORD, INSERT_HOST_DEFAULT_PASSWORD_HASH
+
         assert isinstance(defn, dict), f"{defn!r} is not a dict"
         assert 'TAG' in defn, f"{defn} has no TAG"
 
@@ -47,7 +49,10 @@ class AnswerFile:
         for key, value in defn.items():
             if key == 'CONTENTS':
                 if isinstance(value, str):
-                    new_defn['CONTENTS'] = value
+                    if value == INSERT_HOST_DEFAULT_PASSWORD_HASH:
+                        new_defn['CONTENTS'] = hash_password(HOST_DEFAULT_PASSWORD)
+                    else:
+                        new_defn['CONTENTS'] = value
                 else:
                     new_defn['CONTENTS'] = [
                         AnswerFile._normalize_structure(item)

With INSERT_HOST_DEFAULT_PASSWORD_HASH being initialized in data.py with a sentinel value like:

INSERT_HOST_DEFAULT_PASSWORD_HASH = "<INSERT_HOST_DEFAULT_PASSWORD_HASH>"

But maybe that's outside the scope of this PR.

@glehmann

Copy link
Copy Markdown
Member Author

The scope of this PR was focused on removing an anoying depedency.
I like the idea though. I've done something similar in #491.

@vxgmichel vxgmichel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe document the rounds=5000 since it's not obvious that it's the default value that the system assumes when it's not explicitly provided in the corresponding /etc/shadow line.

LGTM otherwise 👍

Comment thread lib/common.py Outdated
host.xe(f'{xe_prefix}-param-clear', args)

def hash_password(password: str) -> str:
"""Hash password for /etc/password."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Hash password for /etc/password."""
"""Hash password for /etc/shadow."""

@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch 3 times, most recently from 123e95d to f1dc1d7 Compare May 22, 2026 15:29

@ydirson ydirson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

other than that remark, lgtm

Comment thread conftest.py
Comment on lines -25 to -27
setup_formatted_and_mounted_disk,
shortened_nodeid,
teardown_formatted_and_mounted_disk,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this does not seem to belong to this commit

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed!

@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch from f1dc1d7 to 406cf12 Compare May 22, 2026 16:59
@glehmann
glehmann requested a review from ydirson May 22, 2026 16:59
@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch from 406cf12 to 435faac Compare May 29, 2026 11:37
@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch 2 times, most recently from 4352f75 to 903b4d8 Compare June 8, 2026 09:43
legacycrypt requires to install libcrypt.so.1 manually

Keep legacycrypt as a dependency for now, to ease the transition to those
who already have a custom data.py.

Move hash_password() to lib.common, to make easier to change the
implementation in the future.

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>
@glehmann
glehmann force-pushed the gln/passlib-integration-kuyq branch from 903b4d8 to 4004073 Compare June 10, 2026 08:57
@stormi

stormi commented Jun 22, 2026

Copy link
Copy Markdown
Member

@glehmann are you waiting on something before the merge?

@vxgmichel

Copy link
Copy Markdown
Contributor

Tested on tests/install as well 👍

@glehmann
glehmann merged commit f9706d0 into master Jul 8, 2026
11 checks passed
@glehmann
glehmann deleted the gln/passlib-integration-kuyq branch July 8, 2026 20:45
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.

5 participants