Quotes must be escaped in sensuctl command exec arguments

[amdprophet]

Expected Behavior

String arguments wrapped in quotes should not need to be escaped.

Current Behavior

String arguments only work when they are escaped.

Possible Solution

We will likely need to change how Cobra parses any additional arguments.

Steps to Reproduce (for bugs)

See screenshot.

Screenshot

[amdprophet]

I ensured that Go parses these arguments correctly:

pi@sensu-backend-1:~/code/argtest$ ./argtest test 1 2 -- --command 'echo "hello world"' --subscriptions linux
args length: 9
arg[0]: ./argtest
arg[1]: test
arg[2]: 1
arg[3]: 2
arg[4]: --
arg[5]: --command
arg[6]: echo "hello world"
arg[7]: --subscriptions
arg[8]: linux

I'm expecting the bug to exist in Cobra or the way we're using Cobra.

[nikkictl]

After getting distracted by https://github.com/calebhailey/sensu-runbook/pull/2, https://github.com/sensu/sensu-staging/pull/222, and https://github.com/sensu/sensu-go-qa-crucible/pull/127, I was able to reproduce and confirm that cobra is actually loading in the args correctly here. Then, we correctly pass those args to the command execution for unix and windows. However, unless the quotes are escaped in the respective shell syntax, sensu doesn’t add those quotation characters back in, thus creating execution errors for fields that have spaces.

I've done much research around how arguments are read in golang (shell specific) and passed to command execution, and I've determined 2 courses of action:

1. Rule that this is not a bug and leave it to the respective shell to escape characters how the operator sees fit
2. Implement a hack to surround all non-flag command arguments with quotations using strconv.Quote (see hacked branch, https://github.com/sensu/sensu-go/compare/master...hack/quoted-args)

Neither approach is really clean, but then again, when is dealing with multiple shells really ever clean?

[nikkictl]

Based on some Slack discussion, it was agreed that we should follow suggestion 1, passing the contents of the arguments as faithfully as possible to the command.

[ccressent]

Thank you for spending time on this @nikkictl, and sorry for only having a look now after you asked for my opinion last week.

While 1. sort of works, it's very counter-intuitive if not borderline wrong: as an operator, I shouldn't have to know about the internals of how those plugins work in order to guess how I should quote my arguments! Plus it removes the "seamlessness" of the plugin experience. In my opinion, using those plugins should look as close as possible as having an enhanced sensuctl.

I think you're onto something with 2., but I don't think it's necessarily a hack if done in a slightly different way. And I completely agree when you mention we should pass the arguments as faithfully as possible.

To me, the bug is here: https://github.com/sensu/sensu-go/blob/2c567fca6f677de7c704cbce8d2a6b12b6e6e490/cli/cmdmanager/manager.go#L256

Regardless of shell, it all boils down to what programs see as their argv, and we break argv along the way with this Join(). When we run:

program --command 'echo "Hello World"'

The argv array is:

argv[0] = "program"
argv[1] = "--command"
argv[2] = "echo "Hello World""

When we join with a space, we end up with program --command echo "Hello World", which has an equivalent argv array of:

argv[0] = "program"
argv[1] = "--command"
argv[2] = "echo"
argv[3] = "Hello World"

As you can see, we broke the semantics of the initial command! I think you had correctly figured that out and wanted to counter that by quoting some arguments. But here is what I would add:

• you can quote every single element of argv without breaking anything
• if you quote with ' instead of ", that properly preserves the argument
• the only exception is that we can't have ' within 's

I do think there is a bug here and that we should re-open the issue. I currently have something that mostly work, but that last exception is proving to be a pain. We probably need to better understand the relevant POSIX chapter.

What do you think @nikkictl?

[nikkictl]

Echoing @echlebek's argument here:

I believe users will need to, in many cases, use single quotes to form the full set of arguments so that the shell leaves the contents alone. This is actually quite common for command line tools. We should pass the contents as faithfully as possible to the command, without trying to be clever, IMO.

I totally agree that as an operator, I would want a straightforward and simple way to define my arguments without having to know the internal workings of the command doing the parsing. However I think the concern is when a plugin has more complex argument expectations, for example a program that requires certain special characters to be escaped.

I can modify the branch I shared to fit the additional requirements, but there is much more at play when you consider compatibility with Windows powershell.

[nikkictl]

God's speed, @ccressent