-
Notifications
You must be signed in to change notification settings - Fork 272
💥 [BREAKING] enable TLS if api key specified #2126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7672721
905b36e
f8bb569
2a0a678
350769f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| // 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 | ||
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 } | ||
|
|
||
|
|
||
| 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") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the code here, this is not true, TLS will still be enable if the
TLSfield is set. IMO you should either 1) disallow both this being true and theTLSfield being set (my preference), or 2) document that this being true means anything in theTLSfield is ignored, and then respect that in code.There was a problem hiding this comment.
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