-
Notifications
You must be signed in to change notification settings - Fork 704
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
Ubuntu enable unix #12660
Ubuntu enable unix #12660
Conversation
Hi @alanmcanonical. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
534534e
to
f0fe48c
Compare
43a144b
to
e62ce4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes needed
...ide/system/accounts/accounts-restrictions/password_storage/no_empty_passwords/bash/shared.sh
Outdated
Show resolved
Hide resolved
9430dee
to
140a33f
Compare
…e the comment out hash
1154496
to
2b0a499
Compare
…th-update to avoid the not applicable case of bash_pam_unix_enable macro
00bd140
to
8675619
Compare
872eed7
to
8db82d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few changes
FILE="/etc/pam.d/common-password" | ||
sed -i 's/\(.*pam_unix\.so.*\)\s\<nullok\>\(.*\)/\1\2/g' ${FILE} | ||
{{{ bash_pam_unix_enable() }}} | ||
config_file="/usr/share/pam-configs/unix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be cac_unix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved in 4b4714b
{{% set pam_unix_algorithms = "(sha512|yescrypt|gost_yescrypt|blowfish|sha256|md5|bigcrypt)" %}} | ||
{{% set hashing_pattern = line_pattern + "(?!.*" + pam_unix_algorithms + ".*" + pam_unix_algorithms + ").*" + pam_unix_algorithms + ".*$" %}} | ||
{{% set pam_unix_algorithms = "\\b(sha512|yescrypt|gost_yescrypt|blowfish|sha256|md5|bigcrypt)\\b" %}} | ||
{{% set hashing_pattern = line_pattern + "(?!.*" + pam_unix_algorithms + "[^#]*" + pam_unix_algorithms + ")[^#]*" + pam_unix_algorithms + ".*$" %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to the regexes affect all platforms. Would be better to make a separate PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small change from any character (.) to non-comment character ([^#]) and add word boundary to pam_unix_algorithms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Code Climate has analyzed commit 4b4714b and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 60.9% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
Description:
Rationale: