fix: do not use '.exp.sed' as redact source #3900
Conversation
|
|
||
| # exclude regex | ||
| retval = _process_content_redaction(test_file.name, ['[[:digit:]]+', 'a*(b|c)'], True) | ||
| assert retval == 'test\npassword: ********\n'.encode('utf-8') | ||
| assert retval == 'test\npassword: pAsswOrd\n'.encode('utf-8') |
There was a problem hiding this comment.
@xiangce so this change removes password redaction completely?
There was a problem hiding this comment.
@ryan-blakley, yes, we want to do that, since the password redaction caused some issues (#3858, #3885, and another not-reported one in the insalled_rpms see this attachment) and resulted in false positives of some rules.
and, the current logic of the .exp.sed is not sufficient, e.g. the keyword password is not the only one for indicating passwords (there are passwd and pwd, etc.). Considering these, and we have a strict review process for Specs, We'd like to remove it from the collection. What do you think?
There was a problem hiding this comment.
Ps, the _process_content_redaction method will be removed in the next Move obfuscation/reaction to core collection (#3679). The relevant tests will also be removed then.
There was a problem hiding this comment.
Gotcha yeah I noticed the issues that were caused by it. Is the plan to not merge this until the new obfuscation/redaction is merged? The reason I ask, I just don't want passwords to get stored in warehouse after making this change, and then have to purge them.
There was a problem hiding this comment.
Sorry @ryan-blakley, this .exp.sed or the equivalent process will not be used/implemented in the new obfuscation/redaction. Our current target is to completely discard it from the core (and count on spec reviews to avoid collecting PII information) due to 1. it causes issues on specs 2. it's not sufficient.
However, if you have come across any real cases where this "password" script works as expected, for example, actually obfuscated the password, can you share some of these cases with me?
We have talked about the above in the sync meeting too. I believe Bob will also ask you the same for me. It won't hurt that we start another e-mail or Slack thread to continue discussing this matter.
There was a problem hiding this comment.
@xiangce Gotcha, I apologize I thought the current .exp.sed script was actively redacting passwords. Looking through several older archives I have downloaded I see how bad it is now. I did see it redacted the proxy password from the /etc/rhsm/rhsm.conf file correctly, but that's not as big of a deal. My fear was we would start collecting service user passwords, or database user passwords for like mariadb, redis, etc. But if that's not the case, then the change looks good to me!
There was a problem hiding this comment.
@ryan-blakley - Thanks for the info. So I think it's required for us to go through the existing (non-filterable) specs to check if there are any risks of collecting the sensitive information. We can merge this PR after this check.
And I opened #3916 first for rhsm.conf
There was a problem hiding this comment.
@ryan-blakley - FYI, I opened INSGHTCORE-236 to track the password check of existing specs.
There was a problem hiding this comment.
Sounds perfect to me!
So I made an incorrect understanding, I thought you (the client team) might want to keep these I'll remove them in the next commit soon. |
Oh, I think is no needed. |
- see: INSGHTCORE-227 Signed-off-by: Xiangce Liu <xiangceliu@redhat.com>
Signed-off-by: Xiangce Liu <xiangceliu@redhat.com>
xiangce
force-pushed
the
skip_exp_sed
branch
from
a9c1ee9
to
d6b7010
Compare
|
Can one of the admins verify this patch? |
Directoryfromls_parserhits exception for specical directory name #3858All Pull Requests:
Check all that apply:
Complete Description of Additions/Changes: