config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging #3069

Bravo555 · 2024-08-19T17:38:12Z

Proposed changes

Only use tedge-write for privilege elevation after normal copy fails due to PermissionDenied error and tedge-write is enabled

Put more details into warn and error logs when deploying configuration files using tedge-write:

before: "WARN: `sudo.enable` set to `true`, but sudo not found in $PATH"
after:  "WARN: `sudo.enable` set to `true`, but sudo not found in $PATH, using tedge-write directly"

before: "ERROR: config-manager failed writing updated configuration file: Starting tedge-write process failed"
after:  "ERROR: config-manager failed writing updated configuration file: failed to start process '/usr/bin/tedge-write': No such file or directory (os error 2)"

before: "ERROR: config-manager failed writing updated configuration file: sudo: /usr/bin/tedge-write: command not found"
after:  "ERROR: config-manager failed writing updated configuration file: process returned non-zero exit code (1); stderr="sudo: /usr/bin/tedge-write: command not found""

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

codecov · 2024-08-19T17:54:23Z

Codecov Report

Attention: Patch coverage is 50.44248% with 56 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rates/extensions/tedge_config_manager/src/actor.rs	56.84%	28 Missing and 13 partials ⚠️
crates/core/tedge_write/src/api.rs	12.50%	13 Missing and 1 partial ⚠️
crates/common/tedge_config/src/sudo.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

github-actions · 2024-08-20T07:10:30Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
499	0	2	499	100	1h25m36.385858s

albinsuresh

LGTM, except for one minor unwarp issues.

albinsuresh · 2024-08-20T07:09:56Z

