Connect: add privileged updater service by gzdunek · Pull Request #63572 · gravitational/teleport

gzdunek · 2026-02-06T11:24:16Z

Contributes to #59295
RFD #62545

Part 1/2 of the updater service. The aim of the service is to silently install per-machine updates, without requiring admin privileges from the user.

The update process is described in the RFD https://github.com/gravitational/teleport/blob/rfd/0242-improve-windows-installation-experience/rfd/0242-improve-windows-installation-experience.md#update-process.

While working on the implementation, I've made slight adjustments to the update process (I also updated the RFD):

I flattened the directory structure. Previously it was %ProgramData%\TeleportConnect\Updates\<GUID>; it is now %ProgramData%\TeleportConnectUpdater\<GUID>. This makes the directory easier to secure.
Initially, if the directory already existed, the plan was to verify that its ACLs matched the expected ones (and exit early on a mismatch). In practice, this check appears to be a really complex thing in Windows, and it also introduced a potential denial-of-service vector: a standard user could pre-create the directory and block the service. Instead, the service now takes ownership of the directory and resets its ACLs to the desired configuration.

Manual Test Plan

Tested along with #63573. The test plan is available there.

cthach

I didn't review the whole thing yet, but I noticed that at a glance, there are no tests.

Can we please add tests for this new addition?

gzdunek · 2026-02-09T11:36:54Z

Yeah, there are no tests right now 😞 mainly because I didn't have a good way to test this. The code is heavily Windows-specific, while our CI runs on Linux.
I don't think it makes much sense to extract and test only the platform-independent parts, such as ensureIsUpgrade or verifyUpdateChecksum. That wouldn't add much value and would likely make the code harder to read, especially since this logic is only needed on Windows.

However, I've just realized that we already run some integration tests on Windows (#57059). I'll work on adding integration tests for this updater as well. This will require some minor changes, for example adding an option to run the service implementation directly, instead of through Windows SCM.

I'll open a separate PR for it so this one doesn't grow further.

russjones · 2026-02-10T18:28:19Z

@gzdunek Can we add test coverage to this PR?

The main thing with test coverage is to encode expected behavior into the test. How you do that is up to you, but let's make sure that is covered in this PR so we don't have someone accidentally cause a regression because that expectation was just in our collective understanding rather than in test code.

Spending extra time doing that now will save us more time in the future when someone has to figure out those implicit expectations when resolving a regression under time pressure.

cthach · 2026-02-10T19:34:45Z

Yeah, there are no tests right now 😞 mainly because I didn't have a good way to test this. The code is heavily Windows-specific, while our CI runs on Linux. I don't think it makes much sense to extract and test only the platform-independent parts, such as ensureIsUpgrade or verifyUpdateChecksum. That wouldn't add much value and would likely make the code harder to read, especially since this logic is only needed on Windows.

However, I've just realized that we already run some integration tests on Windows (#57059). I'll work on adding integration tests for this updater as well. This will require some minor changes, for example adding an option to run the service implementation directly, instead of through Windows SCM.

I'll open a separate PR for it so this one doesn't grow further.

Adding on to @russjones, perhaps instead of a separate PR for tests, we do that here?

The pro is we have related code and tests in a single commit. The con is as you mentioned, it'll be longer in size, but I feel like the tradeoff is worth it so we have the confidence that the code that we intend to commit is and will continue to behave as expected/intended.

From a security angle, it is personally hard for me to convince myself that this privileged piece of code that has the ability to autonomously install more code should go live in prod (edit: eventually, I'm aware merging doesn't go straight to prod) without high levels of confidence it will do exactly what we expect it to do all scenarios. While we can't test everything, some tests are better than no tests.

gzdunek · 2026-02-11T15:45:43Z

Sure, I added the integrations tests to this PR. These tests will run on CI once we merge #63732.

I'll also include the manual testing steps once I get the tag builds published. I’m running into some random errors with that at the moment.

cthach · 2026-02-12T13:35:25Z

Sure, I added the integrations tests to this PR. These tests will run on CI once we merge #63732.

I'll also include the manual testing steps once I get the tag builds published. I’m running into some random errors with that at the moment.

Thank you!

I just reviewed the RFD to get some context. Now I'm reviewing this specific PR now.

cthach

I don't have much Windows experience, so I reviewed what I could.

Please ensure we get a SME with Windows expertise to give this a review.

Also, please document any manual tests you did.

ravicious

Got to waitForSingleClient in lib/teleterm/autoupdate/privileged_updater_service_windows.go, I'll continue the review next week.

ravicious · 2026-02-13T16:57:17Z

+	// Allow SYSTEM/Admins Full Control, Authenticated Users read/write, implicitly denies everyone else.
+	pipeSecurityDescriptor = "D:" + // DACL
+		"(A;;GA;;;SY)" + // Allow (A);; Generic All (GA);;; SYSTEM (SY)
+		"(A;;GA;;;BA)" + // Allow (A);; Generic All (GA);;; Built-in Admins (BA)
+		"(A;;GRGW;;;AU)" // Allow (A);; Generic Read/Write (GRGW);;; Authenticated Users (AU)
+	updateDirSecurityDescriptor = "O:SY" + // Owner SYSTEM
+		"D:P" + // 'P' blocks permissions inheritance from the parent directory
+		"(A;OICI;GA;;;SY)" + // Allow System Full Access
+		"(A;OICI;GA;;;BA)" // Allow Built-in Administrators Full Access


Is there a some kind of an article about pipes, folders and DACL that we could use to cross-check these descriptors against?

The RFD isn't super specific on this. But I also see it says "First try to create a directory with correct ACLs, granting write access only to SYSTEM and Administrators" but here we grant them GA.

In other words, how do we know this is secure? :)

The RFD isn't super specific on this. But I also see it says "First try to create a directory with correct ACLs, granting write access only to SYSTEM and Administrators" but here we grant them GA.

