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

add-optional-permissions: add ability to change permissions of screen recording file and path #475

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
14 changes: 12 additions & 2 deletions src/libguac/guacamole/recording.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

#include <guacamole/client.h>

#include <sys/types.h>

/**
* Provides functions and structures to be use for session recording.
*
Expand Down Expand Up @@ -122,6 +124,14 @@ typedef struct guac_recording {
* written, or non-zero if the path should be created if it does not yet
* exist.
*
* @param file_permissions
* The permissions to apply for the recording file created within the specified
* path.
*
* @param path_permissions
* The permissions to apply for the recording path created within the specified
* path.
*
* @param include_output
* Non-zero if output which is broadcast to each connected client
* (graphics, streams, etc.) should be included in the session recording,
Expand Down Expand Up @@ -154,8 +164,8 @@ typedef struct guac_recording {
* recording will be written, NULL otherwise.
*/
guac_recording* guac_recording_create(guac_client* client,
const char* path, const char* name, int create_path,
int include_output, int include_mouse, int include_touch,
const char* path, const char* name, int create_path, mode_t file_permissions,
mode_t path_permissions, int include_output, int include_mouse, int include_touch,
Comment on lines +167 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about directly coupling the API for screen recordings to POSIX-specific mode_t when libguac is generally intended to support Windows as a platform. Perhaps there's a way to implement this without making guac_recording_create() non-portable?

If it were possible to implement this in a way that doesn't require API/ABI-breaking changes, that would also be ideal.

Copy link
Author

Choose a reason for hiding this comment

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

i dont have the jira account for create issues at the moment.

thanks for letting me know and giving attention to this PR, i understand it is very delicate change.

we could change from mode_t to int, i think it would be agnostic. or creating own mode_t type.

i was reading documentation of c in microsoft and they use type int for mode.

int _open(
   const char *filename,
   int oflag [,
   int pmode]
);
int _wopen(
   const wchar_t *filename,
   int oflag [,
   int pmode]
);
errno_t _sopen_s(
   int* pfh,
   const char *filename,
   int oflag,
   int shflag,
   int pmode
);
errno_t _wsopen_s(
   int* pfh,
   const wchar_t *filename,
   int oflag,
   int shflag,
   int pmode,
);

int include_keys);

/**
Expand Down
32 changes: 32 additions & 0 deletions src/libguac/guacamole/user.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@

#include <pthread.h>
#include <stdarg.h>
#include <sys/types.h>

struct guac_user_info {

Expand Down Expand Up @@ -1001,5 +1002,36 @@ int guac_user_parse_args_int(guac_user* user, const char** arg_names,
int guac_user_parse_args_boolean(guac_user* user, const char** arg_names,
const char** argv, int index, int default_value);

/**
* Parses a string representation of file permissions (e.g., "rwxrwxrwx")
* and converts it to a mode_t value.
*
* @param user
* The user joining the connection and providing the given arguments.
*
* @param arg_names
* A NULL-terminated array of argument names, corresponding to the provided
* array of argument values. This array must be exactly the same size as
* the argument value array, with one additional entry for the NULL
* terminator.
*
* @param argv
* An array of all argument values, corresponding to the provided array of
* argument names. This array must be exactly the same size as the argument
* name array, with the exception of the NULL terminator.
*
* @param index
* The index of the entry in both the arg_names and argv arrays which
* corresponds to the argument being parsed.
*
* @param default_value
* The value to return if the provided argument is blank or invalid.
*
* @return
* The parsed mode_t value.
*/
mode_t guac_user_parse_args_mode(guac_user* user, const char** arg_names,
const char** argv, int index, mode_t default_value);

#endif