crates/core/tedge_write/src/api.rs


        if !output.status.success() {
+            let stderr = String::from_utf8(output.stderr).expect("output should be utf-8");


I'm assuming that we're using expect here as we know that the output of tedge-write process can't be non-UTF8.

albinsuresh · 2024-08-20T07:11:22Z

crates/core/tedge_write/src/api.rs


        if !output.status.success() {
+            let stderr = String::from_utf8(output.stderr).expect("output should be utf-8");
+            let stderr = stderr.trim();
+            let exit_code = output.status.code().unwrap();


Here, isn't it better to be defenesive and account for the None status code case as well, as there is the theoretical possibility of the tedge-write process getting killed?

Hmm, I think that tedge-write process being terminated by a signal is very unlikely, as it is spawned and joined by tedge-agent, but you're right, it's better for that edge case to be properly handled as well.

didier-wenzek

First, as this as been an issue observed by a user, it would be good to have a issue giving the context (#3073). Notably, it is not obvious reading this, that the root cause is that configuration management is not working on a device where /usr/bin is readonly the path for tedge_write being hard coded.

Second, instead of an option to disable tedge_write, it would be better to have a setting telling where tedge_write has been installed.

crates/common/tedge_config/src/sudo.rs

didier-wenzek

We need to discuss what is the best option : tedge_write.disableor tedge_write.path.

Bravo555 · 2024-08-20T10:20:49Z

During the call with @albinsuresh and @didier-wenzek we decided the following:

A ticket with details about original user issue and their environment which required changes for tedge-write to be made will be created. It should explain the interaction between tedge-write and sudoers which makes it tricky.
configuration update fails due to tedge-write when thin-edge.io is installed in a non-standard location #3073
In the above mentioned user environment, tedge-agent is run as root. If tedge-agent runs as root, there is no reason for privilege elevation, so tedge-agent should never spawn tedge-write and just do a normal copy. This way we solve the original issue more elegantly, without adding yet more configuration options. This PR will be changed to use that approach instead of adding another configuration option.
EDIT: Instead of checking if we're running as root, we should just try copying normally and only try to elevate once we fail with PermissionDenied. Limiting ourselves to root makes no sense if we could have permissions by some other means, and checking permissions ahead of time is also bad because it leads to TOCTOU bugs. Normal copy + tedge-write if failed is what should have been done from the very beginning.
Ability to customise the path of tedge-write: if a user has a custom environment where they install thin-edge.io in non-standard locations, they should have an ability to customise paths of all thin-edge components, including tedge-write. However, for tedge-write to work as intended, there needs to be an entry in /etc/sudoers with a full path to tedge-write, e.g. /usr/bin/tedge-write and that full path also needs to be provided when spawning the process. If we allow users to use an alternative location for tedge-write, we need to make sure to let the user know they need to change the appropriate /etc/sudoers entry for privilege elevation to work.
Thus, instead of hardcoding the path we can use another flow:
1. use which to find full path of tedge-write, e.g. /opt/thinedge/bin/tedge-write
2. spawn a sudo process using that path, i.e. sudo /opt/thinedge/bin/tedge-write
3. Check if we're running as sudo/have necessary write permissions/copying file returns a success. If not, then we need to print a hint telling the user to edit their /etc/sudoers configuration.

didier-wenzek · 2024-08-27T07:45:25Z

Side remark. It's a pity that the unit tests fail for a reason already highlighted by clippy: unused import: crate::FileEntry. We should not run the unit tests with RUSTFLAGS -D warnings

crates/extensions/tedge_config_manager/src/actor.rs

Bravo555 · 2024-08-27T16:06:33Z

crates/extensions/tedge_config_manager/src/actor.rs

-                let error_message = format!("config-manager failed downloading a file: {err}",);
+                let err =
+                    anyhow::Error::from(err).context("config-manager failed downloading a file");
+                let error_message = format!("{err:#}");


{err:#} on anyhow::Error prints all the lower level causes in error 1: error 2: error 3 format.

Put more details into warn and error logs when deploying configuration files using `tedge-write`: - before: "WARN: `sudo.enable` set to `true`, but sudo not found in $PATH" - after: "WARN: `sudo.enable` set to `true`, but sudo not found in $PATH, using tedge-write directly" - before: "ERROR: config-manager failed writing updated configuration file: Starting tedge-write process failed" - after: "ERROR: config-manager failed writing updated configuration file: failed to start process '/usr/bin/tedge-write': No such file or directory (os error 2)" - before: "ERROR: config-manager failed writing updated configuration file: sudo: /usr/bin/tedge-write: command not found" - after: "ERROR: config-manager failed writing updated configuration file: process '/usr/bin/sudo' returned non-zero exit code (1); stderr="sudo: /usr/bin/tedge-write: command not found"" Signed-off-by: Marcel Guzik <[email protected]>

Configuration file deployment was changed to always do a regular copy first, and only use tedge-write if copy failes due to lacking permissions. Signed-off-by: Marcel Guzik <[email protected]>

`Trigger config_update operation from another workflow` and `Trigger config_snapshot operation from another operation` were failing if started directly and not part of test suite. Because of the missing `Set Device Context` call, workflow toml files were being copied to the wrong device. Signed-off-by: Marcel Guzik <[email protected]>

didier-wenzek

Approved

didier-wenzek · 2024-08-29T07:34:26Z

crates/extensions/tedge_config_manager/src/actor.rs

+                // TODO: source temporary file should be cleaned up automatically
+                let _ = std::fs::remove_file(from);


This has definitely to be done.

But not necessarily here. This file has been created and populated outside this method. It should be automatically cleaned by the caller. Not easy to implement with the current design, though. Can this be done once merged this refactoring

albinsuresh · 2024-08-29T10:47:35Z

crates/extensions/tedge_config_manager/src/actor.rs

+/// - `src` doesn't exist
+/// - user or group doesn't exist
+/// - we have no write/execute permissions to the directory
+fn move_file_atomic_set_permissions_if_doesnt_exist(


Suggested change

fn move_file_atomic_set_permissions_if_doesnt_exist(

fn move_file_preserving_target_permissions(

Can we also move this API into tedge_utils so that it can be reused? After removing the dependency on FileEntry, of course. It could take the expected permissions instead.

Yes, I have this done for a follow-up PR

albinsuresh · 2024-08-29T10:51:02Z

crates/extensions/tedge_config_manager/src/actor.rs

+        .tempfile_in(dest.parent().context("invalid path")?)
+        .with_context(|| format!("could not create temporary file at '{dest}'"))?;
+
+    std::io::copy(&mut src_file, &mut tempfile).context("failed to copy")?;


The function name implies move but we're doing a copy here. Can we attempt a file move at least when the source and target files are under the same parent directory?

Yeah, move is probably a little misleading for now, but this function will be used by both config-manager directly and by tedge-write, which reads from stdin, so the function will be renamed to write_... in a follow-up PR

albinsuresh · 2024-08-29T10:53:35Z

crates/extensions/tedge_config_manager/src/actor.rs

+    tempfile
+        .as_file()
+        .set_permissions(std::fs::Permissions::from_mode(target_permissions.mode))
+        .context("failed to set mode on the destination file")?;
+
+    fchown(
+        tempfile.as_file(),
+        Some(target_permissions.uid),
+        Some(target_permissions.gid),
+    )
+    .context("failed to change ownership of the destination file")?;


Could have been done with tedge_utils::file::PermissionEntry::apply() as well, right?

AFAIK PermissionEntry::apply() uses a chown, but since we have a file descriptor open and want all these operations finished before the final rename, I decided to use fchown, which should complete before the file descriptor is closed.

There is also one problem with PermissionEntry::apply() when used in this context:

Since we're doing an atomic write as either tedge or root, the temporary file will be owned by tedge or root but the target configuration file will likely be owned by some other user, so most likely we will always have to change file owner. We want the atomic write to look like a normal write so it has to preserve original ownership no matter what it is.

But it is technically possible for a file to have uid/gid of a user that does not exist on the system. Perhaps the file was created by a program coming from a package that set up its own user, and the package was since uninstalled and the user was removed. Still in this case we still want to preserve original permissions, so we have to use original uid/gid directly. But PermissionEntry::apply() uses a name, so if we wanted to use it here we'd need to look up the name of the owner of the file, which can fail.

So not using PermissionEntry::apply() avoids an extra failure mode.

albinsuresh · 2024-08-29T10:54:27Z

crates/extensions/tedge_config_manager/src/actor.rs

+    Ok(Permissions { uid, gid, mode })
+}
+
+struct Permissions {


Why not reuse tedge_utils::file::PermissionEntry?

#3069 (comment)

We want to use uid/gid directly instead of names, since doing so avoids an extra failure mode.

gligorisaev

Checked the test and it is doing the expected verification

Bravo555 requested review from didier-wenzek, albinsuresh, jarhodes314 and rina23q as code owners August 19, 2024 17:38

Bravo555 temporarily deployed to Test Pull Request August 19, 2024 17:38 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto August 20, 2024 06:55 — with GitHub Actions Inactive

albinsuresh approved these changes Aug 20, 2024

View reviewed changes

didier-wenzek reviewed Aug 20, 2024

View reviewed changes

crates/common/tedge_config/src/sudo.rs Outdated Show resolved Hide resolved

didier-wenzek requested changes Aug 20, 2024

View reviewed changes

Bravo555 marked this pull request as draft August 20, 2024 10:21

Bravo555 force-pushed the fix/tedge-write-disable branch from f9fc679 to 2884a0f Compare August 20, 2024 17:37

Bravo555 temporarily deployed to Test Pull Request August 20, 2024 17:37 — with GitHub Actions Inactive

Bravo555 changed the title ~~Allow user to disable tedge-write and improve its logging~~ config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging Aug 20, 2024

Bravo555 temporarily deployed to Test Auto August 21, 2024 08:16 — with GitHub Actions Inactive

Bravo555 mentioned this pull request Aug 21, 2024

configuration update fails due to tedge-write when thin-edge.io is installed in a non-standard location #3073

Closed

4 tasks

Bravo555 force-pushed the fix/tedge-write-disable branch from 2884a0f to e130de6 Compare August 21, 2024 19:30

Bravo555 temporarily deployed to Test Pull Request August 21, 2024 19:30 — with GitHub Actions Inactive

Bravo555 had a problem deploying to Test Auto August 22, 2024 07:56 — with GitHub Actions Failure

Bravo555 force-pushed the fix/tedge-write-disable branch from e130de6 to febaea6 Compare August 26, 2024 18:44

Bravo555 temporarily deployed to Test Pull Request August 26, 2024 18:44 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto August 26, 2024 19:37 — with GitHub Actions Inactive

Bravo555 force-pushed the fix/tedge-write-disable branch from febaea6 to 9c7a381 Compare August 27, 2024 08:15

Bravo555 temporarily deployed to Test Pull Request August 27, 2024 08:15 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto August 27, 2024 08:33 — with GitHub Actions Inactive

Bravo555 mentioned this pull request Aug 27, 2024

ci: don't fail cargo test on warnings #3084

Merged

11 tasks

Bravo555 force-pushed the fix/tedge-write-disable branch from 9c7a381 to ec8562c Compare August 27, 2024 14:58

Bravo555 had a problem deploying to Test Pull Request August 27, 2024 14:59 — with GitHub Actions Failure

Bravo555 force-pushed the fix/tedge-write-disable branch from ec8562c to 8f6bf66 Compare August 27, 2024 15:08

Bravo555 had a problem deploying to Test Pull Request August 27, 2024 15:08 — with GitHub Actions Failure

didier-wenzek reviewed Aug 27, 2024

View reviewed changes

crates/extensions/tedge_config_manager/src/actor.rs Show resolved Hide resolved

Bravo555 force-pushed the fix/tedge-write-disable branch from 8f6bf66 to b89069c Compare August 27, 2024 16:04

Bravo555 had a problem deploying to Test Pull Request August 27, 2024 16:04 — with GitHub Actions Failure

Bravo555 force-pushed the fix/tedge-write-disable branch from b89069c to 2d755e5 Compare August 27, 2024 16:10

Bravo555 temporarily deployed to Test Pull Request August 27, 2024 16:10 — with GitHub Actions Inactive

Bravo555 commented Aug 27, 2024

View reviewed changes

Bravo555 marked this pull request as ready for review August 27, 2024 16:11

Bravo555 requested review from gligorisaev, a team and reubenmiller as code owners August 27, 2024 16:11

Bravo555 requested a review from albinsuresh August 27, 2024 16:20

Bravo555 had a problem deploying to Test Auto August 27, 2024 16:22 — with GitHub Actions Failure

Bravo555 added the theme:configuration Theme: Configuration management label Aug 28, 2024

Bravo555 added 3 commits August 28, 2024 16:02

only use tedge-write if no write permissions

73d2df0

Configuration file deployment was changed to always do a regular copy first, and only use tedge-write if copy failes due to lacking permissions. Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 force-pushed the fix/tedge-write-disable branch from 2d755e5 to 33d220d Compare August 28, 2024 14:02

Bravo555 temporarily deployed to Test Pull Request August 28, 2024 14:02 — with GitHub Actions Inactive

Bravo555 had a problem deploying to Test Auto August 28, 2024 14:40 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Auto August 28, 2024 15:09 — with GitHub Actions Inactive

didier-wenzek approved these changes Aug 29, 2024

View reviewed changes

albinsuresh reviewed Aug 29, 2024

View reviewed changes

Bravo555 added this pull request to the merge queue Aug 29, 2024

Merged via the queue into thin-edge:main with commit d41cd84 Aug 29, 2024
31 checks passed

gligorisaev reviewed Aug 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging #3069

config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging #3069

Bravo555 commented Aug 19, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 20, 2024 •

edited

Loading

albinsuresh left a comment

albinsuresh Aug 20, 2024

albinsuresh Aug 20, 2024

Bravo555 Aug 20, 2024

didier-wenzek left a comment •

edited by Bravo555

Loading

didier-wenzek left a comment

Bravo555 commented Aug 20, 2024 •

edited

Loading

didier-wenzek commented Aug 27, 2024

Bravo555 Aug 27, 2024

didier-wenzek left a comment

didier-wenzek Aug 29, 2024

albinsuresh Aug 29, 2024

Bravo555 Aug 29, 2024

albinsuresh Aug 29, 2024

Bravo555 Aug 29, 2024

albinsuresh Aug 29, 2024

Bravo555 Aug 29, 2024

albinsuresh Aug 29, 2024

Bravo555 Aug 29, 2024

gligorisaev left a comment


		if !output.status.success() {
		let stderr = String::from_utf8(output.stderr).expect("output should be utf-8");

		// TODO: source temporary file should be cleaned up automatically
		let _ = std::fs::remove_file(from);

	fn move_file_atomic_set_permissions_if_doesnt_exist(
	fn move_file_preserving_target_permissions(

config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging #3069

config-manager: Only use tedge-write after normal copy fails due to permissions and improve tedge-write logging #3069

Conversation

Bravo555 commented Aug 19, 2024 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 20, 2024 • edited Loading

Robot Results

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment • edited by Bravo555 Loading

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 commented Aug 20, 2024 • edited Loading

didier-wenzek commented Aug 27, 2024

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gligorisaev left a comment

Choose a reason for hiding this comment

Bravo555 commented Aug 19, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 20, 2024 •

edited

Loading

didier-wenzek left a comment •

edited by Bravo555

Loading

Bravo555 commented Aug 20, 2024 •

edited

Loading