Page MenuHomeVyOS Platform

unify all the ways commands are run
Open, Requires assessmentPublicFEATURE REQUEST

Description

There is currently multiple functions to run command this duplication should be removed.

  • some modules define their run command (_cmd)
  • some modules use os.system
  • some modules use subprocess and a variation (call, run, Popen, check_output, ...)

There is therefore not one way it is done and error messages are not consistently presented back to the user.
All the function call should be channelled via vyos.util where helper functions should be provided.

It will also allow to centrally log all command failure futher helping debugging.

Details

Difficulty level
Unknown (require assessment)
Version
-
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Related Objects

Event Timeline

pasik added a subscriber: pasik.Apr 5 2020, 10:02 AM
thomas-mangin renamed this task from remove duplication to unify all the ways command are run.Apr 5 2020, 12:07 PM
thomas-mangin renamed this task from unify all the ways command are run to unify all the ways commands are run.
thomas-mangin updated the task description. (Show Details)
thomas-mangin closed this task as Resolved.Apr 7 2020, 3:01 PM
thomas-mangin claimed this task.
thomas-mangin reopened this task as Open.Apr 11 2020, 10:26 PM
thomas-mangin added a subscriber: zsdc.

@zsdc asked if we could find out a better API for the command and there is an issue where it seems that the environment dict passed to Popen() is not working as expected.

jjakob added a subscriber: jjakob.Apr 23 2020, 12:55 PM

Bug: if I create '/config/vyos.log.debug' with contents '/config/vyos.debug.log', /config/vyos.debug.log is created as root:root rw-rw-r--, and later scripts that use call() but aren't root, don't have the permission to write to it, and fail. For example any validator like mac-address fails as if the value is invalid, but if we turn on its debug flag, we see the real error is [Errno 13]: Permission denied: '/config/vyos.debug.log' because 'src/helpers/validate-value.py' uses call(), but it isn't ran with root privileges. The debug log should be probably created with 666 permissions or its group changed so that all scripts, even non-root, can write to it.

thomas-mangin added a comment.EditedApr 23 2020, 8:32 PM

Yes, I also have a patch ... somewhere .. which prevent to change any of the /config files on the system (so that a user can not make damage to the system).
I also need to add a check to only use the file if the user and permission are what is expected. I will do it :-)
The file should be created as group vyattacfg tho with rights allowing both the user and root to write to it.

All that's needed is to chmod 666 the file after creation - I did that
after I found the error and all scripts could write to it fine.

The file should have been 660 and it should have worked .. I will check

660 is not enough as the created file is already 660 root:root, which
means just root has the rights to write to it. The failing script isn't
run as root while in a config session (validate-value.py). I'm not
exactly sure which user (vyos or vyattacfg), but as this is a debug log,
it's not security critical and can be set to 666. Otherwise it'd need to
be owned by a group into which all the writing users must be put into,
and can stay 660.

@jjakob this is the wrong phabricator entry to discuss it but I got your point ;-)