Use getpwuid(3) in corehost for the $HOME-less user

[am11]

In other parts of dotnet/runtime repo ^{[1] [2] [3]}, we attempt to retrieve user home directory from getpwuid_r when the environment variable HOME is not set. Use the same fallback in corehost.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In other parts of dotnet/runtime repo ^{[1] [2] [3]}, we attempt to retrieve user home directory from getpwuid_r when the environment variable HOME is not set. Use the same fallback in corehost.

Author:	am11
Assignees:	-
Labels:	area-Host, community-contribution
Milestone:	-

[vitek-karas]

I have to defer to @janvorli on this. It looks good, but:

• we want the directory to be only accessible by the current user - paths accessible by other (non-root) users are potentially problematic as other users could DoS the extraction by pre-creating the right files/directories.
• if no such directory is available, I think it's better to fail and ask users to specify the extraction directory explicitly

In short under no condition should we extract to potentially DoS sensitive location by default.

If the getpwuid offers such guarantees, I'd be happy to use that as it solves real issues. But if these guarantees are not there, I think it would be better to let it fail. (I don't know if the other usages in the runtime may have similar requirements or not).

[janvorli]

@vitek-karas the getpwuid gets a record for the specified user (in the change, it is the current user) from the OS user database, the /etc/passwd file. That database, besides user names also contains the user home directory. The HOME env variable, if present, comes from the same source. It is set by login, ssh or a graphics login manager.

[am11]

we want the directory to be only accessible by the current user - paths accessible by other (non-root) users are potentially problematic as other users could DoS the extraction by pre-creating the right files/directories.

Based on my understanding of this, the else case being modified here:

1. has at least same level of guarantee that its pairing if block is providing, where we read $HOME environment variable.
2. is more or at least as "reliable" than the if case, since getpwuid(3) reads information from system user database (same database which holds credentials that let user log in to the system).

[vitek-karas]

Thanks a lot for confirmation!

I think the only missing piece is to add a test. Something like - remember $HOME, run an extraction with $HOME set to empty, see if it extracted to the location matches the remembered location. Similar to the tests here: https://github.com/dotnet/runtime/blob/main/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleExtractToSpecificPath.cs

[am11]

Sure, I am working on a test case. However, BundleExtractToSpecificPath are not currently running in the CI. Installer legs are green, but e.g.

runtime/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleExtractToSpecificPath.cs

Line 233 in a56b732

.And.HaveStdErrContaining("Failed to determine default extraction location. Environment variable '$HOME' is not defined.");

didn't fail. Note that I changed the error message such that there is no period after is not defined anymore. Tests are apparently skipped due to this condition:

runtime/src/installer/tests/Directory.Build.props

Lines 14 to 15 in 57bfe47

	<TestRunnerAdditionalArguments Condition="'$(RunOnStaging)' != 'true'">$(TestRunnerAdditionalArguments) -notrait category=FlakyAppHostTests</TestRunnerAdditionalArguments>
	<TestRunnerAdditionalArguments Condition="'$(RunOnStaging)' == 'true'">$(TestRunnerAdditionalArguments) -trait category=FlakyAppHostTests</TestRunnerAdditionalArguments>

BundleExtractToSpecificPath is marked with the trait FlakyAppHostTests that only runs on runtime-staging pipeline, and staging doesn't seem to have any installer leg.

I will, however, run them locally on macOS (11.5.2) and Linux (Ubuntu 20.04) before pushing the test, assuming that'd be enough. :)

[vitek-karas]

Hmm - @agocke has been looking into the "flakiness" here #53587. Last I heard it's basically a race condition in the OS and we're looking into potential workarounds. I wasn't aware of the fact that we actually disabled these tests in the CI. They should be at the very least enabled everywhere but on Linux.

[janvorli]

LGTM, thank you!

[vitek-karas]

Thanks a lot!

[vitek-karas]

I think we should wait a couple of days. The CI is under water due to the high load right before RC2 cut-off (and there are also some issues causing more trouble). Since this is for 7 only, it's OK if we wait on this a bit and rebase-rerun and merge then.

[am11]

I ran all bundle tests locally and they passed (works on my machine ™️)

it's OK if we wait on this a bit and rebase-rerun and merge then.

TYT, there is no rush but TBH, blocking this PR on the whole "bundle test flakiness", which has been going on since before .NET 5 release, is an overkill. There is no PR on horizon which is fixing/removing the FlakyAppHostTests trait. I was considering this PR as a minor improvement over your patch merged just yesterday 1517985, both patches are touching/improving same test methods.

[vitek-karas]

Sorry for not being clear. I didn't mean that we should hold this on the test flakiness. My comment about holding this PR for a bit was solely to not stress CI even more than it already is today (and also hopefully we'll see less infra issues after the RC 2 cutoff).

[vitek-karas]

Thanks a lot @am11!