diff --git a/cliext/client.go b/cliext/client.go index 48eaa7508..e723c1d73 100644 --- a/cliext/client.go +++ b/cliext/client.go @@ -2,7 +2,9 @@ package cliext import ( "context" + "errors" "fmt" + "io/fs" "log/slog" "net/http" "strings" @@ -34,6 +36,19 @@ type ClientOptionsBuilder struct { // configured. Callers can use it to decode payloads outside the gRPC // interceptor chain (e.g. payloads nested inside opaque proto bytes). PayloadCodec converter.PayloadCodec + + // ResolvedAddress is populated by Build with the final server address, + // after profile resolution and namespace templating. + ResolvedAddress string + // ResolvedAddressSource is populated by Build with where the final address + // came from: "flag", "profile", or "default". + ResolvedAddressSource string + // ResolvedProfileName is populated by Build with the config profile name + // used, if any. + ResolvedProfileName string + // HasAPIKey is populated by Build and reports whether API-key credentials + // are configured (via flag, profile, or OAuth). + HasAPIKey bool } type oauthCredentials struct { @@ -108,8 +123,12 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error // override the profile version unless it was _explicitly_ set. if cfg.FlagSet != nil && cfg.FlagSet.Changed("address") { profile.Address = cfg.Address + b.ResolvedAddressSource = "flag" } else if profile.Address == "" { profile.Address = cfg.Address + b.ResolvedAddressSource = "default" + } else { + b.ResolvedAddressSource = "profile" } var namespaceExplicitlySet bool if cfg.FlagSet != nil && cfg.FlagSet.Changed("namespace") { @@ -126,6 +145,7 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error if addressHasNamespaceTemplate { profile.Address = strings.ReplaceAll(profile.Address, addressNamespaceTemplate, profile.Namespace) } + b.ResolvedAddress = profile.Address // Set API key on profile if provided if cfg.ApiKey != "" { @@ -204,9 +224,18 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profile.Codec.Auth = cfg.CodecAuth } + b.ResolvedProfileName = common.Profile + b.HasAPIKey = profile.APIKey != "" + // Convert profile to client options. clientOpts, err := profile.ToClientOptions(envconfig.ToClientOptionsRequest{}) if err != nil { + // File-path errors (e.g. an unreadable TLS cert) are common enough to + // deserve naming the offending file. + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + return client.Options{}, fmt.Errorf("failed to build client options: cannot read file %q: %w", pathErr.Path, err) + } return client.Options{}, fmt.Errorf("failed to build client options: %w", err) } @@ -249,6 +278,7 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profileName: result.ProfileName, } clientOpts.Credentials = client.NewAPIKeyDynamicCredentials(creds.getToken) + b.HasAPIKey = true } } diff --git a/internal/temporalcli/client.go b/internal/temporalcli/client.go index 230ff5a35..81dfe97c7 100644 --- a/internal/temporalcli/client.go +++ b/internal/temporalcli/client.go @@ -2,7 +2,9 @@ package temporalcli import ( "context" + "errors" "fmt" + "io/fs" "os" "os/user" @@ -56,6 +58,17 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. } clientOpts, err := builder.Build(cctx) if err != nil { + // An unreadable TLS/credential file is a connection-setup problem + // worth a suggestion, even though no dial happened. + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + diag := &connectDiagnosis{ + Address: builder.ResolvedAddress, + Cause: causeCertFileUnreadable, + Detail: pathErr.Path, + } + return nil, nil, newConnectError(diag, connectMetaFromBuilder(cctx, builder), err) + } return nil, nil, err } @@ -87,7 +100,7 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. cl, err := client.DialContext(dialCtx, clientOpts) if err != nil { - return nil, nil, err + return nil, nil, dialConnectError(cctx, dialCtx, builder, clientOpts, err) } // Since this namespace value is used by many commands after this call, @@ -97,6 +110,54 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. return cl, builder.PayloadCodec, nil } +// dialConnectError enriches a client.DialContext failure with a staged +// connection diagnosis and a suggested fix. See connectdiag.go. +func dialConnectError( + cctx *CommandContext, + dialCtx context.Context, + builder *cliext.ClientOptionsBuilder, + clientOpts client.Options, + origErr error, +) error { + // If a CLI-side timeout fired, surface its descriptive cause, which is + // otherwise lost ("context deadline exceeded" wins over "command timed + // out after 5s"). + if dialCtx.Err() != nil { + if cause := context.Cause(dialCtx); cause != nil && !errors.Is(cause, dialCtx.Err()) { + origErr = fmt.Errorf("%w (%v)", origErr, cause) + } + } + // Never probe after an interrupt or when the whole command timed out. + if cctx.Err() != nil { + return origErr + } + if v, _ := cctx.Options.EnvLookup.LookupEnv("TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS"); v != "" { + return fmt.Errorf("failed connecting to Temporal server at %v: %w", clientOpts.HostPort, origErr) + } + // The probe has its own internal budget, but never let it exceed the + // user's explicit connect timeout. + probeCtx := context.Context(cctx) + if timeout := cctx.RootCommand.CommonOptions.ClientConnectTimeout.Duration(); timeout > 0 { + var cancel context.CancelFunc + probeCtx, cancel = context.WithTimeout(cctx, timeout) + defer cancel() + } + diag := diagnoseConnection(probeCtx, clientOpts.HostPort, clientOpts.ConnectionOptions.TLS, origErr) + meta := connectMetaFromBuilder(cctx, builder) + meta.TLSConfigured = clientOpts.ConnectionOptions.TLS != nil + return newConnectError(diag, meta, origErr) +} + +func connectMetaFromBuilder(cctx *CommandContext, builder *cliext.ClientOptionsBuilder) connectMeta { + return connectMeta{ + Args: cctx.Options.Args, + Address: builder.ResolvedAddress, + AddressSource: builder.ResolvedAddressSource, + ProfileName: builder.ResolvedProfileName, + HasAPIKey: builder.HasAPIKey, + } +} + func fixedHeaderOverrideInterceptor( ctx context.Context, method string, req, reply any, diff --git a/internal/temporalcli/client_test.go b/internal/temporalcli/client_test.go index 2aa7ae391..aa37fa370 100644 --- a/internal/temporalcli/client_test.go +++ b/internal/temporalcli/client_test.go @@ -1,7 +1,17 @@ package temporalcli_test import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" "net" + "os" "testing" "time" @@ -9,11 +19,13 @@ import ( "github.com/stretchr/testify/require" ) -func TestClientConnectTimeout(t *testing.T) { - // Start a listener that accepts connections but never responds +// startBlackHoleListener starts a listener that accepts connections but never +// responds. +func startBlackHoleListener(t *testing.T) net.Listener { + t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - defer ln.Close() + t.Cleanup(func() { ln.Close() }) go func() { for { conn, err := ln.Accept() @@ -24,6 +36,62 @@ func TestClientConnectTimeout(t *testing.T) { select {} } }() + return ln +} + +// startMTLSListener starts a TLS listener that requires client certificates, +// returning its address and the server CA cert as PEM. +func startMTLSListener(t *testing.T) (addr, caPEM string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "client-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + leaf, err := x509.ParseCertificate(der) + require.NoError(t, err) + pool := x509.NewCertPool() + pool.AddCert(leaf) + + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{ + Certificates: []tls.Certificate{{Certificate: [][]byte{der}, PrivateKey: key}}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: pool, + }) + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func() { + if tlsConn, ok := conn.(*tls.Conn); ok { + _ = tlsConn.HandshakeContext(context.Background()) + buf := make([]byte, 1) + _ = tlsConn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, _ = tlsConn.Read(buf) + } + conn.Close() + }() + } + }() + return ln.Addr().String(), string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})) +} + +func TestClientConnectTimeout(t *testing.T) { + ln := startBlackHoleListener(t) h := NewCommandHarness(t) @@ -36,6 +104,163 @@ func TestClientConnectTimeout(t *testing.T) { elapsed := time.Since(start) require.Error(t, res.Err) + assert.Contains(t, res.Err.Error(), "failed connecting to Temporal server at") assert.Contains(t, res.Err.Error(), "deadline exceeded") - assert.Less(t, elapsed, time.Second, "dial should have timed out quickly") + // The friendly cause attached to the connect timeout must survive. + assert.Contains(t, res.Err.Error(), "command timed out after 50ms") + assert.Less(t, elapsed, 2*time.Second, "dial and diagnosis should respect the connect timeout") +} + +func TestConnectDiagnosis_MTLSRequired(t *testing.T) { + addr, caPEM := startMTLSListener(t) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--tls", + "--tls-ca-data", caPEM, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "Connecting to "+addr) + assert.Contains(t, msg, "✓ TCP connection established") + assert.Contains(t, msg, "✗ TLS handshake failed: server requires mTLS") + assert.Contains(t, msg, "--tls-cert-path YourCert.pem") + assert.Contains(t, msg, "--tls-key-path YourKey.pem") +} + +func TestConnectDiagnosis_Refused(t *testing.T) { + // Grab a port and close it so nothing is listening. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "connection refused") + assert.Contains(t, msg, "✗ TCP connection refused") + assert.Contains(t, msg, "Nothing is listening at "+addr) +} + +func TestConnectDiagnosis_DNSFailure(t *testing.T) { + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + // .invalid is reserved (RFC 2606) and can never resolve. + "--address", "does-not-exist.invalid:7233", + "--client-connect-timeout", "5s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "could not resolve host") + assert.Contains(t, msg, "✗ DNS lookup") + assert.Contains(t, msg, `Could not resolve "does-not-exist.invalid"`) +} + +func TestConnectDiagnosis_JSONOutputHasNoANSI(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + "-o", "json", + ) + + require.Error(t, res.Err) + assert.NotContains(t, res.Err.Error(), "\x1b[", "diagnosis must not contain ANSI escapes in JSON mode") + assert.Empty(t, res.Stdout.String(), "connection failures must not write to stdout") +} + +func TestConnectDiagnosis_Disabled(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + h.Options.EnvLookup = EnvLookupMap{"TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS": "1"} + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at "+addr) + assert.NotContains(t, msg, "✗", "diagnosis must be suppressed when disabled") +} + +func TestConnectDiagnosis_CertFileMissing(t *testing.T) { + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", "127.0.0.1:7233", + "--tls-cert-path", "/definitely/does/not/exist.pem", + "--tls-key-path", "/definitely/does/not/exist.key", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "cannot read file") + assert.Contains(t, msg, "/definitely/does/not/exist") + assert.NotContains(t, msg, "✓", "no probe stages expected when the dial never happened") +} + +func TestConnectDiagnosis_ProfileAddressNamed(t *testing.T) { + // When the failing address comes from a config profile, the suggestion + // should say so and point at `temporal config get`. + f, err := os.CreateTemp("", "") + require.NoError(t, err) + t.Cleanup(func() { os.Remove(f.Name()) }) + _, err = f.WriteString(` +[profile.default] +address = "does-not-exist.invalid:7233" +`) + require.NoError(t, err) + require.NoError(t, f.Close()) + + h := NewCommandHarness(t) + h.Options.EnvLookup = EnvLookupMap{"TEMPORAL_CONFIG_FILE": f.Name()} + res := h.Execute( + "workflow", "list", + "--client-connect-timeout", "5s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at does-not-exist.invalid:7233") + assert.Contains(t, msg, `The address comes from config profile "default"`) + assert.Contains(t, msg, "temporal config get --prop address") +} + +func TestConnectDiagnosis_CommandTimeoutCauseSurvives(t *testing.T) { + ln := startBlackHoleListener(t) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", ln.Addr().String(), + "--command-timeout", "1s", + ) + + require.Error(t, res.Err) + assert.Contains(t, res.Err.Error(), "command timed out after 1s") } diff --git a/internal/temporalcli/commands.go b/internal/temporalcli/commands.go index ecb52515d..df5fcdb19 100644 --- a/internal/temporalcli/commands.go +++ b/internal/temporalcli/commands.go @@ -140,10 +140,15 @@ func (c *CommandContext) preprocessOptions() error { // Setup default fail callback if c.Options.Fail == nil { c.Options.Fail = func(err error) { - // If context is closed, say that the program was interrupted and ignore - // the actual error + // If context is closed, say that the program was interrupted and + // ignore the actual error — unless the context carries a + // descriptive cause such as a --command-timeout expiry. if c.Err() != nil { - err = fmt.Errorf("program interrupted") + if cause := context.Cause(c); cause != nil && !errors.Is(cause, context.Canceled) { + err = cause + } else { + err = fmt.Errorf("program interrupted") + } } fmt.Fprintf(c.Options.Stderr, "Error: %v\n", err) os.Exit(1) diff --git a/internal/temporalcli/connectdiag.go b/internal/temporalcli/connectdiag.go new file mode 100644 index 000000000..e6eda32a7 --- /dev/null +++ b/internal/temporalcli/connectdiag.go @@ -0,0 +1,268 @@ +package temporalcli + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net" + "strings" + "syscall" + "time" + + "go.temporal.io/api/serviceerror" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// connectCause classifies why a connection to the server failed. It drives +// the suggested fix shown alongside the staged diagnosis. +type connectCause int + +const ( + causeUnknown connectCause = iota + causeDNS + causeTCPRefused + causeTCPTimeout + // causeServerPlaintext means TLS was configured but the server responded + // with a non-TLS handshake. + causeServerPlaintext + // causeServerSpeaksTLS means TLS was not configured but the server + // completed a TLS handshake when offered one. + causeServerSpeaksTLS + causeClientCertRequired + causeCAVerify + causeHostnameMismatch + // causeCertFileUnreadable is set by the caller when building client + // options fails on a file read; the probe never runs for it. + causeCertFileUnreadable + causeAuth + causeTimeout +) + +type diagStatus int + +const ( + diagOK diagStatus = iota + diagFail +) + +type diagStage struct { + Status diagStatus + Label string +} + +// connectDiagnosis is the result of probing a failed connection. +type connectDiagnosis struct { + Address string + Stages []diagStage + Cause connectCause + // Detail carries cause-specific info (e.g. an unreadable file path or the + // raw TLS alert text) for use in stage labels and suggestions. + Detail string +} + +const ( + connectDiagnosisBudget = 3 * time.Second + connectDiagnosisReadProbe = 500 * time.Millisecond +) + +// diagnoseConnection probes address in stages (DNS, TCP, TLS) to pinpoint why +// a dial failed, and classifies origErr for anything past the transport. It +// must only be called on an already-failed dial: it makes fresh network +// connections (including, when TLS is not configured, one anonymous TLS +// handshake to detect a TLS-only server, which may appear in server logs). +func diagnoseConnection(ctx context.Context, address string, tlsCfg *tls.Config, origErr error) *connectDiagnosis { + d := &connectDiagnosis{Address: address, Cause: causeUnknown} + ctx, cancel := context.WithTimeout(ctx, connectDiagnosisBudget) + defer cancel() + + host, _, err := net.SplitHostPort(address) + if err != nil { + // Not a host:port we can probe; fall back to classifying the original + // error only. + d.Cause, d.Detail = classifyGRPCError(origErr) + return d + } + + // Stage: DNS (skipped for IP literals) + if net.ParseIP(host) == nil { + addrs, err := net.DefaultResolver.LookupHost(ctx, host) + if err != nil { + d.fail(fmt.Sprintf("DNS lookup for %q failed: %v", host, dnsErrShort(err))) + d.Cause = causeDNS + return d + } + plural := "es" + if len(addrs) == 1 { + plural = "" + } + d.ok(fmt.Sprintf("DNS resolved (%d address%s)", len(addrs), plural)) + } + + // Stage: TCP + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", address) + if err != nil { + if errors.Is(err, syscall.ECONNREFUSED) { + d.fail("TCP connection refused: nothing is listening at " + address) + d.Cause = causeTCPRefused + } else if isTimeout(err) { + d.fail("TCP connection timed out") + d.Cause = causeTCPTimeout + } else { + d.fail(fmt.Sprintf("TCP connection failed: %v", err)) + d.Cause = causeTCPTimeout + } + return d + } + defer conn.Close() + d.ok("TCP connection established") + + if tlsCfg != nil { + d.probeTLS(ctx, conn, host, tlsCfg) + if d.Cause != causeUnknown { + return d + } + } else if cause, detail := probeServerSpeaksTLS(ctx, conn, host); cause != causeUnknown { + d.fail("server expects TLS, but the CLI is connecting without it" + detail) + d.Cause = cause + return d + } + + // Stage: gRPC — no re-dial; classify the original error. + d.Cause, d.Detail = classifyGRPCError(origErr) + d.fail("gRPC connection failed: " + shortErr(origErr)) + return d +} + +// probeTLS performs a TLS handshake over conn using the client's own config +// and classifies the failure, if any. On handshake success it briefly reads to +// catch post-handshake rejections: TLS 1.3 servers requiring client +// certificates complete the handshake first and only then send an alert. +func (d *connectDiagnosis) probeTLS(ctx context.Context, conn net.Conn, host string, tlsCfg *tls.Config) { + cfg := tlsCfg.Clone() + if cfg.ServerName == "" && !cfg.InsecureSkipVerify { + cfg.ServerName = host + } + tlsConn := tls.Client(conn, cfg) + if err := tlsConn.HandshakeContext(ctx); err != nil { + d.classifyTLSError(err) + return + } + // Handshake OK; a short read distinguishes an mTLS rejection alert from a + // genuinely healthy connection (where the read just times out). + _ = tlsConn.SetReadDeadline(time.Now().Add(connectDiagnosisReadProbe)) + buf := make([]byte, 1) + _, err := tlsConn.Read(buf) + if err != nil && !isTimeout(err) { + if cause := classifyTLSAlert(err); cause != causeUnknown { + d.classifyTLSError(err) + return + } + } + d.ok("TLS handshake succeeded") +} + +func (d *connectDiagnosis) classifyTLSError(err error) { + var recordErr tls.RecordHeaderError + var certErr *tls.CertificateVerificationError + var unknownAuthErr x509.UnknownAuthorityError + var hostnameErr x509.HostnameError + switch { + case errors.As(err, &recordErr): + d.fail("TLS handshake failed: server did not respond with TLS (it may be a plaintext gRPC endpoint)") + d.Cause = causeServerPlaintext + case errors.As(err, &hostnameErr): + d.fail("TLS handshake failed: server certificate is not valid for this host: " + shortErr(err)) + d.Cause = causeHostnameMismatch + case errors.As(err, &unknownAuthErr), errors.As(err, &certErr): + d.fail("TLS handshake failed: cannot verify server certificate: " + shortErr(err)) + d.Cause = causeCAVerify + case classifyTLSAlert(err) == causeClientCertRequired: + d.fail("TLS handshake failed: server requires mTLS, no valid client certificate was provided") + d.Cause = causeClientCertRequired + default: + d.fail("TLS handshake failed: " + shortErr(err)) + d.Cause = causeUnknown + d.Detail = err.Error() + } +} + +// classifyTLSAlert detects remote TLS alerts that indicate the server wants a +// (different) client certificate. Go does not export alert types for remote +// errors, so this matches the alert descriptions crypto/tls emits: alert 116 +// "certificate required" (TLS 1.3), alert 42 "bad certificate", and alert 40 +// "handshake failure" (how TLS 1.2 servers commonly reject missing client +// certs). Matching is scoped to TLS probe errors only; if these strings drift +// in a future Go release, unit tests pin them and the diagnosis degrades to +// showing the raw error without a suggestion. +func classifyTLSAlert(err error) connectCause { + msg := err.Error() + if strings.Contains(msg, "certificate required") || + strings.Contains(msg, "bad certificate") || + strings.Contains(msg, "handshake failure") { + return causeClientCertRequired + } + return causeUnknown +} + +// probeServerSpeaksTLS opportunistically offers a TLS handshake to a server +// the CLI is configured to reach in plaintext. If the server negotiates TLS +// (or rejects us at the certificate step, which still means it spoke TLS), the +// mismatch is the likely root cause. +func probeServerSpeaksTLS(ctx context.Context, conn net.Conn, host string) (connectCause, string) { + tlsConn := tls.Client(conn, &tls.Config{InsecureSkipVerify: true, ServerName: host}) + err := tlsConn.HandshakeContext(ctx) + if err == nil { + return causeServerSpeaksTLS, "" + } + if classifyTLSAlert(err) == causeClientCertRequired { + return causeServerSpeaksTLS, " (and it requires client certificates)" + } + return causeUnknown, "" +} + +func classifyGRPCError(err error) (connectCause, string) { + var deadlineErr *serviceerror.DeadlineExceeded + if errors.As(err, &deadlineErr) || errors.Is(err, context.DeadlineExceeded) { + return causeTimeout, "" + } + if unwrapped := errors.Unwrap(err); unwrapped != nil { + if st, ok := status.FromError(unwrapped); ok { + switch st.Code() { + case codes.Unauthenticated, codes.PermissionDenied: + return causeAuth, st.Message() + case codes.DeadlineExceeded: + return causeTimeout, "" + } + } + } + return causeUnknown, "" +} + +func (d *connectDiagnosis) ok(label string) { d.Stages = append(d.Stages, diagStage{diagOK, label}) } +func (d *connectDiagnosis) fail(label string) { + d.Stages = append(d.Stages, diagStage{diagFail, label}) +} + +func isTimeout(err error) bool { + var netErr net.Error + return errors.Is(err, context.DeadlineExceeded) || + (errors.As(err, &netErr) && netErr.Timeout()) +} + +// shortErr renders an error compactly for a stage line, trimming the SDK's +// "failed reaching server:" prefix that would be redundant inside a +// connection diagnosis. +func shortErr(err error) string { + return strings.TrimPrefix(err.Error(), "failed reaching server: ") +} + +func dnsErrShort(err error) string { + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) && dnsErr.IsNotFound { + return "no such host" + } + return err.Error() +} diff --git a/internal/temporalcli/connectdiag_test.go b/internal/temporalcli/connectdiag_test.go new file mode 100644 index 000000000..12e5d3414 --- /dev/null +++ b/internal/temporalcli/connectdiag_test.go @@ -0,0 +1,383 @@ +package temporalcli + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "fmt" + "math/big" + "net" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.temporal.io/api/serviceerror" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// newTestCert generates a self-signed server certificate for 127.0.0.1. +func newTestCert(t *testing.T) tls.Certificate { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "connectdiag-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + DNSNames: []string{"localhost"}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key, Leaf: mustParseCert(t, der)} +} + +func mustParseCert(t *testing.T, der []byte) *x509.Certificate { + t.Helper() + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + return cert +} + +func certPool(t *testing.T, cert tls.Certificate) *x509.CertPool { + t.Helper() + pool := x509.NewCertPool() + pool.AddCert(cert.Leaf) + return pool +} + +// serveTLS accepts connections and completes TLS handshakes until the +// listener closes. Returned address is host:port. +func serveTLS(t *testing.T, cfg *tls.Config) string { + t.Helper() + ln, err := tls.Listen("tcp", "127.0.0.1:0", cfg) + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func() { + // Drive the handshake; keep the connection open briefly so + // post-handshake alerts (TLS 1.3 client-cert enforcement) + // reach the client, then close. + if tlsConn, ok := conn.(*tls.Conn); ok { + _ = tlsConn.HandshakeContext(context.Background()) + buf := make([]byte, 1) + _ = tlsConn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, _ = tlsConn.Read(buf) + } + conn.Close() + }() + } + }() + return ln.Addr().String() +} + +func testDiagnose(t *testing.T, address string, tlsCfg *tls.Config) *connectDiagnosis { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // Mirror the SDK's eager GetSystemInfo failure shape. + origErr := fmt.Errorf("failed reaching server: %w", serviceerror.NewDeadlineExceeded("context deadline exceeded")) + return diagnoseConnection(ctx, address, tlsCfg, origErr) +} + +func TestDiagnoseConnection_ClientCertRequired(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{ + Certificates: []tls.Certificate{cert}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: certPool(t, cert), + }) + + // Client trusts the server CA but has no client cert. + d := testDiagnose(t, addr, &tls.Config{RootCAs: certPool(t, cert)}) + // This test also pins the Go crypto/tls alert text ("certificate + // required" / "bad certificate" / "handshake failure") that + // classifyTLSAlert depends on; if it fails after a Go upgrade, revisit + // classifyTLSAlert. + assert.Equal(t, causeClientCertRequired, d.Cause) + requireStage(t, d, diagOK, "TCP connection established") + requireStage(t, d, diagFail, "server requires mTLS") +} + +func TestDiagnoseConnection_ServerPlaintext(t *testing.T) { + // Plain TCP listener that immediately writes a non-TLS response. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + _, _ = conn.Write([]byte("HTTP/1.1 400 Bad Request\r\n\r\n")) + conn.Close() + } + }() + + d := testDiagnose(t, ln.Addr().String(), &tls.Config{InsecureSkipVerify: true}) + assert.Equal(t, causeServerPlaintext, d.Cause) + requireStage(t, d, diagFail, "did not respond with TLS") +} + +func TestDiagnoseConnection_ServerSpeaksTLS(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // No TLS configured on the client. + d := testDiagnose(t, addr, nil) + assert.Equal(t, causeServerSpeaksTLS, d.Cause) + requireStage(t, d, diagFail, "server expects TLS") +} + +func TestDiagnoseConnection_Refused(t *testing.T) { + // Grab a port and close it so nothing is listening. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + d := testDiagnose(t, addr, nil) + assert.Equal(t, causeTCPRefused, d.Cause) + requireStage(t, d, diagFail, "TCP connection refused") +} + +func TestDiagnoseConnection_DNSFailure(t *testing.T) { + // .invalid is reserved (RFC 2606) and can never resolve. + d := testDiagnose(t, "does-not-exist.invalid:7233", nil) + assert.Equal(t, causeDNS, d.Cause) + requireStage(t, d, diagFail, "DNS lookup") +} + +func TestDiagnoseConnection_UnknownCA(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // Client does not trust the server's self-signed cert. + d := testDiagnose(t, addr, &tls.Config{}) + assert.Equal(t, causeCAVerify, d.Cause) + requireStage(t, d, diagFail, "cannot verify server certificate") +} + +func TestDiagnoseConnection_HealthyTLSFallsThroughToGRPC(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // TLS is fine; the original (gRPC-level) error should be classified. + d := testDiagnose(t, addr, &tls.Config{RootCAs: certPool(t, cert)}) + assert.Equal(t, causeTimeout, d.Cause) + requireStage(t, d, diagOK, "TLS handshake succeeded") + requireStage(t, d, diagFail, "gRPC connection failed") +} + +func requireStage(t *testing.T, d *connectDiagnosis, status diagStatus, labelSubstr string) { + t.Helper() + for _, stage := range d.Stages { + if stage.Status == status && strings.Contains(stage.Label, labelSubstr) { + return + } + } + t.Fatalf("no stage with status %v containing %q in %+v", status, labelSubstr, d.Stages) +} + +func TestClassifyGRPCError(t *testing.T) { + tests := []struct { + name string + err error + cause connectCause + detail string + }{ + { + name: "serviceerror deadline exceeded", + err: fmt.Errorf("failed reaching server: %w", serviceerror.NewDeadlineExceeded("context deadline exceeded")), + cause: causeTimeout, + }, + { + name: "plain context deadline", + err: fmt.Errorf("failed reaching server: %w", context.DeadlineExceeded), + cause: causeTimeout, + }, + { + name: "unauthenticated status", + err: fmt.Errorf("failed reaching server: %w", status.Error(codes.Unauthenticated, "bad credentials")), + cause: causeAuth, + detail: "bad credentials", + }, + { + name: "permission denied status", + err: fmt.Errorf("failed reaching server: %w", status.Error(codes.PermissionDenied, "nope")), + cause: causeAuth, + }, + { + name: "unclassified error", + err: fmt.Errorf("something else entirely"), + cause: causeUnknown, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cause, detail := classifyGRPCError(tt.err) + assert.Equal(t, tt.cause, cause) + if tt.detail != "" { + assert.Contains(t, detail, tt.detail) + } + }) + } +} + +func TestConnectSummary(t *testing.T) { + origErr := fmt.Errorf("failed reaching server: context deadline exceeded") + tests := []struct { + cause connectCause + want string + }{ + {causeDNS, "could not resolve host"}, + {causeTCPRefused, "connection refused"}, + {causeTCPTimeout, "connection timed out"}, + {causeServerPlaintext, "TLS is enabled but the server did not respond with TLS"}, + {causeServerSpeaksTLS, "the server requires TLS but the CLI is connecting without it"}, + {causeClientCertRequired, "server requires client certificate (mTLS)"}, + {causeCAVerify, "cannot verify server TLS certificate"}, + {causeHostnameMismatch, "server TLS certificate does not match host"}, + // Unclassified failures keep the original error text (minus the SDK + // prefix) so scripts matching on it keep working. + {causeUnknown, "context deadline exceeded"}, + {causeTimeout, "context deadline exceeded"}, + } + for _, tt := range tests { + got := connectSummary(&connectDiagnosis{Cause: tt.cause}, origErr) + assert.Contains(t, got, tt.want, "cause %v", tt.cause) + } +} + +func TestSuggestFix(t *testing.T) { + cloudMeta := connectMeta{ + Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, + Address: "foo.bar.tmprl.cloud:7233", + } + tests := []struct { + name string + diag *connectDiagnosis + meta connectMeta + contains []string + }{ + { + name: "mTLS on cloud endpoint", + diag: &connectDiagnosis{Cause: causeClientCertRequired}, + meta: cloudMeta, + contains: []string{"Temporal Cloud", "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem", "temporal config set --prop tls.client_cert_path"}, + }, + { + name: "mTLS on generic endpoint", + diag: &connectDiagnosis{Cause: causeClientCertRequired}, + meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"requires client certificates", "--tls-cert-path YourCert.pem"}, + }, + { + name: "refused on local default port", + diag: &connectDiagnosis{Cause: causeTCPRefused}, + meta: connectMeta{Address: "127.0.0.1:7233"}, + contains: []string{"temporal server start-dev"}, + }, + { + name: "refused elsewhere", + diag: &connectDiagnosis{Cause: causeTCPRefused}, + meta: connectMeta{Address: "myhost:9999"}, + contains: []string{"Nothing is listening at myhost:9999"}, + }, + { + name: "dns failure from profile address", + diag: &connectDiagnosis{Cause: causeDNS}, + meta: connectMeta{Address: "typo.example.com:7233", AddressSource: "profile", ProfileName: "prod"}, + contains: []string{`Could not resolve "typo.example.com"`, `profile "prod"`, "temporal config get --prop address"}, + }, + { + name: "server speaks TLS", + diag: &connectDiagnosis{Cause: causeServerSpeaksTLS}, + meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"Add --tls"}, + }, + { + name: "server plaintext", + diag: &connectDiagnosis{Cause: causeServerPlaintext}, + meta: connectMeta{Address: "myhost:7233", TLSConfigured: true}, + contains: []string{"does not appear to use TLS"}, + }, + { + name: "api key rejected", + diag: &connectDiagnosis{Cause: causeAuth}, + meta: connectMeta{Address: "us-west-2.aws.api.temporal.io:7233", HasAPIKey: true}, + contains: []string{"rejected the provided API key"}, + }, + { + name: "cert file unreadable", + diag: &connectDiagnosis{Cause: causeCertFileUnreadable, Detail: "/nope.pem"}, + meta: connectMeta{Address: "myhost:7233"}, + contains: []string{`Cannot read "/nope.pem"`}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := suggestFix(tt.diag, tt.meta) + for _, want := range tt.contains { + assert.Contains(t, got, want) + } + }) + } +} + +func TestReconstructCommand(t *testing.T) { + got := reconstructCommand( + []string{"workflow", "list", "--address", "foo:7233", "--query", "WorkflowType = 'x'"}, + "--tls-cert-path YourCert.pem", + ) + assert.Equal(t, + "temporal workflow list \\\n"+ + " --address foo:7233 \\\n"+ + " --query \"WorkflowType = 'x'\" \\\n"+ + " --tls-cert-path YourCert.pem", + got) +} + +func TestConnectErrorRendering(t *testing.T) { + origErr := fmt.Errorf("failed reaching server: context deadline exceeded") + d := &connectDiagnosis{ + Address: "foo.bar.tmprl.cloud:7233", + Cause: causeClientCertRequired, + Stages: []diagStage{ + {diagOK, "DNS resolved (3 addresses)"}, + {diagOK, "TCP connection established"}, + {diagFail, "TLS handshake failed: server requires mTLS, no valid client certificate was provided"}, + }, + } + err := newConnectError(d, connectMeta{ + Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, + Address: "foo.bar.tmprl.cloud:7233", + }, origErr) + + msg := err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at foo.bar.tmprl.cloud:7233: TLS handshake failed: server requires client certificate (mTLS)") + assert.Contains(t, msg, "Connecting to foo.bar.tmprl.cloud:7233") + assert.Contains(t, msg, "✓ DNS resolved (3 addresses)") + assert.Contains(t, msg, "✗ TLS handshake failed") + assert.Contains(t, msg, "--tls-cert-path YourCert.pem") + // Unwrap preserves the original error. + assert.ErrorContains(t, err.Unwrap(), "context deadline exceeded") +} diff --git a/internal/temporalcli/connecterror.go b/internal/temporalcli/connecterror.go new file mode 100644 index 000000000..76866cf0d --- /dev/null +++ b/internal/temporalcli/connecterror.go @@ -0,0 +1,216 @@ +package temporalcli + +import ( + "fmt" + "net" + "strings" + + "github.com/fatih/color" +) + +// connectError is returned by dialClient when connecting to the server fails. +// Error() renders the full multi-line diagnosis (summary, probe stages, and a +// suggested fix); Unwrap() preserves the original dial error for +// errors.Is/As. +type connectError struct { + rendered string + cause error +} + +func (e *connectError) Error() string { return e.rendered } +func (e *connectError) Unwrap() error { return e.cause } + +// connectMeta carries the request context the suggestion engine needs to +// propose a concrete fix (e.g. reconstructing the exact command the user +// typed with the missing flags appended). +type connectMeta struct { + // Args are the CLI args as typed (without the binary name). + Args []string + Address string + AddressSource string // "flag", "profile", or "default" + ProfileName string + HasAPIKey bool + TLSConfigured bool +} + +// newConnectError renders a connection failure into a connectError. It must +// be called while command execution is active so that color state (JSON mode, +// --color) is applied correctly; the rendering is captured eagerly. +func newConnectError(d *connectDiagnosis, meta connectMeta, origErr error) *connectError { + var b strings.Builder + b.WriteString("failed connecting to Temporal server at ") + b.WriteString(meta.Address) + b.WriteString(": ") + b.WriteString(connectSummary(d, origErr)) + if len(d.Stages) > 0 { + b.WriteString("\n\n Connecting to ") + b.WriteString(meta.Address) + for _, stage := range d.Stages { + if stage.Status == diagOK { + b.WriteString("\n " + color.GreenString("✓") + " " + stage.Label) + } else { + b.WriteString("\n " + color.RedString("✗") + " " + stage.Label) + } + } + } + if suggestion := suggestFix(d, meta); suggestion != "" { + b.WriteString("\n\n") + b.WriteString(indentLines(suggestion, " ")) + } + return &connectError{rendered: b.String(), cause: origErr} +} + +// connectSummary is the one-line cause appended to the first error line. For +// unclassified failures and timeouts it keeps the original error text so +// scripts matching on strings like "context deadline exceeded" keep working. +func connectSummary(d *connectDiagnosis, origErr error) string { + switch d.Cause { + case causeDNS: + return "could not resolve host" + case causeTCPRefused: + return "connection refused" + case causeTCPTimeout: + return "connection timed out" + case causeServerPlaintext: + return "TLS is enabled but the server did not respond with TLS" + case causeServerSpeaksTLS: + return "the server requires TLS but the CLI is connecting without it" + case causeClientCertRequired: + return "TLS handshake failed: server requires client certificate (mTLS)" + case causeCAVerify: + return "cannot verify server TLS certificate" + case causeHostnameMismatch: + return "server TLS certificate does not match host" + case causeCertFileUnreadable: + return fmt.Sprintf("cannot read file %q", d.Detail) + case causeAuth: + if d.Detail != "" { + return "authentication failed: " + d.Detail + } + return "authentication failed" + default: + return shortErr(origErr) + } +} + +// suggestFix maps a classified failure to one concrete next step. First match +// wins; an empty return means no suggestion (the stages still render). +func suggestFix(d *connectDiagnosis, meta connectMeta) string { + host, port, _ := net.SplitHostPort(meta.Address) + switch d.Cause { + case causeCertFileUnreadable: + return fmt.Sprintf("Cannot read %q — check that the path exists and is readable.", d.Detail) + case causeClientCertRequired: + var b strings.Builder + if isCloudHost(host) { + b.WriteString("This looks like a Temporal Cloud endpoint secured with mTLS. Provide client certificates:") + } else { + b.WriteString("The server requires client certificates (mTLS). Provide them:") + } + b.WriteString("\n\n") + b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem"), " ")) + b.WriteString("\n\nOr configure once:\n\n") + b.WriteString(" temporal config set --prop tls.client_cert_path YourCert.pem\n") + b.WriteString(" temporal config set --prop tls.client_key_path YourKey.pem") + if strings.HasSuffix(host, ".api.temporal.io") { + b.WriteString("\n\nIf your namespace uses API-key auth instead, pass --api-key YourApiKey.") + } + return b.String() + case causeAuth: + if meta.HasAPIKey { + return "The server rejected the provided API key. Verify the key is valid and that the address is your namespace's gRPC endpoint (for Temporal Cloud API keys, a regional endpoint like us-west-2.aws.api.temporal.io:7233)." + } + return "The server rejected the request as unauthenticated. If it requires an API key, pass --api-key; if it requires mTLS, pass --tls-cert-path and --tls-key-path." + case causeServerPlaintext: + return fmt.Sprintf("The server at %s does not appear to use TLS. Remove --tls and certificate flags, or double-check the address and port.", meta.Address) + case causeServerSpeaksTLS: + var b strings.Builder + b.WriteString("The server requires TLS. Add --tls:") + b.WriteString("\n\n") + b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls"), " ")) + if isCloudHost(host) { + b.WriteString("\n\nTemporal Cloud endpoints also need client credentials: --tls-cert-path/--tls-key-path or --api-key.") + } + return b.String() + case causeDNS: + s := fmt.Sprintf("Could not resolve %q — check the server address.", host) + if meta.AddressSource == "profile" { + profile := meta.ProfileName + if profile == "" { + profile = "default" + } + s += fmt.Sprintf("\nThe address comes from config profile %q — inspect it with:\n\n temporal config get --prop address", profile) + } + return s + case causeTCPRefused: + if isLoopbackHost(host) && port == "7233" { + return fmt.Sprintf("No Temporal server is running at %s. Start a local dev server with:\n\n temporal server start-dev", meta.Address) + } + return fmt.Sprintf("Nothing is listening at %s — verify the address and that the server is running.", meta.Address) + case causeCAVerify: + return "The server's TLS certificate is not trusted by your system roots. If the server uses a private CA, pass --tls-ca-path YourServerCA.pem." + case causeHostnameMismatch: + return fmt.Sprintf("The server's TLS certificate is not valid for %q. If you connect via an IP or alternate name, set --tls-server-name to the name in the certificate.", host) + case causeTCPTimeout, causeTimeout: + return fmt.Sprintf("The connection stalled — a firewall or proxy may be blocking traffic to %s. Verify the address, port, and network path. Bound the wait with --client-connect-timeout.", meta.Address) + } + return "" +} + +func isCloudHost(host string) bool { + return strings.HasSuffix(host, ".tmprl.cloud") || strings.HasSuffix(host, ".api.temporal.io") +} + +func isLoopbackHost(host string) bool { + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +// reconstructCommand re-renders the command the user typed, one option per +// line (matching the style used in help text), with extraFlags appended. +func reconstructCommand(args []string, extraFlags ...string) string { + // Split leading command words from options. + var words, opts []string + for i, arg := range args { + if strings.HasPrefix(arg, "-") { + opts = args[i:] + break + } + words = append(words, arg) + } + var lines []string + lines = append(lines, "temporal "+strings.Join(words, " ")) + for i := 0; i < len(opts); i++ { + line := opts[i] + // Attach the option's value, if the next arg isn't another option. + if !strings.Contains(line, "=") && i+1 < len(opts) && !strings.HasPrefix(opts[i+1], "-") { + i++ + line += " " + quoteArg(opts[i]) + } + lines = append(lines, " "+line) + } + for _, flag := range extraFlags { + lines = append(lines, " "+flag) + } + return strings.Join(lines, " \\\n") +} + +func quoteArg(arg string) string { + if strings.ContainsAny(arg, " \t\"'") { + return fmt.Sprintf("%q", arg) + } + return arg +} + +func indentLines(s, indent string) string { + lines := strings.Split(s, "\n") + for i, line := range lines { + if line != "" { + lines[i] = indent + line + } + } + return strings.Join(lines, "\n") +}