-
Notifications
You must be signed in to change notification settings - Fork 19
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
Change socket location and add checks #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a lot more secure for clients!! Thanks 🎉
/// Checks if the socket is inside a folder with correct owners and permissions to make sure it | ||
/// is from the Parsec service. | ||
#[cfg(not(testing))] | ||
fn secure_parsec_socket_folder(&self) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth making this check conditional (as in, not only on testing
, but also on another feature) in case an admin can't/doesn't want to follow these directives 100%?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are probably right, admins could use a client that does not use this security feature. Will add another feature.
Cargo.toml
Outdated
|
||
[dev-dependencies] | ||
mockstream = "0.0.3" | ||
|
||
[features] | ||
testing = ["parsec-interface/testing"] | ||
testing = ["parsec-interface/testing", "no-security-check"] | ||
no-security-check = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is a bit extreme, maybe something like no-fs-permission-check
- a bit lengthy but closer in meaning. Also, I guess it should be documented somewhere (both in Readme and in docs.rs?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified the name and added it to the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Adds a check for the directory containing the socket. It must be owned by the parsec user and parsec-clients group and be 750. This check is disabled for testing. Signed-off-by: Hugues de Valon <[email protected]>
Adds a check for the directory containing the socket. It must be owned
by the parsec user and parsec-clients group and be 750. This check is
disabled for testing.
Signed-off-by: Hugues de Valon [email protected]