Add support for sensitive arguments and environment variables

[TibbsTerry]

Sensitive parameters can currently be exposed in command details and exception details. Added SensitiveString class to ensure this information is not exposed in logging or exception details

[Tyrrrz]

What's the use for this? Masking secrets is generally something that should be done by the environment (e.g. your CI provider).

Also, you should've really created an issue first so that this could've been discussed. There is arguably a simpler and more efficient solution: add an option that disables printing command line arguments in Command.ToString() in exceptions. But even then I doubt that it's worth it given that it should already be handled upstream.

[TibbsTerry]

I agree that SecureString is probably overkill. (Though I wouldn't say that it 'doesn't do anything on .NET')

An example use case would be passing a connection string. dotnet ef database update 20220421113221_InitialCreate --connection "Server=server;Database=database;Username=user;Password=Password" A user would want to pass the connection argument as above, but say have a masked value Server=server;Database=database;Username=user;Password=***** that displays in any output (eg logging, exception messages etc)

The masked value(s) should be returned in Command.ToString(), but masking the whole command would be inappropriate as details of the other arguments should still be visible. ie logging of Command should show masked values for 'sensitive' arguments

logger.LogInformation($"Command executing:  {command}");
/// Command executing:  dotnet ef database update 20220421113221_InitialCreate --connection "Server=server;Database=database;Username=user;Password=*****"

The only sensible way I see to support this would involve breaking changes Changing ICommandConfiguration.Arguments & ICommandConfiguration.EnvironmentVariables to something like:

IReadOnlyDictionary<string, Argument> Arguments { get; }
IReadOnlyDictionary<string, Argument?> EnvironmentVariables { get; }

(I think Arguments would be better modelled as a collection anyway) where Argument is along the lines of:

public class Argument
{
    private readonly string _value;
    private readonly string _mask;
    private readonly bool _sensitive;

    public Argument(string value, string mask, bool sensitive, bool escape)
    {
        _value = value;
        _mask = mask;
        _sensitive = sensitive;
        Escape = escape;
    }

    public override string ToString() => _sensitive ? _mask : _value;

    internal string InsecureString => _value;

    public bool Escape { get; }
}

InsecureString and Escape to be used to build up the actual command line used (as in ArgumentsBuilder logic)

[Tyrrrz]

I would argue that this should be the responsibility of the logger. Otherwise, what about secrets potentially leaked in other places? What if someone forgets to set the sensitive flag? The only effective way to avoid leaking secrets is by replacing them right before they're logged.

Also, in your example, the whole value of the --connection option will be masked out, not just the password part.

[TibbsTerry]

Note, the example above the mask is not '*****' but 'Server=server;Database=database;Username=user;Password=*****'

Not exposing secrets is a responsibility along all of the chain

• The calling application (ie the logger here) should not expose secrets in logging or 'other places'
• CliWrap should not expose secrets. Here in Command.ToString() and exception details
• and the cli tool should not expose secrets, for example in StdOut

Replacing the secrets right before they are logged is actually a lot of work for a user of the CliWrap library. You would have to override Command.ToString, CommandExecutionException CliWrapException somehow or parse their messages. For example, you would have to probably

• catch instances of CommandExecutionException
• parse the CommandExecutionException.Message
• generate a new message with secrets masked
• and then rethrow an a different (custom) exception

Just parsing the exception message is actually quite involved. First a regex to find the 'Command:' bit. Then replacing {command.Arguments}, which is not just a simple replace because the arguments may also be escaped. This would involve replicating most of the logic in ArgumentsBuilder.Escape.

For me it was easier (& more flexible) to fork the library than do all of the above.

[Tyrrrz]

Replacing the secrets right before they are logged is actually a lot of work for a user of the CliWrap library. You would have to override Command.ToString, CommandExecutionException CliWrapException somehow or parse their messages. For example, you would have to probably

Hmm, maybe I'm missing something but what I meant was something like this:

public class ScrubbingLogger : ILogger
{
    private readonly ILogger _innerLogger;
    private readonly SecretsProvider _secretsProvider;

   // ...

   // Simplified, I don't remember the exact interface
   public void Log(string message)
   {
       var buffer = new StringBuilder(message);

       foreach (var secret in _secretsProvider.GetAllSecrets())
             buffer.Replace(secret, "******");

       _innerLogger.Log(buffer.ToString());
   }
}

I agree that exposing the command line arguments in ToString() and the exception is potentially unsafe. As far as I know, since .NET 5, System.Diagnostics.Process also exposes command line arguments if an exception is thrown (or maybe it was just the executable path, I'm not sure).

In any case, the only option to do some masking without introducing breaking changes (or some sort of mess with Arguments and ArgumentsActual), is to have an option to disable printing arguments in ToString() and exceptions altogether.