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

Added user and group checks. Auto create socket dir. #205

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ picky = "5.0.0"
psa-crypto = { version = "0.3.0" , default-features = false, features = ["operations"], optional = true }
zeroize = { version = "1.1.0", features = ["zeroize_derive"] }
picky-asn1-x509 = { version = "0.1.0", optional = true }
users = "0.10.0"
libc = "0.2.72"

[dev-dependencies]
ring = "0.16.12"
Expand All @@ -62,6 +64,7 @@ features = ["docs"]

[features]
default = []
no-parsec-user-and-clients-group = []
mbed-crypto-provider = ["psa-crypto"]
pkcs11-provider = ["pkcs11", "picky-asn1-der", "picky-asn1", "picky-asn1-x509"]
tpm-provider = ["tss-esapi", "picky-asn1-der", "picky-asn1", "picky-asn1-x509"]
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ This project uses the following third party crates:
* sha2 (MIT and Apache-2.0)
* hex (MIT and Apache-2.0)
* picky (MIT and Apache-2.0)
* users (MIT)
* libc (MIT and Apache-2.0)

This project uses the following third party libraries:
* [**Mbed Crypto**](https://github.com/ARMmbed/mbed-crypto) (Apache-2.0)
4 changes: 2 additions & 2 deletions ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ while [ "$#" -gt 0 ]; do
PROVIDER_NAME=$1
cp $(pwd)/e2e_tests/provider_cfg/$1/config.toml $CONFIG_PATH
if [ "$PROVIDER_NAME" = "all" ]; then
FEATURES="--features=all-providers"
FEATURES="--features=all-providers,no-parsec-user-and-clients-group"
else
FEATURES="--features=$1-provider"
FEATURES="--features=$1-provider,no-parsec-user-and-clients-group"
fi
;;
*)
Expand Down
113 changes: 105 additions & 8 deletions src/front/domain_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ use super::listener;
use listener::Connection;
use listener::Listen;
use log::error;
#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
use std::ffi::CString;
use std::fs;
use std::fs::Permissions;
use std::io::{Error, ErrorKind, Result};
Expand All @@ -18,6 +20,10 @@ use std::path::Path;
use std::time::Duration;

static SOCKET_PATH: &str = "/tmp/parsec/parsec.sock";
#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
const PARSEC_USERNAME: &str = "parsec";
#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
const PARSEC_GROUPNAME: &str = "parsec-clients";

/// Unix Domain Socket IPC manager
///
Expand All @@ -32,24 +38,26 @@ pub struct DomainSocketListener {

impl DomainSocketListener {
/// Initialise the connection to the Unix socket.
///
/// # Panics
/// - if a file/socket exists at the path specified for the socket and `remove_file`
/// fails
/// - if binding to the socket path fails
pub fn new(timeout: Duration) -> Result<Self> {
// If this Parsec instance was socket activated (see the `parsec.socket`
#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
DomainSocketListener::check_user_details()?;

// is Parsec instance was socket activated (see the `parsec.socket`
// file), the listener will be opened by systemd and passed to the
// process.
// If Parsec was service activated or not started under systemd, this
// will return `0`.
let listener = match sd_notify::listen_fds()? {
0 => {
let socket = Path::new(SOCKET_PATH);

if socket.exists() {
let parent_dir = socket.parent().unwrap();
if !parent_dir.exists() {
fs::create_dir_all(parent_dir)?;
} else if socket.exists() {
sbailey-arm marked this conversation as resolved.
Show resolved Hide resolved
fs::remove_file(&socket)?;
}
#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
DomainSocketListener::set_socket_dir_permissions(parent_dir)?;

let listener = UnixListener::bind(SOCKET_PATH)?;
listener.set_nonblocking(true)?;
Expand Down Expand Up @@ -84,6 +92,95 @@ impl DomainSocketListener {

Ok(Self { listener, timeout })
}

#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
fn set_socket_dir_permissions(parent_dir: &Path) -> Result<()> {
if let Some(parent_dir_str) = parent_dir.to_str() {
fs::set_permissions(parent_dir, Permissions::from_mode(0o750))?;
// Although `parsec` has to be part of the `parsec_clients` group, it may not be the primary group. Therefore force group ownership to `parsec_clients`
if unsafe {
let parent_dir_cstr = CString::new(parent_dir_str)
.expect("Failed to convert socket path parent to cstring");
{
libc::chown(
parent_dir_cstr.as_ptr(),
users::get_current_uid(), // To get to this point, user has to be `parsec`
users::get_group_by_name(PARSEC_GROUPNAME).unwrap().gid(), // `parsec_clients` exists by this point so should be safe
)
}
} != 0
{
error!(
"Changing ownership of {} to user {} and group {} failed.",
parent_dir_str, PARSEC_USERNAME, PARSEC_GROUPNAME
);
return Err(Error::new(
ErrorKind::Other,
"Changing ownership of socket directory failed",
));
}
} else {
error!(
"Error converting {} parent directory to string.",
SOCKET_PATH
);
return Err(Error::new(
ErrorKind::InvalidInput,
"Error retrieving parent directory for socket",
));
}
Ok(())
}

#[cfg(not(feature = "no-parsec-user-and-clients-group"))]
fn check_user_details() -> Result<()> {
// Check Parsec is running as parsec user
if users::get_current_username() != Some(PARSEC_USERNAME.into()) {
error!(
"Incorrect user. Parsec should be run as user {}.",
PARSEC_USERNAME
);
return Err(Error::new(
ErrorKind::PermissionDenied,
"Parsec run as incorrect user",
));
}
// Check Parsec client group exists and parsec user is a member of it
Copy link
Member

Choose a reason for hiding this comment

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

Does parsec need to be part of parsec-clients? Since parsec owns the socket dir, can't they use it how they want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A user has to be a member of a group in order to change permissions of the group to that group. So if parsec is not a member of parsec-clients, it cannot change the group permission of the directory to parsec-clients.

Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense, thanks.

if let Some(parsec_clients_group) = users::get_group_by_name(PARSEC_GROUPNAME) {
if let Some(groups) = users::get_user_groups(PARSEC_USERNAME, users::get_current_gid())
{
// Split to make `clippy` happy
let parsec_user_in_parsec_clients_group = groups.into_iter().any(|group| {
group.gid() == parsec_clients_group.gid()
&& group.name() == parsec_clients_group.name()
});
// Check the parsec user is a member of the parsec clients group
if parsec_user_in_parsec_clients_group {
return Ok(());
}
error!(
"{} user not a member of {}.",
PARSEC_USERNAME, PARSEC_GROUPNAME
);
Err(Error::new(
ErrorKind::PermissionDenied,
"User permissions incorrect",
))
} else {
error!("Retrieval of groups for user {} failed.", PARSEC_USERNAME);
Err(Error::new(
ErrorKind::InvalidInput,
"Failed to retrieve user groups",
))
}
} else {
error!("{} group does not exist.", PARSEC_GROUPNAME);
Err(Error::new(
ErrorKind::PermissionDenied,
"Group permissions incorrect",
))
}
}
}

impl Listen for DomainSocketListener {
Expand Down