Re-create TLS files if IPs or domains changed #3932

halseth · 2020-01-20T10:25:30Z

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.

The new bahavior is gated behind a new flag --tlsautorefresh

Roasbeef

Solid fix! No major comments on my end after a first pass review. Have reproduced a live scenario to test this fix to ensure that the upgrade to 0.9 will be seamless for any existing mobile apps that package the current form of lnd on mobile?

Roasbeef · 2020-01-21T03:41:23Z

cert/selfsigned.go

@@ -100,6 +100,52 @@ func dnsNames(tlsExtraDomains []string) (string, []string) {
 	return host, dnsNames
 }

+// Outdated returns whether the given certificate is outdated wrt the IPs and


Naming suggestion: IsOutdated?

wrt -> w.r.t

godoc comment could also use further elaboration on what "outdated" means in this context (IPs or domains aren't a sub-set of what's currently in the cert), as a few of our related packages use this cert package to handle routine cert operations.

Roasbeef · 2020-01-21T03:43:42Z

cert/selfsigned.go

+
+	// If the certificate has a different number of IP addresses, it is
+	// definitely out of date.
+	if len(ips) != len(cert.IPAddresses) {


Alternatively, should we relax this to allow more IPs in the cert than are specified in our config? A sample case being a cert that a user has generated themselves, and synced with the existing configuration options.

The problem then is that even though the user removes an IP from the config, the cert will still allow it.

this will return a false positive if the user accidentally includes a duplicate IP, when in fact we don't need to change the cert. imo it'd be better to convert both to sets, then compare length.

one possible downside of this is that certs generated before this may not have checked for duplicates, so it would cause those to be refreshed. perhaps we should start checking for duplicate entries before creating new certs as well?

Yep, fixed: 7aba914

Roasbeef · 2020-01-21T03:46:41Z

lnd.go

@@ -744,6 +744,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)


Depending on where we go with this PR, we may want to consider splitting off this commit (either into a new PR or to be cherry-picked onto the next release branch).

do we know why our unit tests didn't catch this?

I don't think we have any lnd.go level unit tests?

halseth · 2020-01-28T19:11:06Z

Gate behind cmd line flag

bhandras

LGTM, nice and clean pr, only nits

bhandras · 2020-02-05T15:23:09Z

cert/selfsigned_test.go

+			}
+
+			// We expect it to be considered outdated if the IPs or
+			// domains don't match exaactly what we created.


nit: exactly

bhandras · 2020-02-05T15:25:54Z

cert/selfsigned_test.go

