Skip to content

💥 [BREAKING] enable TLS if api key provided #2740

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.

Sign up for GitHub

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

Open
THardy98 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

💥 [BREAKING] enable TLS if api key provided #2740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7762ee
enable TLS if api key provided
THardy98 Dec 2, 2025
36fae11
remove explicit tls enabled flag, use Boolean instead of boolean instead
THardy98 Dec 3, 2025
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
18 changes: 14 additions & 4 deletions temporal-serviceclient/src/main/java/io/temporal/serviceclient/ServiceStubsOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ public String toString() {
public static class Builder<T extends Builder<T>> {
private ManagedChannel channel;
private SslContext sslContext;
private boolean enableHttps;
private Boolean enableHttps;
Copy link
Member

@cretz cretz Dec 4, 2025

Choose a reason for hiding this comment

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

I assume we don't want to support a user calling setEnableHttps(null) to restore to "default"? I don't mind, just want to confirm

Copy link
Member

@cretz cretz Dec 4, 2025

Choose a reason for hiding this comment

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

Here's a contrived situation:

  • I create service stubs options with no API key and having set nothing about TLS whatsoever, thereby making enableHttps default to false on the service stub options
  • I want to create new service stub options build from the existing options, so now enableHttps is false even though I never set it
  • I do a addApiKey on this new service stub options I have never set any TLS setting on, but it still won't default to TLS because with the current code, it's impossible to create a builder from existing options and retain default TLS behavior

I don't mind if we break and/or don't-support this scenario, just wanted to mention it

Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Dec 4, 2025

Choose a reason for hiding this comment

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

I think we should at least document that scenario since it is confusing. Arguably if enableHttps wasn't explicitly set when the service stub options was created we should preserve that information when we go back to a builder

Copy link
Contributor Author

@THardy98 THardy98 Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, maybe should revert the change:
this.enableHttps != null ? this.enableHttps : false,

and make enableHttps a Boolean in ServiceStubOptions as well

The semantics might be clearer this way tbh

Copy link
Member

@cretz cretz Dec 4, 2025

Choose a reason for hiding this comment

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

If we don't consider changing the return type of ServiceStubOptions.getEnableHttps() to Boolean instead of boolean to be too big of a compat break, I agree that is the better/clearer approach

Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Dec 5, 2025

Choose a reason for hiding this comment

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

Hm I think changing the return type to a Boolean is a bit dangerous since if we return a null and users expect it to be a bool then they could get a null pointer exception. Can we just internally keep track of it as a Boolean, but keep the return type the same?

Copy link
Member

@cretz cretz Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't figure it'd be that common, but meh. This works for me, but what should be the return value of getEnableHttps be if enableHttps is null? Would the logic be return (this.enableHttps != null && this.enableHttps) || (enableHttps == null && this.apiKeyProvided && this.sslContext == null && this.channel == null)? I also assume users may never want to differentiate whether it's explicitly set or not? Maybe a different field name/getter and getEnableHttps is that more advanced logic but the getter still exists for the wrapper?

private String target;
private Consumer<ManagedChannelBuilder<?>> channelInitializer;
private Duration healthCheckAttemptTimeout;
Expand All @@ -435,6 +435,7 @@ public static class Builder<T extends Builder<T>> {
private Collection<GrpcMetadataProvider> grpcMetadataProviders;
private Collection<ClientInterceptor> grpcClientInterceptors;
private Scope metricsScope;
private boolean apiKeyProvided;

protected Builder() {}

Expand Down Expand Up @@ -613,6 +614,7 @@ public T addGrpcMetadataProvider(GrpcMetadataProvider grpcMetadataProvider) {
* @return {@code this}
*/
public T addApiKey(AuthorizationTokenSupplier apiKey) {
this.apiKeyProvided = true;
addGrpcMetadataProvider(
new AuthorizationGrpcMetadataProvider(() -> "Bearer " + apiKey.supply()));
return self();
Expand Down Expand Up @@ -803,7 +805,7 @@ public ServiceStubsOptions build() {
this.channel,
this.target,
this.channelInitializer,
this.enableHttps,
this.enableHttps != null ? this.enableHttps : false,
this.sslContext,
this.healthCheckAttemptTimeout,
this.healthCheckTimeout,
Expand Down Expand Up @@ -837,7 +839,7 @@ public ServiceStubsOptions validateAndBuildWithDefaults() {
"Only one of the 'sslContext' or 'channel' options can be set at a time");
}

if (this.enableHttps && this.channel != null) {
if (Boolean.TRUE.equals(this.enableHttps) && this.channel != null) {
throw new IllegalStateException(
"Only one of the 'enableHttps' or 'channel' options can be set at a time");
}
Expand All @@ -851,6 +853,14 @@ public ServiceStubsOptions validateAndBuildWithDefaults() {
Collection<ClientInterceptor> grpcClientInterceptors =
MoreObjects.firstNonNull(this.grpcClientInterceptors, Collections.emptyList());

// Resolve enableHttps: explicit value, auto-enable with API key, or default false
boolean enableHttps = false;
if (this.enableHttps != null) {
enableHttps = this.enableHttps;
} else if (this.apiKeyProvided && this.sslContext == null && this.channel == null) {
enableHttps = true;
}

Scope metricsScope = this.metricsScope != null ? this.metricsScope : new NoopScope();
Duration healthCheckAttemptTimeout =
this.healthCheckAttemptTimeout != null
Expand All @@ -865,7 +875,7 @@ public ServiceStubsOptions validateAndBuildWithDefaults() {
this.channel,
target,
this.channelInitializer,
this.enableHttps,
enableHttps,
this.sslContext,
healthCheckAttemptTimeout,
healthCheckTimeout,
Expand Down
100 changes: 100 additions & 0 deletions temporal-serviceclient/src/test/java/io/temporal/serviceclient/ServiceStubsOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package io.temporal.serviceclient;

import static org.junit.Assert.*;
import static org.mockito.Mockito.mock;

import io.grpc.ManagedChannel;
import io.grpc.netty.shaded.io.netty.handler.ssl.SslContext;
import org.junit.Test;

public class ServiceStubsOptionsTest {

@Test
public void testTLSEnabledByDefaultWhenAPIKeyProvided() {
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.addApiKey(() -> "test-api-key")
.validateAndBuildWithDefaults();

assertTrue(options.getEnableHttps());
}

@Test
public void testExplicitTLSDisableBeforeAPIKeyStillDisables() {
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.setEnableHttps(false)
.addApiKey(() -> "test-api-key")
.validateAndBuildWithDefaults();

// Explicit TLS=false should take precedence regardless of order
assertFalse(options.getEnableHttps());
}

@Test
public void testExplicitTLSDisableAfterAPIKeyStillDisables() {
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.addApiKey(() -> "test-api-key")
.setEnableHttps(false)
.validateAndBuildWithDefaults();

// Explicit TLS=false should take precedence regardless of order
assertFalse(options.getEnableHttps());
}

@Test
public void testTLSDisabledByDefaultWithoutAPIKey() {
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.validateAndBuildWithDefaults();

assertFalse(options.getEnableHttps());
}

@Test
public void testExplicitTLSEnableWithoutAPIKey() {
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.setEnableHttps(true)
.validateAndBuildWithDefaults();

assertTrue(options.getEnableHttps());
}

@Test
public void testTLSNotAutoEnabledWhenSslContextProvided() {
// When user provides custom sslContext, they're handling TLS themselves
// so enableHttps should not be auto-enabled
SslContext sslContext = mock(SslContext.class);
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setTarget("localhost:7233")
.addApiKey(() -> "test-api-key")
.setSslContext(sslContext)
.validateAndBuildWithDefaults();

// enableHttps stays false because sslContext handles TLS
assertFalse(options.getEnableHttps());
assertNotNull(options.getSslContext());
}

@Test
public void testTLSNotAutoEnabledWhenCustomChannelProvided() {
// When user provides custom channel, they're managing connection themselves
// so enableHttps should not be auto-enabled
ManagedChannel channel = mock(ManagedChannel.class);
ServiceStubsOptions options =
WorkflowServiceStubsOptions.newBuilder()
.setChannel(channel)
.addApiKey(() -> "test-api-key")
.validateAndBuildWithDefaults();

assertFalse(options.getEnableHttps());
}
}
Loading