Respect XDG_CONFIG_HOME for policy.json #1011
Conversation
|
@davidscherer looks like you've some whitespace issues somewhere. |
The default path for (rootless) policy.json should respect XDG_CONFIG_HOME if present, as the paths for other configuration files like storage.conf and containers.conf do. Signed-off-by: David Scherer <[email protected]>
davidscherer
force-pushed
the
davidscherer-xdg-config-home
branch
from
ed0c7be to
6c6cc28
Compare
signature/policy_config.go
Outdated
| @@ -61,7 +63,11 @@ func defaultPolicyPath(sys *types.SystemContext) string { | |||
| if sys != nil && sys.SignaturePolicyPath != "" { | |||
| return sys.SignaturePolicyPath | |||
| } | |||
| userPolicyFilePath := filepath.Join(homedir.Get(), userPolicyFile) | |||
| configHomeDir := os.Getenv("XDG_CONFIG_HOME") | |||
There was a problem hiding this comment.
Please use containers/storage/pkg/homedir GetConfigHome()
Signed-off-by: David Scherer <[email protected]>
There was a problem hiding this comment.
This would currently break non-Linux systems, where GetConfigHome always fails. I don’t know what’s the right thing to do on Windows (probably not hard-coding .config), or whether we care much, but on macOS it really should be possible to use a per-user path, and by now it would break users to change the default.
At a first glance, homedir.GetConfigHome should probably use the hard-coded .config (it does fall back to that path on Linux already!) on non-XDG systems, or at least on non-XDG Unixes — but I haven’t looked at all at any other users to see whether it would be safe.
|
@davidscherer @vrothberg @mtrmac what should we do with this PR? |
|
I have been happily using the patch from the first commit to this PR, which fixes the problem on Linux, is probably better than nothing on OS X, and as far as I know doesn't cause problems on Windows. I made the requested change to use GetConfigHome, but that function turns out to be unimplemented on Windows. So the options are, I suppose:
There's a case to be made for (2), but I am not in a great position to do it. IMO (1) is an improvement over the status quo, and the conflicts look easy to resolve. |
|
What are the current obstacles to getting this merged? |
|
Thanks for the ping; it turns out that c/storage/pkg/homedir is no longer hard-failing on non-Linux systems, after commit e2cffe5c610054b088029431230d4143a0e6b1e5 , so this might now be possible to do. Given that, the PR would need to be updated to be testable without relying on external state (i.e. to turn defaultPolicyPathWithHomeDir into something like |
|
4 years later this is still an issue, and we are still happily applying our patch. Is there any interest in trying to get this upstreamed so that the rest of the world doesn't hit this issue too? |
|
The code is using exactly the documented path, so this is being tracked as a feature request, not a bug. Sure, it would be nice to have that feature; but we do need ~perfect tests in this security-critical package, and it’s not something I can work on myself in the immediate future. |
The default path for (rootless) policy.json should respect XDG_CONFIG_HOME if present, as the paths for other configuration files like storage.conf and containers.conf do.
See also containers/common#248