Marcos' auto-password generation + bits...#74
Marcos' auto-password generation + bits...#74JedMeister wants to merge 2 commits intoturnkeylinux:masterfrom
Conversation
- Add generate_password() using secrets module (CSPRNG) - get_password() now offers 'Generate' (recommended) or 'Manual' menu - Generated passwords shown in centered reverse-video box with colors - Confirmation step ensures user saved the password before proceeding - Fully backward compatible: offer_generate=True by default - Pass offer_generate=False for original behavior - Imports: added secrets, string (stdlib only)
OnGle
left a comment
There was a problem hiding this comment.
Looks pretty good but there are issues that need addressing.
| self.console.add_persistent_args(["--no-collapse"]) | ||
| self.console.add_persistent_args(["--backtitle", title]) | ||
| self.console.add_persistent_args(["--no-mouse"]) | ||
| self.console.add_persistent_args(["--colors"]) |
There was a problem hiding this comment.
Color support should be detected automatically, if we force it it'll make non-color supporting terminals break output inconsistent output.
| def generate_password(length: int = 20) -> str: | ||
| """Generate a cryptographically secure random password. | ||
|
|
||
| Uses the secrets module (CSPRNG). Guarantees at least one character | ||
| from each of the 4 complexity categories (uppercase, lowercase, | ||
| digit, symbol). Avoids shell-problematic characters. | ||
| """ | ||
| if length < 12: | ||
| length = 12 | ||
|
|
||
| uppercase = string.ascii_uppercase | ||
| lowercase = string.ascii_lowercase | ||
| digits = string.digits | ||
| symbols = "!@#%^&*_+-=?" | ||
|
|
||
| required = [ | ||
| secrets.choice(uppercase), | ||
| secrets.choice(lowercase), | ||
| secrets.choice(digits), | ||
| secrets.choice(symbols), | ||
| ] | ||
|
|
||
| all_chars = uppercase + lowercase + digits + symbols | ||
| remaining = [secrets.choice(all_chars) for _ in range(length - len(required))] | ||
|
|
||
| chars = required + remaining | ||
| for i in range(len(chars) - 1, 0, -1): | ||
| j = secrets.randbelow(i + 1) | ||
| chars[i], chars[j] = chars[j], chars[i] | ||
|
|
||
| return "".join(chars) |
There was a problem hiding this comment.
This is actually quite nice. I will note that as bad as it is, we will need to remove the length < 12 check, or we should assert(length >= 12) because if an appliance actually requires a password to be smaller than that, this will silently cause a broken password.
| assert isinstance(return_tuple, tuple) | ||
| return return_tuple[0] |
There was a problem hiding this comment.
Inconsistent handling of type assertions, earlier we wrapped a value in a str(...) call, here we assert it's type. We should do this consistently unless we have a good reason not too.
|
Thanks for the review @OnGle, all fair points. Here's my take on each:
Agreed. I'll remove the forced Type assertion consistencyGood catch. I'll unify on
|
I personally don't like defensive programming on anything that isn't public facing, but this is a side concern for a later date, probably in the policy discussion. For now this addresses my explicit concern.
Good point, but out of scope for this PR.
Good points, but out of scope for this PR.
I agree broadly, but this may break legacy systems, legacy game servers, services that users might need for backwards compatibility with existing systems they are not able to upgrade or migrate and aren't publicly exposed anyway. Setting the requirement too high here risks causing issues for developers who never even intended on producing public images or using them on a public network.
I appreciate your intention and agree, but please try and keep discussion relevant to the specific PRs/issues being worked on. At this time in our release cycle larger more broad concerns should be added as separate issues and deferred until after 19.0 Also I just realized that we need to be able to configure which characters are included and which are not, I don't recall if there still is but I believe there was at least one appliance that explicitly blacklisted |
This is #71 plus docstrings replaced (removed in PR?) and typing fixed (the original typing was wrong (not show how?!).
The functionality that it adds remains the same as @marcos-mendez added in #71 - i.e.: