Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Command Argument STDOUT/capistrano.log Hiding #430

Merged
merged 11 commits into from
Jun 30, 2018
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ bin/rake
.vagrant*
test/tmp
Gemfile.lock
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not put IDE-specific patterns in our gitignore. Those are more appropriate for a global gitignore (see https://thomashunter.name/blog/global-gitignore-vs-repository-gitignore/)

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ appear at the top.
## [Unreleased][]

* Your contribution here!
* [#430](https://github.com/capistrano/sshkit/pull/430): [Feature] Command Argument STDOUT/capistrano.log Hiding - [@NorseGaud](https://github.com/NorseGaud)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to the Unreleased section (next to the "Your contribution here!" bullet).


## [1.16.1][] (2018-05-20)

Expand Down
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,33 @@ SSHKit.config.output = SSHKit::Formatter::Pretty.new(output)
SSHKit.config.output = SSHKit::Formatter::SimpleText.new(File.open('log/deploy.log', 'wb'))
```

#### Output & Log Redaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, documentation!! ❤️


If necessary, redact() can be used on a section of your execute arguments to hide it from both STDOUT and the capistrano.log. It does not support an Array or Hash.

```ruby
# Example from capistrano-postgresql gem
execute(:psql, fetch(:pg_system_db), '-c', %Q{"CREATE USER \\"#{fetch(:pg_username)}\\" PASSWORD}, redact("'#{fetch(:pg_password)}'"), %Q{;"})
```
*Take a special note:* SSHKIT will automatically add a space in-between each argument in `execute`. When quoting a password, like the above, you should include the quotes within redact().
Once wrapped, sshkit logging will replace the actual pg_password with a *REDACTED* value:

```
# STDOUT
00:00 postgresql:create_database_user
01 sudo -i -u postgres psql -d postgres -c "CREATE USER \"db_admin_user\" PASSWORD *REDACTED* ;"
01 CREATE ROLE
✔ 01 user@localhost 0.099s

# capistrano.log
INFO [59dbd2ba] Running /usr/bin/env sudo -i -u postgres psql -d postgres -c "CREATE USER \"db_admin_user\" PASSWORD *REDACTED* ;" as user@localhost
DEBUG [59dbd2ba] Command: ( export PATH="$HOME/.gem/ruby/2.5.0/bin:$PATH" ; /usr/bin/env sudo -i -u postgres psql -d postgres -c "CREATE USER \"db_admin_user\" PASSWORD *REDACTED* ;" )
DEBUG [529b623c] CREATE ROLE

```

Yet, the created database user will have the value from `fetch(:pg_password)`.

#### Output Colors

By default, SSHKit will color the output using ANSI color escape sequences
Expand Down
8 changes: 8 additions & 0 deletions lib/sshkit/backends/netssh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ def config
end
end


def redact(arg) # Used in execute_command to hide redact() values user passes in
raise ArgumentError, 'redact() does not support Array or Hash' if arg.is_a?(Array) || arg.is_a?(Hash)
['*REDACTED*',arg]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works and I am happy to accept this PR as-is, but there might be a more elegant solution if you want to explore it. I am thinking of using a module as a way to "mark" the arg being redacted. In other words:

module Redacted
  # intentionally empty
end

Then the redact method becomes:

def redact(arg)
  arg.extend(Redacted)
end

Finally, testing whether something is redacted is:

arg.is_a?(Redacted)

The advantage to this approach is that the arguments are modified in a way that should have no side-effects. Whereas replacing a string with a specially-formed array inherently modifies the arguments in a way that has to explicitly reversed later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I anticipate that some SSHKit users will complain that redact works for SSH backends but not for local backends. Generally speaking we try to keep the two in parity. Is there an easy way to add the redact functionality to the local backend as well?

end

private

def transfer_summarizer(action, options = {})
Expand Down Expand Up @@ -123,7 +129,9 @@ def transfer_summarizer(action, options = {})
end

def execute_command(cmd)
cmd.options[:redacted] = true
output.log_command_start(cmd)
cmd.options[:redacted] = false
Copy link
Member

@mattbrictson mattbrictson Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code strikes me as being a bit brittle. We are setting an option purely to get a desirable side-effect, when a more direct approach would be more self-explanatory.

What if the Command object had a method to get a redacted version of itself, and that is what gets logged?

I am open to suggestions on the name, but one possibility is:

output.log_command_start(cmd.with_sensitive_args_redacted)

The with_sensitive_args_redacted method would be responsible for returning a copy of the command with a different set of args.

def with_sensitive_args_redacted
  new_args = args.map( ... ) # logic to replace redacted args goes here
  redacted_cmd = dup
  redacted_cmd.instance_variable_set(:@args, new_args)
  redacted_cmd
end

cmd.started = true
exit_status = nil
with_ssh do |ssh|
Expand Down
9 changes: 8 additions & 1 deletion lib/sshkit/command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,14 @@ def to_command

def to_s
if should_map?
[SSHKit.config.command_map[command.to_sym], *Array(args)].join(' ')
if options[:redacted]
new_args = args.map{|arg| arg.is_a?(Array) && arg[0] == '*REDACTED*' ? arg[0] : arg }.join(' ')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the join(' ') here is necessary

elsif !options[:redacted]
new_args = args.map{|arg| arg.is_a?(Array) && arg[0] == '*REDACTED*' ? arg[1] : arg }
else
new_args = args
end
[SSHKit.config.command_map[command.to_sym], *Array(new_args)].join(' ')
else
command.to_s
end
Expand Down
34 changes: 33 additions & 1 deletion test/functional/backends/test_netssh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,45 @@ def test_simple_netssh
], command_lines
end

def test_redaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test! 💯

# Be sure redaction in the logs is showing *REDACTED*
Netssh.new(a_host) do
execute :echo, 'password:', redact('PASSWORD')
execute :echo, 'password:', redact(10000)
end.run
command_lines = @output.lines.select { |line| line.start_with?('Command:') }
assert_equal [
"Command: /usr/bin/env echo password: *REDACTED*\n",
"Command: /usr/bin/env echo password: *REDACTED*\n",
], command_lines
# Be sure the actual command executed without *REDACTED*
Netssh.new(a_host) do
execute :touch, redact('test.file')
execute :ls, 'test.file'
end.run
ls_lines = @output.lines.select { |line| line.start_with?("\ttest.file") }
assert_equal [
"\ttest.file\n"
], ls_lines
# Error when user passes in Array to redact()
err = assert_raises ArgumentError do
Netssh.new(a_host) do |_host|
execute :echo, 'password array:', redact(['PASSWORD_IN_ARRAY'])
end.run
end
assert_equal "redact() does not support Array or Hash", err.message
# Cleanup
Netssh.new(a_host) do
execute :rm, ' -f test.file'
end.run
end

def test_group_netssh
Netssh.new(a_host) do
as user: :root, group: :admin do
execute :touch, 'restart.txt'
end
end.run

command_lines = @output.lines.select { |line| line.start_with?('Command:') }
assert_equal [
"Command: if ! sudo -u root whoami > /dev/null; then echo \"You cannot switch to user 'root' using sudo, please check the sudoers file\" 1>&2; false; fi\n",
Expand Down