I phrased it incorrectly. Instead of "granting write-only access to" I should have said "granting access only to"(as in Doyensec's comment). We can't grant write-only access, since the service also needs permission to read and delete data.

Is there a some kind of an article about pipes, folders and DACL that we could use to cross-check these descriptors against?

It's surprisingly difficult to find good examples of how to configure a DACL for requirements like ours. Here's some documentation explaining how the different parts of a security descriptor work:

General DACL structure:
https://techcommunity.microsoft.com/blog/askds/the-security-descriptor-definition-language-of-love-part-1/395202
https://techcommunity.microsoft.com/blog/askds/the-security-descriptor-definition-language-of-love-part-2/395258

Securing pipes:
https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-security-and-access-rights
https://stackoverflow.com/questions/29947524/c-let-user-process-write-to-local-system-named-pipe-custom-security-descrip

Based on these references, I've narrowed down the pipe's access configuration slightly. Microsoft's documentation notes that the default FILE_GENERIC_WRITE permission can be problematic, as it may allow unintended users (in our case authenticated users) to open additional pipe instances. We can fix this by constructing a custom access mask (Generic Read + File Write Data) that grants only the necessary I/O rights.

ravicious · 2026-02-19T17:03:15Z

+// This function runs with SYSTEM privileges and relies on the Go standard library’s
+// os.RemoveAll implementation on Windows. It detects reparse points (symlinks and
+// junctions) and removes the link itself without ever recursing into the target,
+// mitigating junction/symlink crossing attacks.


I cannot find much about this in the godoc or the source of os.RemoveAll.

https://cs.opensource.google/go/go/+/refs/tags/go1.26.0:src/os/path.go;l=68-75
https://cs.opensource.google/go/go/+/refs/tags/go1.26.0:src/os/removeall_at.go;l=15

I mean maybe these two lines point to it? https://cs.opensource.google/go/go/+/refs/tags/go1.26.0:src/os/removeall_at.go;l=67-68 But it's very obscure.

Where did you find that? :D

Yeah, I don't think this behavior is explicitly documented anywhere. I based my understanding on this change: kkpan11/go@6d41809 (which switches to removefileat, as in your third link).

Under the hood, removefileat is implemented as:

windows.Deleteat(dirfd, name, windows.FILE_NON_DIRECTORY_FILE)

My understanding is that this deletes name if it is a regular file, a symlink, or a junction. If it's an actual directory, the call fails due to the FILE_NON_DIRECTORY_FILE flag, and removeAll then falls back to recursively removing its contents.

Before opening the PR, I manually tested that it doesn't recurse into symlinks/junctions, and we also have an integration test for that:

teleport/integration/autoupdate/tools/connect_privileged_updater_windows_test.go

Line 209 in 4822f37

require.NoError(t, err, "cleanup must not remove files outside base dir via junction traversal")

ravicious · 2026-02-19T17:18:10Z

+// It starts the update service, sends update metadata, and transfers the binary for validation and installation.
+func RunServiceAndInstallUpdateFromClient(ctx context.Context, path string, forceRun bool, version string) error {
+	if err := ensureServiceRunning(ctx); err != nil {
+		// Service failed to start; fall back to client-side install (UAC).


This comment would make a good error log message and we could also include err to know what the error was, no? At the moment err is just lost.

Yeah, the error is currently being swallowed. The issue is that there’s no way to log it at that point - Connect has already exited, and tsh.exe doesn't write logs to file (we would need a separate file just for this call).

I addressed this in #63573 (comment). Connect now invokes RunServiceAndInstallUpdateFromClient synchronously, which allows it to properly capture and handle any errors returned from that call.

I also moved the UAC fallback logic into the JS layer, which feels like a more appropriate place for it.

cthach

🚀

backport-bot-workflows · 2026-03-06T13:34:50Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v17	Failed
branch/v18	Failed

* Add privileged updater service * Add integration tests for updater * Review fixes * Move privileged updater to its own module * Fix comments * Interpolate registry pathnames, switch errors to AccessDenied * Improve error handling in `waitForSingleClient` * Use stricter DACL for named pipe * Close `conn` on context cancellation * Move reading update meta to separate function * `trace.LimitExceeded` -> `trace.Errorf` * Fix test * Ensure updater only allows HTTPS * Use TLS server in tests * Fix tests

* Add privileged updater service * Add integration tests for updater * Review fixes * Move privileged updater to its own module * Fix comments * Interpolate registry pathnames, switch errors to AccessDenied * Improve error handling in `waitForSingleClient` * Use stricter DACL for named pipe * Close `conn` on context cancellation * Move reading update meta to separate function * `trace.LimitExceeded` -> `trace.Errorf` * Fix test * Ensure updater only allows HTTPS * Use TLS server in tests * Fix tests (cherry picked from commit ad36d4e)

adrian-doyensec

This looks sound from the security point of view. (I’m ignoring the missing certificate verification here since that is addressed in #63573)

adrian-doyensec · 2026-03-30T10:07:22Z

+	dacl, _, err := sa.SecurityDescriptor.DACL()
+	if err != nil {
+		return trace.Wrap(err, "reading DACL from security descriptor")
+	}


Minor: you could defensively check also dacl != nil here, since a NULL DACL means unrestricted access. That said, this is only a sanity check, not real ACL validation. The descriptor is currently hardcoded, and a non-nil DACL could still be maliciously weak.

* Add privileged updater service * Add integration tests for updater * Review fixes * Move privileged updater to its own module * Fix comments * Interpolate registry pathnames, switch errors to AccessDenied * Improve error handling in `waitForSingleClient` * Use stricter DACL for named pipe * Close `conn` on context cancellation * Move reading update meta to separate function * `trace.LimitExceeded` -> `trace.Errorf` * Fix test * Ensure updater only allows HTTPS * Use TLS server in tests * Fix tests (cherry picked from commit ad36d4e)

Add privileged updater service

9650e79

gzdunek added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 backport/branch/v18 labels Feb 6, 2026

github-actions Bot requested review from cthach and hugoShaka February 6, 2026 11:24

github-actions Bot added the size/lg label Feb 6, 2026

gzdunek requested review from nklaassen and ravicious and removed request for cthach and hugoShaka February 6, 2026 11:25

gzdunek mentioned this pull request Feb 6, 2026

Connect: install updates via privileged service, add signature verification #63573

Merged

5 tasks

cthach reviewed Feb 6, 2026

View reviewed changes

Add integration tests for updater

44125cf

gzdunek mentioned this pull request Feb 11, 2026

Run Connect autoupdate integration tests on Windows #63732

Merged

gzdunek requested a review from cthach February 11, 2026 16:31

cthach reviewed Feb 12, 2026

View reviewed changes

Review fixes

f475108

ravicious reviewed Feb 13, 2026

View reviewed changes

ravicious self-requested a review February 13, 2026 17:28

gzdunek added 5 commits February 17, 2026 11:06

Move privileged updater to its own module

0a56298

Fix comments

b7f5da4

Interpolate registry pathnames, switch errors to AccessDenied

d8066ff

Improve error handling in waitForSingleClient

bdcab1f

Use stricter DACL for named pipe

c14c00a

cthach self-requested a review February 18, 2026 01:21

nklaassen reviewed Feb 19, 2026

View reviewed changes

Comment thread lib/teleterm/autoupdate/privilegedupdater/service_windows.go Outdated

Close conn on context cancellation

4822f37

gzdunek requested a review from nklaassen February 19, 2026 15:37

nklaassen reviewed Feb 19, 2026

View reviewed changes

Comment thread lib/teleterm/autoupdate/privilegedupdater/service_windows.go Outdated

ravicious approved these changes Feb 19, 2026

View reviewed changes

gzdunek added 4 commits February 20, 2026 10:33

Move reading update meta to separate function

cc9a692

trace.LimitExceeded -> trace.Errorf

6c70b3a

Merge branch 'master' into gzdunek/connect-updater-1

7c41617

Fix test

bae3b1f

gzdunek requested a review from nklaassen February 20, 2026 10:05

nklaassen approved these changes Feb 26, 2026

View reviewed changes

cthach approved these changes Feb 27, 2026

View reviewed changes

gzdunek added this pull request to the merge queue Mar 2, 2026

gzdunek removed this pull request from the merge queue due to a manual request Mar 2, 2026

gzdunek added 4 commits March 2, 2026 19:17

Merge branch 'master' into gzdunek/connect-updater-1

5854e50

Ensure updater only allows HTTPS

eed2bfc

Use TLS server in tests

316691e

Fix tests

1cadc54

gzdunek added this pull request to the merge queue Mar 6, 2026

Merged via the queue into master with commit ad36d4e Mar 6, 2026
39 checks passed

gzdunek deleted the gzdunek/connect-updater-1 branch March 6, 2026 13:32

adrian-doyensec reviewed Mar 30, 2026

View reviewed changes

gzdunek mentioned this pull request Mar 30, 2026

Connect: fix failing fresh silent NSIS installs #65170

Merged

1 task

gzdunek mentioned this pull request Mar 30, 2026

[v18] Connect: dual installer mode on Windows, updates config in system registry, privileged updater service, no downgrade policy #65173

Open

10 tasks

Conversation

gzdunek commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual Test Plan

Uh oh!

cthach left a comment

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Feb 9, 2026

Uh oh!

russjones commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cthach commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzdunek commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cthach commented Feb 12, 2026

Uh oh!

cthach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

ravicious Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

gzdunek Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

gzdunek Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ravicious Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

gzdunek Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

cthach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

backport-bot-workflows Bot commented Mar 6, 2026

Uh oh!

adrian-doyensec left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-doyensec Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

gzdunek commented Feb 6, 2026 •

edited

Loading

russjones commented Feb 10, 2026 •

edited

Loading

cthach commented Feb 10, 2026 •

edited

Loading

gzdunek commented Feb 11, 2026 •

edited

Loading