Add security hardening and additional auth tests
Security improvements: - Add password strength validation (min 8 characters) - Invalidate all sessions on password change/reset - Add timing-safe user lookup to prevent enumeration attacks - Fix SQLAlchemy boolean comparisons (== True -> is_(True)) - Change default admin password to 'changeme123' (meets min length) New tests (7 additional): - Inactive user login attempt blocked - Short password rejected on create/change/reset - Duplicate username rejected (409) - Non-owner API key deletion blocked (403) - Sessions invalidated on password change
This commit is contained in:
@@ -22,6 +22,26 @@ API_KEY_PREFIX = "orch_"
|
||||
# Session duration (24 hours default)
|
||||
SESSION_DURATION_HOURS = 24
|
||||
|
||||
# Password requirements
|
||||
MIN_PASSWORD_LENGTH = 8
|
||||
|
||||
|
||||
class PasswordTooShortError(ValueError):
|
||||
"""Raised when password doesn't meet minimum length requirement."""
|
||||
|
||||
pass
|
||||
|
||||
|
||||
def validate_password_strength(password: str) -> None:
|
||||
"""Validate password meets minimum requirements.
|
||||
|
||||
Raises PasswordTooShortError if password is too short.
|
||||
"""
|
||||
if not password or len(password) < MIN_PASSWORD_LENGTH:
|
||||
raise PasswordTooShortError(
|
||||
f"Password must be at least {MIN_PASSWORD_LENGTH} characters"
|
||||
)
|
||||
|
||||
|
||||
def hash_password(password: str) -> str:
|
||||
"""Hash a password using bcrypt."""
|
||||
@@ -93,23 +113,37 @@ class AuthService:
|
||||
"""Authenticate a user with username and password.
|
||||
|
||||
Returns the user if authentication succeeds, None otherwise.
|
||||
Uses constant-time comparison to prevent timing-based user enumeration.
|
||||
"""
|
||||
user = self.get_user_by_username(username)
|
||||
|
||||
# Always perform password verification to prevent timing attacks
|
||||
# Use a dummy hash if user doesn't exist
|
||||
dummy_hash = "$2b$12$LQv3c1yqBWVHxkd0LHAkCOYz6TtxMQJqhN8/X4.VTtYA1vQ9S9sXa"
|
||||
password_hash = user.password_hash if user and user.password_hash else dummy_hash
|
||||
|
||||
# Verify password (constant time even if user doesn't exist)
|
||||
password_valid = verify_password(password, password_hash)
|
||||
|
||||
# Check all conditions
|
||||
if not user:
|
||||
return None
|
||||
if not user.password_hash:
|
||||
return None # OIDC-only user
|
||||
if not user.is_active:
|
||||
return None
|
||||
if not verify_password(password, user.password_hash):
|
||||
if not password_valid:
|
||||
return None
|
||||
return user
|
||||
|
||||
def change_password(self, user: User, new_password: str) -> None:
|
||||
"""Change a user's password."""
|
||||
"""Change a user's password and invalidate all existing sessions."""
|
||||
validate_password_strength(new_password)
|
||||
user.password_hash = hash_password(new_password)
|
||||
user.must_change_password = False
|
||||
self.db.commit()
|
||||
# Invalidate all existing sessions for security
|
||||
self.delete_user_sessions(user)
|
||||
|
||||
def update_last_login(self, user: User) -> None:
|
||||
"""Update the user's last login timestamp."""
|
||||
@@ -120,7 +154,7 @@ class AuthService:
|
||||
"""List all users."""
|
||||
query = self.db.query(User)
|
||||
if not include_inactive:
|
||||
query = query.filter(User.is_active == True)
|
||||
query = query.filter(User.is_active.is_(True))
|
||||
return query.order_by(User.username).all()
|
||||
|
||||
def set_user_active(self, user: User, is_active: bool) -> None:
|
||||
@@ -134,10 +168,13 @@ class AuthService:
|
||||
self.db.commit()
|
||||
|
||||
def reset_user_password(self, user: User, new_password: str) -> None:
|
||||
"""Reset a user's password (admin action)."""
|
||||
"""Reset a user's password (admin action) and invalidate all sessions."""
|
||||
validate_password_strength(new_password)
|
||||
user.password_hash = hash_password(new_password)
|
||||
user.must_change_password = True
|
||||
self.db.commit()
|
||||
# Invalidate all existing sessions for security
|
||||
self.delete_user_sessions(user)
|
||||
|
||||
# --- Session Operations ---
|
||||
|
||||
@@ -159,7 +196,8 @@ class AuthService:
|
||||
session = UserSession(
|
||||
user_id=user.id,
|
||||
token_hash=token_hash,
|
||||
expires_at=datetime.now(timezone.utc) + timedelta(hours=SESSION_DURATION_HOURS),
|
||||
expires_at=datetime.now(timezone.utc)
|
||||
+ timedelta(hours=SESSION_DURATION_HOURS),
|
||||
user_agent=user_agent,
|
||||
ip_address=ip_address,
|
||||
)
|
||||
@@ -328,7 +366,7 @@ def create_default_admin(db: Session) -> Optional[User]:
|
||||
auth_service = AuthService(db)
|
||||
admin = auth_service.create_user(
|
||||
username="admin",
|
||||
password="admin",
|
||||
password="changeme123",
|
||||
is_admin=True,
|
||||
must_change_password=True,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user