chunked: restrict dedup with hard links #1012
Conversation
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
this is a preparation change for the next commit. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
before deduplicating with hard links make sure the two files share the same UID, GID, file mode and extended attributes. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Using unix.AT_EMPTY_PATH requires CAP_DAC_READ_SEARCH. Use an equivalent variant that uses /proc/self/fd that can be used with rootless. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
chunked-hard-links-fixes
branch
from
8a6b02b
to
980f24e
Compare
| // findFileInOtherLayers finds the specified file in other layers. | ||
| // file is the file to look for. | ||
| // dirfd is an open file descriptor to the checkout root directory. | ||
| // layersMetadata contains the metadata for each layer in the storage. | ||
| // layersTarget maps each layer to its checkout on disk. | ||
| // useHardLinks defines whether the deduplication can be performed using hard links. | ||
| func findFileInOtherLayers(file internal.FileMetadata, dirfd int, layersMetadata map[string]map[string]*internal.FileMetadata, layersTarget map[string]string, useHardLinks bool) (bool, *os.File, int64, error) { | ||
| func findFileInOtherLayers(file *internal.FileMetadata, dirfd int, layersMetadata map[string]map[string][]*internal.FileMetadata, layersTarget map[string]string, useHardLinks bool) (bool, *os.File, int64, error) { |
There was a problem hiding this comment.
No error seems returned from this func?
| } | ||
|
|
||
| xattrsToIgnore := map[string]interface{}{ | ||
| "security.selinux": true, |
There was a problem hiding this comment.
Why is this {necessary, safe}?
There was a problem hiding this comment.
I think @giuseppe is thinking that containers use a volume mount and do not rely on the label. But as @mtrmac this might actually break things, since a container might not be able to use the SELinux label in the lower. The problem is going to be figuring out which labels can and can not be used. If the label came from container storage it should be fine, and the check would not be necessary. If the link came from the host system like in /usr, it probably would work. If the link came from /var or other locations on the host system, it probably will not be readable within the confined container.
There was a problem hiding this comment.
IIRC host reuse only uses /usr, so this might be fine — but having the full thinking documented in the code seems like something pretty likely to be useful 3–5 years from now, both to find out what the original motivation was, and so that future programmers and PRs that touch the same code notice that there’s something to think about.
This PR comment thread (hopefully we settle on a full definite story) can help recording what we were thinking, but a comment directly in code would help draw attention to the issue.
There was a problem hiding this comment.
The question would be is can a user turn this off if it does cause problems?
There was a problem hiding this comment.
“Problems?”
This seems to beme to be the kind of thing where expert programmers should either determine that it works and that it is safe, or not have that feature; I find it unintuitive to treat this like performance tuning knob where there are many legitimate options and the best one is based on circumstances.
There was a problem hiding this comment.
$ sesearch -A -s container_t -t usr_t -c file -p read
allow domain base_ro_file_type:file { getattr ioctl lock open read };
allow svirt_sandbox_domain exec_type:file { entrypoint execute execute_no_trans getattr ioctl lock map open read };
$ seinfo -abase_ro_file_type -x
Type Attributes: 1
attribute base_ro_file_type;
bin_t
boot_t
etc_runtime_t
etc_t
lib_t
shell_exec_t
src_t
system_conf_t
system_db_t
textrel_shlib_t
usr_t
There was a problem hiding this comment.
We could just hard code this list into opencontainers/selinux and then have storage use it.
There was a problem hiding this comment.
Yup, that looks like a consistent story to (ignorant) me. Is the list of base_ro_file_type types relatively static? Or would we want to extract it at RPM build time? (OTOH that could potentially unexpectedly change the list on simple rebuilds.)
before deduplicating with hard links make sure the two files share the same UID, GID, file mode and extended attributes.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com