Skip to content
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

T6353: Add password complexity validation for system login user #4390

Open
wants to merge 1 commit into
base: current
Choose a base branch
from

Conversation

oniko94
Copy link
Contributor

@oniko94 oniko94 commented Mar 13, 2025

Change summary

  • Add password complexity check function based on cracklib FascistCheck and entropy calculation
  • Add checking user password to install image command in operation mode
  • Add checking new user password to system login command in configuration mode

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

  • Built a .deb package using the official Docker image vyos/vyos-build:current
  • Built an ISO image
  • Boot up a virtual machine (qemu-system-x86_64, host: Linux Mint) from the live ISO image built in the previous step
  • Run install image
  • Check:
    • Password shorter than 4 characters
    • Password with only lowercase letters'
    • Password with uppercase, lowercase letters and digits
    • Made sure in all cases the actual output matched the expected output

Smoketest output:

test_add_linux_system_user (__main__.TestSystemLogin.test_add_linux_system_user) ... ok
test_delete_current_user (__main__.TestSystemLogin.test_delete_current_user) ... ok
test_radius_kernel_features (__main__.TestSystemLogin.test_radius_kernel_features) ... ok
test_system_login_max_login_session (__main__.TestSystemLogin.test_system_login_max_login_session) ... ok
test_system_login_otp (__main__.TestSystemLogin.test_system_login_otp) ... ok
test_system_login_radius_ipv4 (__main__.TestSystemLogin.test_system_login_radius_ipv4) ... ok
test_system_login_radius_ipv6 (__main__.TestSystemLogin.test_system_login_radius_ipv6) ... ok
test_system_login_tacacs (__main__.TestSystemLogin.test_system_login_tacacs) ... ok
test_system_login_user (__main__.TestSystemLogin.test_system_login_user) ... ok
test_system_login_weak_password_warning (__main__.TestSystemLogin.test_system_login_weak_password_warning) ... ok
test_system_user_ssh_key (__main__.TestSystemLogin.test_system_user_ssh_key) ... ok

----------------------------------------------------------------------
Ran 11 tests in 68.096s

OK

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@oniko94 oniko94 requested a review from a team as a code owner March 13, 2025 20:43
Copy link

github-actions bot commented Mar 13, 2025

👍
No issues in PR Title / Commit Title

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces password complexity validation for system login users by adding an entropy‐based strength evaluation and integrating password checks into both operation and configuration modes.

  • Added new utility functions in python/vyos/utils/auth.py to calculate entropy and evaluate password strength using cracklib.
  • Integrated password strength feedback in the image installer and enforced complexity in the system login configuration.
  • Updated smoketests to verify weak password handling.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

File Description
python/vyos/utils/auth.py Added functions for entropy calculation and password strength evaluation with constants.
smoketest/scripts/cli/test_system_login.py Updated tests to set passwords and validate weak password error messages.
src/op_mode/image_installer.py Integrated the new password evaluation logic into the image installation process.
src/conf_mode/system_login.py Enforced password complexity checks in system login configuration mode.
@oniko94 oniko94 force-pushed the feature/T6353-add-password-complexity-validation branch from d991fa0 to 7c1f41a Compare March 13, 2025 20:56
@sever-sever
Copy link
Member

In my opinion, configuration password complexity should be a configurable option. This check will not let me commit.
Or at least it should be a WARNING until password complexity is not configured explicitly.

For example, 98% of instances that I configure are just labs or test VMs. I understand why I use them and why I use some weak passwords.

It would be interesting to hear other people's opinions.

@oniko94
Copy link
Contributor Author

oniko94 commented Mar 14, 2025

In my opinion, configuration password complexity should be a configurable option. This check will not let me commit. Or at least it should be a WARNING until password complexity is not configured explicitly.

For example, 98% of instances that I configure are just labs or test VMs. I understand why I use them and why I use some weak passwords.

It would be interesting to hear other people's opinions.

@sever-sever Implementing it using a Warning class was my initial intention, but what I have encountered is that (possibly due to airbag as hinted by @jestabro) it works quite unpredictably - both standard library Warning and our custom one. The biggest issue I've faced was that I couldn't smoke test it in any way - if I used Python default Warning class, or some subclasses like urllib3.SecurityWarning, it was never caught in self.assertWarns and the tests failed. If I used our custom Warning class from vyos.base and tried to catch it printing - both stdout and stderr - there was still nothing. If there is any correct way to catch output in smoke tests (if a function prints for example), and I missed it - I would be extremely grateful for an insight, and refactor it right away!

@oniko94 oniko94 marked this pull request as draft March 14, 2025 15:18
@oniko94 oniko94 force-pushed the feature/T6353-add-password-complexity-validation branch from 0d3f7b6 to e18788f Compare March 17, 2025 11:08
Copy link

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests (no interfaces) ❌ failed
  • CLI Smoketests (interfaces only) ❌ failed
  • Config tests ❌ failed
  • RAID1 tests ❌ failed
  • TPM tests ❌ failed

@oniko94 oniko94 marked this pull request as ready for review March 17, 2025 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants