add support for mTLS on incoming client connections - #3981
Open
TheConcierge wants to merge 1 commit into
Open
TheConcierge wants to merge 1 commit into
TheConcierge wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
TheConcierge
commented
Oct 2, 2026
| // Validate checks the UI server's inbound TLS configuration. | ||
| func (t UIServerTLS) Validate() error { | ||
| if (t.CertFile == "") != (t.KeyFile == "") { | ||
| return errors.New("uiServerTLS.certFile and uiServerTLS.keyFile must both be set or both unset") |
Author
There was a problem hiding this comment.
this is actually a behavior change i'd like to point out explicitly.
Before, if you were missing one of these, we'd just default to no TLS. this could silently cause insecure connections when slightly misconfigured, which didn't seem desirable. The Validation now explicitly rejects when only one of these are set, as we can't know which they intended (no tls or are expecting working tls).
If we anticipate this being a problem for some subset of open source users (and care about the backward compatibility with configuration), I'm open to changing how strict validation is.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description & motivation 💭
In production temporal, we currently have an nginx proxy sitting between envoy and the
ui-server. At this point, it mainly serves the purpose of TLS termination and cert reloading. Thenginxproxy has been giving us (minor) issues and it makes more sense to remove it instead of trying to troubleshoot.This change adds support for mTLS between the ui-server and incoming client connections, removing the need for an intermediary proxy. Considering we also want to utilize the cert loader, this was moved out into a shared library so both upstream and downstream connections can utilize the same code.
Screenshots (if applicable) 📸
Design Considerations 🎨
The main thing is, since this is open source, we are introducing new configuration options that we are likely bound to support in whichever format we ship. I'm unsure if we have teams or team members who have strong opinions on this, but if so, happy to change whatever.
Testing 🧪
How was this tested 👻
Manual testing was a bit weird on this one. I ended up porting this change to the ui-server repo, forking it, replacing the dependency in our internal build repo, and manually patching a cell with the new version (as well as rerouting traffic away from the nginx reverse proxy). From there, I loaded up a workflow page, making sure that everything still loaded, and checked the logs to make sure traffic was, indeed, flowing through the new web api instance.
Steps for others to test: 🚶🏽♂️🚶🏽♀️
My testing was...a fairly manual setup. I'm happy to share some of those internal changes or codify my steps in a runbook if you all think that this is a decent enough testing strategy. If there is a better strat, please let me know and I'm happy to try another way.
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
Any docs updates needed?
I'm not seeing anywhere in the README or
docs.temporal.iothat reference any of theTEMPORAL_UI_SERVERvars. This may be something to address in general.