allow web UI to be configured when using mTLS in API #138

tomcz · 2022-09-22T23:55:46Z

What changed?

Two changes:

Allow temporal web/ui to be configured from a yaml file so that the web UI does not break when the API is configured for mTLS.
Since the UI cannot be run via HTTPS, allow the UI to be bound to a different IP than the API. This can facilitate the API with mTLS being exposed to non-local clients, while keeping the UI available to localhost.

Why?

Temporalite is great, and I want to use it safely in situations where I'm experimenting with mTLS in temporal, especially while implementing mTLS in workers, without the UI being broken.

How did you test it?

Ran temporalite locally without any TLS, and with TLS certificates created by a self-signed root CA. The latter required the creation of temporalite.yaml and temporalite-ui.yaml files in a configuration directory, to allow mTLS to be set up for temporal API components, and allow the UI to make requests to the API via mTLS.

Updated the ui unit test to verify that it can still create a valid configuration when the temporalite-ui.yaml file is absent even though a config directory has been provided, and that it loads the file when it is present.

Updated the mtls test to verify proper integration between the UI and the API when mtls is enabled.

temporalite.yaml

global:
  tls:
    internode:
      server:
        certFile: dist/local.dev+2-client.pem
        keyFile: dist/local.dev+2-client-key.pem
        requireClientAuth: true
        clientCaFiles:
          - dist/rootCA.pem
      client:
        serverName: local.dev
        rootCaFiles:
          - dist/rootCA.pem
    frontend:
      server:
        certFile: dist/local.dev+2-client.pem
        keyFile: dist/local.dev+2-client-key.pem
        requireClientAuth: true
        clientCaFiles:
          - dist/rootCA.pem
      client:
        serverName: local.dev
        rootCaFiles:
          - dist/rootCA.pem

# dummy values, required by yaml parser
# replaced at runtime by temporalite
persistence:
  defaultStore: default
  numHistoryShards: 1

temporalite-ui.yaml

tls:
  caFile: dist/rootCA.pem
  certFile: dist/client.pem
  keyFile: dist/client-key.pem
  serverName: local.dev

Potential risks

Applications with existing command line configuration will still work as advertised.

Is hotfix candidate?

No.

CLAassistant · 2022-09-22T23:55:54Z

All committers have signed the CLA.

cretz · 2022-09-23T17:54:21Z

cmd/temporalite/ui.go

