Do not fail on $HOME not containing .config directory #2147

rhatdan · 2024-09-03T13:42:36Z

Fixes: containers/podman#23818

openshift-ci · 2024-09-03T13:42:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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~~ [rhatdan]

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

rhatdan · 2024-09-03T13:42:47Z

@Luap99 PTAL

Luap99 · 2024-09-03T14:35:04Z

pkg/config/default.go

+	// NOTE: For now we want Windows to use system locations.
+	// GetRootlessUID == -1 on Windows, so exclude negative range
+	if unshare.GetRootlessUID() > 0 {
+		configHome, err := homedir.GetConfigHome()


I don't see how this fixes containers/podman#23818
Without an extensive code analysis I would assume there are more code paths calling into this

I would think the proper fix is to make homedir.GetConfigHome() not error on ENOENT IMO

I disagree, Callers might want to make different decisions on no ConfigHome and having to look at "", nil versus "", ENOENT allows them to fail if they expect to have a ConfigHome.

It should fix the specific error, and seems like a good change in general, since it is expected that the SigPath does not exist in the users HomeDir is common.

Why would a caller expect this this path to exists? For pretty much all other tools this path is used to read option config files. I do not even understand why thins function tries to create the given path.
IMO this should return the path,nil even if the underlying path does not actually exists.

If you call something like os.UserHomeDir() you also juts the the path but no guarantee that it exists.

Ok you could change the function, but this would be potentially breaking change for callers of the function.

Without an extensive code analysis I would assume there are more code paths calling into this

There are several, incl. things like pkg/machine/env.GetConfDir which uses the return value to maintain state. We can’t just silently ignore failures without inventing some other place where to store data. And if we were to invent some other place, in a situation where ~/.config both doesn’t exist and can’t be created … I can’t imagine what that would be.

We must fail in this situation, ultimately.

Callers might want to make different decisions on no ConfigHome and having to look at "", nil versus "", ENOENT allows them to fail if they expect to have a ConfigHome.

(I can’t parse this, but, anyway):

Please make a table of:

on-filesystem situation

environment setting

expected behavior

After we have the expected behavior, we can discuss what the API should look like.

I generally think that it is undesirable for callers to make any kinds of individual decisions based on the situation. All callers who wanted “$XDG_CONFIG_HOME or a fallback” should get exactly the same “$XDG_CONFIG_HOME or a fallback” and generally behave the same in the fallback situation. It seems to me, as a first approximation, undesirable to expose the details of the fallback to the callers; instead, the fallback logic should all be inside GetConfigHome.

[Or, to put it another way, if some callers don’t want the standard $XDG_CONFIG_HOME behavior, they shouldn’t be using the $XDG_CONFIG_HOME variable and name at all; they should define some other variable and name for that custom behavior.]

… personally, I suggest that we don’t want all of this complexity, and “each Linux user must have a modifiable $ConfigHome” is a reasonable thing which we just assume, and fail otherwise.

Especially consider the case where GetConfigHome is intentionally returning an error, where $XDG_CONFIG_HOME is set to a value but that value is not writeable by the current user (typically using su without --login). That’s an unreasonable, potentially dangerous, setup, and we should not be triggering any fallback behavior; we should fail at the first opportunity.

containers/podman#23818 (comment)

Especially consider the case where GetConfigHome is intentionally returning an error, where $XDG_CONFIG_HOME is set to a value but that value is not writeable by the current user (typically using su without --login). That’s an unreasonable, potentially dangerous, setup, and we should not be triggering any fallback behavior; we should fail at the first opportunity.

Well yeah but do we have to fail when the files are never needed in the first place? I would assume code fails anyway if we get EACCESS when reading the config files as we only ignore ENOENT AFAIK.

The doc comments on GetConfigHome() make no such claim that they check for the dir existence, in fact when XDG_CONFIG_HOME is set it doesn't try to create/stat it there so your described scenario isn't failing there.

Added containers/storage#2084 to handle XDG_CONFIG_HOME versus $HOME/.config

Well yeah but do we have to fail when the files are never needed in the first place?

I think that’s an very reasonable point when looking for general config files.

When looking for security-relevant configuration like policy.json, it gets… complicated, because it’s possible the user caused this situation by a mistaken chmod, and falling back to the system-wide config might be a much more permissive policy and not be what the user wanted. Now, to an extent, that’s just inherent in the “check if a per-user config exists, otherwise fall back” idea — we can protect the user against some mistakes but we can’t protect against e.g. a typo in the config file name.

Luap99 · 2024-09-03T14:35:56Z

pkg/config/default.go

+				return sigPath, nil
+			}
+		} else {
+			if !errors.Is(err, unix.ENOENT) {


should use fs.ErrNotExist

mtrmac

AFAICS:

This is not the right fix for the motivating issue. That is really a Podman dependency aspect
Even unrelated from the motivating issue, this is not at all sufficient to handle GetConfigHome failures throughout Podman, and I don’t know what that would look like.

mtrmac · 2024-09-03T20:34:44Z

pkg/config/config_test.go

+	It("HomeDirTest", func() {
+		oldHOMEDIR, set := os.LookupEnv("HOME")
+		dir, err := os.MkdirTemp("", "configTest")
+		gomega.Expect(err).ToNot(gomega.HaveOccurred())
+		defer os.RemoveAll(dir)
+
+		os.Setenv("HOME", dir)
+		_, err = defaultConfig()
+		gomega.Expect(err).ToNot(gomega.HaveOccurred())
+
+		if set {
+			os.Setenv("HOME", oldHOMEDIR)
+		} else {
+			os.Unsetenv("HOME")
+		}
+	})
+


This is completely unrepresentative of containers/podman#23818 .

Ordinarily, if $HOME is an empty but writable directory, GetConfigHome transparently creates ~/.config, and nothing fails.

The reported failure is that .config does not exist and the user does not have the permissions to create it. In that case GetConfigHome fails.

Yup. Will work to fix.

mtrmac · 2024-09-03T20:37:39Z

pkg/config/default.go

@@ -188,6 +189,25 @@ const (
 	DefaultVolumePluginTimeout = 5
 )

+func defaultSigPath() (string, error) {


If this should exist at all:

Suggested change

func defaultSigPath() (string, error) {

// defaultSigPath is an internal implementation detail of defaultConfig. Do not add any other callers.

func defaultSigPath() (string, error) {

mtrmac

.

mtrmac · 2024-09-03T20:51:35Z

pkg/config/default.go

+	// NOTE: For now we want Windows to use system locations.
+	// GetRootlessUID == -1 on Windows, so exclude negative range
+	if unshare.GetRootlessUID() > 0 {
+		configHome, err := homedir.GetConfigHome()


Without an extensive code analysis I would assume there are more code paths calling into this

There are several, incl. things like pkg/machine/env.GetConfDir which uses the return value to maintain state. We can’t just silently ignore failures without inventing some other place where to store data. And if we were to invent some other place, in a situation where ~/.config both doesn’t exist and can’t be created … I can’t imagine what that would be.

We must fail in this situation, ultimately.

Callers might want to make different decisions on no ConfigHome and having to look at "", nil versus "", ENOENT allows them to fail if they expect to have a ConfigHome.

(I can’t parse this, but, anyway):

Please make a table of:

on-filesystem situation

environment setting

expected behavior

After we have the expected behavior, we can discuss what the API should look like.

I generally think that it is undesirable for callers to make any kinds of individual decisions based on the situation. All callers who wanted “$XDG_CONFIG_HOME or a fallback” should get exactly the same “$XDG_CONFIG_HOME or a fallback” and generally behave the same in the fallback situation. It seems to me, as a first approximation, undesirable to expose the details of the fallback to the callers; instead, the fallback logic should all be inside GetConfigHome.

[Or, to put it another way, if some callers don’t want the standard $XDG_CONFIG_HOME behavior, they shouldn’t be using the $XDG_CONFIG_HOME variable and name at all; they should define some other variable and name for that custom behavior.]

… personally, I suggest that we don’t want all of this complexity, and “each Linux user must have a modifiable $ConfigHome” is a reasonable thing which we just assume, and fail otherwise.

Especially consider the case where GetConfigHome is intentionally returning an error, where $XDG_CONFIG_HOME is set to a value but that value is not writeable by the current user (typically using su without --login). That’s an unreasonable, potentially dangerous, setup, and we should not be triggering any fallback behavior; we should fail at the first opportunity.

mtrmac · 2024-09-03T20:55:11Z

pkg/config/default.go

+			}
+		}
+	}
+	return DefaultSignaturePolicyPath, nil


There’s, also… a whole another discussion topic in this problem space: In general, c/image callers should not have their own default path logic at all because they are all going to diverge over time.

Compare containers/image#1726 .

All of this code really should not exist in the first place.

… but, a reminder, removing this code would not really be a fix for containers/podman#23818 .

… now, c/image does look for a policy.json in the home directory, but it currently uses a hard-coded .config/containers/policy.json and it does not interpret $XDG_CONFIG_HOME ( containers/image#2297 ). So just removing this code right now could break things.

packit-as-a-service · 2024-09-04T10:51:06Z

Ephemeral COPR build failed. @containers/packit-build please check.

Fixes: containers/podman#23818 Signed-off-by: Daniel J Walsh <[email protected]>

openshift-ci bot added the approved label Sep 3, 2024

rhatdan force-pushed the config branch from 2f57410 to 54f176f Compare September 3, 2024 13:46

rhatdan changed the title ~~Do not fail on /home/dwalsh not containing .config directory~~ Do not fail on $HOME not containing .config directory Sep 3, 2024

rhatdan force-pushed the config branch from 54f176f to 2d0025d Compare September 3, 2024 13:47

Luap99 reviewed Sep 3, 2024

View reviewed changes

rhatdan force-pushed the config branch from 2d0025d to dc7a663 Compare September 3, 2024 15:44

mtrmac reviewed Sep 3, 2024

View reviewed changes

mtrmac requested changes Sep 3, 2024

View reviewed changes

rhatdan force-pushed the config branch 2 times, most recently from a90e70e to 7d96c3f Compare September 4, 2024 10:48

rhatdan force-pushed the config branch from 7d96c3f to f34213d Compare September 4, 2024 10:51

Do not fail on $HOME not containing .config directory

874b258

Fixes: containers/podman#23818 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the config branch from f34213d to 874b258 Compare September 4, 2024 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not fail on $HOME not containing .config directory #2147

Do not fail on $HOME not containing .config directory #2147

rhatdan commented Sep 3, 2024

openshift-ci bot commented Sep 3, 2024

rhatdan commented Sep 3, 2024

Luap99 Sep 3, 2024

rhatdan Sep 3, 2024

rhatdan Sep 3, 2024

Luap99 Sep 3, 2024

rhatdan Sep 3, 2024

mtrmac Sep 3, 2024

Luap99 Sep 4, 2024

rhatdan Sep 4, 2024

mtrmac Sep 4, 2024

Luap99 Sep 3, 2024

mtrmac left a comment

mtrmac Sep 3, 2024

rhatdan Sep 4, 2024

mtrmac Sep 3, 2024

mtrmac left a comment •

edited

Loading

mtrmac Sep 3, 2024

mtrmac Sep 3, 2024

packit-as-a-service bot commented Sep 4, 2024

	func defaultSigPath() (string, error) {
	// defaultSigPath is an internal implementation detail of defaultConfig. Do not add any other callers.
	func defaultSigPath() (string, error) {

Do not fail on $HOME not containing .config directory #2147

Are you sure you want to change the base?

Do not fail on $HOME not containing .config directory #2147

Conversation

rhatdan commented Sep 3, 2024

openshift-ci bot commented Sep 3, 2024

rhatdan commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Sep 4, 2024

mtrmac left a comment •

edited

Loading