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 functionality to write to a temp file through url arguments #747

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vaishalikumanan
Copy link

@vaishalikumanan vaishalikumanan commented Aug 5, 2021

This feature is similar to the existing option that allows url arguments to be passed in as command line arguments. Instead, we create a temporary file in /tmp/ and write the url arguments to this file, separated by newlines. The temporary file name is then passed to the command as a command line argument.
Because of this behaviour, the command line args option and the temporary file option should be mutually exclusive.

We can then use this to pass secret values to the running process as command line arguments are easily visible through process status.

return false;
}
}

Copy link

Choose a reason for hiding this comment

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

need to close the file

@@ -76,6 +76,7 @@ OPTIONS:
-g, --gid Group id to run with
-s, --signal Signal to send to the command when exit it (default: 1, SIGHUP)
-a, --url-arg Allow client to send command line arguments in URL (eg: http://localhost:7681?arg=foo&arg=bar)
-f, --arg-file File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})
Copy link

Choose a reason for hiding this comment

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

Suggested change
-f, --arg-file File prefix for a unique generated temp file that URL arguments are written to (ex. /tmp/prefix); the generated file's full path is then passed in as a command line argument (ex. /tmp/prefix{unique string})
-f, --arg-file Similar to --url-arg but the command line argument is a file containing new line separated values

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if this would make it clear that the argument file prefix needs to first be passed in and that the contents would be the URL arguments

Choose a reason for hiding this comment

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

I think we should also say that the child process can do whatever it wants with the file (including deleting it)

(how you say it is a matter of taste and given that im not a maintainer of this repo, i will not give my suggestion :p )

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with @dotslash. and we need to tell user that the temp file will not be deleted by ttyd, or add some logic to delete it automatically on websocket closing?

Copy link

Choose a reason for hiding this comment

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

added a note to the end

