PMM-15186 Fix for invalid TLS when using change.

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
 	}
 }
 

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
+}

admin/commands/base.go

@@ -123,16 +123,30 @@ 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.
 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,
 	}
 }
 

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)