-
Notifications
You must be signed in to change notification settings - Fork 198
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
passwd: scrub last-changed timestamp from system accounts in /etc/shadow #3174
Conversation
Hi @jeamland. Thanks for your PR. I'm waiting for a coreos 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/test-infra repository. |
2fd7769
to
547f3a6
Compare
Force-pushed to fix a formatting issue. |
/ok-to-test |
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.
Looks fine overall. There is a wrong usage of expect()
, which is causing failures in CI.
|
||
#[test] | ||
fn test_parse_lines() { | ||
let content = r#" |
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.
Self-note: we did start using indoc recently, so later on it would be a good idea to do a pass on the whole nameservice
module and switch it over (to avoid the limitations of raw strings and indentation).
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.
Did you want to hold this up on that or would that be ok as a separate change?
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.
This can go in a followup PR, as it affects all of nameservice
anyway. It was mostly a reminder to myself, as I realized that you used some other code as a template, which should be fixed as well.
The format of /etc/shadow contains several fields relating to password aging. One of these, `lstchg`, contains a timestamp that represents the epoch day of the last password change. Tools that don't respect SOURCE_DATE_EPOCH, which includes systemd's `sysusers` tool, will set this to a value based on the current time of the build. This causes a lack of reproducibility. The `lstchg` field can be safely made empty. This disables the password aging features. This change rewrites /etc/shadow to remove all `lstchg` values. This removal could be made conditional on the entry containing an encrypted password field indicating that the account is either locked or otherwise restricted from using passwords, such as `*` or anything starting with `!`.
547f3a6
to
26f5ef8
Compare
} | ||
|
||
#[context("Rewriting /etc/shadow to remove lstchg field")] | ||
pub(crate) fn normalize_etc_shadow(rootfs: &openat::Dir) -> Result<()> { |
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.
If you have some time in the near future before context-switching to something else, I'd really appreciate having a unit test covering the relevant cases for this function as well.
This splits out the normalisation of
/etc/shadow
from #3165 while addressing several comments made in that PR.Firstly we add parsers and data structures for shadow(5) to the
nameservice
module. This is largely based on what was there previously but adapted for the slight change in format. I'm unaware of/etc/shadow
files having empty lines and/or comments in general practice but, well, having code to handle that doesn't hurt anything. This code replaces the slightly more naive code that was present in the original change.Building on that, we then remove the
lstchg
values from the file but, in a further request change from the original PR, we only modify entries that are either locked or otherwise indicated as not using a password through this mechanism, defined as any entry that starts with a*
or a!
.