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

Don't expose account names when creating tar files with hard-coded account IDs #1765

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Nov 28, 2023

A prerequisite for fixing containers/image#1627 .

…count IDs

A prerequisite for fixing containers/image#1627 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Copy link
Contributor

openshift-ci bot commented Nov 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mtrmac added a commit to mtrmac/image that referenced this pull request Nov 28, 2023
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
// Don’t expose the user names from the local system; they probably don’t match the ta.ChownOpts value anyway,
// and they unnecessarily give recipients of the tar file potentially private data.
hdr.Uname = ""
hdr.Gname = ""
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind terribly if these two lines weren't conditional on ChownOpts being set.

Copy link
Member

Choose a reason for hiding this comment

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

Not that that's a blocker, mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nalind That’s quite attractive, I think it would be a replacement for containers/buildah#4181 (with better performance).

OTOH it would make the archives worse for “ordinary tar” operations — in that case matching by account names better captures the users’ intent than matching by UIDs, in case the tar file is moved to a machine with different ID allocations.

I don’t know what we can automatically tell which one the caller wants… maybe based on ta.ChownOpts != nil || !ta.IDMappings.Empty()?

Or maybe there are no “ordinary tar” callers of this codebase anyway?! There are several callers which use this for non-container-layer purposes, but it seems possible that they are only confined to a single host (where the distinction is moot), to a remote client-server operation (where we don’t want any Chown to happen), or to cross-namespace operations with layer/container data (where the configured ID mappings is what matters).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you make a good point about the intent being hard to discern for cases where the names are preferred over IDs. That makes the "filter the output to tailor it" approach more attractive to me in general than trying to add an option field to cover every possible case.
Given the either/or nature of the Uid/Uname and Gid/Gname fields, though, this would be a good exception to such a rule, so LGTM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Conceptually, I would prefer for layerStore.Diff and the like to return already-scrubbed tar files (in all situations with all graph drivers); cleaning that only later in Buildah works but feels a bit unclean to me.

The downside is that that ~silently changing the behavior of pkg/{chroot,}archive is a hard-to-manage API transition; at least it would require a detailed analysis of all users (most of which I’m unfamiliar with, notably the Podman checkpoint code).

@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 28, 2023

(I have manually tested that this + containers/image#2202 fixes the linked bug.)

@mtrmac mtrmac marked this pull request as ready for review November 28, 2023 20:35
mtrmac added a commit to mtrmac/image that referenced this pull request Nov 28, 2023
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 29, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit e9f9f66 into containers:main Nov 29, 2023
@mtrmac mtrmac deleted the chown-privacy branch November 29, 2023 18:36
mtrmac added a commit to mtrmac/image that referenced this pull request Nov 29, 2023
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants