diff --git a/admin/cli/cli.go b/admin/cli/cli.go index 167cd51fe08..2608e07ab65 100644 --- a/admin/cli/cli.go +++ b/admin/cli/cli.go @@ -29,6 +29,7 @@ import ( "github.com/percona/pmm/admin/commands/inventory" "github.com/percona/pmm/admin/commands/management" "github.com/percona/pmm/admin/pkg/flags" + "github.com/percona/pmm/utils/servererror" ) // GlobalFlagsGetter supports retrieving GlobalFlags. @@ -137,7 +138,14 @@ func printResponse(opts *flags.GlobalFlags, res commands.Result, err error) erro } } - return err + // Transport-level failures never reach the cases above. Point the user at + // --server-insecure-tls when PMM Server presents a certificate we cannot verify. + var host string + if opts.ServerURL != nil { + host = opts.ServerURL.Hostname() + } + + return servererror.WrapTLSError(err, host, opts.SkipTLSCertificateCheck) } func printSuccessResult(opts *flags.GlobalFlags, res commands.Result) { @@ -164,11 +172,7 @@ func printErrorResponse(opts *flags.GlobalFlags, err commands.ErrorResponse) { } fmt.Printf("%s\n", b) //nolint:forbidigo } else { - msg := e.Error - if e.Code == 401 { //nolint:mnd - msg += ". Please check username and password." - } - fmt.Println(msg) //nolint:forbidigo + fmt.Println(commands.ServerErrorMessage(e)) //nolint:forbidigo } } diff --git a/admin/cli/cli_test.go b/admin/cli/cli_test.go new file mode 100644 index 00000000000..afc835c8ad4 --- /dev/null +++ b/admin/cli/cli_test.go @@ -0,0 +1,178 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cli + +import ( + "context" + "crypto/x509" + "encoding/json" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + + httptransport "github.com/go-openapi/runtime/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/percona/pmm/admin/commands/base" + "github.com/percona/pmm/admin/commands/inventory" + "github.com/percona/pmm/admin/pkg/flags" + inventoryClient "github.com/percona/pmm/api/inventory/v1/json/client" + "github.com/percona/pmm/utils/servererror" +) + +func TestPrintResponseTLSError(t *testing.T) { + t.Parallel() + + serverURL, err := url.Parse("https://admin:admin@pmm-server-second:8443/") + require.NoError(t, err) + + certErr := &url.Error{ + Op: "Put", + URL: "https://pmm-server-second:8443/v1/inventory/agents/722fbfc8", + Err: x509.HostnameError{Certificate: &x509.Certificate{}, Host: "pmm-server-second"}, + } + + t.Run("hint added", func(t *testing.T) { + t.Parallel() + + opts := &flags.GlobalFlags{ServerURL: serverURL} //nolint:exhaustruct + + wrapped := printResponse(opts, nil, certErr) + require.Error(t, wrapped) + assert.Contains(t, wrapped.Error(), servererror.InsecureTLSFlag) + }) + + t.Run("no hint when validation is already disabled", func(t *testing.T) { + t.Parallel() + + opts := &flags.GlobalFlags{ServerURL: serverURL, SkipTLSCertificateCheck: true} //nolint:exhaustruct + + assert.Equal(t, certErr, printResponse(opts, nil, certErr)) + }) + + t.Run("unrelated errors are untouched", func(t *testing.T) { + t.Parallel() + + opts := &flags.GlobalFlags{ServerURL: serverURL} //nolint:exhaustruct + other := errors.New("connection refused") + + assert.Equal(t, other, printResponse(opts, nil, other)) + }) +} + +// redirectToTestServer makes the configured PMM Server clients dial srv while still using the +// host name from --server-url for the TLS handshake, so that a certificate mismatch can be +// reproduced without touching DNS. +func redirectToTestServer(t *testing.T, srv *httptest.Server) { + t.Helper() + + target, err := url.Parse(srv.URL) + require.NoError(t, err) + + runtime, ok := inventoryClient.Default.Transport.(*httptransport.Runtime) + require.True(t, ok) + + transport, ok := runtime.Transport.(*http.Transport) + require.True(t, ok) + + transport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, target.Host) + } +} + +// TestChangeAgentAgainstMismatchedCertificate reproduces PMM-15186 end to end: pointing +// --server-url at a PMM Server whose certificate is issued for a different host name must +// fail with a message naming --server-insecure-tls, and must succeed once that flag is set. +func TestChangeAgentAgainstMismatchedCertificate(t *testing.T) { + // Not parallel: SetupClients configures the package-level API clients. + const agentID = "722fbfc8-8497-4acc-839b-ec53983cf398" + + // Written by the handler goroutine, read by the test goroutine. + var ( + mu sync.Mutex + gotAuth string + ) + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + gotAuth = r.Header.Get("Authorization") + mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "postgres_exporter": map[string]any{"agent_id": agentID}, + })) + })) + t.Cleanup(srv.Close) + + // The httptest certificate is issued for example.com and 127.0.0.1 only, mirroring the + // localhost-only certificate PMM Server ships with. + serverURL, err := url.Parse("https://admin:admin@pmm-server-second:8443/") + require.NoError(t, err) + + t.Run("without --server-insecure-tls", func(t *testing.T) { + opts := &flags.GlobalFlags{ServerURL: cloneURL(t, serverURL)} //nolint:exhaustruct + base.SetupClients(opts) + redirectToTestServer(t, srv) + + cmd := &inventory.ChangeAgentPostgresExporterCommand{AgentID: agentID} //nolint:exhaustruct + _, cmdErr := cmd.RunCmd() + require.Error(t, cmdErr) + + wrapped := printResponse(opts, nil, cmdErr) + require.Error(t, wrapped) + + msg := wrapped.Error() + assert.True(t, servererror.IsTLSCertificateError(wrapped), msg) + assert.Contains(t, msg, "PMM Server TLS certificate could not be verified") + assert.Contains(t, msg, `not valid for host "pmm-server-second"`) + assert.Contains(t, msg, servererror.InsecureTLSFlag) + }) + + t.Run("with --server-insecure-tls", func(t *testing.T) { + opts := &flags.GlobalFlags{ //nolint:exhaustruct + ServerURL: cloneURL(t, serverURL), + SkipTLSCertificateCheck: true, + } + base.SetupClients(opts) + redirectToTestServer(t, srv) + + cmd := &inventory.ChangeAgentPostgresExporterCommand{AgentID: agentID} //nolint:exhaustruct + res, cmdErr := cmd.RunCmd() + require.NoError(t, cmdErr) + require.NotNil(t, res) + + // Credentials from --server-url must reach PMM Server; the ticket comment + // reported them being rejected. + mu.Lock() + defer mu.Unlock() + assert.Equal(t, "Basic YWRtaW46YWRtaW4=", gotAuth) + }) +} + +// cloneURL returns a copy of u, since SetupClients mutates the URL it is given. +func cloneURL(t *testing.T, u *url.URL) *url.URL { + t.Helper() + + c, err := url.Parse(u.String()) + require.NoError(t, err) + + return c +} diff --git a/admin/commands/base.go b/admin/commands/base.go index 62e6f2ef74f..5f5f5647c8d 100644 --- a/admin/commands/base.go +++ b/admin/commands/base.go @@ -123,6 +123,12 @@ type ErrorResponse interface { type Error struct { Code int `json:"code"` Error string `json:"error"` + + // GRPCCode is the gRPC status code carried in the response payload. It is kept out of + // the JSON output to preserve the documented shape of `pmm-admin --json` errors. + // PMM Server maps several gRPC codes onto HTTP 401, so Code alone cannot tell an + // authentication failure apart from an internal error - see ServerErrorMessage. + GRPCCode int32 `json:"-"` } // GetError converts an ErrorResponse to an Error. @@ -130,9 +136,17 @@ func GetError(err ErrorResponse) Error { v := reflect.ValueOf(err) p := v.Elem().FieldByName("Payload") e := p.Elem().FieldByName("Message") + + // Not every generated payload carries a code, and older PMM Servers may leave it unset. + var grpcCode int32 + if c := p.Elem().FieldByName("Code"); c.IsValid() && c.CanInt() { + grpcCode = int32(c.Int()) //nolint:gosec + } + return Error{ - Code: err.Code(), - Error: e.String(), + Code: err.Code(), + Error: e.String(), + GRPCCode: grpcCode, } } diff --git a/admin/commands/base/setup.go b/admin/commands/base/setup.go index 26c5e4c983f..5bc9ee28c96 100644 --- a/admin/commands/base/setup.go +++ b/admin/commands/base/setup.go @@ -50,6 +50,22 @@ var ( _ fmt.GoStringer = nginxError("") ) +// applyAgentServerParams fills in the PMM Server connection parameters reported by the local +// pmm-agent. An explicitly passed --server-insecure-tls is preserved: the flag is opt-in only, +// so a user asking to skip validation must not have that request dropped just because the +// local pmm-agent is configured to validate certificates. +func applyAgentServerParams(globalFlags *flags.GlobalFlags, status *agentlocal.Status) error { + u, err := url.Parse(status.ServerURL) + if err != nil { + return err + } + + globalFlags.ServerURL = u + globalFlags.SkipTLSCertificateCheck = globalFlags.SkipTLSCertificateCheck || status.ServerInsecureTLS + + return nil +} + // SetupClients configures local and PMM Server API clients. func SetupClients(globalFlags *flags.GlobalFlags) { //nolint:nestif @@ -67,8 +83,11 @@ func SetupClients(globalFlags *flags.GlobalFlags) { logrus.Fatalf("Failed to get PMM Server parameters from local pmm-agent: %s.\n"+ "Please use --server-url flag to specify PMM Server URL.", err) } - globalFlags.ServerURL, _ = url.Parse(status.ServerURL) - globalFlags.SkipTLSCertificateCheck = status.ServerInsecureTLS + err = applyAgentServerParams(globalFlags, status) + if err != nil { + logrus.Fatalf("Failed to parse PMM Server URL %q reported by local pmm-agent: %s.\n"+ + "Please use --server-url flag to specify PMM Server URL.", status.ServerURL, err) + } } else { if globalFlags.ServerURL.Path == "" { globalFlags.ServerURL.Path = "/" @@ -112,17 +131,24 @@ func SetupClients(globalFlags *flags.GlobalFlags) { } // disable HTTP/2, set TLS config - httpTransport, ok := transport.Transport.(*http.Transport) + defaultTransport, ok := transport.Transport.(*http.Transport) if !ok { panic("cannot assert transport as http.Transport") } + // go-openapi hands out http.DefaultTransport, so work on a clone: reconfiguring TLS on + // the process-wide transport would leak into every other HTTP client in the process. + httpTransport := defaultTransport.Clone() + + // A non-nil TLSNextProto is the documented way to disable HTTP/2, and it takes + // precedence over the ForceAttemptHTTP2 that Clone carries over from the default. httpTransport.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper) if globalFlags.ServerURL.Scheme == "https" { httpTransport.TLSClientConfig = tlsconfig.Get() httpTransport.TLSClientConfig.ServerName = globalFlags.ServerURL.Hostname() httpTransport.TLSClientConfig.InsecureSkipVerify = globalFlags.SkipTLSCertificateCheck } + transport.Transport = httpTransport inventoryClient.Default.SetTransport(transport) managementClient.Default.SetTransport(transport) diff --git a/admin/commands/base/setup_test.go b/admin/commands/base/setup_test.go new file mode 100644 index 00000000000..b5bd80e647e --- /dev/null +++ b/admin/commands/base/setup_test.go @@ -0,0 +1,190 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package base + +import ( + "net/http" + "net/url" + "testing" + + httptransport "github.com/go-openapi/runtime/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/percona/pmm/admin/agentlocal" + "github.com/percona/pmm/admin/pkg/flags" + inventoryClient "github.com/percona/pmm/api/inventory/v1/json/client" +) + +func TestApplyAgentServerParams(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { + flagInsecureTLS bool + agentInsecureTLS bool + expected bool + }{ + "both secure": {false, false, false}, + "agent configured as insecure": {false, true, true}, + // PMM-15186: --server-insecure-tls is opt-in only, so it must survive the + // parameters read from a pmm-agent which validates certificates. + "flag wins over secure agent": {true, false, true}, + "both insecure": {true, true, true}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + globals := &flags.GlobalFlags{SkipTLSCertificateCheck: tc.flagInsecureTLS} //nolint:exhaustruct + status := &agentlocal.Status{ //nolint:exhaustruct + ServerURL: "https://admin:admin@pmm-server:8443/", + ServerInsecureTLS: tc.agentInsecureTLS, + } + + require.NoError(t, applyAgentServerParams(globals, status)) + + assert.Equal(t, tc.expected, globals.SkipTLSCertificateCheck) + require.NotNil(t, globals.ServerURL) + assert.Equal(t, "https://admin:admin@pmm-server:8443/", globals.ServerURL.String()) + }) + } +} + +// TestApplyAgentServerParamsInvalidURL checks that an unparseable URL is reported instead of +// silently leaving ServerURL nil for SetupClients to dereference. +func TestApplyAgentServerParamsInvalidURL(t *testing.T) { + t.Parallel() + + globals := &flags.GlobalFlags{} //nolint:exhaustruct + status := &agentlocal.Status{ServerURL: "https://pmm-server:8443/%zz"} //nolint:exhaustruct + + require.Error(t, applyAgentServerParams(globals, status)) + assert.Nil(t, globals.ServerURL) +} + +// tlsConfigOf returns the TLS configuration the PMM Server API clients were set up with. +func tlsConfigOf(t *testing.T) *http.Transport { + t.Helper() + + runtime, ok := inventoryClient.Default.Transport.(*httptransport.Runtime) + require.True(t, ok, "expected the inventory client to use a go-openapi runtime") + + transport, ok := runtime.Transport.(*http.Transport) + require.True(t, ok, "expected an *http.Transport") + + return transport +} + +// TestSetupClientsServerURL covers PMM-15186: a --server-url pointing at a host the PMM +// Server certificate was not issued for must be reachable with --server-insecure-tls, and +// must keep validating certificates without it. +func TestSetupClientsServerURL(t *testing.T) { + // Not parallel: SetupClients configures the package-level API clients. + for name, tc := range map[string]struct { + serverURL string + insecureTLS bool + + expectedInsecure bool + expectedServerName string + expectedAuth bool + }{ + "https with insecure tls": { + serverURL: "https://admin:admin@pmm-server-second:8443/", + insecureTLS: true, + expectedInsecure: true, + expectedServerName: "pmm-server-second", + expectedAuth: true, + }, + "https validating certificates": { + serverURL: "https://admin:admin@pmm-server-second:8443/", + insecureTLS: false, + expectedInsecure: false, + expectedServerName: "pmm-server-second", + expectedAuth: true, + }, + "https without credentials": { + serverURL: "https://pmm-server-second:8443/", + insecureTLS: true, + expectedInsecure: true, + expectedServerName: "pmm-server-second", + expectedAuth: false, + }, + } { + t.Run(name, func(t *testing.T) { + u, err := url.Parse(tc.serverURL) + require.NoError(t, err) + + globals := &flags.GlobalFlags{ //nolint:exhaustruct + ServerURL: u, + SkipTLSCertificateCheck: tc.insecureTLS, + } + + SetupClients(globals) + + transport := tlsConfigOf(t) + require.NotNil(t, transport.TLSClientConfig) + assert.Equal(t, tc.expectedInsecure, transport.TLSClientConfig.InsecureSkipVerify) + // ServerName is taken from the URL host, which is what makes the + // certificate mismatch reported in PMM-15186 detectable at all. + assert.Equal(t, tc.expectedServerName, transport.TLSClientConfig.ServerName) + // HTTP/2 must stay disabled. + assert.NotNil(t, transport.TLSNextProto) + assert.Empty(t, transport.TLSNextProto) + + runtime, ok := inventoryClient.Default.Transport.(*httptransport.Runtime) + require.True(t, ok) + if tc.expectedAuth { + assert.NotNil(t, runtime.DefaultAuthentication, "credentials from --server-url must be sent") + } else { + assert.Nil(t, runtime.DefaultAuthentication) + } + }) + } +} + +// TestSetupClientsAddsTrailingPath documents that a --server-url without a path is usable: +// go-openapi requires a base path. +func TestSetupClientsAddsTrailingPath(t *testing.T) { + u, err := url.Parse("https://admin:admin@pmm-server-second:8443") + require.NoError(t, err) + + globals := &flags.GlobalFlags{ServerURL: u, SkipTLSCertificateCheck: true} //nolint:exhaustruct + SetupClients(globals) + + assert.Equal(t, "/", globals.ServerURL.Path) +} + +// TestSetupClientsClonesTransport guards against reconfiguring TLS on http.DefaultTransport. +// Because go-openapi hands that global out, mutating it in place would leak PMM's TLS settings +// into every other HTTP client in the process - and, in tests, into every later test in the binary. +func TestSetupClientsClonesTransport(t *testing.T) { + // Not parallel: SetupClients configures the package-level API clients. + def, ok := http.DefaultTransport.(*http.Transport) + require.True(t, ok) + + u, err := url.Parse("https://admin:admin@pmm-server-second:8443/") + require.NoError(t, err) + + globals := &flags.GlobalFlags{ServerURL: u, SkipTLSCertificateCheck: true} //nolint:exhaustruct + SetupClients(globals) + + assert.NotSame(t, def, tlsConfigOf(t), "SetupClients must configure a clone of http.DefaultTransport") + + // The HTTP/2 machinery may install an empty TLS config on the global, but none of PMM's + // settings may end up there. + if c := def.TLSClientConfig; c != nil { + assert.Empty(t, c.ServerName) + assert.False(t, c.InsecureSkipVerify) + } +} diff --git a/admin/commands/servererror.go b/admin/commands/servererror.go new file mode 100644 index 00000000000..e2229ac389f --- /dev/null +++ b/admin/commands/servererror.go @@ -0,0 +1,34 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "strings" + + "github.com/percona/pmm/utils/servererror" +) + +// ServerErrorMessage renders a PMM Server error response for humans, adding a hint about the +// likely cause where one can be derived from the response. +func ServerErrorMessage(e Error) string { + msg := e.Error + if hint := servererror.AuthHint(e.Code, e.GRPCCode); hint != "" { + // PMM Server messages usually already end with a period, so trim it instead of + // producing "Internal server error.. Please check PMM Server logs.". + msg = strings.TrimRight(msg, ". ") + ". " + hint + "." + } + + return msg +} diff --git a/admin/commands/servererror_test.go b/admin/commands/servererror_test.go new file mode 100644 index 00000000000..0d4fe2cfa06 --- /dev/null +++ b/admin/commands/servererror_test.go @@ -0,0 +1,128 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + agents "github.com/percona/pmm/api/inventory/v1/json/client/agents_service" +) + +func TestServerErrorMessage(t *testing.T) { + t.Parallel() + + const ( + grpcUnauthenticated = 16 + grpcInternal = 13 + grpcNotFound = 5 + ) + + for name, tc := range map[string]struct { + err Error + expected string + }{ + "rejected credentials": { + err: Error{Code: 401, Error: "Invalid username or password", GRPCCode: grpcUnauthenticated}, + expected: "Invalid username or password. Please check username and password.", + }, + "internal error mapped to 401": { + // PMM-15186 reported this exact response being blamed on the credentials. + // The trailing period of the message must not be doubled up. + err: Error{Code: 401, Error: "Internal server error.", GRPCCode: grpcInternal}, + expected: "Internal server error. Please check PMM Server logs.", + }, + "401 without a gRPC code": { + err: Error{Code: 401, Error: "Unauthorized"}, + expected: "Unauthorized. Please check username and password.", + }, + "not found": { + err: Error{Code: 404, Error: "Agent with ID 722fbfc8 not found.", GRPCCode: grpcNotFound}, + expected: "Agent with ID 722fbfc8 not found.", + }, + "forbidden": { + err: Error{Code: 403, Error: "Access denied", GRPCCode: 7}, + expected: "Access denied. Please check that your PMM user has sufficient permissions.", + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.expected, ServerErrorMessage(tc.err)) + }) + } +} + +// TestGetErrorFromGeneratedResponse checks the reflection in GetError against a real +// generated response type, including the gRPC code ServerErrorMessage relies on. +func TestGetErrorFromGeneratedResponse(t *testing.T) { + t.Parallel() + + t.Run("with gRPC code", func(t *testing.T) { + t.Parallel() + + resp := agents.NewChangeAgentDefault(401) + resp.Payload = &agents.ChangeAgentDefaultBody{ //nolint:exhaustruct + Code: 16, // codes.Unauthenticated + Message: "Invalid username or password", + } + + e := GetError(resp) + assert.Equal(t, 401, e.Code) + assert.Equal(t, "Invalid username or password", e.Error) + assert.Equal(t, int32(16), e.GRPCCode) + assert.Equal(t, "Invalid username or password. Please check username and password.", ServerErrorMessage(e)) + }) + + t.Run("internal error mapped to 401", func(t *testing.T) { + t.Parallel() + + // The exact response reported in PMM-15186. + resp := agents.NewChangeAgentDefault(401) + resp.Payload = &agents.ChangeAgentDefaultBody{ //nolint:exhaustruct + Code: 13, // codes.Internal + Message: "Internal server error.", + } + + e := GetError(resp) + assert.Equal(t, int32(13), e.GRPCCode) + assert.Equal(t, "Internal server error. Please check PMM Server logs.", ServerErrorMessage(e)) + }) + + t.Run("without gRPC code", func(t *testing.T) { + t.Parallel() + + resp := agents.NewChangeAgentDefault(404) + resp.Payload = &agents.ChangeAgentDefaultBody{Message: "Agent not found."} //nolint:exhaustruct + + e := GetError(resp) + assert.Equal(t, 404, e.Code) + assert.Zero(t, e.GRPCCode) + assert.Equal(t, "Agent not found.", ServerErrorMessage(e)) + }) +} + +// TestServerErrorJSONShapeUnchanged guards the documented `pmm-admin --json` error shape: +// GRPCCode is internal and must not leak into it. +func TestServerErrorJSONShapeUnchanged(t *testing.T) { + t.Parallel() + + b, err := json.Marshal(Error{Code: 401, Error: "Invalid username or password", GRPCCode: 16}) + require.NoError(t, err) + assert.JSONEq(t, `{"code":401,"error":"Invalid username or password"}`, string(b)) +} diff --git a/agent/commands/setup.go b/agent/commands/setup.go index d674398a497..0a1b10de70e 100644 --- a/agent/commands/setup.go +++ b/agent/commands/setup.go @@ -28,6 +28,7 @@ import ( "github.com/percona/pmm/agent/config" agent_local "github.com/percona/pmm/api/agentlocal/v1/json/client/agent_local_service" mservice "github.com/percona/pmm/api/management/v1/json/client/management_service" + "github.com/percona/pmm/utils/servererror" ) // Setup implements `pmm-agent setup` command. @@ -132,6 +133,36 @@ func checkStatus(configFilepath string, l *logrus.Entry) (string, bool) { } } +// registerErrorMessage explains why registering on PMM Server failed. The host argument is the +// PMM Server host name the TLS certificate was checked against, and insecureTLS reports whether +// that check was disabled. +func registerErrorMessage(err error, host string, insecureTLS bool) string { + // Point the user at --server-insecure-tls when PMM Server presents a certificate we + // cannot verify - its shipped certificate is issued for localhost only. + err = servererror.WrapTLSError(err, host, insecureTLS) + + msg := err.Error() + + e, ok := errors.AsType[*mservice.RegisterNodeDefault](err) + if ok { + msg = e.Payload.Message + if e.Code() == http.StatusConflict { + msg += " If you want override node, use --force option" + } + // The HTTP status alone cannot tell an authentication failure apart from an + // internal one, since PMM Server maps several gRPC codes onto HTTP 401. + if hint := servererror.AuthHint(e.Code(), e.Payload.Code); hint != "" { + msg += "\n" + hint + } + } + + if _, ok := errors.AsType[nginxError](err); ok { + msg += ".\nPlease check pmm-managed logs." + } + + return msg +} + func register(cfg *config.Config, l *logrus.Entry) { fmt.Printf("Registering pmm-agent on PMM Server...\n") @@ -145,22 +176,7 @@ func register(cfg *config.Config, l *logrus.Entry) { agentID, token, err := serverRegister(&cfg.Setup) l.Debugf("Register error: %#v", err) if err != nil { - msg := err.Error() - e, ok := errors.AsType[*mservice.RegisterNodeDefault](err) - if ok { - msg = e.Payload.Message + "" - switch e.Code() { - case http.StatusConflict: - msg += " If you want override node, use --force option" - case http.StatusUnauthorized, http.StatusForbidden: - msg += "\nPlease check username and password" - } - } - if _, ok := err.(nginxError); ok { //nolint:errorlint - msg += ".\nPlease check pmm-managed logs." - } - - fmt.Printf("Failed to register pmm-agent on PMM Server: %s.\n", msg) + fmt.Printf("Failed to register pmm-agent on PMM Server: %s.\n", registerErrorMessage(err, u.Hostname(), cfg.Server.InsecureTLS)) os.Exit(1) } cfg.ID = agentID diff --git a/agent/commands/setup_test.go b/agent/commands/setup_test.go new file mode 100644 index 00000000000..7963d8948a3 --- /dev/null +++ b/agent/commands/setup_test.go @@ -0,0 +1,128 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package commands + +import ( + "crypto/x509" + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + + mservice "github.com/percona/pmm/api/management/v1/json/client/management_service" + "github.com/percona/pmm/utils/servererror" +) + +// registerDefault builds the error the generated client returns for a failed registration. +func registerDefault(httpCode int, grpcCode int32, message string) *mservice.RegisterNodeDefault { + resp := mservice.NewRegisterNodeDefault(httpCode) + resp.Payload = &mservice.RegisterNodeDefaultBody{ //nolint:exhaustruct + Code: grpcCode, + Message: message, + } + + return resp +} + +func TestRegisterErrorMessage(t *testing.T) { + t.Parallel() + + const ( + grpcUnauthenticated = 16 + grpcInternal = 13 + grpcPermissionDenied = 7 + grpcAlreadyExists = 6 + ) + + t.Run("rejected credentials", func(t *testing.T) { + t.Parallel() + + msg := registerErrorMessage( + registerDefault(http.StatusUnauthorized, grpcUnauthenticated, "Invalid username or password"), + "pmm-server", false, + ) + assert.Equal(t, "Invalid username or password\nPlease check username and password", msg) + }) + + t.Run("internal error mapped to 401", func(t *testing.T) { + t.Parallel() + + // PMM Server maps internal authentication errors onto HTTP 401 as well, so the + // credentials must not be blamed for them. + msg := registerErrorMessage( + registerDefault(http.StatusUnauthorized, grpcInternal, "Internal server error."), + "pmm-server", false, + ) + assert.Equal(t, "Internal server error.\nPlease check PMM Server logs", msg) + }) + + t.Run("access denied", func(t *testing.T) { + t.Parallel() + + // Not a credentials problem: the user authenticated but lacks the required role. + msg := registerErrorMessage( + registerDefault(http.StatusForbidden, grpcPermissionDenied, "Access denied"), + "pmm-server", false, + ) + assert.Equal(t, "Access denied\nPlease check that your PMM user has sufficient permissions", msg) + }) + + t.Run("node already exists", func(t *testing.T) { + t.Parallel() + + msg := registerErrorMessage( + registerDefault(http.StatusConflict, grpcAlreadyExists, "Node with name \"node\" already exists."), + "pmm-server", false, + ) + assert.Equal(t, "Node with name \"node\" already exists. If you want override node, use --force option", msg) + }) + + t.Run("certificate cannot be verified", func(t *testing.T) { + t.Parallel() + + certErr := &url.Error{ + Op: "Post", + URL: "https://pmm-server:8443/v1/management/nodes", + Err: x509.HostnameError{Certificate: &x509.Certificate{}, Host: "pmm-server"}, //nolint:exhaustruct + } + + msg := registerErrorMessage(certErr, "pmm-server", false) + assert.Contains(t, msg, "PMM Server TLS certificate could not be verified") + assert.Contains(t, msg, `not valid for host "pmm-server"`) + assert.Contains(t, msg, servererror.InsecureTLSFlag) + }) + + t.Run("certificate error with validation disabled", func(t *testing.T) { + t.Parallel() + + certErr := &url.Error{ + Op: "Post", + URL: "https://pmm-server:8443/v1/management/nodes", + Err: x509.HostnameError{Certificate: &x509.Certificate{}, Host: "pmm-server"}, //nolint:exhaustruct + } + + msg := registerErrorMessage(certErr, "pmm-server", true) + assert.Equal(t, certErr.Error(), msg) + assert.NotContains(t, msg, servererror.InsecureTLSFlag) + }) + + t.Run("nginx response", func(t *testing.T) { + t.Parallel() + + msg := registerErrorMessage(nginxError("502 Bad Gateway"), "pmm-server", false) + assert.Equal(t, "response from nginx: 502 Bad Gateway.\nPlease check pmm-managed logs.", msg) + }) +} diff --git a/utils/servererror/servererror.go b/utils/servererror/servererror.go new file mode 100644 index 00000000000..d8084745585 --- /dev/null +++ b/utils/servererror/servererror.go @@ -0,0 +1,117 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package servererror turns errors returned by PMM Server API calls into messages a CLI user +// can act on. It is shared by pmm-admin and pmm-agent: both talk to PMM Server over the same +// transport and both expose the same --server-insecure-tls flag, so both need the same hints. +package servererror + +import ( + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net/http" +) + +// InsecureTLSFlag is the name of the flag which disables PMM Server TLS certificate +// validation; pmm-admin and pmm-agent spell it the same way. +const InsecureTLSFlag = "--server-insecure-tls" + +// gRPC status codes carried in PMM Server response payloads. They are part of the wire format +// (https://grpc.io/docs/guides/status-codes/) and are spelled out here rather than taken from +// google.golang.org/grpc/codes so that pmm-admin does not have to link the gRPC packages. +const ( + codePermissionDenied = 7 + codeUnauthenticated = 16 +) + +// IsTLSCertificateError reports whether err was caused by a failure to verify the TLS +// certificate presented by PMM Server. PMM Server is shipped with a self-signed certificate +// issued for localhost only, so this is the expected outcome of addressing it by any other +// host name. +func IsTLSCertificateError(err error) bool { + if err == nil { + return false + } + + var ( + verificationErr *tls.CertificateVerificationError + hostnameErr x509.HostnameError + authorityErr x509.UnknownAuthorityError + invalidErr x509.CertificateInvalidError + ) + + // Since Go 1.20 crypto/tls wraps both chain and host name failures in + // tls.CertificateVerificationError on every platform, including the macOS and Windows + // system verifiers. The bare x509 errors are matched too, so that errors constructed or + // re-wrapped by callers - and by this package's tests - are still recognised. + return errors.As(err, &verificationErr) || + errors.As(err, &hostnameErr) || + errors.As(err, &authorityErr) || + errors.As(err, &invalidErr) +} + +// WrapTLSError appends a hint about --server-insecure-tls to TLS certificate verification +// failures, naming host as the address the certificate was checked against. Other errors, and +// failures seen while certificate validation is already disabled, are returned unchanged. +func WrapTLSError(err error, host string, insecureTLS bool) error { + if insecureTLS || !IsTLSCertificateError(err) { + return err + } + + reason := "PMM Server TLS certificate could not be verified: it is either self-signed or not valid for the requested host." + if host != "" { + reason = fmt.Sprintf( + "PMM Server TLS certificate could not be verified: it is either self-signed or not valid for host %q.", + host, + ) + } + + return fmt.Errorf("%w.\n%s\nRe-run the command with %s to skip PMM Server TLS certificate validation, "+ + "or configure PMM Server with a certificate valid for that host", err, reason, InsecureTLSFlag) +} + +// AuthHint returns a sentence, without trailing punctuation, explaining an authentication +// error reported by PMM Server. It returns an empty string when the response does not describe +// one. The httpCode argument is the HTTP status and grpcCode the gRPC code carried in the +// response payload, which is zero when PMM Server did not send one. +// +// Callers own the punctuation and the separator: pmm-admin appends the hint to a single-line +// message, pmm-agent puts it on a line of its own. +func AuthHint(httpCode int, grpcCode int32) string { + switch { + // PMM Server reports rejected credentials with the gRPC Unauthenticated code. It sends + // no gRPC code only on paths which never reach the API, so a bare HTTP 401 means the + // credentials were rejected as well. + case grpcCode == codeUnauthenticated, + httpCode == http.StatusUnauthorized && grpcCode == 0: + return "Please check username and password" + + // The user authenticated but their role does not allow the request. nginx serves PMM + // Server's PermissionDenied as HTTP 403 with a static body carrying the same gRPC code, + // so this is not a credentials problem and must not be reported as one. + case grpcCode == codePermissionDenied, + httpCode == http.StatusForbidden && grpcCode == 0: + return "Please check that your PMM user has sufficient permissions" + + // nginx auth_request accepts 401 and 403 only, so PMM Server maps every other + // authentication error - internal ones included - onto HTTP 401 as well. Those are not + // caused by wrong credentials and must not be reported as such. + case httpCode == http.StatusUnauthorized: + return "Please check PMM Server logs" + } + + return "" +} diff --git a/utils/servererror/servererror_test.go b/utils/servererror/servererror_test.go new file mode 100644 index 00000000000..9c631cbc22b --- /dev/null +++ b/utils/servererror/servererror_test.go @@ -0,0 +1,210 @@ +// Copyright (C) 2023 Percona LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package servererror + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// tlsServerError performs a request against a TLS test server without trusting its +// certificate, returning the error a PMM Server call would fail with. The hostname argument +// selects the name the client uses in the TLS handshake, which is how the two distinct +// failures are produced: an untrusted issuer, and a certificate issued for a different host. +func tlsServerError(t *testing.T, hostname string) error { + t.Helper() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + u, err := url.Parse(srv.URL) + require.NoError(t, err) + + client := &http.Client{ //nolint:exhaustruct + Transport: &http.Transport{ //nolint:exhaustruct + // httptest certificates are issued for "example.com" and 127.0.0.1, so + // dialing the server while claiming a different name mismatches the SANs. + DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, u.Host) + }, + }, + } + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://"+hostname+"/v1/inventory/agents", nil) + require.NoError(t, err) + + resp, err := client.Do(req) + if resp != nil { + require.NoError(t, resp.Body.Close()) + } + + require.Error(t, err) + + return err +} + +func TestIsTLSCertificateError(t *testing.T) { + t.Parallel() + + t.Run("untrusted issuer", func(t *testing.T) { + t.Parallel() + + assert.True(t, IsTLSCertificateError(tlsServerError(t, "127.0.0.1"))) + }) + + t.Run("host name mismatch", func(t *testing.T) { + t.Parallel() + + assert.True(t, IsTLSCertificateError(tlsServerError(t, "pmm-server-second"))) + }) + + t.Run("bare x509 errors", func(t *testing.T) { + t.Parallel() + + // crypto/tls wraps these in tls.CertificateVerificationError itself, but errors + // constructed or re-wrapped by callers must be recognised too. + for name, err := range map[string]error{ + "hostname": x509.HostnameError{Certificate: &x509.Certificate{}, Host: "pmm-server-second"}, //nolint:exhaustruct + "authority": x509.UnknownAuthorityError{}, //nolint:exhaustruct + "invalid": x509.CertificateInvalidError{Cert: &x509.Certificate{}, Reason: x509.Expired}, //nolint:exhaustruct + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.True(t, IsTLSCertificateError(err)) + // Errors reach us wrapped in *url.Error by net/http. + assert.True(t, IsTLSCertificateError(&url.Error{Op: "Put", URL: "https://pmm/", Err: err})) + }) + } + }) + + t.Run("unrelated errors", func(t *testing.T) { + t.Parallel() + + assert.False(t, IsTLSCertificateError(nil)) + assert.False(t, IsTLSCertificateError(errors.New("connection refused"))) + assert.False(t, IsTLSCertificateError(&url.Error{ + Op: "Put", URL: "https://pmm/", Err: errors.New("EOF"), + })) + // A handshake failure which is not about the certificate must not be reported + // as one, otherwise --server-insecure-tls would be suggested for nothing. + assert.False(t, IsTLSCertificateError(tls.RecordHeaderError{ //nolint:exhaustruct + Msg: "first record does not look like a TLS handshake", + })) + }) +} + +func TestWrapTLSError(t *testing.T) { + t.Parallel() + + certErr := &url.Error{ + Op: "Put", + URL: "https://pmm-server-second:8443/v1/inventory/agents/722fbfc8", + Err: x509.HostnameError{Certificate: &x509.Certificate{}, Host: "pmm-server-second"}, //nolint:exhaustruct + } + + t.Run("adds hint", func(t *testing.T) { + t.Parallel() + + wrapped := WrapTLSError(certErr, "pmm-server-second", false) + require.Error(t, wrapped) + + msg := wrapped.Error() + // The original error stays first so existing output remains greppable. + assert.True(t, strings.HasPrefix(msg, certErr.Error()+"."), msg) + assert.Contains(t, msg, `not valid for host "pmm-server-second"`) + assert.Contains(t, msg, InsecureTLSFlag) + // The wrapped error must stay inspectable. + require.ErrorIs(t, wrapped, certErr) + assert.True(t, IsTLSCertificateError(wrapped)) + }) + + t.Run("no hint once validation is disabled", func(t *testing.T) { + t.Parallel() + + // Suggesting the flag the user already passed would be nonsense. + assert.Equal(t, certErr, WrapTLSError(certErr, "pmm-server-second", true)) + }) + + t.Run("no hint for unrelated errors", func(t *testing.T) { + t.Parallel() + + other := errors.New("connection refused") + assert.Equal(t, other, WrapTLSError(other, "pmm-server-second", false)) + assert.NoError(t, WrapTLSError(nil, "pmm-server-second", false)) + }) + + t.Run("without a host", func(t *testing.T) { + t.Parallel() + + msg := WrapTLSError(certErr, "", false).Error() + assert.Contains(t, msg, "not valid for the requested host") + assert.Contains(t, msg, InsecureTLSFlag) + }) +} + +func TestAuthHint(t *testing.T) { + t.Parallel() + + const ( + grpcUnauthenticated = 16 + grpcInternal = 13 + grpcPermissionDenied = 7 + grpcNotFound = 5 + ) + + for name, tc := range map[string]struct { + httpCode int + grpcCode int32 + expected string + }{ + // What PMM Server actually returns for a wrong password. + "rejected credentials": {401, grpcUnauthenticated, "Please check username and password"}, + // nginx auth_request accepts 401/403 only, so PMM Server reports internal auth + // failures with HTTP 401 too. PMM-15186: those were misreported as bad credentials. + "internal error mapped to 401": {401, grpcInternal, "Please check PMM Server logs"}, + // Older PMM Servers, and responses which never reach the API, carry no code. + "401 without a gRPC code": {401, 0, "Please check username and password"}, + // Unauthenticated is conclusive on its own, whatever the status. + "unauthenticated behind another status": {500, grpcUnauthenticated, "Please check username and password"}, + // A valid user without the required role: nginx serves this as a static 403 body + // carrying code 7. Blaming the credentials for it was wrong. + "permission denied": {403, grpcPermissionDenied, "Please check that your PMM user has sufficient permissions"}, + "403 without a gRPC code": {403, 0, "Please check that your PMM user has sufficient permissions"}, + "permission denied behind another status": {500, grpcPermissionDenied, "Please check that your PMM user has sufficient permissions"}, + "not found": {404, grpcNotFound, ""}, + "conflict": {409, 6, ""}, + "success": {200, 0, ""}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.expected, AuthHint(tc.httpCode, tc.grpcCode)) + }) + } +}