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

Add CI/CD check to avoid uses of plain password on data directory #21136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tinawang123
Copy link
Contributor

@tinawang123 tinawang123 commented Feb 7, 2025

Add CI/CD check to avoid uses of plain password on data directory in pull requests

@tinawang123 tinawang123 added the qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them label Feb 7, 2025
@tinawang123 tinawang123 changed the title Add CI/CD check to avoid uses of plain password in pull requests Add CI/CD check to avoid uses of plain password on data directory Feb 7, 2025
@tinawang123 tinawang123 requested review from jknphy and rfan1 February 7, 2025 03:47
t/01_style.t Outdated
@@ -12,4 +12,6 @@ ok system(qq{git grep -I -l '[#/ ]*SPDX-License-Identifier ' ':!t/01_style.t'})
$out = qx{git grep -ne "check_var('ARCH',.*)" -e "check_var('BACKEND',.*)" ':!lib/Utils/Architectures.pm' ':!lib/Utils/Backends.pm' 'lib' 'tests'};
ok $? != 0 && $out eq '', 'No check_var function to verify ARCH/BACKEND types' or diag $out;
ok system(qq{git grep -I -l \\( -e "egrep" -e "fgrep" \\) ':!t/01_style.t' ':!CONTRIBUTING.md'}) != 0, 'No usage of the deprecated egrep and fgrep commands';
$out = qx{git grep -I -l 'nots3cr3t' ':!data/wsl/Autounattend_*.xml' ':!data/yam/agama/auto/*plain_password.jsonnet' ':!data/yam/agama/auto/lvm_encrypted.jsonnet' 'data'};
Copy link
Contributor

Choose a reason for hiding this comment

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

I asked developers about other plain password not in user or root, it will be very inefficient add a file to exclude here for each encryption scenario that we will have. You will have to exclude:
"luks2": { "password": "nots3cr3t" } and
"luks1": { "password": "nots3cr3t" } but now we have a way to pass it from the code like with AutoYaST, so we can avoid this.

Copy link
Contributor Author

@tinawang123 tinawang123 Feb 7, 2025

Choose a reason for hiding this comment

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

At here, we can only exclude file name, but if every encryption scenario include 'encrypted' in the files' name, I can exclude them with ':!data/yam/agama/auto/*_encrypted.jsonnet', Now I excluded all files which include plain password. If any update later, I can update this line again.

Copy link
Contributor

@jknphy jknphy Feb 7, 2025

Choose a reason for hiding this comment

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

Give us a time to pass the password in a different way. My suggestion is that you remove all the files in the exclusion list or at least the one in Yam and not merge this yet. The CI should not force name of files and I believe the check as it is now with exclusion could be removed by someone if enough annoyed for the need to add the file to the exclusion list each time encryption is used in a profile, a proper solution should be offered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello https://suse.slack.com/archives/C02CSAZLAR4/p1738922909487469 to add more context.
You could use $password like

type_password "$password\n";
to avoid hardcoding it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants