Tighten up module loading and custom command registration

[prvyk]

Garnet documentation says that custom command registration requires defining ExtensionBinPaths.

This isn't what the code actually does. If no path is defined, a user can give an absolute path and the module/command will be loaded just fine if user has admin permissions or ACL is not set up.

It's a dangerous ability since user can load code to run under the server's context, and therefore I believe should be more restricted.

This PR:

• Adds a check to ensure module loading is disabled if paths are not defined. This is overriden when --loadmodulecs command line option is used.

• --loadmodulecs did not check module signature, which seems counterintuitive and unnecessary. The option text doesn't mention this, there's a switch to enable loading unsigned modules so the expectation is opposite, anyone who could add one option can add another, and Garnet should have a safe default. Also, change option text to be more descriptive.

• Add supports to redis's enable-module-command directive for compatibility, though the default is to only use Garnet's restriction mechanism. Add to redis.conf import test. It works same way as enable-debug-command: Access to commands MODULE and REGISTERCS is limited by connection type. Default is 'yes' (no limit except by ExtensionBinPath), and other options are 'local' (only for local connections) or 'no' (disable the commands entirely).

• Since I was already touching the same file, add support for DEBUG LOG for testing use.

[TalZaccai]

If no allowed bin paths are specified, no user should be able to register anything, so it is in fact disabled by default.
Which user should be allowed to run a specific command should be defined in the ACL.
I don't understand why we need this extra configuration parameter...

[prvyk]

If no allowed bin paths are specified, no user should be able to register anything, so it is in fact disabled by default.

Ah, that's the intent? Because this doesn't work. Just use an absolute path for the module command (when this allowed bin path isn't defined at all) and Garnet will load anything.

I don't understand why we need this extra configuration parameter... Which user should be allowed to run a specific command should be defined in the ACL.

Because the other mechanism didn't work and I wasn't sure if this was intentional. Also, ACL is disabled by default. The current situation is that module loading is enabled by default so long as the user uses an absolute path. (Also the extra parameter is used in redis and it's a bit different type of blocking)

Can I pivot this part to fixing the bin path check? My suggestion:

1. Fix the path check to disable anything when path is not defined, unless --loadmodulecs is used.
2. Keep the new 'check signatures for loadmodulecs option'.
3. Keep the new parameter for redis compat, but change it to allow by default and hide it.

[TalZaccai]

If no allowed bin paths are specified, no user should be able to register anything, so it is in fact disabled by default.

Ah, that's the intent? Because this doesn't work. Just use an absolute path for the module command (when this allowed bin path isn't defined at all) and Garnet will load anything.

I don't understand why we need this extra configuration parameter... Which user should be allowed to run a specific command should be defined in the ACL.

Because the other mechanism didn't work and I wasn't sure if this was intentional. Also, ACL is disabled by default. The current situation is that module loading is enabled by default so long as the user uses an absolute path. (Also the extra parameter is used in redis and it's a bit different type of blocking)

Can I pivot this part to fixing the bin path check? My suggestion:

1. Fix the path check to disable anything when path is not defined, unless --loadmodulecs is used.
2. Keep the new 'check signatures for loadmodulecs option'.
3. Keep the new parameter for redis compat, but change it to allow by default and hide it.

Yeah if that's the case that's a very bad oversight on our part...
From a quick look it seems like in ModuleUtils.LoadAssemblies you need to return an error even when allowedExtensionPaths is null. (so if (allowedExtensionPaths == null || binaryFiles.Any(f =>...)))

[prvyk]

If no allowed bin paths are specified, no user should be able to register anything, so it is in fact disabled by default.
Can I pivot this part to fixing the bin path check? My suggestion:

1. Fix the path check to disable anything when path is not defined, unless --loadmodulecs is used.
2. Keep the new 'check signatures for loadmodulecs option'.
3. Keep the new parameter for redis compat, but change it to allow by default and hide it.

Yeah if that's the case that's a very bad oversight on our part... From a quick look it seems like in ModuleUtils.LoadAssemblies you need to return an error even when allowedExtensionPaths is null. (so if (allowedExtensionPaths == null || binaryFiles.Any(f =>...)))

Did this. Left an opening to specify an absolute path via --loadmodulecs. If you can start a server you have the same permissions anyway, so it's not a hole?

Without looking at the git history, I suspect that's the underlying cause: --loadmodulecs was left open as much as possible (to make testing modules easier?), but opening that path accidentally opened all the module loading paths.