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

Run Chip Tool without sudo #55

Merged
merged 10 commits into from
Jun 13, 2024
Merged

Run Chip Tool without sudo #55

merged 10 commits into from
Jun 13, 2024

Conversation

jpm-canonical
Copy link
Contributor

@jpm-canonical jpm-canonical commented May 2, 2024

We had to use sudo because of the directory where Chip Tool stores its config and data (Resolves #17). After an upstream changes we can set the TMPDIR env var to tell Chip Tool where to store its data.

This PR tries to:

  • Set TMPDIR to snap home dir (SNAP_USER_COMMON) using command chain
  • Removed outdated path rewrites and unnecessary mounts

⚠️ Testing is needed to see if this change is actually effective, and previous functionality is unchanged.

Log output of Chip Tool on where exactly data is stored:

$ chip-tool onoff toggle 110 1
[1714639755.999521][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /home/jpmeijers/snap/chip-tool/common/chip_tool_kvs
[1714639755.999830][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_kvs
[1714639755.999840][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Attempt to re-initialize with KVS config file: /tmp/chip_kvs
[1714639756.001540][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_factory.ini
[1714639756.001573][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_config.ini
[1714639756.001588][286634:286634] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_counters.ini
[1714639756.001656][286634:286634] CHIP:DL: writing settings to file (/tmp/chip_counters.ini-mbO1FA)
[1714639756.001757][286634:286634] CHIP:DL: renamed tmp file to file (/tmp/chip_counters.ini)
...
[1714639756.260402][286634:286634] CHIP:DL: writing settings to file (/tmp/chip_counters.ini-OSmUHU)
[1714639756.260762][286634:286634] CHIP:DL: renamed tmp file to file (/tmp/chip_counters.ini)
[1714639756.260837][286634:286634] CHIP:DL: NVS set: chip-counters/total-operational-hours = 0 (0x0)

This looks like chip_tool_* files are stored in the correct persistent directory, but chip_* files are still stored under /tmp.

@jpm-canonical
Copy link
Contributor Author

jpm-canonical commented May 2, 2024

✅ Previously there was an issue where history is not persisted (#20).

Using this branch, running a few commands in interactive mode, we can see the history is now stored in $SNAP_USER_COMMON.

$ chip-tool interactive start
>>> onoff toggle 110 1
[1714638608.210295][285852:285852] CHIP:TOO: Command: onoff toggle 110 1
>>> quit
[1714638614.446034][285852:285852] CHIP:TOO: Command: quit 
[1714638614.446330][285852:285852] CHIP:TOO: Unknown cluster or command set: quit
>>> exit
[1714638616.938403][285852:285852] CHIP:TOO: Command: exit 
[1714638616.938520][285852:285852] CHIP:TOO: Unknown cluster or command set: exit
^C
$ cd /home/jpmeijers/snap/chip-tool/common
$ ls
chip_tool_config.alpha.ini  chip_tool_history
chip_tool_config.ini        chip_tool_kvs
$ cat chip_tool_history 
onoff toggle 110 1
quit
exit

@jpm-canonical
Copy link
Contributor Author

jpm-canonical commented May 2, 2024

✅ There is a concern that a system reboot will wipe /tmp, causing chip-tool to forget paired devices. An experiment was done by restarting the host computer, and then trying to control a previously paired device. The device was still paired and controllable.

Tested this on two device:

  • local laptop
  • Raspberry Pi 5

/tmp before reboot

$ snap run --shell chip-tool.chip-tool
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

jpmeijers@raspi-b:/home/jpmeijers$ cd /tmp
jpmeijers@raspi-b:/tmp$ ls
chip_config.ini  chip_counters.ini  chip_factory.ini
jpmeijers@raspi-b:/tmp$ cat chip_config.ini 
[DEFAULT]
location-capability=2
regulatory-location=0

jpmeijers@raspi-b:/tmp$ cat chip_counters.ini 
[DEFAULT]
boot-reason=0
reboot-count=4
total-operational-hours=0

jpmeijers@raspi-b:/tmp$ cat chip_factory.ini 
[DEFAULT]
product-id=32769
unique-id=75942E11A61116F5
vendor-id=65521

/tmp after reboot

$ snap run --shell chip-tool.chip-tool
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

jpmeijers@raspi-b:/home/jpmeijers$ cd /tmp
jpmeijers@raspi-b:/tmp$ ls
jpmeijers@raspi-b:/tmp$ 

/tmp after command

$ snap run --shell chip-tool.chip-tool
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

jpmeijers@raspi-b:/home/jpmeijers$ ls /tmp
chip_config.ini  chip_counters.ini  chip_factory.ini
jpmeijers@raspi-b:/home/jpmeijers$ cat /tmp/chip_config.ini 
[DEFAULT]
location-capability=2
regulatory-location=0

jpmeijers@raspi-b:/home/jpmeijers$ cat /tmp/chip_counters.ini 
[DEFAULT]
boot-reason=0
reboot-count=1
total-operational-hours=0

jpmeijers@raspi-b:/home/jpmeijers$ cat /tmp/chip_factory.ini 
[DEFAULT]
product-id=32769
unique-id=4123A74F3743DF08
vendor-id=65521

chip-tool logs first-run-after-reboot.txt

@jpm-canonical
Copy link
Contributor Author

jpm-canonical commented May 3, 2024

❌ If chip-tool was run with sudo, and you try to run it without sudo after that, it can't open the files in /tmp, as they are owned by root.

$ chip-tool pairing onnetwork 110 20202021
[1714744727.897352][25324:25324] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /home/jpmeijers/snap/chip-tool/common/chip_tool_kvs
[1714744727.897477][25324:25324] CHIP:DL: writing settings to file (/home/jpmeijers/snap/chip-tool/common/chip_tool_kvs-LoTsJE)
[1714744727.897558][25324:25324] CHIP:DL: renamed tmp file to file (/home/jpmeijers/snap/chip-tool/common/chip_tool_kvs)
[1714744727.897986][25324:25324] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_kvs
[1714744727.897996][25324:25324] CHIP:DL: ChipLinuxStorage::Init: Attempt to re-initialize with KVS config file: /tmp/chip_kvs
[1714744727.900503][25324:25324] CHIP:DL: ChipLinuxStorage::Init: Using KVS config file: /tmp/chip_factory.ini
[1714744727.900593][25324:25324] CHIP:DL: writing settings to file (/tmp/chip_factory.ini-qTVr56)
[1714744727.900640][25324:25324] CHIP:DL: failed to rename (/tmp/chip_factory.ini-qTVr56), Operation not permitted (1)
[1714744727.900659][25324:25324] CHIP:DL: Configuration Manager initialization failed: src/platform/Linux/CHIPLinuxStorageIni.cpp:116: CHIP Error 0x000000AF: Write to file failed
[1714744727.900669][25324:25324] CHIP:-: src/platform/Linux/CHIPLinuxStorageIni.cpp:116: CHIP Error 0x000000AF: Write to file failed at ../examples/chip-tool/commands/common/CHIPCommand.cpp:142
[1714744727.900679][25324:25324] CHIP:TOO: Run command failure: src/platform/Linux/CHIPLinuxStorageIni.cpp:116: CHIP Error 0x000000AF: Write to file failed
[1714744727.912484][25324:25324] CHIP:SPT: VerifyOrDie failure at src/system/SystemLayerImplSelect.h:59: mLayerState.Destroy()
Aborted (core dumped)
$ snap run --shell chip-tool.chip-tool
To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

jpmeijers@raspi-b:/home/jpmeijers/chip-tool-snap$ ls -lah /tmp
total 16K
drwxrwxrwt  2 root      root      4.0K May  3 13:58 .
drwxr-xr-x 21 root      root       520 May  3 12:10 ..
-rw-------  1 root      root        55 May  3 12:10 chip_config.ini
-rw-------  1 root      root        66 May  3 13:50 chip_counters.ini
-rw-------  1 root      root        71 May  3 12:10 chip_factory.ini
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:55 chip_factory.ini-HsQwFC
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:53 chip_factory.ini-XVVQNn
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:57 chip_factory.ini-bAk05q
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:53 chip_factory.ini-gOJqkg
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:53 chip_factory.ini-lGLmQq
-rw-------  1 jpmeijers jpmeijers    0 May  3 13:58 chip_factory.ini-qTVr56

Need to run snap run as sudo to be able to delete:

$ sudo snap run --shell chip-tool.chip-tool
root@raspi-b:/home/jpmeijers/chip-tool-snap# rm /tmp/*

@jpm-canonical jpm-canonical marked this pull request as ready for review May 27, 2024 08:34
@jpm-canonical jpm-canonical changed the title [WIP] Run Chip Tool without sudo Run Chip Tool without sudo May 27, 2024
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Thanks for the change and thorough testing.

I believe this will result in a backward incompatible change, because existing installations will lose their state once upgraded to use a different directory.

The post-refresh hook could be used to stage the upgrade. In this case, to copy files to the new location (SNAP_USER_COMMON for the root user). I'm not so concerned about the functionality issue for those switching users. What matters is enabling upgraded installations to continue working (with sudo).

snap/snapcraft.yaml Outdated Show resolved Hide resolved
snap/snapcraft.yaml Outdated Show resolved Hide resolved
Removed outdated path rewrites and unnecessary mounts.
Copy system wide common files to root user specific common directory.
Make sure we can still control a paired device after upgrading
Snapd already prints that hook is running, so no need for more logging
Add a section explaining the change to no requiring sudo
Add commands to move to a new user and fix a know error
tests/common.go Outdated Show resolved Hide resolved
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Thanks. It looks good apart from a few minor issues.

README.md Outdated Show resolved Hide resolved
tests/common.go Show resolved Hide resolved
tests/common.go Outdated Show resolved Hide resolved
tests/upgrade_test.go Outdated Show resolved Hide resolved
tests/upgrade_test.go Outdated Show resolved Hide resolved
tests/upgrade_test.go Outdated Show resolved Hide resolved
tests/upgrade_test.go Outdated Show resolved Hide resolved
tests/upgrade_test.go Outdated Show resolved Hide resolved
Use version functions from utils package
Pass test struct everywhere we want to fail
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Great. Thanks a lot for the fix and adding tests.

@farshidtz farshidtz merged commit 1661240 into main Jun 13, 2024
6 checks passed
@farshidtz farshidtz deleted the feature/no-sudo branch June 13, 2024 14:49
farshidtz added a commit to canonical/matter-docs that referenced this pull request Jul 1, 2024
Chip Tool snap has changes to work without root privileges:
canonical/chip-tool-snap#55
farshidtz added a commit to canonical/matter-docs that referenced this pull request Jul 1, 2024
Chip Tool snap has changed and now works without root privileges:
canonical/chip-tool-snap#55
farshidtz added a commit to canonical/matter-docs that referenced this pull request Jul 4, 2024
Chip Tool snap has changed and now works without root privileges:
canonical/chip-tool-snap#55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make chip-tool command work without root privileges
2 participants