From 2eb67eecab78477cb0abcd286b74b1ece8964563 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Fri, 2 Dec 2022 14:28:53 -0800 Subject: [PATCH 1/8] Add more helpful message on plugin setup failure The current error message shown when a plugin does not respond correctly can be obtuse to the end user of, for example, Vault. Here, we add potential reasons why the plugin did not respond correctly, and some additional debugging information (provided as a best effort) including the CPU architecture that the plugin was compiled for, the current CPU architecture, and the permissions of the plugin. Hopefully this will help users diagnose why their plugin is not loading. We also added a `testdata/` directory that is optionally populated with executables in various formats to help test the additional debugging information. The binaries are over 1 MB each though, so are not checked in, and the test will be skipped if they have not been compiled. --- client.go | 55 ++++++++++++++++++++++++++++++++++++++++----- client_test.go | 42 +++++++++++++++++++++++++++++++++- plugin_test.go | 2 +- testdata/.gitignore | 5 +++++ testdata/Makefile | 29 ++++++++++++++++++++++++ testdata/README.md | 1 + testdata/go.mod | 3 +++ testdata/go.sum | 0 testdata/minimal.go | 4 ++++ 9 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 testdata/.gitignore create mode 100644 testdata/Makefile create mode 100644 testdata/README.md create mode 100644 testdata/go.mod create mode 100644 testdata/go.sum create mode 100644 testdata/minimal.go diff --git a/client.go b/client.go index 2e86f621..439dc25a 100644 --- a/client.go +++ b/client.go @@ -6,6 +6,9 @@ import ( "crypto/subtle" "crypto/tls" "crypto/x509" + "debug/elf" + "debug/macho" + "debug/pe" "encoding/base64" "errors" "fmt" @@ -16,6 +19,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" "sync" @@ -26,6 +30,14 @@ import ( "google.golang.org/grpc" ) +const unrecognizedRemotePluginMessage = `Unrecognized remote plugin message: %s +This usually means + the plugin was not compiled for this architecture, + the plugin is missing dynamic-link libraries necessary to run, + the plugin is not readable by this process, or + the plugin was compiled with an incompatible version of the go-plugin protocol +%s` + // If this is 1, then we've called CleanupClients. This can be used // by plugin RPC implementations to change error behavior since you // can expected network connection errors at this point. This should be @@ -473,7 +485,43 @@ func (c *Client) Kill() { c.l.Unlock() } -// Starts the underlying subprocess, communicating with it to negotiate +// peTypes is a list of Portable Executable (PE) machine types from https://learn.microsoft.com/en-us/windows/win32/debug/pe-format +// mapped to GOARCH types. It is not comprehensive, and only includes machine types that Go supports. +var peTypes = map[uint16]string{ + 0x14c: "386", + 0x1c0: "arm", + 0x6264: "loong64", + 0x8664: "amd64", + 0xaa64: "arm64", +} + +// additionalNotesAboutCommand tries to get additional information about a command that might help diagnose +// why it won't run correctly. It runs as a best effort only. +func additionalNotesAboutCommand(path string) (notes string) { + stat, err := os.Stat(path) + if err != nil { + return + } + + notes += "\nAdditional notes about plugin:" + notes += fmt.Sprintf(" Path: %s\n", path) + notes += fmt.Sprintf(" Mode: %s\n", stat.Mode()) + + if elfFile, err := elf.Open(path); err == nil { + notes += fmt.Sprintf(" ELF architecture: %s (current architecture: %s)\n", elfFile.Machine, runtime.GOARCH) + } else if machoFile, err := macho.Open(path); err == nil { + notes += fmt.Sprintf(" MachO architecture: %s (current architecture: %s)\n", machoFile.Cpu, runtime.GOARCH) + } else if peFile, err := pe.Open(path); err == nil { + machine, ok := peTypes[peFile.Machine] + if !ok { + machine = "unknown" + } + notes += fmt.Sprintf(" PE architecture: %s (current architecture: %s)\n", machine, runtime.GOARCH) + } + return +} + +// Start the underlying subprocess, communicating with it to negotiate // a port for RPC connections, and returning the address to connect via RPC. // // This method is safe to call multiple times. Subsequent calls have no effect. @@ -697,10 +745,7 @@ func (c *Client) Start() (addr net.Addr, err error) { line = strings.TrimSpace(line) parts := strings.SplitN(line, "|", 6) if len(parts) < 4 { - err = fmt.Errorf( - "Unrecognized remote plugin message: %s\n\n"+ - "This usually means that the plugin is either invalid or simply\n"+ - "needs to be recompiled to support the latest protocol.", line) + err = fmt.Errorf(unrecognizedRemotePluginMessage, line, additionalNotesAboutCommand(cmd.Path)) return } diff --git a/client_test.go b/client_test.go index 7c821545..176234f2 100644 --- a/client_test.go +++ b/client_test.go @@ -10,13 +10,14 @@ import ( "net" "os" "os/exec" + "path" "path/filepath" "strings" "sync" "testing" "time" - hclog "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-hclog" ) func TestClient(t *testing.T) { @@ -1395,3 +1396,42 @@ this line is short t.Fatalf("\nexpected output: %q\ngot output: %q", msg, read) } } + +func TestAdditionalNotesAboutCommand(t *testing.T) { + files := []string{ + "windows-amd64.exe", + "windows-386.exe", + "linux-amd64", + "darwin-amd64", + "darwin-arm64", + } + for _, file := range files { + fullFile := path.Join("testdata", file) + if _, err := os.Stat(fullFile); os.IsNotExist(err) { + t.Skipf("testdata executables not present; please run 'make' in testdata/ directory for this test") + } + + notes := additionalNotesAboutCommand(fullFile) + if strings.Contains(file, "windows") && !strings.Contains(notes, "PE") { + t.Errorf("Expected notes to contain Windows information:\n%s", notes) + } + if strings.Contains(file, "linux") && !strings.Contains(notes, "ELF") { + t.Errorf("Expected notes to contain Linux information:\n%s", notes) + } + if strings.Contains(file, "darwin") && !strings.Contains(notes, "MachO") { + t.Errorf("Expected notes to contain macOS information:\n%s", notes) + } + + if strings.Contains(file, "amd64") && !(strings.Contains(notes, "amd64") || strings.Contains(notes, "EM_X86_64") || strings.Contains(notes, "CpuAmd64")) { + t.Errorf("Expected notes to contain amd64 information:\n%s", notes) + } + + if strings.Contains(file, "arm64") && !strings.Contains(notes, "CpuArm64") { + t.Errorf("Expected notes to contain arm64 information:\n%s", notes) + } + if strings.Contains(file, "386") && !strings.Contains(notes, "386") { + t.Errorf("Expected notes to contain 386 information:\n%s", notes) + } + + } +} diff --git a/plugin_test.go b/plugin_test.go index 7ce24aa6..ba5e9839 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -15,7 +15,7 @@ import ( "time" "github.com/golang/protobuf/ptypes/empty" - hclog "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-hclog" grpctest "github.com/hashicorp/go-plugin/test/grpc" "golang.org/x/net/context" "google.golang.org/grpc" diff --git a/testdata/.gitignore b/testdata/.gitignore new file mode 100644 index 00000000..81665891 --- /dev/null +++ b/testdata/.gitignore @@ -0,0 +1,5 @@ +windows-amd64.exe +windows-386.exe +linux-amd64 +darwin-amd64 +darwin-arm64 diff --git a/testdata/Makefile b/testdata/Makefile new file mode 100644 index 00000000..85ce1be6 --- /dev/null +++ b/testdata/Makefile @@ -0,0 +1,29 @@ +default: all +.PHONY: default + +.PHONY: clean +clean: + rm -f windows-amd64.exe windows-386.exe linux-amd64 darwin-amd64 darwin-arm64 + +.PHONY: all +all: windows-amd64.exe windows-386.exe linux-amd64 darwin-amd64 darwin-arm64 + +.PHONY: windows-amd64.exe +windows-amd64.exe: + GOOS=windows GOARCH=amd64 go build -o $@ + +.PHONY: windows-386.exe +windows-386.exe: + GOOS=windows GOARCH=386 go build -o $@ + +.PHONY: linux-amd64 +linux-amd64: + GOOS=linux GOARCH=amd64 go build -o $@ + +.PHONY: darwin-amd64 +darwin-amd64: + GOOS=darwin GOARCH=amd64 go build -o $@ + +.PHONY: darwin-arm64 +darwin-arm64: + GOOS=darwin GOARCH=arm64 go build -o $@ \ No newline at end of file diff --git a/testdata/README.md b/testdata/README.md new file mode 100644 index 00000000..f089c4fe --- /dev/null +++ b/testdata/README.md @@ -0,0 +1 @@ +This files contains a minimal Go program so that we can obtain example binaries of programs for various architectures for use in tests. \ No newline at end of file diff --git a/testdata/go.mod b/testdata/go.mod new file mode 100644 index 00000000..8dacb692 --- /dev/null +++ b/testdata/go.mod @@ -0,0 +1,3 @@ +module example.com/testadata + +go 1.19 diff --git a/testdata/go.sum b/testdata/go.sum new file mode 100644 index 00000000..e69de29b diff --git a/testdata/minimal.go b/testdata/minimal.go new file mode 100644 index 00000000..da29a2ca --- /dev/null +++ b/testdata/minimal.go @@ -0,0 +1,4 @@ +package main + +func main() { +} From b68c43a434df61cd10cb2312220a29d3afb107d4 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Fri, 2 Dec 2022 15:24:40 -0800 Subject: [PATCH 2/8] Don't use named return value --- client.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/client.go b/client.go index 439dc25a..66749d31 100644 --- a/client.go +++ b/client.go @@ -497,10 +497,11 @@ var peTypes = map[uint16]string{ // additionalNotesAboutCommand tries to get additional information about a command that might help diagnose // why it won't run correctly. It runs as a best effort only. -func additionalNotesAboutCommand(path string) (notes string) { +func additionalNotesAboutCommand(path string) string { + notes := "" stat, err := os.Stat(path) if err != nil { - return + return notes } notes += "\nAdditional notes about plugin:" @@ -518,7 +519,7 @@ func additionalNotesAboutCommand(path string) (notes string) { } notes += fmt.Sprintf(" PE architecture: %s (current architecture: %s)\n", machine, runtime.GOARCH) } - return + return notes } // Start the underlying subprocess, communicating with it to negotiate From e3c0d2e67034564aa3ff3673e2eefb884d7a6553 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Fri, 2 Dec 2022 15:25:18 -0800 Subject: [PATCH 3/8] Update client.go Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 66749d31..e7a2ea10 100644 --- a/client.go +++ b/client.go @@ -504,7 +504,7 @@ func additionalNotesAboutCommand(path string) string { return notes } - notes += "\nAdditional notes about plugin:" + notes += "\nAdditional notes about plugin:\n" notes += fmt.Sprintf(" Path: %s\n", path) notes += fmt.Sprintf(" Mode: %s\n", stat.Mode()) From 450593f4a7658aa67e0744642cfadf33cf47221a Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Fri, 2 Dec 2022 15:30:40 -0800 Subject: [PATCH 4/8] Unimportant typo fix --- testdata/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testdata/go.mod b/testdata/go.mod index 8dacb692..68834454 100644 --- a/testdata/go.mod +++ b/testdata/go.mod @@ -1,3 +1,3 @@ -module example.com/testadata +module example.com/testdata go 1.19 From cfea36eb109b33f410d8761031d4d42c53c56322 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Mon, 5 Dec 2022 09:04:36 -0800 Subject: [PATCH 5/8] Update client.go Co-authored-by: Tom Proctor --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index e7a2ea10..cf5608c6 100644 --- a/client.go +++ b/client.go @@ -35,7 +35,7 @@ This usually means the plugin was not compiled for this architecture, the plugin is missing dynamic-link libraries necessary to run, the plugin is not readable by this process, or - the plugin was compiled with an incompatible version of the go-plugin protocol + the plugin failed to negotiate the initial go-plugin protocol handshake %s` // If this is 1, then we've called CleanupClients. This can be used From 996b4d63e7133f3ad9f3a9cae2143482c5d42a3d Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Mon, 5 Dec 2022 09:04:43 -0800 Subject: [PATCH 6/8] Update testdata/README.md Co-authored-by: Tom Proctor --- testdata/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testdata/README.md b/testdata/README.md index f089c4fe..cb81911d 100644 --- a/testdata/README.md +++ b/testdata/README.md @@ -1 +1 @@ -This files contains a minimal Go program so that we can obtain example binaries of programs for various architectures for use in tests. \ No newline at end of file +This folder contains a minimal Go program so that we can obtain example binaries of programs for various architectures for use in tests. \ No newline at end of file From b47fd6448fbb3f16124eddf9eb9fc18f781a99b3 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Mon, 5 Dec 2022 09:36:41 -0800 Subject: [PATCH 7/8] Update client.go Co-authored-by: Tom Proctor --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index cf5608c6..3778a390 100644 --- a/client.go +++ b/client.go @@ -34,7 +34,7 @@ const unrecognizedRemotePluginMessage = `Unrecognized remote plugin message: %s This usually means the plugin was not compiled for this architecture, the plugin is missing dynamic-link libraries necessary to run, - the plugin is not readable by this process, or + the plugin is not executable by this process due to file permissions, or the plugin failed to negotiate the initial go-plugin protocol handshake %s` From b616f4e8634a677829a60f06f2834789cf84ce3d Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Mon, 5 Dec 2022 09:47:09 -0800 Subject: [PATCH 8/8] Add additional notes about users and groups --- client.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/client.go b/client.go index 3778a390..33bc85a5 100644 --- a/client.go +++ b/client.go @@ -18,12 +18,14 @@ import ( "net" "os" "os/exec" + "os/user" "path/filepath" "runtime" "strconv" "strings" "sync" "sync/atomic" + "syscall" "time" "github.com/hashicorp/go-hclog" @@ -507,6 +509,27 @@ func additionalNotesAboutCommand(path string) string { notes += "\nAdditional notes about plugin:\n" notes += fmt.Sprintf(" Path: %s\n", path) notes += fmt.Sprintf(" Mode: %s\n", stat.Mode()) + statT, ok := stat.Sys().(*syscall.Stat_t) + if ok { + currentUsername := "?" + if u, err := user.LookupId(strconv.FormatUint(uint64(os.Getuid()), 10)); err == nil { + currentUsername = u.Username + } + currentGroup := "?" + if g, err := user.LookupGroupId(strconv.FormatUint(uint64(os.Getgid()), 10)); err == nil { + currentGroup = g.Name + } + username := "?" + if u, err := user.LookupId(strconv.FormatUint(uint64(statT.Uid), 10)); err == nil { + username = u.Username + } + group := "?" + if g, err := user.LookupGroupId(strconv.FormatUint(uint64(statT.Gid), 10)); err == nil { + group = g.Name + } + notes += fmt.Sprintf(" Owner: %d [%s] (current: %d [%s])\n", statT.Uid, username, os.Getuid(), currentUsername) + notes += fmt.Sprintf(" Group: %d [%s] (current: %d [%s])\n", statT.Gid, group, os.Getgid(), currentGroup) + } if elfFile, err := elf.Open(path); err == nil { notes += fmt.Sprintf(" ELF architecture: %s (current architecture: %s)\n", elfFile.Machine, runtime.GOARCH)