chore(discovery,upgrade): use regular TLS certificate verification (#9673)

This changes the two remaining instances where we use insecure HTTPS to
use standard HTTPS certificate verification.

When we introduced these things, almost a decade ago, HTTPS certificates
were expensive and annoying to get, much of the web was still HTTP, and
many devices seemed to not have up-to-date CA bundles.

Nowadays _all_ of the web is HTTPS and I'm skeptical that any device can
work well without understanding LetsEncrypt certificates in particular.

Our current discovery servers use hardcoded certificates which has
several issues:
- Not great for security if it leaks as there is no way to rotate it
- Not great for infrastructure flexibility as we can't use many load
balancer or TLS termination services
- The certificate is a very oddball ECDSA-SHA384 type certificate which
has higher CPU cost than a more regular certificate, which has real
effects on our infrastructure

Using normal TLS certificates here improves these things.

I expect there will be some very few devices out there for which this
doesn't work. For the foreseeable future they can simply change the
config to use the old URLs and parameters -- it'll be years before we
can retire those entirely.

For the upgrade client this simply seems like better hygiene. While our
releases are signed anyway, protecting the metadata exchange is _better_
and, again, I doubt many clients will fail this today.
This commit is contained in:
Jakob Borg 2024-09-11 09:29:19 +02:00 committed by GitHub
parent 29f7510f5a
commit 718b1ce2b7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 8 additions and 17 deletions

View File

@ -53,14 +53,14 @@ var (
// DefaultDiscoveryServersV4 should be substituted when the configuration // DefaultDiscoveryServersV4 should be substituted when the configuration
// contains <globalAnnounceServer>default-v4</globalAnnounceServer>. // contains <globalAnnounceServer>default-v4</globalAnnounceServer>.
DefaultDiscoveryServersV4 = []string{ DefaultDiscoveryServersV4 = []string{
"https://discovery.syncthing.net/v2/?noannounce&id=LYXKCHX-VI3NYZR-ALCJBHF-WMZYSPK-QG6QJA3-MPFYMSO-U56GTUK-NA2MIAW", "https://discovery-lookup.syncthing.net/v2/?noannounce",
"https://discovery-v4.syncthing.net/v2/?nolookup&id=LYXKCHX-VI3NYZR-ALCJBHF-WMZYSPK-QG6QJA3-MPFYMSO-U56GTUK-NA2MIAW", "https://discovery-announce-v4.syncthing.net/v2/?nolookup",
} }
// DefaultDiscoveryServersV6 should be substituted when the configuration // DefaultDiscoveryServersV6 should be substituted when the configuration
// contains <globalAnnounceServer>default-v6</globalAnnounceServer>. // contains <globalAnnounceServer>default-v6</globalAnnounceServer>.
DefaultDiscoveryServersV6 = []string{ DefaultDiscoveryServersV6 = []string{
"https://discovery.syncthing.net/v2/?noannounce&id=LYXKCHX-VI3NYZR-ALCJBHF-WMZYSPK-QG6QJA3-MPFYMSO-U56GTUK-NA2MIAW", "https://discovery-lookup.syncthing.net/v2/?noannounce",
"https://discovery-v6.syncthing.net/v2/?nolookup&id=LYXKCHX-VI3NYZR-ALCJBHF-WMZYSPK-QG6QJA3-MPFYMSO-U56GTUK-NA2MIAW", "https://discovery-announce-v6.syncthing.net/v2/?nolookup",
} }
// DefaultDiscoveryServers should be substituted when the configuration // DefaultDiscoveryServers should be substituted when the configuration
// contains <globalAnnounceServer>default</globalAnnounceServer>. // contains <globalAnnounceServer>default</globalAnnounceServer>.

View File

@ -14,7 +14,6 @@ import (
"archive/zip" "archive/zip"
"bytes" "bytes"
"compress/gzip" "compress/gzip"
"crypto/tls"
"encoding/json" "encoding/json"
"errors" "errors"
"fmt" "fmt"
@ -61,26 +60,18 @@ const (
maxMetadataSize = 10 << 20 // 10 MiB maxMetadataSize = 10 << 20 // 10 MiB
) )
// This is an HTTP/HTTPS client that does *not* perform certificate var upgradeClient = &http.Client{
// validation. We do this because some systems where Syncthing runs have
// issues with old or missing CA roots. It doesn't actually matter that we
// load the upgrade insecurely as we verify an ECDSA signature of the actual
// binary contents before accepting the upgrade.
var insecureHTTP = &http.Client{
Timeout: readTimeout, Timeout: readTimeout,
Transport: &http.Transport{ Transport: &http.Transport{
DialContext: dialer.DialContext, DialContext: dialer.DialContext,
Proxy: http.ProxyFromEnvironment, Proxy: http.ProxyFromEnvironment,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
}, },
} }
var osVersion string var osVersion string
func init() { func init() {
_ = http2.ConfigureTransport(insecureHTTP.Transport.(*http.Transport)) _ = http2.ConfigureTransport(upgradeClient.Transport.(*http.Transport))
osVersion, _ = host.KernelVersion() osVersion, _ = host.KernelVersion()
osVersion = strings.TrimSpace(osVersion) osVersion = strings.TrimSpace(osVersion)
} }
@ -95,7 +86,7 @@ func insecureGet(url, version string) (*http.Response, error) {
if osVersion != "" { if osVersion != "" {
req.Header.Set("Syncthing-Os-Version", osVersion) req.Header.Set("Syncthing-Os-Version", osVersion)
} }
return insecureHTTP.Do(req) return upgradeClient.Do(req)
} }
// FetchLatestReleases returns the latest releases. The "current" parameter // FetchLatestReleases returns the latest releases. The "current" parameter
@ -233,7 +224,7 @@ func readRelease(archiveName, dir, url string) (string, error) {
} }
req.Header.Add("Accept", "application/octet-stream") req.Header.Add("Accept", "application/octet-stream")
resp, err := insecureHTTP.Do(req) resp, err := upgradeClient.Do(req)
if err != nil { if err != nil {
return "", err return "", err
} }