Add functionality to write to a temp file through url arguments #747

vaishalikumanan · 2021-08-05T21:04:07Z

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.

kahing · 2021-08-05T21:18:32Z

src/protocol.c

+        return false;
+      }
+    }
+


need to close the file

kahing · 2021-08-06T18:34:42Z

README.md

@@ -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})


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

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

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 )

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?

added a note to the end

kahing · 2021-08-06T18:38:02Z

src/server.c

@@ -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'},


why is this required argument?

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.

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)

ttyd mycmd : Our code should not trigger

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

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!

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

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.

kahing · 2021-08-06T18:38:56Z

src/protocol.c

  }
+  else if (server->arg_file != NULL) {
+    int fd = -1;
+    char *filePath = strcat(server->arg_file, "XXXXXX");


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

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

dotslash · 2021-08-11T21:52:58Z

src/protocol.c

  }
+  else if (server->arg_file != NULL) {


nit:

typical code style

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

as opposed to

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

consistent with ttyd :

ttyd/src/protocol.c

Line 65 in 5a3ad2f

} else {

dotslash · 2021-08-11T22:05:26Z

src/server.c

@@ -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'},


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)

ttyd mycmd : Our code should not trigger

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

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!

dotslash · 2021-08-11T22:09:42Z

src/server.c

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


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)

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.

dotslash · 2021-08-11T22:11:45Z

src/protocol.c

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


XXXXXX is len 6. no?

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

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

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

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

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

dotslash · 2021-08-11T22:21:32Z

src/protocol.c

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


you might want to

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

tsl0922 · 2021-08-18T16:31:43Z

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

kahing · 2021-08-18T18:19:53Z

@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 · 2021-08-25T22:20:50Z

@tsl0922 any more thoughts on this?

tsl0922 · 2021-08-27T14:25:40Z

src/protocol.c

+    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;


this will leak fd too.

filePath is not freed too.

tsl0922 · 2021-08-27T14:48:16Z

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

tsl0922 · 2021-08-27T15:00:22Z

src/protocol.c

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


missing ;

tsl0922 · 2021-08-27T15:31:40Z

src/protocol.c

+    char *filePath = xmalloc(file_path_len);
+    snprintf(filePath, file_path_len, "%sXXXXXX", server->arg_file);
+
+    if ((fd = mkstemp(filePath)) != -1) {


should be == -1 ?

kahing · 2021-09-02T19:45:04Z

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

kahing · 2021-09-09T23:03:24Z

@tsl0922 Hi any updates?

kahing · 2021-09-28T20:54:05Z

@tsl0922 Hi any updates?

kahing · 2021-10-27T21:52:11Z

@tsl0922 hi any updates?

kahing · 2021-12-13T19:09:48Z

@tsl0922 Hi any updates on this PR?

vaishalikumanan · 2023-05-03T21:02:39Z

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

kahing · 2023-06-07T17:44:33Z

@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

Add functionality to write to a temp file through url arguments

ed9511a

kahing reviewed Aug 5, 2021

View reviewed changes

src/protocol.c

return false;

}

}

Copy link

kahing Aug 5, 2021

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

vaishalikumanan reacted with thumbs up emoji

vaishalikumanan added 2 commits August 6, 2021 12:09

close file

cde0982

Add argument for temp file path/prefix

25f91ea

vaishalikumanan force-pushed the add-tmp-file branch from 8a7db24 to 25f91ea Compare August 6, 2021 17:17

kahing reviewed Aug 6, 2021

View reviewed changes

vaishalikumanan added 2 commits August 9, 2021 13:28

Concat temp file suffix without strcat

53565c7

use snprintf instead of sprintf

5a3ad2f

dotslash reviewed Aug 11, 2021

View reviewed changes

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

Reformat code

4be5009

kahing mentioned this pull request Aug 18, 2021

Add functionality to write to a temp file through url arguments databricks/databricks-ttyd#5

Merged

tsl0922 reviewed Aug 27, 2021

View reviewed changes

kahing added 4 commits August 27, 2021 16:09

Merge branch 'main' into add-tmp-file

a487955

fix build and error check

092dc3d

fix a merge problem and addressed feedback

b9b962b

add a note to -f

c01676c

tsl0922 force-pushed the main branch from bf89510 to 2f7e0e0 Compare October 18, 2022 15:30

tsl0922 force-pushed the main branch 2 times, most recently from df062bf to 7b300d5 Compare November 3, 2022 03:20

tsl0922 force-pushed the main branch 4 times, most recently from 4a52209 to a8cae75 Compare January 18, 2023 03:43

tsl0922 force-pushed the main branch from 818cac2 to 7f2d51c Compare April 28, 2023 14:53

tsl0922 force-pushed the main branch 4 times, most recently from a27c81d to ae4a7c4 Compare October 29, 2023 08:46

	-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

Add functionality to write to a temp file through url arguments #747

Are you sure you want to change the base?

Add functionality to write to a temp file through url arguments #747

Conversation

vaishalikumanan commented Aug 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsl0922 commented Aug 18, 2021 • edited

kahing commented Aug 18, 2021

kahing commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsl0922 commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahing commented Sep 2, 2021

kahing commented Sep 9, 2021

kahing commented Sep 28, 2021

kahing commented Oct 27, 2021

kahing commented Dec 13, 2021

vaishalikumanan commented May 3, 2023

kahing commented Jun 7, 2023

vaishalikumanan commented Aug 5, 2021 •

edited

tsl0922 commented Aug 18, 2021 •

edited