From d3161077498a58940fe2bd084218bcf62fddb38d Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 5 Feb 2020 13:51:48 +0100 Subject: [PATCH 1/4] cert: extract IP and DNS parsing into methods --- cert/selfsigned.go | 126 ++++++++++++++++++++++++++------------------- 1 file changed, 73 insertions(+), 53 deletions(-) diff --git a/cert/selfsigned.go b/cert/selfsigned.go index e428e5eb5..574a37f33 100644 --- a/cert/selfsigned.go +++ b/cert/selfsigned.go @@ -31,6 +31,75 @@ var ( serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128) ) +// ipAddresses returns the parserd IP addresses to use when creating the TLS +// certificate. +func ipAddresses(tlsExtraIPs []string) ([]net.IP, error) { + // Collect the host's IP addresses, including loopback, in a slice. + ipAddresses := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")} + + // addIP appends an IP address only if it isn't already in the slice. + addIP := func(ipAddr net.IP) { + for _, ip := range ipAddresses { + if ip.Equal(ipAddr) { + return + } + } + ipAddresses = append(ipAddresses, ipAddr) + } + + // Add all the interface IPs that aren't already in the slice. + addrs, err := net.InterfaceAddrs() + if err != nil { + return nil, err + } + for _, a := range addrs { + ipAddr, _, err := net.ParseCIDR(a.String()) + if err == nil { + addIP(ipAddr) + } + } + + // Add extra IPs to the slice. + for _, ip := range tlsExtraIPs { + ipAddr := net.ParseIP(ip) + if ipAddr != nil { + addIP(ipAddr) + } + } + + return ipAddresses, nil +} + +// dnsNames returns the host and DNS names to use when creating the TLS +// ceftificate. +func dnsNames(tlsExtraDomains []string) (string, []string) { + // Collect the host's names into a slice. + host, err := os.Hostname() + if err != nil { + // Nothing much we can do here, other than falling back to + // localhost as fallback. A hostname can still be provided with + // the tlsExtraDomain parameter if the problem persists on a + // system. + host = "localhost" + } + + dnsNames := []string{host} + if host != "localhost" { + dnsNames = append(dnsNames, "localhost") + } + dnsNames = append(dnsNames, tlsExtraDomains...) + + // Also add fake hostnames for unix sockets, otherwise hostname + // verification will fail in the client. + dnsNames = append(dnsNames, "unix", "unixpacket") + + // Also add hostnames for 'bufconn' which is the hostname used for the + // in-memory connections used on mobile. + dnsNames = append(dnsNames, "bufconn") + + return host, dnsNames +} + // GenCertPair generates a key/cert pair to the paths provided. The // auto-generated certificates should *not* be used in production for public // access as they're self-signed and don't necessarily contain all of the @@ -56,62 +125,13 @@ func GenCertPair(org, certFile, keyFile string, tlsExtraIPs, return fmt.Errorf("failed to generate serial number: %s", err) } - // Collect the host's IP addresses, including loopback, in a slice. - ipAddresses := []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")} - - // addIP appends an IP address only if it isn't already in the slice. - addIP := func(ipAddr net.IP) { - for _, ip := range ipAddresses { - if ip.Equal(ipAddr) { - return - } - } - ipAddresses = append(ipAddresses, ipAddr) - } - - // Add all the interface IPs that aren't already in the slice. - addrs, err := net.InterfaceAddrs() + // Get all DNS names and IP addresses to use when creating the + // certificate. + host, dnsNames := dnsNames(tlsExtraDomains) + ipAddresses, err := ipAddresses(tlsExtraIPs) if err != nil { return err } - for _, a := range addrs { - ipAddr, _, err := net.ParseCIDR(a.String()) - if err == nil { - addIP(ipAddr) - } - } - - // Add extra IPs to the slice. - for _, ip := range tlsExtraIPs { - ipAddr := net.ParseIP(ip) - if ipAddr != nil { - addIP(ipAddr) - } - } - - // Collect the host's names into a slice. - host, err := os.Hostname() - if err != nil { - // Nothing much we can do here, other than falling back to - // localhost as fallback. A hostname can still be provided with - // the tlsExtraDomain parameter if the problem persists on a - // system. - host = "localhost" - } - - dnsNames := []string{host} - if host != "localhost" { - dnsNames = append(dnsNames, "localhost") - } - dnsNames = append(dnsNames, tlsExtraDomains...) - - // Also add fake hostnames for unix sockets, otherwise hostname - // verification will fail in the client. - dnsNames = append(dnsNames, "unix", "unixpacket") - - // Also add hostnames for 'bufconn' which is the hostname used for the - // in-memory connections used on mobile. - dnsNames = append(dnsNames, "bufconn") // Generate a private key for the certificate. priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) From 83dcf95f929dfb3b604a9dc6978e34c14b0651a1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 5 Feb 2020 13:51:48 +0100 Subject: [PATCH 2/4] cert+test: IsOutdated check for TLS files if IPs or DNS changed This commit creates a new utility method IsOutdated that can be used to check whether a TLS certificate mathces the extra IPs and domains given in the lnd config. --- cert/selfsigned.go | 77 +++++++++++++++++++++++ cert/selfsigned_test.go | 135 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 212 insertions(+) create mode 100644 cert/selfsigned_test.go diff --git a/cert/selfsigned.go b/cert/selfsigned.go index 574a37f33..f67e8cd8a 100644 --- a/cert/selfsigned.go +++ b/cert/selfsigned.go @@ -100,6 +100,83 @@ func dnsNames(tlsExtraDomains []string) (string, []string) { return host, dnsNames } +// IsOutdated returns whether the given certificate is outdated w.r.t. the IPs +// and domains given. The certificate is considered up to date if it was +// created with _exactly_ the IPs and domains given. +func IsOutdated(cert *x509.Certificate, tlsExtraIPs, + tlsExtraDomains []string) (bool, error) { + + // Parse the slice of IP strings. + ips, err := ipAddresses(tlsExtraIPs) + if err != nil { + return false, err + } + + // To not consider the certificate outdated if it has duplicate IPs or + // if only the order has changed, we create two maps from the slice of + // IPs to compare. + ips1 := make(map[string]net.IP) + for _, ip := range ips { + ips1[ip.String()] = ip + } + + ips2 := make(map[string]net.IP) + for _, ip := range cert.IPAddresses { + ips2[ip.String()] = ip + } + + // If the certificate has a different number of IP addresses, it is + // definitely out of date. + if len(ips1) != len(ips2) { + return true, nil + } + + // Go through each IP address, and check that they are equal. We expect + // both the string representation and the exact IP to match. + for s, ip1 := range ips1 { + // Assert the IP string is found in both sets. + ip2, ok := ips2[s] + if !ok { + return true, nil + } + + // And that the IPs are considered equal. + if !ip1.Equal(ip2) { + return true, nil + } + } + + // Get the full list of DNS names to use. + _, dnsNames := dnsNames(tlsExtraDomains) + + // We do the same kind of deduplication for the DNS names. + dns1 := make(map[string]struct{}) + for _, n := range cert.DNSNames { + dns1[n] = struct{}{} + } + + dns2 := make(map[string]struct{}) + for _, n := range dnsNames { + dns2[n] = struct{}{} + } + + // If the number of domains are different, it is out of date. + if len(dns1) != len(dns2) { + return true, nil + } + + // Similarly, check that each DNS name matches what is found in the + // certificate. + for k := range dns1 { + if _, ok := dns2[k]; !ok { + return true, nil + } + } + + // Certificate was up-to-date. + return false, nil +} + // GenCertPair generates a key/cert pair to the paths provided. The // auto-generated certificates should *not* be used in production for public // access as they're self-signed and don't necessarily contain all of the diff --git a/cert/selfsigned_test.go b/cert/selfsigned_test.go new file mode 100644 index 000000000..0a5888a96 --- /dev/null +++ b/cert/selfsigned_test.go @@ -0,0 +1,135 @@ +package cert_test + +import ( + "io/ioutil" + "testing" + + "github.com/lightningnetwork/lnd/cert" +) + +var ( + extraIPs = []string{"1.1.1.1", "123.123.123.1", "199.189.12.12"} + extraDomains = []string{"home", "and", "away"} +) + +// TestIsOutdatedCert checks that we'll consider the TLS certificate outdated +// if the ip addresses or dns names don't match. +func TestIsOutdatedCert(t *testing.T) { + tempDir, err := ioutil.TempDir("", "certtest") + if err != nil { + t.Fatal(err) + } + + certPath := tempDir + "/tls.cert" + keyPath := tempDir + "/tls.key" + + // Generate TLS files with two extra IPs and domains. + err = cert.GenCertPair( + "lnd autogenerated cert", certPath, keyPath, extraIPs[:2], + extraDomains[:2], cert.DefaultAutogenValidity, + ) + if err != nil { + t.Fatal(err) + } + + // We'll attempt to check up-to-date status for all variants of 1-3 + // number of IPs and domains. + for numIPs := 1; numIPs <= len(extraIPs); numIPs++ { + for numDomains := 1; numDomains <= len(extraDomains); numDomains++ { + _, parsedCert, err := cert.LoadCert( + certPath, keyPath, + ) + if err != nil { + t.Fatal(err) + } + + // Using the test case's number of IPs and domains, get + // the outdated status of the certificate we created + // above. + outdated, err := cert.IsOutdated( + parsedCert, extraIPs[:numIPs], + extraDomains[:numDomains], + ) + if err != nil { + t.Fatal(err) + } + + // We expect it to be considered outdated if the IPs or + // domains don't match exactly what we created. + expected := numIPs != 2 || numDomains != 2 + if outdated != expected { + t.Fatalf("expected certificate to be "+ + "outdated=%v, got=%v", expected, + outdated) + } + } + } +} + +// TestIsOutdatedPermutation tests that the order of listed IPs or DNS names, +// nor dulicates in the lists, matter for whether we consider the certificate +// outdated. +func TestIsOutdatedPermutation(t *testing.T) { + tempDir, err := ioutil.TempDir("", "certtest") + if err != nil { + t.Fatal(err) + } + + certPath := tempDir + "/tls.cert" + keyPath := tempDir + "/tls.key" + + // Generate TLS files from the IPs and domains. + err = cert.GenCertPair( + "lnd autogenerated cert", certPath, keyPath, extraIPs[:], + extraDomains[:], cert.DefaultAutogenValidity, + ) + if err != nil { + t.Fatal(err) + } + _, parsedCert, err := cert.LoadCert(certPath, keyPath) + if err != nil { + t.Fatal(err) + } + + // If we have duplicate IPs or DNS names listed, that shouldn't matter. + dupIPs := make([]string, len(extraIPs)*2) + for i := range dupIPs { + dupIPs[i] = extraIPs[i/2] + } + + dupDNS := make([]string, len(extraDomains)*2) + for i := range dupDNS { + dupDNS[i] = extraDomains[i/2] + } + + outdated, err := cert.IsOutdated(parsedCert, dupIPs, dupDNS) + if err != nil { + t.Fatal(err) + } + + if outdated { + t.Fatalf("did not expect duplicate IPs or DNS names be " + + "considered outdated") + } + + // Similarly, the order of the lists shouldn't matter. + revIPs := make([]string, len(extraIPs)) + for i := range revIPs { + revIPs[i] = extraIPs[len(extraIPs)-1-i] + } + + revDNS := make([]string, len(extraDomains)) + for i := range revDNS { + revDNS[i] = extraDomains[len(extraDomains)-1-i] + } + + outdated, err = cert.IsOutdated(parsedCert, revIPs, revDNS) + if err != nil { + t.Fatal(err) + } + + if outdated { + t.Fatalf("did not expect reversed IPs or DNS names be " + + "considered outdated") + } +} From f7a85e07b0c130375d57611f2a50581c349e7127 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 5 Feb 2020 13:51:48 +0100 Subject: [PATCH 3/4] lnd+cert: recreate TLS files if IPs or DNS changed This commit makes lnd recreate its TLS certificate if the config's tlsextradomains or tlsextraips changed. This is useful, since earlier user would have to manually delete the files to trigger lnd to recreate them. To ensure users don't accidentally have their TLS certificate recreated, we gate it behind a flag --tlsautorefresh that defaults to false. --- config.go | 25 ++++++++++++++----------- lnd.go | 23 +++++++++++++++++++---- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/config.go b/config.go index 89b934a8b..2308cd6b5 100644 --- a/config.go +++ b/config.go @@ -232,21 +232,24 @@ type torConfig struct { type config struct { ShowVersion bool `short:"V" long:"version" description:"Display version information and exit"` - LndDir string `long:"lnddir" description:"The base directory that contains lnd's data, logs, configuration file, etc."` - ConfigFile string `short:"C" long:"configfile" description:"Path to configuration file"` - DataDir string `short:"b" long:"datadir" description:"The directory to store lnd's data within"` - SyncFreelist bool `long:"sync-freelist" description:"Whether the databases used within lnd should sync their freelist to disk. This is disabled by default resulting in improved memory performance during operation, but with an increase in startup time."` + LndDir string `long:"lnddir" description:"The base directory that contains lnd's data, logs, configuration file, etc."` + ConfigFile string `short:"C" long:"configfile" description:"Path to configuration file"` + DataDir string `short:"b" long:"datadir" description:"The directory to store lnd's data within"` + SyncFreelist bool `long:"sync-freelist" description:"Whether the databases used within lnd should sync their freelist to disk. This is disabled by default resulting in improved memory performance during operation, but with an increase in startup time."` + TLSCertPath string `long:"tlscertpath" description:"Path to write the TLS certificate for lnd's RPC and REST services"` TLSKeyPath string `long:"tlskeypath" description:"Path to write the TLS private key for lnd's RPC and REST services"` TLSExtraIPs []string `long:"tlsextraip" description:"Adds an extra ip to the generated certificate"` TLSExtraDomains []string `long:"tlsextradomain" description:"Adds an extra domain to the generated certificate"` - NoMacaroons bool `long:"no-macaroons" description:"Disable macaroon authentication"` - AdminMacPath string `long:"adminmacaroonpath" description:"Path to write the admin macaroon for lnd's RPC and REST services if it doesn't exist"` - ReadMacPath string `long:"readonlymacaroonpath" description:"Path to write the read-only macaroon for lnd's RPC and REST services if it doesn't exist"` - InvoiceMacPath string `long:"invoicemacaroonpath" description:"Path to the invoice-only macaroon for lnd's RPC and REST services if it doesn't exist"` - LogDir string `long:"logdir" description:"Directory to log output."` - MaxLogFiles int `long:"maxlogfiles" description:"Maximum logfiles to keep (0 for no rotation)"` - MaxLogFileSize int `long:"maxlogfilesize" description:"Maximum logfile size in MB"` + TLSAutoRefresh bool `long:"tlsautorefresh" description:"Re-generate TLS certificate and key if the IPs or domains are changed"` + + NoMacaroons bool `long:"no-macaroons" description:"Disable macaroon authentication"` + AdminMacPath string `long:"adminmacaroonpath" description:"Path to write the admin macaroon for lnd's RPC and REST services if it doesn't exist"` + ReadMacPath string `long:"readonlymacaroonpath" description:"Path to write the read-only macaroon for lnd's RPC and REST services if it doesn't exist"` + InvoiceMacPath string `long:"invoicemacaroonpath" description:"Path to the invoice-only macaroon for lnd's RPC and REST services if it doesn't exist"` + LogDir string `long:"logdir" description:"Directory to log output."` + MaxLogFiles int `long:"maxlogfiles" description:"Maximum logfiles to keep (0 for no rotation)"` + MaxLogFileSize int `long:"maxlogfilesize" description:"Maximum logfile size in MB"` // We'll parse these 'raw' string arguments into real net.Addrs in the // loadConfig function. We need to expose the 'raw' strings so the diff --git a/lnd.go b/lnd.go index 1c10c5464..1a28cebcf 100644 --- a/lnd.go +++ b/lnd.go @@ -717,10 +717,25 @@ func getTLSConfig(tlsCertPath string, tlsKeyPath string, tlsExtraIPs, return nil, nil, "", err } - // If the certificate expired, delete it and the TLS key and generate a - // new pair. - if time.Now().After(parsedCert.NotAfter) { - ltndLog.Info("TLS certificate is expired, generating a new one") + // We check whether the certifcate we have on disk match the IPs and + // domains specified by the config. If the extra IPs or domains have + // changed from when the certificate was created, we will refresh the + // certificate if auto refresh is active. + refresh := false + if cfg.TLSAutoRefresh { + refresh, err = cert.IsOutdated( + parsedCert, tlsExtraIPs, tlsExtraDomains, + ) + if err != nil { + return nil, nil, "", err + } + } + + // If the certificate expired or it was outdated, delete it and the TLS + // key and generate a new pair. + if time.Now().After(parsedCert.NotAfter) || refresh { + ltndLog.Info("TLS certificate is expired or outdated, " + + "generating a new one") err := os.Remove(tlsCertPath) if err != nil { From ba38bda5f06843e0c3add40130efb615b58bf21e Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Wed, 5 Feb 2020 13:51:48 +0100 Subject: [PATCH 4/4] lnd: reload cert data after renewal After renewing the certificate, the new certificate wasn't actually loaded and used, causing the old one to be used until lnd was restarted. This fixes that by reloading it after it has been written. --- lnd.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lnd.go b/lnd.go index 1a28cebcf..a080f5a7b 100644 --- a/lnd.go +++ b/lnd.go @@ -757,6 +757,12 @@ func getTLSConfig(tlsCertPath string, tlsKeyPath string, tlsExtraIPs, return nil, nil, "", err } rpcsLog.Infof("Done renewing TLS certificates") + + // Reload the certificate data. + certData, _, err = cert.LoadCert(tlsCertPath, tlsKeyPath) + if err != nil { + return nil, nil, "", err + } } tlsCfg := cert.TLSConfFromCert(certData)