-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(webui): enforce 12-char dashboard password policy with backend+frontend validation #7338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
90b79d5
a0159c8
a3abb28
b749f62
a47e39d
5c27cf1
a64d4e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| """Utilities for dashboard password hashing and verification.""" | ||
|
|
||
| import hashlib | ||
| import hmac | ||
| import re | ||
| import secrets | ||
|
|
||
| _PBKDF2_ITERATIONS = 200_000 | ||
| _PBKDF2_SALT_BYTES = 16 | ||
| _PBKDF2_ALGORITHM = "pbkdf2_sha256" | ||
| _PBKDF2_FORMAT = f"{_PBKDF2_ALGORITHM}$" | ||
| _LEGACY_MD5_LENGTH = 32 | ||
| _DASHBOARD_PASSWORD_MIN_LENGTH = 12 | ||
| DEFAULT_DASHBOARD_PASSWORD = "astrbot" | ||
|
|
||
|
|
||
| def hash_dashboard_password(raw_password: str) -> str: | ||
| """Return a salted hash for dashboard password using PBKDF2-HMAC-SHA256.""" | ||
| if not isinstance(raw_password, str) or raw_password == "": | ||
| raise ValueError("Password cannot be empty") | ||
|
|
||
| salt = secrets.token_hex(_PBKDF2_SALT_BYTES) | ||
| digest = hashlib.pbkdf2_hmac( | ||
| "sha256", | ||
| raw_password.encode("utf-8"), | ||
| bytes.fromhex(salt), | ||
| _PBKDF2_ITERATIONS, | ||
| ).hex() | ||
| return f"{_PBKDF2_FORMAT}{_PBKDF2_ITERATIONS}${salt}${digest}" | ||
|
|
||
|
|
||
| def validate_dashboard_password(raw_password: str) -> None: | ||
| """Validate whether dashboard password meets the minimal complexity policy.""" | ||
| if not isinstance(raw_password, str) or raw_password == "": | ||
| raise ValueError("Password cannot be empty") | ||
| if len(raw_password) < _DASHBOARD_PASSWORD_MIN_LENGTH: | ||
| raise ValueError( | ||
| f"Password must be at least {_DASHBOARD_PASSWORD_MIN_LENGTH} characters long" | ||
| ) | ||
|
|
||
| if not re.search(r"[A-Z]", raw_password): | ||
| raise ValueError("Password must include at least one uppercase letter") | ||
| if not re.search(r"[a-z]", raw_password): | ||
| raise ValueError("Password must include at least one lowercase letter") | ||
| if not re.search(r"\d", raw_password): | ||
| raise ValueError("Password must include at least one digit") | ||
Soulter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def normalize_dashboard_password_hash(stored_password: str) -> str: | ||
| """Ensure dashboard password has a value, fallback to default dashboard password hash.""" | ||
| if not stored_password: | ||
| return hash_dashboard_password(DEFAULT_DASHBOARD_PASSWORD) | ||
| return stored_password | ||
|
|
||
|
|
||
| def _is_legacy_md5_hash(stored: str) -> bool: | ||
| return ( | ||
| isinstance(stored, str) | ||
| and len(stored) == _LEGACY_MD5_LENGTH | ||
| and all(c in "0123456789abcdefABCDEF" for c in stored) | ||
| ) | ||
|
|
||
|
|
||
| def _is_pbkdf2_hash(stored: str) -> bool: | ||
| return isinstance(stored, str) and stored.startswith(_PBKDF2_FORMAT) | ||
|
|
||
|
|
||
| def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (complexity): Consider refactoring
For example: from enum import Enum, auto
class _HashScheme(Enum):
LEGACY_MD5 = auto()
PBKDF2 = auto()
UNKNOWN = auto()
def _get_hash_scheme(stored_hash: str) -> _HashScheme:
if _is_legacy_md5_hash(stored_hash):
return _HashScheme.LEGACY_MD5
if _is_pbkdf2_hash(stored_hash):
return _HashScheme.PBKDF2
return _HashScheme.UNKNOWNThen split the verification logic into named functions so the compatibility behavior is explicit: def _verify_legacy_md5_hash(stored_hash: str, candidate_password: str) -> bool:
# Keep compatibility with existing md5-based deployments:
# new clients send plain password, old clients may send md5 of it.
candidate_md5 = hashlib.md5(candidate_password.encode("utf-8")).hexdigest()
stored = stored_hash.lower()
return (
hmac.compare_digest(stored, candidate_md5.lower())
or hmac.compare_digest(stored, candidate_password.lower())
)
def _verify_pbkdf2_hash(stored_hash: str, candidate_password: str) -> bool:
parts: list[str] = stored_hash.split("$")
if len(parts) != 4:
return False
_, iterations_s, salt, digest = parts
try:
iterations = int(iterations_s)
stored_key = bytes.fromhex(digest)
salt_bytes = bytes.fromhex(salt)
except (TypeError, ValueError):
return False
candidate_key = hashlib.pbkdf2_hmac(
"sha256",
candidate_password.encode("utf-8"),
salt_bytes,
iterations,
)
return hmac.compare_digest(stored_key, candidate_key)
def verify_dashboard_password(stored_hash: str, candidate_password: str) -> bool:
if not isinstance(stored_hash, str) or not isinstance(candidate_password, str):
return False
scheme = _get_hash_scheme(stored_hash)
if scheme is _HashScheme.LEGACY_MD5:
return _verify_legacy_md5_hash(stored_hash, candidate_password)
if scheme is _HashScheme.PBKDF2:
return _verify_pbkdf2_hash(stored_hash, candidate_password)
return FalseThis keeps all existing behavior (including the legacy MD5 dual comparison and default-password checks) but makes each piece self-contained and easier to reason about and test individually. |
||
| """Verify password against legacy md5 or new PBKDF2-SHA256 format.""" | ||
| if not isinstance(stored_hash, str) or not isinstance(candidate_password, str): | ||
| return False | ||
|
|
||
| if _is_legacy_md5_hash(stored_hash): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's better to enforce user to update the password by a specially constructed function, rather than being inserted into the validation stream.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currently after user updates astrbot and opens the WebUI, a mandatory password change dialog will appear. |
||
| # Keep compatibility with existing md5-based deployments: | ||
| # new clients send plain password, old clients may send md5 of it. | ||
| candidate_md5 = hashlib.md5(candidate_password.encode("utf-8")).hexdigest() | ||
Check failureCode scanning / CodeQL Use of a broken or weak cryptographic hashing algorithm on sensitive data High Sensitive data (password) Error loading related location Loading Sensitive data (password) Error loading related location Loading Sensitive data (password) is used in a hashing algorithm (MD5) that is insecure for password hashing, since it is not a computationally expensive hash function. Sensitive data (password) is used in a hashing algorithm (MD5) that is insecure for password hashing, since it is not a computationally expensive hash function. Sensitive data (password) is used in a hashing algorithm (MD5) that is insecure for password hashing, since it is not a computationally expensive hash function. |
||
Soulter marked this conversation as resolved.
Dismissed
Show dismissed
Hide dismissed
|
||
| return hmac.compare_digest( | ||
| stored_hash.lower(), candidate_md5.lower() | ||
| ) or hmac.compare_digest( | ||
| stored_hash.lower(), | ||
| candidate_password.lower(), | ||
| ) | ||
|
|
||
| if _is_pbkdf2_hash(stored_hash): | ||
| parts: list[str] = stored_hash.split("$") | ||
| if len(parts) != 4: | ||
| return False | ||
| _, iterations_s, salt, digest = parts | ||
| try: | ||
| iterations = int(iterations_s) | ||
| stored_key = bytes.fromhex(digest) | ||
| salt_bytes = bytes.fromhex(salt) | ||
| except (TypeError, ValueError): | ||
| return False | ||
| candidate_key = hashlib.pbkdf2_hmac( | ||
| "sha256", | ||
| candidate_password.encode("utf-8"), | ||
| salt_bytes, | ||
| iterations, | ||
| ) | ||
| return hmac.compare_digest(stored_key, candidate_key) | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def is_default_dashboard_password(stored_hash: str) -> bool: | ||
| """Check whether the password still equals the built-in default value.""" | ||
| return verify_dashboard_password(stored_hash, DEFAULT_DASHBOARD_PASSWORD) | ||
|
|
||
|
|
||
| def is_legacy_dashboard_password(stored_hash: str) -> bool: | ||
| """Check whether the password is still stored with legacy MD5.""" | ||
| return _is_legacy_md5_hash(stored_hash) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| import aiohttp | ||
| import psutil | ||
| from quart import request | ||
| from sqlmodel import select | ||
| from sqlmodel import col, select | ||
|
|
||
| from astrbot.core import DEMO_MODE, logger | ||
| from astrbot.core.config import VERSION | ||
|
|
@@ -21,6 +21,10 @@ | |
| from astrbot.core.db.migration.helper import check_migration_needed_v4 | ||
| from astrbot.core.db.po import ProviderStat | ||
| from astrbot.core.utils.astrbot_path import get_astrbot_path | ||
| from astrbot.core.utils.auth_password import ( | ||
| is_default_dashboard_password, | ||
| is_legacy_dashboard_password, | ||
| ) | ||
| from astrbot.core.utils.io import get_dashboard_version | ||
| from astrbot.core.utils.storage_cleaner import StorageCleaner | ||
| from astrbot.core.utils.version_comparator import VersionComparator | ||
|
|
@@ -76,7 +80,7 @@ def is_default_cred(self): | |
| password = self.config["dashboard"]["password"] | ||
| return ( | ||
| username == "astrbot" | ||
| and password == "77b90590a8945a7d36c963981a307dc9" | ||
| and is_default_dashboard_password(password) | ||
| and not DEMO_MODE | ||
| ) | ||
|
|
||
|
|
@@ -90,6 +94,9 @@ async def get_version(self): | |
| "version": VERSION, | ||
| "dashboard_version": await get_dashboard_version(), | ||
| "change_pwd_hint": self.is_default_cred(), | ||
| "legacy_pwd_hint": is_legacy_dashboard_password( | ||
| self.config["dashboard"]["password"] | ||
| ), | ||
| "need_migration": need_migration, | ||
| }, | ||
| ) | ||
|
|
@@ -226,7 +233,7 @@ async def get_provider_token_stats(self): | |
| ProviderStat.agent_type == "internal", | ||
| ProviderStat.created_at >= query_start_utc, | ||
| ) | ||
| .order_by(ProviderStat.created_at.asc()) | ||
| .order_by(col(ProviderStat.created_at).asc()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): Using This was previously |
||
| ) | ||
| records = result.scalars().all() | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.