diff --git a/integration/autoupdate/tools/connect_privileged_updater_windows_test.go b/integration/autoupdate/tools/connect_privileged_updater_windows_test.go index a1930c77f4982..f5d7f8f4566ae 100644 --- a/integration/autoupdate/tools/connect_privileged_updater_windows_test.go +++ b/integration/autoupdate/tools/connect_privileged_updater_windows_test.go @@ -261,6 +261,19 @@ func TestPrivilegedUpdateServiceAllowOnlyOneClientConnection(t *testing.T) { } } +func TestPrivilegedUpdateServiceRejectsUnsignedUpdate(t *testing.T) { + up := update{ + version: "999.0.0", + binary: []byte("payload"), + } + + // Keep signature verification enabled for this test to ensure unsigned + // updates are rejected. + err := runPrivilegedUpdaterFlow(t, up, withServiceTestVerifySignature(nil)) + require.Error(t, err) + require.ErrorContains(t, err, "verifying update signature") +} + type serviceConfig struct { privilegedupdater.ServiceTestConfig checksumServerResponseWriter func(http.ResponseWriter) @@ -286,6 +299,12 @@ func withServiceTestPolicyToolsVersion(version string) privilegedServiceMainConf } } +func withServiceTestVerifySignature(fn func(updatePath string) error) privilegedServiceMainConfigOption { + return func(cfg *serviceConfig) { + cfg.VerifySignature = fn + } +} + type update struct { version string binary []byte @@ -332,6 +351,7 @@ func runPrivilegedUpdaterFlow(t *testing.T, update update, opts ...privilegedSer PolicyCDNBaseURL: server.URL, HTTPClient: server.Client(), PipeAuthenticatedUsersAccess: cfg.PipeAuthenticatedUsersAccess, + VerifySignature: cfg.VerifySignature, }) // We are attempting to run a non-exe file. // It will fail, so we check if we ran the correct file. @@ -409,6 +429,8 @@ func getDefaultConfig(t *testing.T) *privilegedupdater.ServiceTestConfig { UpdateBaseDir: t.TempDir(), // Allow Authenticated Users to create the pipe in tests. PipeAuthenticatedUsersAccess: windows.GENERIC_READ | windows.GENERIC_WRITE, + // Integration test updates are unsigned. + VerifySignature: func(string) error { return nil }, } } diff --git a/lib/teleterm/autoupdate/privilegedupdater/authenticode_windows.go b/lib/teleterm/autoupdate/privilegedupdater/authenticode_windows.go index 1dbceafdc2c42..0f3b178181e63 100644 --- a/lib/teleterm/autoupdate/privilegedupdater/authenticode_windows.go +++ b/lib/teleterm/autoupdate/privilegedupdater/authenticode_windows.go @@ -23,11 +23,9 @@ package privilegedupdater import ( "crypto/x509" - "errors" + "crypto/x509/pkix" "fmt" - "os" "slices" - "syscall" "unsafe" "github.com/gravitational/trace" @@ -40,30 +38,8 @@ const ( cmsgSignerCertInfoParam = 7 ) -// verifySignature checks if the update is signed by the same entity as the running service. +// verifySignature checks if the update is signed by Teleport. func verifySignature(updatePath string) error { - servicePath, err := os.Executable() - if err != nil { - return trace.Wrap(err) - } - servicePathPtr, err := windows.UTF16PtrFromString(servicePath) - if err != nil { - return trace.Wrap(err) - } - - if err = verifyTrust(servicePathPtr); err != nil { - if errors.Is(err, syscall.Errno(windows.TRUST_E_NOSIGNATURE)) { - log.Warn("service is not signed, skipping signature verification") - return nil - } - return trace.Wrap(err) - } - - serviceCert, err := getCert(servicePathPtr) - if err != nil { - return trace.Wrap(err, "getting service certificate") - } - updatePathPtr, err := windows.UTF16PtrFromString(updatePath) if err != nil { return trace.Wrap(err) @@ -76,8 +52,8 @@ func verifySignature(updatePath string) error { return trace.Wrap(err, "getting update certificate") } - if !compareSubjectProperties(serviceCert, updateCert) { - return trace.BadParameter("signature verification failed: update and service subjects do not match (service: %s, update: %s)", logCert(serviceCert), logCert(updateCert)) + if !hasTeleportSubject(updateCert) { + return trace.BadParameter("signature verification failed: update subject does not match Teleport subject (teleport: %s, update: %s)", logCert(teleportCert), logCert(updateCert)) } return nil @@ -158,13 +134,9 @@ func getCert(path *uint16) (*x509.Certificate, error) { return cert, trace.Wrap(err) } -// compareSubjectProperties checks whether two certificates appear to belong to the same publisher by comparing a subset -// of their X.509 Subject fields. +// hasTeleportSubject checks whether updateCert has the expected Teleport publisher subject by comparing a subset of X.509 Subject fields. // The cert validity must be checked first with verifyTrust. // -// For Teleport's Windows signing certificate the subject typically looks like: -// CN="Gravitational, Inc.", O="Gravitational, Inc.", L="Oakland", ST="California", C="US" -// // Security notes: // - This does NOT provide cryptographic authenticity guarantees. An attacker could theoretically obtain a certificate // with identical subject fields. However, obtaining such a certificate from a CA trusted by Windows is non-trivial @@ -181,12 +153,12 @@ func getCert(path *uint16) (*x509.Certificate, error) { // https://github.com/electron-userland/electron-builder/blob/02e59ba8a3b02e1b3ab20035ff43f48ea20880b7/packages/electron-updater/src/windowsExecutableCodeSignatureVerifier.ts#L69-L85 // - Tailscale verifies only the CN: // https://github.com/tailscale/tailscale/blob/3ec5be3f510f74738179c1023468343a62a7e00f/clientupdate/clientupdate_windows.go#L70-L74 -func compareSubjectProperties(cert1, cert2 *x509.Certificate) bool { - if cert1 == nil || cert2 == nil { +func hasTeleportSubject(updateCert *x509.Certificate) bool { + if updateCert == nil { return false } - s1, s2 := cert1.Subject, cert2.Subject + s1, s2 := teleportCert.Subject, updateCert.Subject return s1.CommonName == s2.CommonName && slices.Equal(s1.Organization, s2.Organization) && slices.Equal(s1.Locality, s2.Locality) && @@ -194,6 +166,16 @@ func compareSubjectProperties(cert1, cert2 *x509.Certificate) bool { slices.Equal(s1.Country, s2.Country) } +var teleportCert = &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "Gravitational, Inc.", + Organization: []string{"Gravitational, Inc."}, + Locality: []string{"Oakland"}, + Province: []string{"California"}, + Country: []string{"US"}, + }, +} + func logCert(cert *x509.Certificate) string { if cert == nil { return "" diff --git a/lib/teleterm/autoupdate/privilegedupdater/service_windows.go b/lib/teleterm/autoupdate/privilegedupdater/service_windows.go index 14338fe27c3af..977afa0720f94 100644 --- a/lib/teleterm/autoupdate/privilegedupdater/service_windows.go +++ b/lib/teleterm/autoupdate/privilegedupdater/service_windows.go @@ -99,6 +99,9 @@ type ServiceTestConfig struct { // PipeAuthenticatedUsersAccess overrides Authenticated Users access mask in // the named pipe DACL. If zero, SafePipeReadWriteAccess is used. PipeAuthenticatedUsersAccess uint32 + // VerifySignature overrides signature verification. + // If nil, verifySignature is used. + VerifySignature func(updatePath string) error } // InstallService installs the Teleport Connect privileged update service. @@ -177,7 +180,11 @@ func (h *handler) Execute(ctx context.Context, _ []string) (err error) { return trace.Wrap(err, "checking if update is upgrade") } - if err = verifySignature(updatePath); err != nil { + verifySignatureFn := verifySignature + if h.testCfg.VerifySignature != nil { + verifySignatureFn = h.testCfg.VerifySignature + } + if err = verifySignatureFn(updatePath); err != nil { return trace.Wrap(err, "verifying update signature") } diff --git a/web/packages/teleterm/README.md b/web/packages/teleterm/README.md index 5918dcb24a161..655ab94735ca8 100644 --- a/web/packages/teleterm/README.md +++ b/web/packages/teleterm/README.md @@ -145,6 +145,18 @@ GOOS=windows CGO_ENABLED=1 go build -o build/tsh.exe -ldflags '-w -s' -buildvcs It's important for the executable to end with `.exe`. If that command doesn't work, you can always inspect what we currently do in [our Windows build pipeline scripts](https://github.com/gravitational/teleport/blob/983017b23f65e49350615bfbbe52b7f1080ea7b9/build.assets/windows/build.ps1#L377). +#### Certificate requirements for updater + +The privileged updater on Windows (for per-machine updates) verifies update signatures before running the installer: + +- `WinVerifyTrust` must succeed for the update executable. +- The signer subject must match the Teleport publisher subject: + `CN=Gravitational, Inc., O=Gravitational, Inc., L=Oakland, ST=California, C=US`. + +The hardcoded values are kept in `authenticode_windows.go`. +As a result, the service binary itself does not need to be signed (e.g., in OSS builds), but all updates must be +properly signed. + #### Native dependencies on Windows On Windows, you need to pay special attention to [the dev tools needed by node-pty](https://github.com/microsoft/node-pty?tab=readme-ov-file#windows),