src/server.c Outdated
@@ -73,6 +73,7 @@ static const struct option options[] = {
{"ssl-key", required_argument, NULL, 'K'},
{"ssl-ca", required_argument, NULL, 'A'},
{"url-arg", no_argument, NULL, 'a'},
{"arg-file", required_argument, NULL, 'f'},
Copy link

Choose a reason for hiding this comment

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

why is this required argument?

Copy link
Author

Choose a reason for hiding this comment

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

I believe an optional argument must be passed in without a space (-f{argument} instead of -f {argument}). I figured this might be confusing for users when the rest of the options can be listed as -{option} {argument}, especially since using -f {argument} would make ttyd try to use this argument as a command instead.

If this isn't an issue, I can change the argument to be optional.

Choose a reason for hiding this comment

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

TLDR: We can hold off discussion on this until ttyd maintainers respond as i feel this is a matter of taste mostly and we should please the reviewer 😛

skimmed through https://man7.org/linux/man-pages/man3/getopt.3.html. It seems the for optional arguments -f arg -f=arg both should work.

believe an optional argument must be passed in without a space
have you checked it?

In either case, optional might be the right thing to do. But if its an optional argument is there a way to distinguish between the the user running ttyd -f mycmd and ttyd mycmd?

IMO Ideally (you might have different thoughts, which is reasonable)

  1. ttyd mycmd : Our code should not trigger
  2. ttyd -f /tmp/myprefix mycmd : Our code should trigger and create a tmp file with /tmp/myprefix prefix
  3. ttyd -f mycmd : Our code should trigger and create a tmp file with /tmp/ prefix (or something like /tmp/ttyd-args- prefix)

overall this i feel this is quite opinionated!

Choose a reason for hiding this comment

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

also as @kahing was saying in another comment, not supporting the following is quite reasonable

ttyd -f /tmp/myprefix mycmd : Our code should trigger and create a tmp file with /tmp/myprefix prefix

Copy link
Author

Choose a reason for hiding this comment

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

I agree with the cases you have listed there. I've tried making this argument optional but it seems like only cases 1 and 3 work as expected. For ttyd -f /tmp/myprefix mycmd, /tmp/myprefix isn't being passed in as an optional argument for -f, rather it is being used as the ttyd command.

Running ttyd -f=/tmp/myprefix mycmd sets the prefix to =/tmp/myprefix and running ttyd -f/tmp/myprefix mycmd sets the prefix to /tmp/myprefix.
There's a discussion about this behaviour here.

If this behaviour isn't too confusing, I can make the argument optional.

Alternatively, we could do something like -f -prefix /tmp/myprefix where -f has no argument (turns on the url argument to file feature) and -prefix has a required argument that specifies a path. Then if only -f is specified, we use some default prefix like /tmp/ttyd-args.

src/protocol.c Outdated
}
else if (server->arg_file != NULL) {
int fd = -1;
char *filePath = strcat(server->arg_file, "XXXXXX");
Copy link

Choose a reason for hiding this comment

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

I think we should just choose a prefix here and not require users to choose a prefix

Copy link

Choose a reason for hiding this comment

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

also this is not a safe usage of strcat, since you may write past the allocated memory

src/protocol.c Outdated
}
else if (server->arg_file != NULL) {

Choose a reason for hiding this comment

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

nit:

typical code style

if (...) {
 ...
} else if (..) {
 ..
} else {
 ...
}

as opposed to

if (...) {
 ...
} 
else if (..) {
 ..
} 
else {
 ...
}

consistent with ttyd :

} else {

src/server.c Outdated
@@ -73,6 +73,7 @@ static const struct option options[] = {
{"ssl-key", required_argument, NULL, 'K'},
{"ssl-ca", required_argument, NULL, 'A'},
{"url-arg", no_argument, NULL, 'a'},
{"arg-file", required_argument, NULL, 'f'},

Choose a reason for hiding this comment

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

TLDR: We can hold off discussion on this until ttyd maintainers respond as i feel this is a matter of taste mostly and we should please the reviewer 😛

skimmed through https://man7.org/linux/man-pages/man3/getopt.3.html. It seems the for optional arguments -f arg -f=arg both should work.

believe an optional argument must be passed in without a space
have you checked it?

In either case, optional might be the right thing to do. But if its an optional argument is there a way to distinguish between the the user running ttyd -f mycmd and ttyd mycmd?

IMO Ideally (you might have different thoughts, which is reasonable)

  1. ttyd mycmd : Our code should not trigger
  2. ttyd -f /tmp/myprefix mycmd : Our code should trigger and create a tmp file with /tmp/myprefix prefix
  3. ttyd -f mycmd : Our code should trigger and create a tmp file with /tmp/ prefix (or something like /tmp/ttyd-args- prefix)

overall this i feel this is quite opinionated!

@@ -321,6 +324,11 @@ int main(int argc, char **argv) {
break;
case 'a':
server->url_arg = true;
server->arg_file = NULL;

Choose a reason for hiding this comment

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

i think we might want to add a comment in help text, documentation that if both are set, behaviour is non deterministic.

Alternatively we can make a choice that if both are set url_arg will win (or vice versa)

Based on that you want to structure your if .. else if .. in https://github.com/tsl0922/ttyd/pull/747/files#diff-0b6794e089b51873f4effd373d78e8d13d1ee1ca83c0dd1394b09bcad26254c7R105
(eitherways a comment will be helpful)

Copy link
Author

Choose a reason for hiding this comment

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

The behaviour shouldn't be non deterministic here; the last specified argument of -f and -a will be set. I can add this to the help text for clarity.

src/protocol.c Outdated
}
else if (server->arg_file != NULL) {
int fd = -1;
// mkstemp requires the file path to have suffix XXXXXX (len 7)

Choose a reason for hiding this comment

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

XXXXXX is len 6. no?

Choose a reason for hiding this comment

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

int file_path_len = strlen(server->arg_file) + 6 /*XXXXXX*/ + 1 /*null character*/;

Choose a reason for hiding this comment

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

(subjective opinion)
maybe strcpy,strcat might be "simpler" to follow

strcpy(filePath, server->arg_file);
strcat(filePath, "XXXXXX");

Copy link
Author

Choose a reason for hiding this comment

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

Yeah XXXXXX has len 6, but with the null terminator it's 7. I'll change this line to be more clear.

Copy link
Author

Choose a reason for hiding this comment

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

I was using strcat before but it might not be safe: #747 (comment)

src/protocol.c Outdated
}

argv[n++] = filePath;
close(fd);

Choose a reason for hiding this comment

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

you might want to

int close_fd = close(fd)
if (close_fd != 0) {
 ... error handling ...
 return false
}
argv[n++] = filePath;

@tsl0922 tsl0922 force-pushed the main branch 3 times, most recently from f09db36 to 1415e5c Compare August 16, 2021 15:55
@tsl0922
Copy link
Owner

tsl0922 commented Aug 18, 2021

writing to a temp file without cleanup? I prefer setting it as env

@kahing
Copy link

kahing commented Aug 18, 2021

@tsl0922 you are right that with temp file, we are relying on the receiver to delete the file. we considered passing via env var (#745) and one side effect is that env vars are easily visible if you run ps e.

While it's true that users with permissions to /proc/<pid>/environ would also have permission to read the temp file, internal security review prefers we reduce the likelihood of accidental information leakage.

Would you prefer if we add both the env and file options? We will only use the file option but maybe others will find the env option helpful too.

@kahing
Copy link

kahing commented Aug 25, 2021

@tsl0922 any more thoughts on this?

for (i = 0; i < pss->argc; i++) {
if (dprintf(fd, "%s\n", pss->args[i]) < 0) {
lwsl_err("Write to temp file failed with error: %d (%s)\n", errno, strerror(errno));
return false;
Copy link
Owner

Choose a reason for hiding this comment

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

this will leak fd too.

Copy link
Owner

Choose a reason for hiding this comment

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

filePath is not freed too.

Copy link

Choose a reason for hiding this comment

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

fixed

@tsl0922
Copy link
Owner

tsl0922 commented Aug 27, 2021

@kahing sorry I'm a little busy this week, I'm OK with this feature, will review it soon.

src/protocol.c Outdated

if (close(fd) != 0) {
lwsl_err("Close temp file failed with error: %d (%s)\n", errno, strerror(errno));
return false
Copy link
Owner

Choose a reason for hiding this comment

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

missing ;

Copy link

Choose a reason for hiding this comment

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

fixed

src/protocol.c Outdated
char *filePath = xmalloc(file_path_len);
snprintf(filePath, file_path_len, "%sXXXXXX", server->arg_file);

if ((fd = mkstemp(filePath)) != -1) {
Copy link
Owner

Choose a reason for hiding this comment

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

should be == -1 ?

Copy link

Choose a reason for hiding this comment

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

fixed

@kahing
Copy link

kahing commented Sep 2, 2021

@tsl0922 I updated the PR and addressed your comments. Let me know if you have more feedback

@kahing
Copy link

kahing commented Sep 9, 2021

@tsl0922 Hi any updates?

1 similar comment
@kahing
Copy link

kahing commented Sep 28, 2021

@tsl0922 Hi any updates?

@kahing
Copy link

kahing commented Oct 27, 2021

@tsl0922 hi any updates?

@kahing
Copy link

kahing commented Dec 13, 2021

@tsl0922 Hi any updates on this PR?

@vaishalikumanan
Copy link
Author

Hi @tsl0922, would you still be interested in this feature?

@kahing
Copy link

kahing commented Jun 7, 2023

@tsl0922 hi this patch is a bit outdated, but we are happy to do the work to update it if you want to take this

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.

4 participants