Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli-plugins: Fix searching inaccessible directories #5651

Merged
merged 2 commits into from
Nov 29, 2024
+33 −29
Merged

cli-plugins: Fix searching inaccessible directories #5651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6de3d71
cli-plugins: Fix searching inaccessible directories
vvoland Nov 28, 2024
fcd94fe
cli-plugins: Simplify addPluginCandidatesFromDir
vvoland Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 9 additions & 27 deletions cli-plugins/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,12 @@ func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) {
return pluginDirs, nil
}

func addPluginCandidatesFromDir(res map[string][]string, d string) error {
func addPluginCandidatesFromDir(res map[string][]string, d string) {
dentries, err := os.ReadDir(d)
// Silently ignore any directories which we cannot list (e.g. due to
// permissions or anything else) or which is not a directory
Comment on lines +80 to +81
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I was considering (but perhaps it's not a real issue) is if we need to distinguish default ("try -> try next") paths and path(s) that are explicitly configured in ~/.docker/config.json as "additional paths" (cliPluginsExtraDirs).

My train of thought there is that if it's an explicit configuration, then failing to traverse the location (perhaps except for "not exist") could be considered a hard failure.

Happy to hear thoughts on that though!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attempt to use a plugin which isn't accessible will result in a hard failure anyway. Erroring out during plugin scan would prevent the CLI from running in the non-plugin usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, perhaps I'm looking for issues that aren't important, but mostly considering if it should be completely silent. Here's the order of preference in which paths are considered;

cli/cli-plugins/manager/manager.go

Lines 52 to 62 in 41fba28

// getPluginDirs returns the platform-specific locations to search for plugins
// in order of preference.
//
// Plugin-discovery is performed in the following order of preference:
//
// 1. The "cli-plugins" directory inside the CLIs [config.Path] (usually "~/.docker/cli-plugins").
// 2. Additional plugin directories as configured through [ConfigFile.CLIPluginsExtraDirs].
// 3. Platform-specific defaultSystemPluginDirs.
//
// [ConfigFile.CLIPluginsExtraDirs]: https://pkg.go.dev/github.com/docker/[email protected]+incompatible/cli/config/configfile#ConfigFile.CLIPluginsExtraDirs
func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) {

Based on that, the custom path is intended to have a higher priority than the system-wide installation paths. Which could mean "the cli is configured to use these overridden versions". On Docker Desktop, this could be some of the additional plugins shipping with it, but perhaps it's a situation where (say) the system-wide version is provided by the distro you're running and has a vulnerability, so the intent is to use a fixed version through one of the higher-priority paths. If we silently ignore that we aren't able to use that path, it means we're silently falling back to either an older (perhaps vulnerable) version, or missing extra plugins that were supposed to be available.

For the system-wide paths, perhaps that's OK (it's a system-wide path, and current user isn't in the right group to use it), so even for those, I somewhat wonder if we should have some warning. Not sure if anything documents expected permissions though, or at least I couldn't find that in the FHS (Filesystem Hierarchy Standard) docs.

Quick check on some machines showed me that they are accessible;

# CentOS, Fedora machine

ls -ld /usr/libexec /usr/libexec/docker /usr/libexec/docker/cli-plugins /usr/local/libexec /usr/local/libexec/docker /usr/local/libexec/docker/cli-plugins
ls: cannot access '/usr/local/libexec/docker': No such file or directory
ls: cannot access '/usr/local/libexec/docker/cli-plugins': No such file or directory
drwxr-xr-x. 26 root root 4096 Nov 25 10:52 /usr/libexec
drwxr-xr-x.  3 root root   44 Nov 25 10:53 /usr/libexec/docker
drwxr-xr-x.  2 root root   49 Nov 25 10:52 /usr/libexec/docker/cli-plugins
drwxr-xr-x.  2 root root    6 Aug  9  2021 /usr/local/libexec

# Ubuntu machine
ls -ld /usr/libexec /usr/libexec/docker /usr/libexec/docker/cli-plugins /usr/local/libexec /usr/local/libexec/docker /usr/local/libexec/docker/cli-plugins
ls: cannot access '/usr/local/libexec': No such file or directory
ls: cannot access '/usr/local/libexec/docker': No such file or directory
ls: cannot access '/usr/local/libexec/docker/cli-plugins': No such file or directory
drwxr-xr-x 12 root root 4096 Nov 19 16:02 /usr/libexec
drwxr-xr-x  3 root root 4096 Nov 19 16:02 /usr/libexec/docker
drwxr-xr-x  2 root root 4096 Nov 19 16:02 /usr/libexec/docker/cli-plugins

if err != nil {
return err
return
}
for _, dentry := range dentries {
switch dentry.Type() & os.ModeType {
Expand All @@ -99,28 +101,15 @@ func addPluginCandidatesFromDir(res map[string][]string, d string) error {
}
res[name] = append(res[name], filepath.Join(d, dentry.Name()))
}
return nil
}

// listPluginCandidates returns a map from plugin name to the list of (unvalidated) Candidates. The list is in descending order of priority.
func listPluginCandidates(dirs []string) (map[string][]string, error) {
func listPluginCandidates(dirs []string) map[string][]string {
result := make(map[string][]string)
for _, d := range dirs {
// Silently ignore any directories which we cannot
// Stat (e.g. due to permissions or anything else) or
// which is not a directory.
if fi, err := os.Stat(d); err != nil || !fi.IsDir() {
continue
}
if err := addPluginCandidatesFromDir(result, d); err != nil {
// Silently ignore paths which don't exist.
if os.IsNotExist(err) {
continue
}
return nil, err // Or return partial result?
}
addPluginCandidatesFromDir(result, d)
}
return result, nil
return result
}

// GetPlugin returns a plugin on the system by its name
Expand All @@ -130,11 +119,7 @@ func GetPlugin(name string, dockerCli command.Cli, rootcmd *cobra.Command) (*Plu
return nil, err
}

candidates, err := listPluginCandidates(pluginDirs)
if err != nil {
return nil, err
}

candidates := listPluginCandidates(pluginDirs)
if paths, ok := candidates[name]; ok {
if len(paths) == 0 {
return nil, errPluginNotFound(name)
Expand All @@ -160,10 +145,7 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error
return nil, err
}

candidates, err := listPluginCandidates(pluginDirs)
if err != nil {
return nil, err
}
candidates := listPluginCandidates(pluginDirs)

var plugins []Plugin
var mu sync.Mutex
Expand Down
26 changes: 24 additions & 2 deletions cli-plugins/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ func TestListPluginCandidates(t *testing.T) {
dirs = append(dirs, dir.Join(d))
}

candidates, err := listPluginCandidates(dirs)
assert.NilError(t, err)
candidates := listPluginCandidates(dirs)
exp := map[string][]string{
"plugin1": {
dir.Join("plugins1", "docker-plugin1"),
Expand Down Expand Up @@ -82,6 +81,29 @@ func TestListPluginCandidates(t *testing.T) {
assert.DeepEqual(t, candidates, exp)
}

// Regression test for https://github.com/docker/cli/issues/5643.
// Check that inaccessible directories that come before accessible ones are ignored
// and do not prevent the latter from being processed.
func TestListPluginCandidatesInaccesibleDir(t *testing.T) {
dir := fs.NewDir(t, t.Name(),
fs.WithDir("no-perm", fs.WithMode(0)),
fs.WithDir("plugins",
fs.WithFile("docker-buildx", ""),
),
)
defer dir.Remove()

candidates := listPluginCandidates([]string{
dir.Join("no-perm"),
dir.Join("plugins"),
})
assert.DeepEqual(t, candidates, map[string][]string{
"buildx": {
dir.Join("plugins", "docker-buildx"),
},
})
}

func TestGetPlugin(t *testing.T) {
dir := fs.NewDir(t, t.Name(),
fs.WithFile("docker-bbb", `
Expand Down
Loading