+
+	// We'll attempt to check up-to-date status for all variants of 1-3
+	// number of IPs and domains.
+	for numIPs := 1; numIPs <= 3; numIPs++ {


you could use len(extraIPs) and len(extraDomains) instead

That is much better indeed!

bhandras · 2020-02-05T15:28:26Z

cert/selfsigned_test.go

+			expected := numIPs != 2 || numDomains != 2
+			if outdated != expected {
+				t.Fatalf("expected certificate to be "+
+					"outdated=%v", expected)


nit: i'd add "got=%v" to be more informative.

cfromknecht · 2020-02-11T22:03:28Z

cert/selfsigned.go

+
+	// If the certificate has a different number of IP addresses, it is
+	// definitely out of date.
+	if len(ips) != len(cert.IPAddresses) {


this will return a false positive if the user accidentally includes a duplicate IP, when in fact we don't need to change the cert. imo it'd be better to convert both to sets, then compare length.

one possible downside of this is that certs generated before this may not have checked for duplicates, so it would cause those to be refreshed. perhaps we should start checking for duplicate entries before creating new certs as well?

cfromknecht · 2020-02-11T22:04:55Z

cert/selfsigned.go

+	// the order to match exactly, as we assume the same tlsExtraIPs were
+	// used when the certificate was first created.
+	for i := range ips {
+		if !ips[i].Equal(cert.IPAddresses[i]) {


related to above, imo order shouldn't matter here. plus we can reuse the sets created above to check. i think it'd be less obvious to users that reordering ip/dns/domains would cause their certs to be renewed

cfromknecht · 2020-02-11T22:05:33Z

cert/selfsigned.go

+	_, dnsNames := dnsNames(tlsExtraDomains)
+
+	// If the number of domains are different, it is out of date.
+	if len(cert.DNSNames) != len(dnsNames) {


similar comment here wrt to duplicates/set matching

cfromknecht · 2020-02-11T22:09:03Z

lnd.go

@@ -744,6 +744,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)


do we know why our unit tests didn't catch this?

cfromknecht · 2020-02-11T22:15:16Z

config.go

-	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"`


nit: recently i think we've been preferring the hyphenated config options, but i see that it would deviate from our other --tls* options :/

yeah, would be nice to move all tls options into its own category, but maybe not for a minor release like this

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.

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.

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.

cfromknecht

non-blocking comment, otherwise LGTM! 🔥

cfromknecht · 2020-02-12T19:44:48Z

config.go

-	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"`


cfromknecht · 2020-02-12T19:45:11Z

cert/selfsigned.go

+
+		// And that the IPs are considered equal.
+		if !ip1.Equal(ip2) {
+			return true, nil


is this reachable? is it possible for two IPs to have the same string representation, but be unequal?

No idea 😂

halseth force-pushed the tls-oudated-check branch from 2a60d00 to 0700680 Compare January 20, 2020 12:03

Roasbeef reviewed Jan 21, 2020

View reviewed changes

Roasbeef added this to the 0.9.1 milestone Jan 21, 2020

halseth force-pushed the tls-oudated-check branch 2 times, most recently from ef5bd29 to 5010ad6 Compare February 5, 2020 14:05

halseth requested review from Roasbeef and bhandras February 5, 2020 14:07

bhandras approved these changes Feb 5, 2020

View reviewed changes

halseth force-pushed the tls-oudated-check branch from 5010ad6 to 2e6ee95 Compare February 7, 2020 08:02

guggero requested review from cfromknecht and removed request for Roasbeef February 11, 2020 19:14

cfromknecht reviewed Feb 11, 2020

View reviewed changes

halseth added 4 commits February 12, 2020 10:59

cert: extract IP and DNS parsing into methods

d316107

cert+test: IsOutdated check for TLS files if IPs or DNS changed

83dcf95

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.

lnd: reload cert data after renewal

ba38bda

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.

halseth force-pushed the tls-oudated-check branch from d964be6 to ba38bda Compare February 12, 2020 10:00

halseth requested a review from cfromknecht February 12, 2020 10:01

cfromknecht approved these changes Feb 12, 2020

View reviewed changes

halseth merged commit 8904b68 into lightningnetwork:master Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-create TLS files if IPs or domains changed #3932

Re-create TLS files if IPs or domains changed #3932

halseth commented Jan 20, 2020 •

edited

Loading

Roasbeef left a comment

Roasbeef Jan 21, 2020

Roasbeef Jan 21, 2020

Roasbeef Jan 21, 2020

halseth Feb 5, 2020

Roasbeef Jan 21, 2020

halseth Feb 5, 2020

cfromknecht Feb 11, 2020

halseth Feb 12, 2020

Roasbeef Jan 21, 2020

cfromknecht Feb 11, 2020

halseth Feb 12, 2020 •

edited

Loading

halseth commented Jan 28, 2020

bhandras left a comment

bhandras Feb 5, 2020

halseth Feb 7, 2020

bhandras Feb 5, 2020

halseth Feb 7, 2020

bhandras Feb 5, 2020

halseth Feb 7, 2020

cfromknecht Feb 11, 2020

cfromknecht Feb 11, 2020

cfromknecht Feb 11, 2020

cfromknecht Feb 11, 2020

cfromknecht Feb 11, 2020

halseth Feb 12, 2020

cfromknecht Feb 12, 2020

cfromknecht left a comment

cfromknecht Feb 12, 2020

cfromknecht Feb 12, 2020

halseth Feb 13, 2020

Re-create TLS files if IPs or domains changed #3932

Re-create TLS files if IPs or domains changed #3932

Conversation

halseth commented Jan 20, 2020 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

halseth commented Jan 28, 2020

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Jan 20, 2020 •

edited

Loading

halseth Feb 12, 2020 •

edited

Loading