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

Tidy up configuration files UNIX permissions #7983

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HorlogeSkynet
Copy link
Contributor


Hello 👋

This patch enforces 0600 UNIX permission on configuration files, for non-Windows hosts.
As they usually contain remote machine credentials, their access should be restricted to their owner.

Thanks, bye 🙏

@rustdesk
Copy link
Owner

rustdesk commented May 8, 2024

Thanks, testing video please, so that we can confirm it can work as expected.

@HorlogeSkynet
Copy link
Contributor Author

Hey @rustdesk, I'm really sorry but I can't manage to find "video" tests. Do you have a pointer or documentation that I could read to add such a test case ?

@rustdesk
Copy link
Owner

rustdesk commented May 9, 2024

Like this, #7988

Showing your PR taking effect, and not break any function.

@HorlogeSkynet
Copy link
Contributor Author

So I eventually got what you mean by "testing video"...
As I'm not setup to do so, and I fail to see how this would be somehow realistic, I've opted for two proper unit test cases for hbb_common (including one checking for file permissions mode, on non-Windows).

During building/testing, I've also encountered an issue related to Docker image and PAM, so I took the liberty to add the missing library to package dependencies list.

Bye 👋

@rustdesk
Copy link
Owner

Unit test is not enough for such case. Integration test is required.

@HorlogeSkynet
Copy link
Contributor Author

There isn't any integration tests from what I see...

I could make a screenshot of RD running and my terminal showing configuration files with 0600 permissions but I don't think it's really relevant as :

  • There is not any "visible" change in the UI ;
  • If peer configurations are 0644 or 0600 it's exactly the same thing from RD PoV (as the "user" dumping the configuration is the same one who loads it afterwards, unless I'm wrong here) ;
  • I could be totally running another version of the codebase and I failed to see how this would be bullet-proof.

Thanks for your time, bye 🙏

@rustdesk
Copy link
Owner

rustdesk commented May 10, 2024

Let's give you an example.

If a ipc file created with 600 permission, does it mean only the creator can access it? then the other process running as the other user can not connect to the IPC?

You need to undertand test, testing is for fixing your "I think".

@HorlogeSkynet
Copy link
Contributor Author

If a ipc file created with 600 permission, does it mean only the creator can access it? then the other process running as the other user can not connect to the IPC?

Fully-agreeing here with your IPC example; Is hbb_common::config::store_path (thus relying on confy, dealing itself with configuration files) actually used for handling IPC (or something different than configuration files) that I'm not aware of ?

You need to undertand test, "I think" is the worst world for testing.

👍 that's why projects usually rely on testing workflows that maintainers trust, and not "videos" recorded on untrusted machines (?), with completely unknown codes/setups/configurations which result in non-reproducible scenarios.

Thanks, see you 👋

@rustdesk
Copy link
Owner

rustdesk commented May 10, 2024

that's why projects usually rely on testing workflows that maintainers trust

Such a flow may be easy to set up for some project, but it is too hard for RustDesk, I hope you can understand.

@HorlogeSkynet
Copy link
Contributor Author

For what it's worth...

  1. Version and UI

Screenshot from 2024-05-10 16-06-23
Screenshot from 2024-05-10 16-05-14

  1. Peer configuration files permission

Screenshot from 2024-05-10 16-08-43

  1. Connection to a Windows host (was being used, so I've blanked screen for privacy reasons...)

Screenshot from 2024-05-10 16-13-09

```
wrapper.h:1:10: fatal error: 'security/pam_appl.h' file not found
```

Signed-off-by: Samuel FORESTIER <samuel+dev@forestier.app>
Signed-off-by: Samuel FORESTIER <samuel+dev@forestier.app>
@HorlogeSkynet
Copy link
Contributor Author

(rebased according to confy submodule "move" @rustdesk)

@HorlogeSkynet
Copy link
Contributor Author

Little up @rustdesk, thanks 🙏

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.

None yet

2 participants