Secure value censorship refactor #2424
Conversation
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2424 +/- ##
==========================================
+ Coverage 91.37% 91.44% +0.07%
==========================================
Files 639 641 +2
Lines 18280 18371 +91
Branches 3847 3945 +98
==========================================
+ Hits 16703 16800 +97
+ Misses 1575 1569 -6
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andrew W. Harn <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
|
📅 Suggested merge-by date: 2/20/2025 |
awharn
requested review from
t1m0thyj,
gejohnston,
zFernand0,
pujal0909,
traeok,
anaxceron,
jace-roell and
ATorrise
Signed-off-by: Andrew W. Harn <[email protected]>
…-refactor Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
Signed-off-by: Andrew W. Harn <[email protected]>
|
Signed-off-by: Andrew W. Harn <[email protected]>
|
Question from a CLI user: Will this resolve the non-printing of secure user and password values in the output from |
That was not the intention with this PR, which was mainly to clean up how we determine what should be considered to be a secure property. However, it's something we should consider (for a future PR). We'll take another look at #2259. |
There was a problem hiding this comment.
Thanks for all your work on this Andrew! I see there was quite a bit of refactoring done to keep our behavior consistent - its much appreciated 🙏 I had a couple comments from the code review, but wanted to post those ahead of time before continuing with manual testing.
| if (censorOpts.config) { | ||
| const secureOptions: Set<string> = new Set(); | ||
|
|
||
| // Try to use the command and inputs to find the profiles being loaded | ||
| if (censorOpts.commandDefinition && censorOpts.commandArguments) { | ||
| const profiles = []; | ||
| for (const prof of censorOpts.commandDefinition.profile?.required || []) { | ||
| profiles.push(prof); | ||
| } | ||
| for (const prof of censorOpts.commandDefinition.profile?.optional || []) { | ||
| profiles.push(prof); | ||
| } | ||
|
|
||
| for (const prof of profiles) { | ||
| // If the profile exists, append all of the secure props to the censored list | ||
| let profName = censorOpts.commandArguments?.[`${prof}-profile`]; | ||
| if (!profName) { profName = this.mConfig.mProperties.defaults[`${prof}`]; } | ||
| if (profName && censorOpts.config.api.profiles.get(profName)) { | ||
| censorOpts.config.api.secure.securePropsForProfile(profName).forEach(prop => secureOptions.add(prop)); | ||
| } | ||
| } | ||
| } else { | ||
| // We only have a configuration file, assume every property that is secured should be censored | ||
| censorOpts.config.api.secure.findSecure(censorOpts.config.mProperties.profiles, "profiles").forEach( | ||
| prop => secureOptions.add(prop.split(".").pop()) | ||
| ); | ||
| } | ||
| secureOptions.forEach(prop => this.addCensoredOption(prop)); | ||
| } |
There was a problem hiding this comment.
SonarCloud flagged the setCensoredOptions function for complexity. I was wondering if we could move this part of the function into a helper function to reduce complexity? I think it would make it a bit easier to follow the different branches of logic, while also resolving the SonarCloud issue as a result.
There was a problem hiding this comment.
We could do that, though the data would need to be passed through to the second helper function, and it would probably only be called in this function. I attempted to do some reduction of calling addCensoredOption in here, which might not be as easily possible if I break this up into multiple functions.
Signed-off-by: Andrew W. Harn <[email protected]>
What It Does
Refactors LoggerUtils into a new Censor class to globally use the same list of censored options, and intelligently add options to censor on the fly. Also removes duplication (and deprecates functions) to the old utilities. Points old utilities to use the new utility when possible.
Fixes #2430
How to Test
Make configuration file modifications, adding various properties to the config's secure array.
Any property that is added to the secure array should be hidden when running any Zowe CLI command with
--show-inputs-only.Additionally, run commands with
ZOWE_APP_LOG_LEVEL=infoandZOWE_IMPERATIVE_LOG_LEVEL=infoenvironment variables, and view the logs. Any secure property should be hidden in the logs, even if passed in on the command line.To test the #2430 fix, set
ZOWE_SHOW_SECURE_ARGSto true, and run a command with--show-inputs-only. Observe secure values are not masked.Review Checklist
I certify that I have:
Additional Comments