FileStatus.Unix/Process.Unix: align caching of user identity.

[tmds]

The effective user, and group of the process won't change.

@dotnet/area-system-io ptal.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The effective user, and group of the process won't change.

@dotnet/area-system-io ptal.

Author:	tmds
Assignees:	-
Labels:	area-System.IO, community-contribution
Milestone:	-

[stephentoub]

The effective user, and group of the process won't change.

What if the process calls setuid, seteuid, or setegid?

[tmds]

What if the process calls setuid, seteuid, or setegid?

It is not supposed to be changed. In case someone changed it, we'd still be checking against the cached values.

In cases where you want to do something as a different user, it's very recommended to start a new process.

We already do this type of caching in Process.Unix:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

Lines 1066 to 1068 in de00deb

	s_euid = Interop.Sys.GetEUid();
	s_egid = Interop.Sys.GetEGid();
	s_groups = Interop.Sys.GetGroups();

[stephentoub]

It is not supposed to be changed.

Do you mean a conforming libc implementation isn't supposed to change the values in response to these functions, or do you mean these functions aren't recommended for use but if someone uses them the values may be changed?

For the former case, caching seems reasonable.
For the latter case, caching (including our existing caching) seems suspicious.

[tmds]

Do you mean a conforming libc implementation isn't supposed to change the values in response to these functions, or do you mean these functions aren't recommended for use but if someone uses them the values may be changed?

Sorry for being unclear. The latter.

You can call these functions to change the value. This means you have the capability to change user (e.g. you're root).

It's best to call them from apps that are in tight control of their resources to limit security issues due to leaking resources of the privileged user.

I'm not aware of apps that do this besides those specifically meant to change user (like su, sudo). That is the only thing they do.

That's why I seriously discourage doing this in a .NET app.

We could, if it makes a difference, avoid caching for root (egid = 0), since that is the most expected privileged user (which can use these calls).

[carlossanlop]

In cases where you want to do something as a different user, it's very recommended to start a new process.

Can you recommend a link I can read explaining this expectation for Unix permissions?

We could, if it makes a difference, avoid caching for root (egid = 0), since that is the most expected privileged user (which can use these calls).

Do you mean that if the initial process owner is root, we should always invoke Interop.Sys.GetEUid()/Interop.Sys.GetEGid() instead of returning the cached value, and if the value changes to a non-root user at some point, we then cache it, and start returning that? That would contradict the above statement, wouldn't it? That the recommendation is to start a new process if the user wants to switch to a different user.

[carlossanlop]

Is this PR being submitted in response to an existing issue?

[tmds]

Is this PR being submitted in response to an existing issue?

No, I wanted to apply the same pattern here that I knew exits in Process.Unix and thought it was going to be a simple change, but I did not anticipate @stephentoub's feedback.

I think we want to do the same thing in both places. I'll take another shot next week.

[stephentoub]

but I did not anticipate @stephentoub's feedback

Sorry :-)

[tmds]

@carlossanlop @stephentoub @jkotas please take another look.

I've aligned the caching between FileStatus and Process, so it is only used on Linux when the user cannot change its identity.
I've also added some additional logic to both classes that make it unnecessary to check the identity in some (common) cases.

[tmds]

I'm going to make another pass at this and remove the caching on Linux because it's adding some complexity, and the additional logic is circumventing the identity check in Process in most cases.

[tmds]

I've eliminated the user identity caching.

FileStatus now directly caches the read only flag.

Process omits the identity check in most cases by looking first if the x-bit is set/not-set for everyone.

FileStatus now also considers all groups (like Process).

This is up for review.

[adamsitnik]

LGTM! It's great to see the identity caching being unified everywhere and the checks optimized for common cases.

[adamsitnik]

@tmds could you please solve the conflicts and answer #60160 (comment) ?