+func newUIConfig(frontendAddr string, uiIP string, uiPort int, configDir string) (*uiconfig.Config, error) {
+	cfg := &uiconfig.Config{}
+	if configDir != "" {
+		if err := config.Load("temporalite-ui", configDir, "", cfg); err != nil {


Even though it appears to basically be the same thing, I wonder if you would rather use https://pkg.go.dev/github.com/temporalio/ui-server/v2/plugins/fs_config_provider#Load to match what the UI server project does

cretz · 2022-09-23T17:54:53Z

cmd/temporalite/ui.go

 	}
+	cfg.TemporalGRPCAddress = frontendAddr
+	cfg.Host = uiIP


Wonder if this should only overwrite if not set from the config file

Sure, I guess the only non-negotiable settings are TemporalGRPCAddress and EnableUI. The others can be overridden by the config file.

cretz · 2022-09-23T17:57:29Z

cmd/temporalite/ui.go

-		EnableUI:            true,
+func newUIConfig(frontendAddr string, uiIP string, uiPort int, configDir string) (*uiconfig.Config, error) {
+	cfg := &uiconfig.Config{}
+	if configDir != "" {


one caveat: if a config directory is present, the new version will look for a "temporalite-ui.yaml" config file and the app will die with a "no config files found" error if a config file cannot be found.

Not the biggest fan of this caveat. It seems reasonable for some people to only want a Temporal config and still get the UI. A file check here is proably fine for now since we don't have a bunch of environments to configure.

Ok, that makes sense.

Update pushed, caveat removed.

jlegrone · 2022-09-23T18:59:01Z

cmd/temporalite/testdata/temporalite-ui.yaml

+  caFile: dist/rootCA.pem
+  certFile: dist/client.pem
+  keyFile: dist/client-key.pem
+  serverName: local.dev


Allow temporal web/ui to be configured from a yaml file so that the web UI does not break when the API is configured for mTLS.

I'm wondering if it would be possible to be smarter about this specific use case in Temporalite so that users don't have to create a UI specific config file as soon as they enable mTLS.

Would there be a way to detect that mTLS is enabled in Temporal's server config and then automatically configure the UI server connection options appropriately?

Possibly, and that might be a thing for another PR. Enabling temporalite for mTLS is something that already requires a config file, so this feels like nothing new.

Also, even though temporal-ui is run by this process, do we really want to have it use the same certificate to talk to the frontend? This is what we would have to do if we wanted to reuse the mTLS configuration from the temporal server. While that is technically possible, it feels weird & odd to have to set up mTLS for internode and frontend and then just have the UI reuse either one. That feels like a decision beyond this PR.

👍 not a blocker; we still should be able to pass in a UI config file anyway. One day maybe we'll be able to do away with all ports in Temporalite other than frontend and the ui-server, which will make "internode" communication less weird since it's all a single process already.

I'm not too concerned about reuse as long as we add an easy readonly mode toggle for the UI.

cretz

Approved, but want @jlegrone's approval too before merge

cretz · 2022-09-23T19:02:20Z

cmd/temporalite/ui.go

+	}
+	if configDir != "" {
+		if err := provider.Load(configDir, cfg, "temporalite-ui"); err != nil {
+			if !strings.HasPrefix(err.Error(), "no config files found") {


Hrmm, would have liked to see if we could prevent the error instead of baking in expectations around its English message. I forget the config file loading whether it's easy to know what files to check for existence eagerly. But I guess it's not that bad since you have a unit test that confirms this conditional works.

Ideally it would be great if the Load function returned an error we can check for with errors.Is since that is the best way of avoiding tedious string comparisons. I also wanted to avoid having to duplicate the file resolution algorithm applied by Load. I reckon for now the unit test check will at least blow up if the algorithm changes or the error changes.

jlegrone · 2022-09-23T19:47:26Z

This could be done in a followup PR, but I'd love to add some documentation around enabling TLS and the web UI together. Perhaps in the internal/examples/mtls directory.

tomcz · 2022-09-23T23:09:37Z

@jlegrone I've not created any extra documentation, but this directory is used by mtls_test.go so I added a template for temporalite-ui.yaml and updated the test to make an api call to temporal-ui just like a browser page would. This at least verifies that temporal-ui can now properly talk to temporal-api.

tomcz · 2022-09-24T01:56:17Z

@cretz the failed macos headless test passes locally on my mac. Not sure how to fix it for the PR since none of the code changed in the PR should have any effect on the test that broke. What am I missing?

go test -tags headless ./...
?   	github.com/temporalio/temporalite	[no test files]
ok  	github.com/temporalio/temporalite/cmd/temporalite	1.478s
?   	github.com/temporalio/temporalite/internal/copyright	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/helloworld	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/mtls	[no test files]
?   	github.com/temporalio/temporalite/internal/liteconfig	[no test files]
ok  	github.com/temporalio/temporalite/temporaltest	3.330s

cretz reviewed Sep 23, 2022

View reviewed changes

jlegrone reviewed Sep 23, 2022

View reviewed changes

tomcz added 2 commits September 24, 2022 05:00

allow web UI to be configured when using mTLS in API

a5b5263

Don't fail startup when ui config file is absent.

9f3bfff

cretz approved these changes Sep 23, 2022

View reviewed changes

jlegrone approved these changes Sep 23, 2022

View reviewed changes

verify temporal-ui to temporal-api integration when mTLS is enabled

167657f

jlegrone approved these changes Sep 23, 2022

View reviewed changes

don't test temporal-ui with headless builds

78c071e

jlegrone merged commit 06b1f4a into temporalio:main Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow web UI to be configured when using mTLS in API #138

allow web UI to be configured when using mTLS in API #138

tomcz commented Sep 22, 2022 •

edited

Loading

CLAassistant commented Sep 22, 2022 •

edited

Loading

cretz Sep 23, 2022

tomcz Sep 23, 2022

cretz Sep 23, 2022

tomcz Sep 23, 2022

cretz Sep 23, 2022

tomcz Sep 23, 2022

tomcz Sep 23, 2022

jlegrone Sep 23, 2022

tomcz Sep 23, 2022

tomcz Sep 23, 2022

jlegrone Sep 23, 2022 •

edited

Loading

cretz left a comment

cretz Sep 23, 2022

tomcz Sep 23, 2022 •

edited

Loading

jlegrone commented Sep 23, 2022 •

edited

Loading

tomcz commented Sep 23, 2022

tomcz commented Sep 24, 2022 •

edited

Loading

allow web UI to be configured when using mTLS in API #138

allow web UI to be configured when using mTLS in API #138

Conversation

tomcz commented Sep 22, 2022 • edited Loading

CLAassistant commented Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlegrone Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomcz Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

jlegrone commented Sep 23, 2022 • edited Loading

tomcz commented Sep 23, 2022

tomcz commented Sep 24, 2022 • edited Loading

tomcz commented Sep 22, 2022 •

edited

Loading

CLAassistant commented Sep 22, 2022 •

edited

Loading

jlegrone Sep 23, 2022 •

edited

Loading

tomcz Sep 23, 2022 •

edited

Loading

jlegrone commented Sep 23, 2022 •

edited

Loading

tomcz commented Sep 24, 2022 •

edited

Loading