Don't call umask in subscriptions

[saschagrunert]

This call will be done in parallel which messes up the umask on CRI-O on container creation. This call will be done in parallel which messes up the umask on CRI-O on container creation. We now call chmod after directory and file creation to enforce the right permissions.

[rhatdan]

This is some ancient code that came from early days of Docker. Are you 100% sure this will not break some user of Podman or Buildah. Rootfull of rootless? Also what about using in podman-remote, where the service umask gets involved?

[rhatdan]

@giuseppe @mtrmac @nalind @vrothberg WDYT?

[saschagrunert]

Are you 100% sure this will not break some user of Podman or Buildah. Rootfull of rootless? Also what about using in podman-remote, where the service umask gets involved?

I can't be sure that it won't break anything, but manually checking the code paths actually involved creating files/dirs with the correct mode. For what would we need the umask then?

I vendored the change into CRI-O to see if it fixes the umask issue: cri-o/cri-o#6785
Refers to bug: https://issues.redhat.com/browse/OCPBUGS-10704
Referencing discussion point: opencontainers/runc#3563 (review)

[rhatdan]

Lets wait for others to comment. This code was added by me, but not written by me, or at least I don't recall.

@mheon @baude Thoughts?

[Luap99]

As I read the code the goal is to keep the same permission as they are on the host. So if your umask is 077 as an example and on the host the dir has 755 as permission then calling mkdir with 755 will result in a directory with 700 permissions because the umask was applied.
Setting the umask to 0 fixes this but is obviously not thread safe, the other fix is to do an explicit chmod on the created directory/file afterwards because chmod ignores the umask.

[mtrmac]

Yes, AFAICS this must use an explicit chmod.

And if it is confusing or risky, it should ideally have unit tests that verify the expected behavior, before&after the refactoring.

---

(It would, also, uh, be useful for the subpackage to document what it does, and to have some discussion for why a single piece of code deals with RHEL subscription and with FIPS state.)

[saschagrunert]

Hm, we need a somewhat urgent workaround for CRI-O, so I’ll see if I can restore the umask after the parallel calls to this method.

Let’s leave this PR open to aim for a cleaner mid-term solution which probably does also not require a backport.

[mtrmac]

A fix adding the Chowns, is IMHO, not that much work.

I worry that adding more Umask calls to a multi-threaded server is very likely to make the problem worse, even short-term, even if it fixes a specific reproducer right now.

[saschagrunert]

A fix adding the Chowns, is IMHO, not that much work.

chmod, correct?

It depends, I’ll have to double check that when I’m back from kubecon how many backports we need for CRI-O.

[mtrmac]

Yes, Chmod. My mistake, I keep confusing the two for some reason.

[saschagrunert]

Gave it a push from the airport, will come up with testing at some later point in time.

[giuseppe]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, saschagrunert

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:

• OWNERS [giuseppe]

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

[rhatdan]

LGTM

[mheon]

LGTM, but it would be nice to have a test before merge

[saschagrunert]

Updated the code as well as added tests.

[mtrmac]

The …IgnoreUmask functions are a nice idea.

[saschagrunert]

I moved the functions to the umask package. We can re-use them in podman later on to fix: https://github.com/containers/podman/blob/3ecb174eeeda7aa218ecce8f20df2af301469d3e/libpod/container_internal_common.go#L2748-L2749

[mtrmac]

We can re-use them in podman later on to fix …

Good thinking.

LGTM. Thanks!

[mheon]

/lgtm

Really like the new umask package, that will definitely be useful.