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

Hide secrets in all log_call invocations #2002

Merged
merged 2 commits into from Mar 29, 2022

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Mar 28, 2022

My first idea was to encrypt the text and then decrypt it when it is needed for the call.

However the concern was about the logs and i found that the only thing that it needs to be done
is to update the log_call, because this is what is called from all the backends. in this way nothing
else seems to need to change in testapi or anywhere else.

Signed-off-by: ybonatakis ybonatakis@suse.com

http://aquarius.suse.cz/tests/9012/logfile?filename=autoinst-log.txt

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #2002 (13181d7) into master (d909b2d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 13181d7 differs from pull request most recent head d08282e. Consider uploading reports for the commit d08282e to get more accurate results

@@            Coverage Diff             @@
##           master    #2002      +/-   ##
==========================================
+ Coverage   77.50%   77.52%   +0.01%     
==========================================
  Files          70       70              
  Lines        7261     7262       +1     
==========================================
+ Hits         5628     5630       +2     
+ Misses       1633     1632       -1     
Impacted Files Coverage Δ
bmwqemu.pm 100.00% <100.00%> (ø)
testapi.pm 64.07% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d909b2d...d08282e. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Mar 28, 2022

Great PR! Please pay attention to the following items before merging:

Files matching testapi.pm:

  • Consider bumping the version number in OpenQA/Isotovideo/Interface.pm for changes in behaviour

This is an automatically generated QA checklist based on modified files

bmwqemu.pm Outdated Show resolved Hide resolved
bmwqemu.pm Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor

Maybe "hide" would be a better word than "stash" here?

bmwqemu.pm Outdated Show resolved Hide resolved
@asmorodskyi
Copy link
Member

LGTM , also I am closing my version which was trying to achieve the same . Nice work Yannis !

The `log_call` here looks duplicated as it is called again when it is inside the
os-autoinst/consoles/serial_screen.pm Thus this is unneeded and removing it, it can clean
up the os-autoinst.txt logs

Signed-off-by: ybonatakis <ybonatakis@suse.com>
@b10n1k
Copy link
Contributor Author

b10n1k commented Mar 29, 2022

LGTM , also I am closing my version which was trying to achieve the same . Nice work Yannis !

I removed the log_call from testapi::type_string and i have the feeling that there are more of those around which could be removed and cleanup the os-autoinst.txt. just keep this in mind.

@okurz okurz changed the title Stash log text when is a secret Hide log text when is a secret Mar 29, 2022
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Better rephrase the "Hide" commit message as "Hide secrets in all log_call invocations"
and please wrap your git commit messages consistently at 80 characters per line.

bmwqemu.pm Outdated Show resolved Hide resolved
@b10n1k
Copy link
Contributor Author

b10n1k commented Mar 29, 2022

@okurz @Martchus @asmorodskyi changes are made. But i cant comment on the conversations anymore, for some reason. I guess also that the recommendation from the bot is not needed, right?

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I am not sure what your intention with the tests is but it should be obvious that we won't merge your PR as long as you have disabled a big chunk of tests with comments :)

bmwqemu.pm Outdated Show resolved Hide resolved
@okurz
Copy link
Member

okurz commented Mar 29, 2022

I guess also that the recommendation from the bot is not needed, right?

Well, if the only real change is that the string output in log messages changes then the API version does not need to be bumped.

@okurz
Copy link
Member

okurz commented Mar 29, 2022

@b10n1k please only push updates if you actually address the review comments. I was getting notified multiple times but have seen the changes not implemented.

@b10n1k b10n1k requested review from Martchus and okurz March 29, 2022 13:10
@b10n1k b10n1k changed the title Hide log text when is a secret Hide secrets in all log_call invocations Mar 29, 2022
Comment on lines +64 to +67
sub log_call_test_secret {
bmwqemu::log_call(text => "passwd\n", secret => 1);
}
stderr_like(\&log_call_test_secret, qr{\Q<<< main::log_call_test_secret(text="[masked]", secret=1)}, 'log_call hides sensitive info');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sub log_call_test_secret {
bmwqemu::log_call(text => "passwd\n", secret => 1);
}
stderr_like(\&log_call_test_secret, qr{\Q<<< main::log_call_test_secret(text="[masked]", secret=1)}, 'log_call hides sensitive info');
sub log_call_test_secret {
bmwqemu::log_call(text => "passwd\n", secret => 1);
}
stderr_like { bmwqemu::log_call(text => "passwd\n", secret => 1) } qr{\Q<<< main::log_call_test_secret(text="[masked]", secret=1)}, 'log_call hides sensitive info';

bmwqemu.pm Outdated Show resolved Hide resolved
My first idea was to encrypt the text and then decrypt it when it is needed for
the call.

However the concern was about the logs and i found that the only thing that it
needs to be done
is to update the log_call, because this is what is called from all the backends.
in this way nothing else seems to need to change (in sense of manipulate the
_text_ before pass to log_call) in testapi or anywhere else.

So i have removed condition in testapi but the question is do you need the `log_call` there.
Seems like an extra line in os-autoinst

```
[2022-03-28T12:34:13.170200+02:00] [debug] tests/console/aaa_base.pm:31 called opensusebasetest::select_serial_terminal -> lib/opensusebasetest.pm:1288 called testapi::select_console -> lib/susedistribution.pm:754 called serial_terminal::login -> lib/serial_terminal.pm:142 called testapi::type_password
[2022-03-28T12:34:13.170414+02:00] [debug] <<< testapi::type_string(text="*****", secret=1, max_interval=100)
[2022-03-28T12:34:13.171423+02:00] [debug] <<< consoles::serial_screen::type_string(max_interval=100, cmd="backend_type_string", text="*****", secret=1, json_cmd_token="nJFsVgxh")
```

Signed-off-by: ybonatakis <ybonatakis@suse.com>
@mergify mergify bot merged commit 9383ae9 into os-autoinst:master Mar 29, 2022
@jlausuch
Copy link
Contributor

This breaks the use of type_password.

With this change, type_password() in testapi.pm passes the argument secret to type_string (see below code), but now type_string() is ALWAYS printing $string to the log, whatever value it is (if it's secret or not). So, all the strings commands made with type_password are logged in autoinst-log.txt

Maybe we should recover my $log = $args{secret} ? 'SECRET STRING' : $string;

I created this ticket https://progress.opensuse.org/issues/111010

sub type_password {
    my ($string, %args) = @_;
    $string //= $password;
    type_string $string, secret => 1, max_interval => ($args{max_interval} // 100), %args;
}

jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
jlausuch added a commit to jlausuch/os-autoinst that referenced this pull request May 12, 2022
Background:
os-autoinst#2002

This PR is printing $string on the logs regardless if it's a
secret or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants