From 2dc2aa5d2104f8f9b3e765f5c80e68a5dd0aaf97 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Thu, 30 Jul 2020 13:36:11 +0200 Subject: [PATCH] lib/connections, lib/tlsutil: Handle certName in Go 1.15 (fixes #6867) (#6868) Our authentication is based on device ID (certificate fingerprint) but we also check the certificate name for ... historical extra security reasons. (I don't think this adds anything but it is what it is.) Since that check breaks in Go 1.15 this change does two things: - Adds a manual check for the peer certificate CommonName, and if they are equal we are happy and don't call the more advanced VerifyHostname() function. This allows our old style certificates to still pass the check. - Adds the cert name "syncthing" as a DNS SAN when generating the certificate. This is the correct way nowadays and makes VerifyHostname() happy in Go 1.15 as well, even without the above patch. --- lib/api/api.go | 23 ++++++++++++++++++----- lib/api/api_test.go | 8 ++++---- lib/connections/service.go | 6 +++++- lib/tlsutil/tlsutil.go | 1 + 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/lib/api/api.go b/lib/api/api.go index cb1842e9f..e5ec85be6 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -149,7 +149,7 @@ func (s *service) getListener(guiCfg config.GUIConfiguration) (net.Listener, err // If the certificate has expired or will expire in the next month, fail // it and generate a new one. if err == nil { - err = checkExpiry(cert) + err = shouldRegenerateCertificate(cert) } if err != nil { l.Infoln("Loading HTTPS certificate:", err) @@ -1736,7 +1736,11 @@ func addressIsLocalhost(addr string) bool { } } -func checkExpiry(cert tls.Certificate) error { +// shouldRegenerateCertificate checks for certificate expiry or other known +// issues with our API/GUI certificate and returns either nil (leave the +// certificate alone) or an error describing the reason the certificate +// should be regenerated. +func shouldRegenerateCertificate(cert tls.Certificate) error { leaf := cert.Leaf if leaf == nil { // Leaf can be nil or not, depending on how parsed the certificate @@ -1752,12 +1756,21 @@ func checkExpiry(cert tls.Certificate) error { } } - if leaf.Subject.String() != leaf.Issuer.String() || - len(leaf.DNSNames) != 0 || len(leaf.IPAddresses) != 0 { - // The certificate is not self signed, or has DNS/IP attributes we don't + if leaf.Subject.String() != leaf.Issuer.String() || len(leaf.IPAddresses) != 0 { + // The certificate is not self signed, or has IP attributes we don't // add, so we leave it alone. return nil } + if len(leaf.DNSNames) > 1 { + // The certificate has more DNS SANs attributes than we ever add, so + // we leave it alone. + return nil + } + if len(leaf.DNSNames) == 1 && leaf.DNSNames[0] != leaf.Issuer.CommonName { + // The one SAN is different from the issuer, so it's not one of our + // newer self signed certificates. + return nil + } if leaf.NotAfter.Before(time.Now()) { return errors.New("certificate has expired") diff --git a/lib/api/api_test.go b/lib/api/api_test.go index 6c6dc74a2..544432711 100644 --- a/lib/api/api_test.go +++ b/lib/api/api_test.go @@ -1136,7 +1136,7 @@ func TestPrefixMatch(t *testing.T) { } } -func TestCheckExpiry(t *testing.T) { +func TestShouldRegenerateCertificate(t *testing.T) { dir, err := ioutil.TempDir("", "syncthing-test") if err != nil { t.Fatal(err) @@ -1149,7 +1149,7 @@ func TestCheckExpiry(t *testing.T) { if err != nil { t.Fatal(err) } - if err := checkExpiry(crt); err == nil { + if err := shouldRegenerateCertificate(crt); err == nil { t.Error("expected expiry error") } @@ -1158,7 +1158,7 @@ func TestCheckExpiry(t *testing.T) { if err != nil { t.Fatal(err) } - if err := checkExpiry(crt); err != nil { + if err := shouldRegenerateCertificate(crt); err != nil { t.Error("expected no error:", err) } @@ -1168,7 +1168,7 @@ func TestCheckExpiry(t *testing.T) { if err != nil { t.Fatal(err) } - if err := checkExpiry(crt); err == nil { + if err := shouldRegenerateCertificate(crt); err == nil { t.Error("expected expiry error") } } diff --git a/lib/connections/service.go b/lib/connections/service.go index b7dca6a0f..1bf9864ed 100644 --- a/lib/connections/service.go +++ b/lib/connections/service.go @@ -305,7 +305,11 @@ func (s *service) handle(ctx context.Context) { if certName == "" { certName = s.tlsDefaultCommonName } - if err := remoteCert.VerifyHostname(certName); err != nil { + if remoteCert.Subject.CommonName == certName { + // All good. We do this check because our old style certificates + // have "syncthing" in the CommonName field and no SANs, which + // is not accepted by VerifyHostname() any more as of Go 1.15. + } else if err := remoteCert.VerifyHostname(certName); err != nil { // Incorrect certificate name is something the user most // likely wants to know about, since it's an advanced // config. Warn instead of Info. diff --git a/lib/tlsutil/tlsutil.go b/lib/tlsutil/tlsutil.go index 774e17e53..f65c35c75 100644 --- a/lib/tlsutil/tlsutil.go +++ b/lib/tlsutil/tlsutil.go @@ -106,6 +106,7 @@ func NewCertificate(certFile, keyFile, commonName string, lifetimeDays int) (tls Subject: pkix.Name{ CommonName: commonName, }, + DNSNames: []string{commonName}, NotBefore: notBefore, NotAfter: notAfter, SignatureAlgorithm: x509.ECDSAWithSHA256,