-
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
shadow: Adjust all deployments #4913
Conversation
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.
Nice! Some minor comments (and I think a bug in the unit test), but LGTM overall.
It was pointed out that in the previous change here we missed the fact that the previous deployments were accessible. - Move the logic into Rust, adding unit tests - Change the code to iterate over all deployments - Add an integration test too Note: A likely future enhancement here will be to finally deny unprivileged access to non-default roots; cc ostreedev/ostree#3211
pub(crate) fn fix_shadow_perms_in_root(root: &Dir) -> Result<bool> { | ||
let zero_perms = Permissions::from_mode(0); | ||
let mut changed = false; | ||
for path in ["etc/shadow", "etc/shadow-", "etc/gshadow", "etc/gshadow-"] { |
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.
@HuijingHei found that we are not fixing /usr/etc/shadow
and /usr/etc/gshadow
. I think this means we need another patch right @cgwalters ?
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.
Conceptually those files are part of the immutable base image defaults, so we "can't"¹ change them per instance.
They need to get fixed on the build server.
Note also, that because we don't have any hardcoded passwords in our base images, it doesn't actually matter if /usr/etc/shadow
is world readable because there's nothing there.
¹ Of course, we could mask them or something but I don't think it matters, we can just explain the above
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 guess since useradd
does not really work in layering yet is not an issue either? and if people are adding layered hardcoded credentials manually to shadow... they just need to fix it "downstream".
It was pointed out that in the previous change here we missed the fact that the previous deployments were accessible.
Note: A likely future enhancement here will be to finally deny unprivileged access to non-default roots; cc
ostreedev/ostree#3211