Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -1514,6 +1514,8 @@ func HistoryFromJSON(r io.Reader, options HistoryJSONOptions) (*historypb.Histor
// Note, this uses a fixed header value for authentication. Many users that want
// to rotate this value without reconnecting should use
// [NewAPIKeyDynamicCredentials].
//
// Note, TLS is auto-enabled when API key is provided and TLS is not explicitly set/disabled.
func NewAPIKeyStaticCredentials(apiKey string) Credentials {
return internal.NewAPIKeyStaticCredentials(apiKey)
}
Expand All @@ -1528,6 +1530,8 @@ func NewAPIKeyStaticCredentials(apiKey string) Credentials {
// "Authorization" header with "Bearer " + the given function result. If the
// resulting string is non-empty, it will overwrite any "Authorization" header
// that may be on the context or from existing header provider.
//
// Note, TLS is auto-enabled when API key is provided and TLS is not explicitly set/disabled.
func NewAPIKeyDynamicCredentials(apiKeyCallback func(context.Context) (string, error)) Credentials {
return internal.NewAPIKeyDynamicCredentials(apiKeyCallback)
}
Expand Down
19 changes: 18 additions & 1 deletion internal/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,12 @@ type (
// TLS configures connection level security credentials.
TLS *tls.Config

// TLSDisabled explicitly disables TLS. When true, TLS will not be used even
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When true, TLS will not be used

Reading the code here, this is not true, TLS will still be enable if the TLS field is set. IMO you should either 1) disallow both this being true and the TLS field being set (my preference), or 2) document that this being true means anything in the TLS field is ignored, and then respect that in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I should've tested this. Added validation to make it mutually exclusive + test

// if API key credentials are provided (which would normally auto-enable TLS).
// This is not recommended for production use as it sends credentials in plaintext.
// This option is mutually exclusive with TLS - an error will be returned if both are set.
TLSDisabled bool

// Authority specifies the value to be used as the :authority pseudo-header.
// This value only used when TLS is nil.
Authority string
Expand Down Expand Up @@ -990,6 +996,11 @@ func newClient(ctx context.Context, options ClientOptions, existing *WorkflowCli
options.Logger.Info("No logger configured for temporal client. Created default one.")
}

// Validate mutually exclusive TLS options
if options.ConnectionOptions.TLS != nil && options.ConnectionOptions.TLSDisabled {
return nil, fmt.Errorf("cannot set both TLS and TLSDisabled in ConnectionOptions")
}

if options.Credentials != nil {
if err := options.Credentials.applyToOptions(&options.ConnectionOptions); err != nil {
return nil, err
Expand Down Expand Up @@ -1210,7 +1221,13 @@ func NewAPIKeyDynamicCredentials(apiKeyCallback func(context.Context) (string, e
return apiKeyCredentials(apiKeyCallback)
}

func (apiKeyCredentials) applyToOptions(*ConnectionOptions) error { return nil }
func (apiKeyCredentials) applyToOptions(opts *ConnectionOptions) error {
// Auto-enable TLS when API key is provided and TLS is not explicitly set/disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well document to users that we do this where API key is set by users (i.e. in the Godoc on the NewAPIKey* functions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a little line to the new api key methods noting the auto TLS behavior

if opts.TLS == nil && !opts.TLSDisabled {
opts.TLS = &tls.Config{}
}
return nil
}

func (a apiKeyCredentials) gRPCInterceptor() grpc.UnaryClientInterceptor { return a.gRPCIntercept }

Expand Down
76 changes: 76 additions & 0 deletions internal/credentials_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package internal

import (
"context"
"crypto/tls"
"testing"

"github.com/stretchr/testify/require"
)

func TestAPIKeyCredentials_TLSEnabledByDefaultWhenAPIKeyProvided(t *testing.T) {
creds := NewAPIKeyStaticCredentials("test-api-key")
opts := &ConnectionOptions{}

err := creds.applyToOptions(opts)
require.NoError(t, err)

// TLS should be auto-enabled when api_key is provided and tls not explicitly set
require.NotNil(t, opts.TLS)
}

func TestAPIKeyCredentials_TLSCanBeExplicitlyDisabledWithAPIKey(t *testing.T) {
// Test that TLS can be explicitly disabled using TLSDisabled field
creds := NewAPIKeyStaticCredentials("test-api-key")
opts := &ConnectionOptions{
TLSDisabled: true,
}

err := creds.applyToOptions(opts)
require.NoError(t, err)

// TLS should remain nil when TLSDisabled is true
require.Nil(t, opts.TLS)
}

func TestConnectionOptions_TLSDisabledByDefaultWithoutAPIKey(t *testing.T) {
// Test that TLS is disabled by default when no API key is provided
opts := &ConnectionOptions{}

// Without API key credentials, TLS should remain nil
require.Nil(t, opts.TLS)
}

func TestAPIKeyCredentials_ExplicitTLSConfigPreservedWithAPIKey(t *testing.T) {
// Test that explicit TLS configuration is preserved when API key is provided
tlsConfig := &tls.Config{
ServerName: "test-domain",
MinVersion: tls.VersionTLS12,
}

creds := NewAPIKeyStaticCredentials("test-api-key")
opts := &ConnectionOptions{
TLS: tlsConfig,
}

err := creds.applyToOptions(opts)
require.NoError(t, err)

// Explicit TLS config should be preserved
require.NotNil(t, opts.TLS)
require.Equal(t, "test-domain", opts.TLS.ServerName)
require.Equal(t, uint16(tls.VersionTLS12), opts.TLS.MinVersion)
}

func TestConnectionOptions_TLSAndTLSDisabledMutuallyExclusive(t *testing.T) {
// Test that setting both TLS and TLSDisabled returns an error
_, err := NewClient(context.Background(), ClientOptions{
HostPort: "localhost:7233",
ConnectionOptions: ConnectionOptions{
TLS: &tls.Config{},
TLSDisabled: true,
},
})
require.Error(t, err)
require.Contains(t, err.Error(), "cannot set both TLS and TLSDisabled")
}
6 changes: 6 additions & 0 deletions internal/grpc_dialer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,9 @@ func TestCredentialsAPIKey(t *testing.T) {
client, err := DialClient(context.Background(), ClientOptions{
HostPort: srv.addr,
Credentials: NewAPIKeyStaticCredentials("my-api-key"),
ConnectionOptions: ConnectionOptions{
TLSDisabled: true, // Test server doesn't use TLS
},
})
require.NoError(t, err)
defer client.Close()
Expand Down Expand Up @@ -484,6 +487,9 @@ func TestCredentialsAPIKey(t *testing.T) {
Credentials: NewAPIKeyDynamicCredentials(func(ctx context.Context) (string, error) {
return "my-callback-api-key", nil
}),
ConnectionOptions: ConnectionOptions{
TLSDisabled: true,
},
})
require.NoError(t, err)
defer client.Close()
Expand Down