BREAKING: Made --api-key enable TLS by default

Author: cretz

.github/workflows/ci.yaml

@@ -56,7 +56,7 @@ jobs:
           go run ./temporalcli/internal/cmd/gen-commands
           git diff --exit-code
 
-      - name: Test cloud
+      - name: Test cloud mTLS
         if: ${{ matrix.cloudTestTarget && env.HAS_SECRETS == 'true' }}
         env:
           TEMPORAL_ADDRESS: ${{ vars.TEMPORAL_CLIENT_NAMESPACE }}.tmprl.cloud:7233
@@ -70,3 +70,20 @@ jobs:
           printf '%s\n' "$TEMPORAL_TLS_CERT_CONTENT" >> client.crt
           printf '%s\n' "$TEMPORAL_TLS_KEY_CONTENT" >> client.key
           go run ./cmd/temporal workflow list --limit 2
+
+      - name: Test cloud API key env var
+        if: ${{ matrix.cloudTestTarget && env.HAS_SECRETS == 'true' }}
+        env:
+          TEMPORAL_ADDRESS: ${{ vars.TEMPORAL_CLIENT_NAMESPACE }}.tmprl.cloud:7233
+          TEMPORAL_NAMESPACE: ${{ vars.TEMPORAL_CLIENT_NAMESPACE }}
+          TEMPORAL_API_KEY: ${{ secrets.TEMPORAL_CLIENT_CLOUD_API_KEY }}
+        shell: bash
+        run: go run ./cmd/temporal workflow list --limit 2
+
+      - name: Test cloud API key arg
+        if: ${{ matrix.cloudTestTarget && env.HAS_SECRETS == 'true' }}
+        env:
+          TEMPORAL_ADDRESS: ${{ vars.TEMPORAL_CLIENT_NAMESPACE }}.tmprl.cloud:7233
+          TEMPORAL_NAMESPACE: ${{ vars.TEMPORAL_CLIENT_NAMESPACE }}
+        shell: bash
+        run: go run ./cmd/temporal workflow list --limit 2 --api-key ${{ secrets.TEMPORAL_CLIENT_CLOUD_API_KEY }}

temporalcli/client.go

@@ -139,13 +139,10 @@ func (c *ClientOptions) dialClient(cctx *CommandContext) (client.Client, error)
 			clientProfile.TLS.DisableHostVerification = c.TlsDisableHostVerification
 		}
 	}
-	// In the past, the presence of API key CLI arg did not imply TLS like it
-	// does with envconfig. Therefore if there is a user-provided API key and
-	// TLS is not present, explicitly disable it so API key presence doesn't
-	// enable it in ToClientOptions below.
-	// TODO(cretz): Or do we want to break compatibility to have TLS defaulted
-	// for all API keys?
-	if c.ApiKey != "" && clientProfile.TLS == nil {
+
+	// If TLS is explicitly disabled, we turn it off. Otherwise it may be
+	// implicitly enabled if API key or any other TLS setting is set.
+	if cctx.CurrentCommand.Flags().Changed("tls") && !c.Tls {
 		clientProfile.TLS = &envconfig.ClientConfigTLS{Disabled: true}
 	}
 

temporalcli/commands.config_test.go

@@ -372,3 +372,24 @@ func TestConfig_Set(t *testing.T) {
 		},
 		all)
 }
+
+func (s *SharedServerSuite) TestAPIKey_DefaultsTLS() {
+	// A workflow list with an API key should fail because TLS is enabled by
+	// default when --api-key is present
+	res := s.Execute(
+		"workflow", "count",
+		"--address", s.Address(),
+		"--api-key", "does-not-matter",
+	)
+	s.ErrorContains(res.Err, "tls")
+
+	// But it should succeed with TLS explicitly disabled
+	res = s.Execute(
+		"workflow", "count",
+		"--address", s.Address(),
+		"--api-key", "does-not-matter",
+		"--tls=false",
+	)
+	s.NoError(res.Err)
+	s.Contains(res.Stdout.String(), "Total")
+}

temporalcli/commands.gen.go

@@ -40,7 +40,7 @@ func (v *ClientOptions) buildFlags(cctx *CommandContext, f *pflag.FlagSet) {
 	f.StringVarP(&v.Namespace, "namespace", "n", "default", "Temporal Service Namespace.")
 	f.StringVar(&v.ApiKey, "api-key", "", "API key for request.")
 	f.StringArrayVar(&v.GrpcMeta, "grpc-meta", nil, "HTTP headers for requests. Format as a `KEY=VALUE` pair. May be passed multiple times to set multiple headers. Can also be made available via environment variable as `TEMPORAL_GRPC_META_[name]`.")
-	f.BoolVar(&v.Tls, "tls", false, "Enable base TLS encryption. Does not have additional options like mTLS or client certs. Unlike some other options, if this is present and set explicitly to false, it can still be overridden by config file or environment variables.")
+	f.BoolVar(&v.Tls, "tls", false, "Enable base TLS encryption. Does not have additional options like mTLS or client certs. This is defaulted to true if api-key or any other TLS options are present. Use --tls=false to explicitly disable.")
 	f.StringVar(&v.TlsCertPath, "tls-cert-path", "", "Path to x509 certificate. Can't be used with --tls-cert-data.")
 	f.StringVar(&v.TlsCertData, "tls-cert-data", "", "Data for x509 certificate. Can't be used with --tls-cert-path.")
 	f.StringVar(&v.TlsKeyPath, "tls-key-path", "", "Path to x509 private key. Can't be used with --tls-key-data.")

temporalcli/commandsgen/commands.yml

@@ -4026,9 +4026,8 @@ option-sets:
         type: bool
         description: |
           Enable base TLS encryption. Does not have additional options like mTLS
-          or client certs. Unlike some other options, if this is present and set
-          explicitly to false, it can still be overridden by config file or
-          environment variables.
+          or client certs. This is defaulted to true if api-key or any other TLS
+          options are present. Use --tls=false to explicitly disable.
         implied-env: TEMPORAL_TLS
       - name: tls-cert-path
         type: string