16 changes: 7 additions & 9 deletions src/libguac/recording.c
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
* failure.
*/
static int guac_recording_open(const char* path,
const char* name, char* basename, int basename_size) {
const char* name, char* basename, int basename_size, mode_t file_permissions) {

int i;

Expand All @@ -83,8 +83,7 @@ static int guac_recording_open(const char* path,

/* Attempt to open recording */
int fd = open(basename,
O_CREAT | O_EXCL | O_WRONLY,
S_IRUSR | S_IWUSR | S_IRGRP);
O_CREAT | O_EXCL | O_WRONLY, file_permissions);

/* Continuously retry with alternate names on failure */
if (fd == -1) {
Expand All @@ -102,8 +101,7 @@ static int guac_recording_open(const char* path,

/* Retry with newly-suffixed filename */
fd = open(basename,
O_CREAT | O_EXCL | O_WRONLY,
S_IRUSR | S_IWUSR | S_IRGRP);
O_CREAT | O_EXCL | O_WRONLY, file_permissions);

}

Expand Down Expand Up @@ -136,15 +134,15 @@ static int guac_recording_open(const char* path,
}

guac_recording* guac_recording_create(guac_client* client,
const char* path, const char* name, int create_path,
int include_output, int include_mouse, int include_touch,
const char* path, const char* name, int create_path, mode_t file_permissions,
mode_t path_permissions, int include_output, int include_mouse, int include_touch,
int include_keys) {

char filename[GUAC_COMMON_RECORDING_MAX_NAME_LENGTH];

/* Create path if it does not exist, fail if impossible */
#ifndef __MINGW32__
if (create_path && mkdir(path, S_IRWXU | S_IRGRP | S_IXGRP)
if (create_path && mkdir(path, path_permissions)
&& errno != EEXIST) {
#else
if (create_path && _mkdir(path) && errno != EEXIST) {
Expand All @@ -155,7 +153,7 @@ guac_recording* guac_recording_create(guac_client* client,
}

/* Attempt to open recording file */
int fd = guac_recording_open(path, name, filename, sizeof(filename));
int fd = guac_recording_open(path, name, filename, sizeof(filename), file_permissions);
if (fd == -1) {
guac_client_log(client, GUAC_LOG_ERROR,
"Creation of recording failed: %s", strerror(errno));
Expand Down
66 changes: 66 additions & 0 deletions src/libguac/user.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
#include <limits.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>

guac_user* guac_user_alloc() {

Expand Down Expand Up @@ -466,3 +468,67 @@ int guac_user_parse_args_boolean(guac_user* user, const char** arg_names,

}

mode_t guac_user_parse_args_mode(guac_user* user, const char** arg_names,
const char** argv, int index, mode_t default_value) {

/* Pull parameter value from argv */
const char* value = argv[index];

/* Use default value if blank */
if (value[0] == 0) {

/* Log use of default */
guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. Using "
"default value of %i.", arg_names[index], default_value);

return default_value;

}

mode_t result = 0;

/* Check if the provided value has the correct length */
if (strlen(value) != 9) {
/* Log a warning and return the default value if the length is incorrect */
guac_user_log(user, GUAC_LOG_WARNING, "Parameter \"%s\" must have length 9. Using default value.", arg_names[index]);
return default_value;
}

// Define the permission bits
const char *permissions = "rwxr-xr-x";

/* Iterate over each character in the permission string */
for (int i = 0; i < 3; i++) {
for (int j = 0; j < 3; j++) {
int permissionIndex = i * 3 + j;
/* If the character '-' is present in the permission string, no action needed */
if (value[permissionIndex] == '-') {
/* No action needed for '-' */
} else if (value[permissionIndex] == permissions[permissionIndex]) {
// Calculate the permission bit based on the position in the permission string:
// Example: For the first set of three characters ("rwx"):
// S_IRUSR is the constant representing the read permission for the owner.
// (S_IRUSR >> (i * 3 + j)) dynamically adjusts the permission bit based on the position.
// When i = 0 and j takes values 0, 1, 2:
// - For j = 0, it gives (0400 >> 0), which is 0400 (read permission).
// - For j = 1, it gives (0400 >> 1), which is 0200 (write permission).
// - For j = 2, it gives (0400 >> 2), which is 0100 (execute permission).
// These values represent the read, write, and execute permission bits for the owner.
result |= (S_IRUSR >> (permissionIndex));
}
/* If the character is equal 'w' for groups and others log a warning and return the default value */
else if (value[permissionIndex] == 'w') {
guac_user_log(user, GUAC_LOG_WARNING, "Parameter \"%s\" with value \"%s\" has invalid permission character at position %d. Writing is locked for the current running process. Using default value %o.", arg_names[index], value, permissionIndex, default_value);
return default_value;
}
/* If the character is neither 'r', 'w', nor 'x', log a warning and return the default value */
else {
guac_user_log(user, GUAC_LOG_WARNING, "Parameter \"%s\" with value \"%s\" has invalid permission character at position %d. Only 'r', 'w', 'x' and '-' characthers are allowed. Using default value %o.", arg_names[index], value, permissionIndex, default_value);
return default_value;
}
}
}

/* Return the parsed mode_t value */
return result;
}
2 changes: 2 additions & 0 deletions src/protocols/kubernetes/kubernetes.c
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ void* guac_kubernetes_client_thread(void* data) {
settings->recording_path,
settings->recording_name,
settings->create_recording_path,
settings->recording_file_permissions,
settings->recording_path_permissions,
!settings->recording_exclude_output,
!settings->recording_exclude_mouse,
0, /* Touch events not supported */
Expand Down
24 changes: 24 additions & 0 deletions src/protocols/kubernetes/settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ const char* GUAC_KUBERNETES_CLIENT_ARGS[] = {
"recording-exclude-mouse",
"recording-include-keys",
"create-recording-path",
"recording-file-permissions",
"recording-path-permissions",
"read-only",
"backspace",
"scrollback",
Expand Down Expand Up @@ -210,6 +212,18 @@ enum KUBERNETES_ARGS_IDX {
*/
IDX_CREATE_RECORDING_PATH,

/**
* The permissions that should be given to screen recording file which is written in
* the given path.
*/
IDX_RECORDING_FILE_PERMISSIONS,

/**
* The permissions that should be given to screen recording path which is written in
* the given path.
*/
IDX_RECORDING_PATH_PERMISSIONS,

/**
* "true" if this connection should be read-only (user input should be
* dropped), "false" or blank otherwise.
Expand Down Expand Up @@ -389,6 +403,16 @@ guac_kubernetes_settings* guac_kubernetes_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
IDX_CREATE_RECORDING_PATH, false);

/* Parse file permissions flag */
settings->recording_file_permissions =
guac_user_parse_args_mode(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
IDX_RECORDING_FILE_PERMISSIONS, GUAC_KUBERNETES_DEFAULT_RECORDING_FILE_PERMISSIONS);

/* Parse path permissions flag */
settings->recording_path_permissions =
guac_user_parse_args_mode(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
IDX_RECORDING_PATH_PERMISSIONS, GUAC_KUBERNETES_DEFAULT_RECORDING_PATH_PERMISSIONS);

/* Parse backspace key code */
settings->backspace =
guac_user_parse_args_int(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
Expand Down
22 changes: 22 additions & 0 deletions src/protocols/kubernetes/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include <guacamole/user.h>

#include <stdbool.h>
#include <sys/stat.h>
#include <sys/types.h>

/**
* The port to connect to when initiating any Kubernetes connection, if no
Expand All @@ -46,6 +48,16 @@
*/
#define GUAC_KUBERNETES_DEFAULT_RECORDING_NAME "recording"

/**
* The permissions to use for the screen recording file, if not specified.
*/
#define GUAC_KUBERNETES_DEFAULT_RECORDING_FILE_PERMISSIONS (S_IRUSR | S_IWUSR | S_IRGRP)

/**
* The permissions to use for the screen recording path, if not specified.
*/
#define GUAC_KUBERNETES_DEFAULT_RECORDING_PATH_PERMISSIONS (S_IRWXU | S_IRGRP | S_IXGRP)

/**
* Settings for the Kubernetes connection. The values for this structure are
* parsed from the arguments given during the Guacamole protocol handshake
Expand Down Expand Up @@ -208,6 +220,16 @@ typedef struct guac_kubernetes_settings {
*/
bool create_recording_path;

/**
* The permissions to apply for the screen recording file.
*/
mode_t recording_file_permissions;

/**
* The permissions to apply for the screen recording file.
*/
mode_t recording_path_permissions;

/**
* Whether output which is broadcast to each connected client (graphics,
* streams, etc.) should NOT be included in the session recording. Output
Expand Down
2 changes: 2 additions & 0 deletions src/protocols/rdp/rdp.c
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,8 @@ void* guac_rdp_client_thread(void* data) {
settings->recording_path,
settings->recording_name,
settings->create_recording_path,
settings->recording_file_permissions,
settings->recording_path_permissions,
!settings->recording_exclude_output,
!settings->recording_exclude_mouse,
!settings->recording_exclude_touch,
Expand Down
24 changes: 24 additions & 0 deletions src/protocols/rdp/settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ const char* GUAC_RDP_CLIENT_ARGS[] = {
"recording-exclude-touch",
"recording-include-keys",
"create-recording-path",
"recording-file-permissions",
"recording-path-permissions",
"resize-method",
"enable-audio-input",
"enable-touch",
Expand Down Expand Up @@ -564,6 +566,18 @@ enum RDP_ARGS_IDX {
*/
IDX_CREATE_RECORDING_PATH,

/**
* The permissions that should be given to screen recording file which is written in
* the given path.
*/
IDX_RECORDING_FILE_PERMISSIONS,

/**
* The permissions that should be given to screen recording path which is written in
* the given path.
*/
IDX_RECORDING_PATH_PERMISSIONS,

/**
* The method to use to apply screen size changes requested by the user.
* Valid values are blank, "display-update", and "reconnect".
Expand Down Expand Up @@ -1161,6 +1175,16 @@ guac_rdp_settings* guac_rdp_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_CREATE_RECORDING_PATH, 0);

/* Parse file permissions flag */
settings->recording_file_permissions =
guac_user_parse_args_mode(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_RECORDING_FILE_PERMISSIONS, GUAC_RDP_DEFAULT_RECORDING_FILE_PERMISSIONS);

/* Parse path permissions flag */
settings->recording_path_permissions =
guac_user_parse_args_mode(user, GUAC_RDP_CLIENT_ARGS, argv,
IDX_RECORDING_PATH_PERMISSIONS, GUAC_RDP_DEFAULT_RECORDING_PATH_PERMISSIONS);

/* No resize method */
if (strcmp(argv[IDX_RESIZE_METHOD], "") == 0) {
guac_user_log(user, GUAC_LOG_INFO, "Resize method: none");
Expand Down
Loading