Add OAuth support for CLI extensions #892

stephanos · 2025-12-18T04:46:37Z

What was changed

Redo of #886.

Adding:

ability to code-gen just options-sets
ability to reference option-sets across codegen YAML input files
shared CommonOptions and ClientOptions generated for cliext
ClientOptionsBuilder for building client dial options
integration of OAuth token into client dial options (plus integration tests)
StoreClientOAuthOptions and LoadClientOAuth for oauth config
NewLogger helper for creating a new logger from flags

Why?

To make OAuth a first-class authentication mechanism in the Temporal CLI and CLI extensions.

Checklist

Closes
How was this tested:

Any docs updates needed?

cretz

Looks great, comments about some detailed things, but the general approach of "external option set" and client config loading and such makes sense.

cretz · 2025-12-18T13:55:09Z

cliext/client.go

+// BuildClientOptions creates SDK client options from a ClientOptionsBuilder.
+// If OAuth is configured and no APIKey is set, OAuth will be used to obtain an access token.
+// Returns the client options and the resolved namespace (which may differ from input if loaded from profile).
+func BuildClientOptions(ctx context.Context, opts ClientOptionsBuilder) (client.Options, string, error) {


Pedantic, but If the second parameter isn't really an "options", might as well just put Build as a method on *ClientOptionsBuilder

cretz · 2025-12-18T13:55:51Z

cliext/client.go

+
+// BuildClientOptions creates SDK client options from a ClientOptionsBuilder.
+// If OAuth is configured and no APIKey is set, OAuth will be used to obtain an access token.
+// Returns the client options and the resolved namespace (which may differ from input if loaded from profile).


Isn't namespace already on the options?

cretz · 2025-12-18T13:58:54Z

cliext/client.oauth.go

+// NewOAuthDynamicTokenProvider creates a function that provides OAuth access tokens dynamically.
+// The returned function loads OAuth configuration on-demand from the config file and refreshes
+// tokens as needed. Returns empty string if no OAuth is configured.
+func NewOAuthDynamicTokenProvider(opts ClientOptionsBuilder) func(context.Context) (string, error) {


Pedantic, but would also consider putting this as a method on the builder (don't even have to have it return a func, can just make the method the func, e.g. func (c *ClientOptionsBuilder) GetOAuthToken(ctx context.Context) (string, error) {, but see comment a couple of lines down above why we'd want some before-anon-func work)

cretz · 2025-12-18T14:11:21Z

cliext/client.oauth.go

+}
+
+// token returns a valid access token, refreshing if necessary.
+func (c *oauthClient) token(ctx context.Context, config *OAuthConfig) (OAuthToken, error) {


I would have expected oauth2 package to handle expiration, refresh, etc. Is this logic basically what oauth2.ReuseTokenSourceWithExpiry does? I wonder if we even need to provide config/token structs ourselves instead of reusing *oauth2.Config and *oauth2.Token (and documenting which values of those we support for load/store). If we do still want our oauth config structs, IMO we should offer a NewTokenSource() func on *OAuthConfig. This will help with some libraries that want the oauth data types, e.g. https://pkg.go.dev/google.golang.org/grpc/credentials/oauth#TokenSource.

Good call! I must have missed ReuseTokenSourceWithExpiry.

re OAuthClientConfig and OAuthToken have the benefit of hiding the oauth2 as an implementation detail. Another benefit is that they allow attaching RequestParams which we need for the Cloud CLI (to provide the audience).

But I can also see how NewTokenSource can be useful for integration as you point out. I don't know what weighs more, the benefit of hiding oauth2 as an impl detail or the convenience of exposing it via NewTokenSource().

re OAuthClientConfig and OAuthToken have the benefit of hiding the oauth2 as an implementation detail

Why hide that implementation detail? Not sure any other Go library using OAuth would (e.g. the Go gRPC library). I see code was updated, so not easily seeing where request params is used in current PR, but would have to see how other Go OAuth clients are setting such request params. Definitely can have something like:

type OAuthConfig struct { ClientConfig *oauth2.Config Token *oauth2.Token ClientRequestParams map[string]string }

👍 okay, let's expose oauth2, I agree it is simpler and seems to really be the library everyone uses anyway.

re request params: since they are not needed for token refresh, it's just a config field as far as cliext is concerned.

But where are the request params used by cliext code?

Right; I also just realized that since AFAIK the audience request param is only required during login, we could let Cloud CLI (and other CLIs) deal with that themselves. I still had it here because originally I thought it was required for token refresh or the CLI's auto-login - but both of these aren't true (anymore). I'll remove it.

cretz · 2025-12-18T14:13:36Z

cliext/client.oauth.go

+// tokens as needed. Returns empty string if no OAuth is configured.
+func NewOAuthDynamicTokenProvider(opts ClientOptionsBuilder) func(context.Context) (string, error) {
+	return func(ctx context.Context) (string, error) {
+		result, err := LoadClientOAuth(LoadClientOAuthOptions{


IMO we should not re-lookup oauth config on each token need, it should be bound to original the loaded configuration. This means long-running CLI commands do not get logged in as maybe another user if the file changes out from under. I'd move this loading part out of to before the func.

cretz · 2025-12-18T14:15:12Z

cliext/client.go

+		clientOpts.Logger = log.NewStructuredLogger(opts.Logger)
+	}
+
+	// Set OAuth credentials if configured and no API key is set.


Not seeing where this is checking whether oauth is configured

cretz · 2025-12-18T14:32:48Z

cliext/flags.gen.go

+	ctx.BindFlagEnvVar(f.Lookup("config-file"), "TEMPORAL_CONFIG_FILE")
+	f.StringVar(&v.Profile, "profile", "", "Profile to use for config file. EXPERIMENTAL.")
+	ctx.BindFlagEnvVar(f.Lookup("profile"), "TEMPORAL_PROFILE")


I think it's clearer if the --config-file and --profile flags are not bound to env vars on the flags themselves. IMO they should default to unset and let envconfig resolve them at runtime as the true canonical place for knowing what these are based on environment (in case we want to change that some day).

If a programmatic user needs to know the defaults of these values when the flag is not otherwise specified, we can provide helpers if what we have is not good enough already.

Also, BindFlagEnvVar is a relic of the env days and since envconfig came about in #764, BindFlagEnvVar was reduced to only applying for env. But now that we are exporting some of this functionality, I would be ok just hardcoding the TEMPORAL_ENV env var mapping somewhere and removing the concept of optionContext altogether. And then, at that point, we might as well remove support for the env: in the YAML file.

👍 on --config-file and --profile
👍 moved out TEMPORAL_ENV and removed optionContext

Regarding implied-env; is that actually used anywhere?

Regarding implied-env; is that actually used anywhere?

That's the only one that's used, it's env: that's basically not used anymore except for the one TEMPORAL_ENV, so now we can probably remove env:. FWIW, env: used to be used in lots of places until envconfig came about and basically took over environment variables except for this one legacy one.

Right; I've removed env: but I can't see where ImpliedEnv is actually used. I can't see it. My AI coding agent suggested appending it to flag descriptions; I didn't commit it here but it seemed like a good idea.

stephanos · 2025-12-19T02:26:35Z

internal/temporalcli/commands.go

^ all moved to cliext/logger.go

stephanos · 2025-12-19T03:58:04Z

cliext/client.go

+	if cfg.ApiKey == "" {
+		if err := b.initOAuthTokenSource(ctx); err != nil {
+			return client.Options{}, fmt.Errorf("failed to initialize OAuth: %w", err)
+		}
+		// Only set credentials if OAuth is configured
+		if b.oauthTokenSource != nil {
+			clientOpts.Credentials = client.NewAPIKeyDynamicCredentials(b.getOAuthToken)
+		}
+	}


Section where we integrate OAuth into the code that was moved here from temporalcli/client.go

stephanos · 2025-12-19T04:45:22Z

internal/temporalcli/commands.go

 	c.Command.Version = VersionString()

+	// Bind TEMPORAL_ENV environment variable to --env flag
+	cctx.BindFlagEnvVar(c.Command.PersistentFlags().Lookup("env"), "TEMPORAL_ENV")


Now manually binding TEMPORAL_ENV here.

stephanos · 2025-12-19T04:54:57Z

internal/commandsgen/code.go

-	if o.Env != "" {
-		w.writeLinef("cctx.BindFlagEnvVar(%v.Lookup(%q), %q)", flagVar, o.Name, o.Env)
-	}


stephanos · 2025-12-19T04:55:25Z

internal/commandsgen/parse.go

+	if o.ImpliedEnv != strings.ToUpper(o.ImpliedEnv) {
 		return fmt.Errorf("env variables must be in all caps")
 	}


Changed over to ImpliedEnv.

cretz

Looking good, I think it's only detail stuff now

cretz · 2025-12-18T14:36:20Z

cliext/client.oauth.go

+	// Token is expired or about to expire, refresh it.
+	cfg := c.createOAuth2Config("")
+	tokenSource := cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: config.RefreshToken})
+	newToken, err := tokenSource.Token()


I am curious on this, do we think we should be re-persisting this refreshed token to disk? What does tcld do?

Good callout; the original PR I closed wrote the new access token to disk. It got lost in the reshuffle. I'll need to update that.

Does tcld re-persist?

Yes, it does: https://github.com/temporalio/tcld/blob/ab73fcc36aede9a87c0ba2a79b4e743a4ef6932b/app/token_config.go#L119-L123

cretz · 2025-12-19T14:04:33Z

cliext/client.oauth.go

+}
+
+// token returns a valid access token, refreshing if necessary.
+func (c *oauthClient) token(ctx context.Context, config *OAuthConfig) (OAuthToken, error) {


re OAuthClientConfig and OAuthToken have the benefit of hiding the oauth2 as an implementation detail

Why hide that implementation detail? Not sure any other Go library using OAuth would (e.g. the Go gRPC library). I see code was updated, so not easily seeing where request params is used in current PR, but would have to see how other Go OAuth clients are setting such request params. Definitely can have something like:

type OAuthConfig struct { ClientConfig *oauth2.Config Token *oauth2.Token ClientRequestParams map[string]string }

cretz · 2025-12-19T14:14:58Z

cliext/client.go

+
+	// Attempt to configure OAuth config if no API key is set.
+	if cfg.ApiKey == "" {
+		result, err := LoadClientOAuth(LoadClientOAuthOptions{


I don't think this properly accounts for if !common.DisableConfigFile || !common.DisableConfigEnv that the other envconfig loading does above.

Would be ok if we wanted to go ahead and have Go SDK's env config support "additional data" to avoid this double-load for everyone that is doing basic no-auth CLI calls (happens commonly in cases like proxies). But also ok if we want to double load while we wait for that.

👍 Absolutely; adding that check now. (just for DisableConfigFile since the OAuth cannot be configured via env vars)

cretz · 2025-12-19T14:16:58Z

cliext/flags.gen.go

+	ctx.BindFlagEnvVar(f.Lookup("config-file"), "TEMPORAL_CONFIG_FILE")
+	f.StringVar(&v.Profile, "profile", "", "Profile to use for config file. EXPERIMENTAL.")
+	ctx.BindFlagEnvVar(f.Lookup("profile"), "TEMPORAL_PROFILE")


Regarding implied-env; is that actually used anywhere?

That's the only one that's used, it's env: that's basically not used anymore except for the one TEMPORAL_ENV, so now we can probably remove env:. FWIW, env: used to be used in lots of places until envconfig came about and basically took over environment variables except for this one legacy one.

cretz · 2025-12-19T14:19:51Z

cliext/option-sets.yaml

+        description: |
+          Path to environment settings file.
+          Defaults to `$HOME/.config/temporalio/temporal.yaml`.
+      - name: config-file


Where did implied-env go for config-file and profile? implied-env basically is just documentation/usage help, it has no runtime component.

👍 it got lost when I moved it, added back

stephanos mentioned this pull request Dec 18, 2025

Add OAuth support for CLI extensions #886

Closed

stephanos force-pushed the oauth2 branch from 3190630 to d8ea803 Compare December 18, 2025 04:53

cretz reviewed Dec 18, 2025

View reviewed changes

stephanos force-pushed the oauth2 branch 3 times, most recently from 4cbfb8c to 6ede038 Compare December 19, 2025 02:23

stephanos commented Dec 19, 2025

View reviewed changes

internal/temporalcli/commands.go

Copy link

Contributor Author

stephanos Dec 19, 2025

Choose a reason for hiding this comment

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

^ all moved to cliext/logger.go

stephanos force-pushed the oauth2 branch from 6ede038 to ede149b Compare December 19, 2025 03:52

stephanos commented Dec 19, 2025

View reviewed changes

stephanos force-pushed the oauth2 branch 5 times, most recently from 2e421e9 to 138af77 Compare December 19, 2025 04:43

stephanos commented Dec 19, 2025

View reviewed changes

stephanos force-pushed the oauth2 branch 4 times, most recently from a498f5e to e1949f7 Compare December 19, 2025 05:00

Add OAuth support for CLI extensions

481e8c2

stephanos force-pushed the oauth2 branch from e1949f7 to 78e65b0 Compare December 19, 2025 05:05

stephanos marked this pull request as ready for review December 19, 2025 05:07

stephanos requested review from a team as code owners December 19, 2025 05:07

stephanos requested a review from cretz December 19, 2025 05:13

stephanos force-pushed the oauth2 branch from 78e65b0 to 7daf952 Compare December 19, 2025 05:17

polish

f85908f

stephanos force-pushed the oauth2 branch from 7daf952 to f85908f Compare December 19, 2025 05:23

cretz reviewed Dec 19, 2025

View reviewed changes

stephanos added 5 commits December 19, 2025 08:32

add missing implied-env

8fcd2f5

remove oauth wrappers

a3020bc

store refresh token

bf4b75e

respect common.DisableConfigFile

2c95b7d

move flag types to cliext

b4b0e51

stephanos force-pushed the oauth2 branch from f3565da to b4b0e51 Compare December 19, 2025 22:20

verify client options

324491d

Add OAuth support for CLI extensions #892

Are you sure you want to change the base?

Add OAuth support for CLI extensions #892

Uh oh!

Conversation

stephanos commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanos Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanos Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanos Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanos Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

stephanos commented Dec 18, 2025 •

edited

Loading

stephanos Dec 18, 2025 •

edited

Loading

stephanos Dec 19, 2025 •

edited

Loading

cretz Dec 18, 2025 •

edited

Loading

stephanos Dec 19, 2025 •

edited

Loading

stephanos Dec 19, 2025 •

edited

Loading