Don't write _SECRET_* vars in vars.json #1074
Conversation
| @@ -217,7 +217,7 @@ sub run_all { | |||
| $died = 1; # test execution died | |||
| } | |||
| eval { | |||
| bmwqemu::save_vars(); | |||
| bmwqemu::save_vars(no_secret => 1); | |||
There was a problem hiding this comment.
it is not the only place where save_vars actually called. Did you check others ? For example save_vars called on each set_var call so looks like your secrets will still leak after this PR.
Generally I don't see why we need to have another flavor of save_vars which actually saving them
There was a problem hiding this comment.
@asmorodskyi It need to be the last part where the save_vars method is called, as this is the one that saves the final vars.json file that will be updated in the end to the webUI.
There was a problem hiding this comment.
@asmorodskyi Good point. Besides, would it ever make sense to call this with no_scret => 0? If not, I would not make it an option at all.
There was a problem hiding this comment.
the problem is that we use vars.json dump and save to transfer it between backend and autotest processes.
There was a problem hiding this comment.
Yes. If I would always avoid writing of such variables, the first test-module sees them, but not the second one during one job.
|
I would also recommend to drop a note in |
There was a problem hiding this comment.
Looks fine to me. I'm not sure if @coolo has a different opinion on how this could have been handled
| @@ -217,7 +217,7 @@ sub run_all { | |||
| $died = 1; # test execution died | |||
| } | |||
| eval { | |||
| bmwqemu::save_vars(); | |||
| bmwqemu::save_vars(no_secret => 1); | |||
There was a problem hiding this comment.
@asmorodskyi It need to be the last part where the save_vars method is called, as this is the one that saves the final vars.json file that will be updated in the end to the webUI.
cfconrad
force-pushed
the
hidden_vars
branch
from
816d666
to
1fe33cc
Compare
|
Dropped note in set_var(). |
Codecov Report
@@ Coverage Diff @@
## master #1074 +/- ##
==========================================
+ Coverage 38.17% 39.49% +1.31%
==========================================
Files 38 38
Lines 4710 4715 +5
Branches 787 788 +1
==========================================
+ Hits 1798 1862 +64
+ Misses 2594 2532 -62
- Partials 318 321 +3
Continue to review full report at Codecov.
|
If a variable name starts with _SECRET_*, it is excluded from writing in to the final vars.json file. During the JOB, the variable is accessible via the normal functions e.g. get_var(). Why: This is needed to store sensitive data (credentials) in workers.ini and prevent them to be visible via the web-ui.
cfconrad
force-pushed
the
hidden_vars
branch
from
1fe33cc
to
e268045
Compare
|
Test cases for bmwqemu::save_vars() added. |
| @@ -103,6 +103,40 @@ subtest 'test PRJDIR default' => sub { | |||
| is($vars{PRJDIR}, $data_dir, 'PRJDIR set to default'); | |||
| }; | |||
|
|
|||
| subtest 'save_vars' => sub { | |||
There was a problem hiding this comment.
Perlcritic won't like the quotes.
If a variable name starts with
_SECRET_*, it is excluded fromwriting in to the final vars.json file.
During the JOB, the variable is accessible via the normal functions
e.g. get_var().
Why: This is needed to store sensitive data (credentials) in workers.ini
and prevent them to be visible via the web-ui.
This change is related to os-autoinst/os-autoinst-distri-opensuse#6346. The discussion on how to provide credentials, turns in the direction of better having hidden vars, then introduce a new configuration file.
Variables which are passed via job configuration or openqa-client are not affected and not in the focus of this PR. These variables still visible in the settings tab.
Ticket: https://progress.opensuse.org/issues/42155
Verification run: http://cfconrad-vm.qa.suse.de/tests/3134#step/extra_log_test/14