Conversation
ravicious
left a comment
There was a problem hiding this comment.
The overall change makes sense to me.
|
|
||
| var teleportCert = &x509.Certificate{ | ||
| Subject: pkix.Name{ | ||
| CommonName: "Gravitational, Inc.", |
There was a problem hiding this comment.
Argh, should we compare just the CN in the end? The more I look at it the more brittle it seems. Especially after re-reading that comment and seeing that Tailscale looks just at CN.
There was a problem hiding this comment.
I based the decision to compare the full DN on what electron-builder is doing:
- Full Distinguished Name (DN) to publisherNames configuration ? electron-userland/electron-builder#7583 (comment)
- https://github.com/electron-userland/electron-builder/blob/02e59ba8a3b02e1b3ab20035ff43f48ea20880b7/packages/electron-updater/src/windowsExecutableCodeSignatureVerifier.ts#L69-L85.
As far as I know, the main issue with comparing only the CN is that it's not globally unique - a company with the same name could be registered in a different jurisdiction. Comparing the full DN reduces that risk significantly (it's unlikely to see another "Gravitational, Inc." registered in California).
Regarding concerns about brittle comparisons: after the last renewal, even the CN (something we wouldn't expect to change) was slightly different. I asked about this on Slack and got confirmation that next time the full DN should not change: https://gravitational.slack.com/archives/C010BD557L6/p1773773823087189?thread_ts=1773772598.878769&cid=C010BD557L6
Taking that into account, comparing only the CN doesn't seem meaningfully less brittle than comparing the full DN, while providing weaker guarantees.
I realized there is a bug in the current signature verification approach (added in #63573).
The verification process currently reads the expected publisher certificate from the running service binary (tsh) and compares it with the update. While I added logic to skip this check for unsigned services, this does not address a situation where the installed service is signed with a certificate that has since expired. In that case,
WinVerifyTrustwould fail, preventing installation of a valid update.While we could ignore this specific
WinVerifyTrusterror and continue reading the certificate from the service binary, that approach is brittle. A better option seems to just hardcode the certificate subject (which is more common in general).I believe I overthought this problem #62545 (comment) in the first place. My main motivation for avoiding hardcoded Teleport publisher identity was to keep updates from OSS builds to OSS working, but that introduced practical issues:
After this change, updates from OSS -> Teleport builds will work, while updates from OSS -> OSS will require either changing the certificate or disabling this logic entirely. I think this is acceptable, as it can be easily adjusted or disabled in custom builds.
Manual Test Plan
Test Environment
Built Teleport Connect from this branch, set client tools updates to
18.7.3.Test Cases