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.

Sign up for GitHub

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

Jump to bottom

configd: T6504: send sudo_user on session init and set env variable #3701

Merged
merged 3 commits into from
Jun 24, 2024
Merged

configd: T6504: send sudo_user on session init and set env variable #3701

merged 3 commits into from
Jun 24, 2024

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Jun 21, 2024 •
edited by c-po
Loading

Change Summary

The environment variable SUDO_USER is checked by system_login.py so as to prevent deleting the current user. Provide from config session and set within configd environment.

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)

Component(s) name

Proposed changes

How to test

Smoketest result

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

@jestabro jestabro self-assigned this Jun 21, 2024
@jestabro jestabro requested a review from a team as a code owner June 21, 2024 19:22
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 21, 2024 •
edited
Loading

👍
No issues in PR Title / Commit Title

@c-po
Copy link
Member

c-po commented Jun 21, 2024

The fix unfortunately does not work.

vyos.utils.auth.get_current_user() returns nobody when beeing executed under vyos-configd

See environment of vyos-configd below

[email protected]# commit
nobody
environ({'LANG': 'en_US.UTF-8', 'PATH': '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin', 'HOME': '/root', 'LOGNAME': 'root', 'USER': 'root', 'SHELL': '/bin/bash', 'INVOCATION_ID': '4042c3114d34401f80de31fb3baaca7f', 'JOURNAL_STREAM': '7:20361', 'SYSTEMD_EXEC_PID': '4522'})
foo is logged in, forcing logout!

@jestabro jestabro marked this pull request as draft June 21, 2024 20:57
@jestabro
Copy link
Contributor Author

Move to draft to revise ...

@jestabro jestabro force-pushed the configd-drop-env-sudo branch from 37fe150 to 322c9c3 Compare June 22, 2024 03:03
@jestabro
configd: T6504: send sudo_user on session init and set env variable
7016f84 
The environment variable SUDO_USER is checked by system_login.py so as
to prevent deleting the current user. Provide from config session and
set within configd environment.
@jestabro jestabro force-pushed the configd-drop-env-sudo branch from 322c9c3 to 7016f84 Compare June 22, 2024 03:04
@jestabro jestabro changed the title configd: T6504: drop unnecessary set of SUDO_* in env configd: T6504: send sudo_user on session init and set env variable Jun 22, 2024
@jestabro jestabro marked this pull request as ready for review June 22, 2024 13:30
@jestabro
Copy link
Contributor Author

@c-po revised with proper solution.

c-po
c-po approved these changes Jun 23, 2024
View reviewed changes
Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

Tests work as exepcted, thanks!

[email protected]# delete system login user cpo
[edit]
[email protected]# commit

Attempting to delete current user: cpo

[[system login]] failed
Commit failed

@c-po c-po enabled auto-merge June 23, 2024 06:17
@c-po
Copy link
Member

c-po commented Jun 24, 2024

This should be considered a backport to circinus and sagitta in the future to fix this behavior.

@c-po
Copy link
Member

c-po commented Jun 24, 2024

Without vyos-configd

$ /usr/libexec/vyos/tests/smoke/cli/test_system_login.py
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_user_ssh_key (__main__.TestSystemLogin.test_system_user_ssh_key) ... ok

----------------------------------------------------------------------
Ran 10 tests in 176.126s

OK

With vyos-configd

$ /usr/libexec/vyos/tests/smoke/cli/test_system_login.py
test_add_linux_system_user (__main__.TestSystemLogin.test_add_linux_system_user) ...
User "backup" can not be created, conflict with local system account!

ok
test_delete_current_user (__main__.TestSystemLogin.test_delete_current_user) ...
Attempting to delete current user: cpo

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) ...
"login timeout" must be configured!

ok
test_system_login_otp (__main__.TestSystemLogin.test_system_login_otp) ... ok
test_system_login_radius_ipv4 (__main__.TestSystemLogin.test_system_login_radius_ipv4) ...
Only one IPv4 source-address can be set!

ok
test_system_login_radius_ipv6 (__main__.TestSystemLogin.test_system_login_radius_ipv6) ...
Only one IPv6 source-address can be set!

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_user_ssh_key (__main__.TestSystemLogin.test_system_user_ssh_key) ...
Missing type for public-key "[email protected]"!

ok

----------------------------------------------------------------------
Ran 10 tests in 171.783s

OK

@c-po c-po merged commit 4cd052b into vyos:current Jun 24, 2024
12 checks passed
@jestabro
Copy link
Contributor Author

@Mergifyio backport circinus

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 24, 2024 •
edited
Loading

backport circinus

✅ Backports have been created

@mergify mergify bot mentioned this pull request Jun 24, 2024
Merged
12 tasks
c-po added a commit that referenced this pull request Jun 24, 2024
@c-po
Merge pull request #3719 from vyos/mergify/bp/circinus/pr-3701
0259c43 
configd: T6504: send sudo_user on session init and set env variable (backport #3701)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c-po c-po c-po approved these changes

@sever-sever sever-sever sever-sever approved these changes

@dmbaturin dmbaturin Awaiting requested review from dmbaturin dmbaturin was automatically assigned from vyos/reviewers

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev was automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc was automatically assigned from vyos/reviewers

Assignees

@jestabro jestabro

Labels
backport current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jestabro @c-po @sever-sever