CLI improvements #3991

walid-git · 2023-09-27T13:42:18Z

No description provided.

nigoroll · 2023-09-27T13:49:25Z

bin/varnishd/mgt/mgt_cli.c

+	for (i = 0; i < ncmds; i++) {
+		if (strcmp(av[1], cmds[i]->request))
+			continue;
+		if (cmds[i]->flags & CLI_F_INTERNAL) {


should we be so secretive about internal commands to not even expose the fact that they are (put aside the potential for a timing side channel)? I would think that saying "this is an internal command" should be fine?

I think we shouldn't expose more than necessary. We are changing the CLI commands table in this very patch series, and I would personally prefer to keep the ability to change the table as we see fit without being concerned about API stability for end-users (especially in the context of back-ports to older branches).

For third-party utilities, I would refer to help -j, like varnishadm does since 38506b3.

What is the point of exposing commands that can/should not be used anyway ? Internal commands are meant to be only issued by the MGT process, so in my opinion, nothing else should be aware of it.

My point is that we should not make it harder than it already is to hunt down errors.

lib/libvarnish/vcli_serve.c

nigoroll · 2023-09-27T13:58:51Z

bin/varnishd/mgt/mgt_cli.c

-		break;
+	cmd = mgt_cmd_lookup(av[1]);
+	if (cmd != NULL && cmd->flags & CLI_F_INTERNAL) {
+		VCLI_Out(cli, "Unknown request.\nType 'help' for more info.\n");


similar to a previous comment: Would it be reasonable to expose that the command is internal?

Since we are reaching out to the mgt CLI server, I would again refrain from mentioning implementation details from the cache CLI server.

bin/varnishd/cache/cache_cli.c

lib/libvarnish/vcli_serve.c

nigoroll · 2023-09-27T14:02:49Z

include/Makefile.am

@@ -12,7 +12,6 @@ nobase_pkginclude_HEADERS = \
 	tbl/beresp_flags.h \
 	tbl/boc_state.h \
 	tbl/body_status.h \
-	tbl/cli_cmds.h \


would it not make sense to keep the commands available to tools?

See previous comment referring to 38506b3.

walid-git · 2023-11-10T16:29:06Z

Added a callback in struct cli_cmd_desc that allows sensitive commands to provide their own logging function. If a sensitive command does not provide an implementation, then only the command name followed by (hidden) will be logged (no arguments). The command response is also logged as (hidden).
Added coverage for internal and sensitive commands by introducing debug commands with the respective flags.
Unknown commands are no longer forwarded to the child, this will prevent command smuggling using quotes.
Also, fixed a bug introduced by cd2cfbd where the last command's response would be logged twice in syslog if cli file doesn't end with a new line.

Some cli commands have flags (h,i,*) that are never parsed in the code, they can safely be removed.

Flags are much more convenient to handle when represented as bits rather than a list of characters. In addition, they better fit in the struct cli_cmd_desc than the struct cli_proto

Internal commands are commands that are only intended to be used for internal communication between MGT process and the child process. Thus, they should not be exposed to the user in cli help/man page.

Hiding internal commands in help is not sufficient, we must ensure they are not executed by the user through cli and handle them as unknown commands in that case.

Since we only have two authentication levels and that we now have proper flags, we can use them to distinguish commands that need an authenticated cli to be used, and get rid of the auth attribute in struct cli_proto. MCF_AUTH and MCF_NOAUTH aren't useful anymore and can be safely removed.

Make the cli function lookup an independent function, and call it prior to the "before" callback method. This will allow to conditionnally execute instructions in the callback method depending on the cli function we are currently handling.

dridi

I understand that the sensitive callback was agreed upon during a bugwash but I don't like how it was articulated.

include/tbl/cli_cmds.h

include/tbl/debug_bits.h

bin/varnishd/cache/cache_cli.c

lib/libvarnish/vcli_serve.c

include/vcli_serve.h

include/tbl/cli_cmds.h

Sensitive commands are commands that accept/return sensitive data as argument/output (ex: private keys). This commit prevents commands with the CLI_F_SENSITIVE flag and their responses from being logged to syslog/vsl. Sensitive commands can provide a custom logging function to define how the command and argument should be logged. If a sensitive command does not provide an implementation, then only the command name followed by "(hidden)" will be logged. For the command response, the status will be logged followed by the string "(hidden)".

When setting +cli_show_sensitive to the debug parameter, all commands will be logged to syslog (if syslog_cli_traffic is enabled) and vsl regardless if they are sensitive or not.

Cli help command does not show commands that we don't have the required auth level to execute. However, when the child process is running and the mgt receives a help command, it asks the child to execute it and return the response to the client. The child sees the command as comming from mgt, which has the highest auth level, and thus displays all the commands. This commit prevents this from happening by not forwarding the help command to child when the cli is not authentified.

This lock was used to guard the child VCLS, however we only interract with it through the cli_thread, which makes the locking pointless.

Make sure that we only interract with a VCLS through the thread that created it.

Since mgt is aware of all known cli commands, unknown commands should be blocked by mgt and not forwarded to the child process to prevent any malicious command smuggling (using quotes for example).

walid-git · 2023-12-26T14:07:49Z

Moved the command logging callback to struct cli_cmd_desc as suggested by dridi so that it can be overridden per command implementation. If the sensitive command is a cache process command, the custom log implementation cannot be used in mgt and the command will be logged with the default logging of sensitive commands (command name with hidden arguments).
Also addressed other minor items. Ready for another review

bsdphk · 2024-01-30T11:40:49Z

I have looked over this PR in relation to the VIPC work I'm doing to enable a future CERT subprocess.

This PR attacks some of the issues we need to solve, but I'm not sure those are the ones we need to or should fix first, nor that they should be fixed this particular way.

In the future world order, we will have multiple "entities" which offer CLI commands: MGT, WRK, CERT but also VMODs and VEXTs, and the MGT needs to know where (and when!) to send commands. (This also implies that we do not know all CLI commands at compile-time any more!)

We will also be passing file-descriptors around in association with CLI commands (the "smuggling" facility).

And finally we will have distinct levels of CLI authority: Being able to load a new CERT is a different authority from loading a new VCL, from adding a BAN and from mucking with a backend (for unspecified values of mucking.)

I'm as of yet undecided if we will also have CLI multiplexing, for instance being able to do a "backend.list" while a new VCL is being compiled, but I'll probably default to "Since we're ripping all this apart anyway…"

As for the logging, that's an entire different kettle of fish. It is not obvious to me that CLI logging belongs in the subprocesses after this reconstruction, maybe MGT needs to grow it's own logging facility (or just use syslog?) and be responsible for all logging related to CLI, including thinking about which authority logs where.

So I'm inclined to call this PR a very valuable prototype, and throw it away (as Brooks commands us.)

nigoroll approved these changes Sep 27, 2023

View reviewed changes

walid-git force-pushed the cli_sensitive_internal_cmd branch from a2b09e5 to 5ac900a Compare November 10, 2023 16:16

walid-git marked this pull request as ready for review November 10, 2023 16:29

walid-git added 6 commits November 29, 2023 09:45

cli: Remove unused flags

d6299c3

Some cli commands have flags (h,i,*) that are never parsed in the code, they can safely be removed.

cli: Turn cmd flags into bitmask and move to struct cli_cmd_desc

860d12f

Flags are much more convenient to handle when represented as bits rather than a list of characters. In addition, they better fit in the struct cli_cmd_desc than the struct cli_proto

cli: Hide internal commands in help and man

512a43e

Internal commands are commands that are only intended to be used for internal communication between MGT process and the child process. Thus, they should not be exposed to the user in cli help/man page.

cli: Prohibit internal commands when called externally

322efa3

Hiding internal commands in help is not sufficient, we must ensure they are not executed by the user through cli and handle them as unknown commands in that case.

cli: Refactor cls_exec

82558a2

Make the cli function lookup an independent function, and call it prior to the "before" callback method. This will allow to conditionnally execute instructions in the callback method depending on the cli function we are currently handling.

walid-git force-pushed the cli_sensitive_internal_cmd branch from 5ac900a to f780ada Compare November 29, 2023 08:53

dridi requested changes Dec 6, 2023

View reviewed changes

walid-git added 9 commits December 26, 2023 10:01

cli: Add cli_show_sensitive to debug parameters

4108c4b

When setting +cli_show_sensitive to the debug parameter, all commands will be logged to syslog (if syslog_cli_traffic is enabled) and vsl regardless if they are sensitive or not.

cache_cli: Get rid of superfluous cli_mtx

d28524b

This lock was used to guard the child VCLS, however we only interract with it through the cli_thread, which makes the locking pointless.

cli: Assert VCLS calls are from owning thread

b62ea87

Make sure that we only interract with a VCLS through the thread that created it.

Makefile.am: Make cli_cmds.h private

1825eb2

cli: Use macro for flags check

1bd61cf

cli: Add coverage for internal and sensitive commands

9bebd98

mgt_cli: Don't pass unknown commands to the child

a8866de

Since mgt is aware of all known cli commands, unknown commands should be blocked by mgt and not forwarded to the child process to prevent any malicious command smuggling (using quotes for example).

walid-git force-pushed the cli_sensitive_internal_cmd branch from f780ada to a8866de Compare December 26, 2023 13:47

dridi marked this pull request as draft January 3, 2024 15:24

bsdphk added a=Bugwash Today and removed a=Bugwash Today labels Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI improvements #3991

CLI improvements #3991

walid-git commented Sep 27, 2023

nigoroll Sep 27, 2023

dridi Sep 28, 2023

walid-git Sep 28, 2023

nigoroll Nov 13, 2023

nigoroll Sep 27, 2023

dridi Sep 28, 2023

nigoroll Sep 27, 2023

dridi Sep 28, 2023

walid-git commented Nov 10, 2023 •

edited

dridi left a comment

walid-git commented Dec 26, 2023

bsdphk commented Jan 30, 2024 •

edited

CLI improvements #3991

Are you sure you want to change the base?

CLI improvements #3991

Conversation

walid-git commented Sep 27, 2023

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

walid-git commented Nov 10, 2023 • edited

dridi left a comment

Choose a reason for hiding this comment

walid-git commented Dec 26, 2023

bsdphk commented Jan 30, 2024 • edited

walid-git commented Nov 10, 2023 •

edited

bsdphk commented Jan 30, 2024 •

edited